Skip to content

Commit f16d2ff

Browse files
abonanderabonander
committed
Warn on pointless #[derive] in more places
This fixes the regression in #49934 and ensures that unused `#[derive]`s on statements, expressions and generic type parameters survive to trip the `unused_attributes` lint. For `#[derive]` on macro invocations it has a hardcoded warning since linting occurs after expansion. This also adds regression testing for some nodes that were already warning properly. closes #49934
1 parent b91e6a2 commit f16d2ff

File tree

7 files changed

+184
-16
lines changed

7 files changed

+184
-16
lines changed

‎src/librustc/hir/intravisit.rs‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -404,7 +404,7 @@ pub fn walk_local<'v, V: Visitor<'v>>(visitor: &mut V, local: &'v Local) {
404404
// Intentionally visiting the expr first - the initialization expr
405405
// dominates the local's definition.
406406
walk_list!(visitor, visit_expr, &local.init);
407-
407+
walk_list!(visitor, visit_attribute, local.attrs.iter());
408408
visitor.visit_id(local.id);
409409
visitor.visit_pat(&local.pat);
410410
walk_list!(visitor, visit_ty, &local.ty);
@@ -731,6 +731,7 @@ pub fn walk_generic_param<'v, V: Visitor<'v>>(visitor: &mut V, param: &'v Generi
731731
visitor.visit_name(ty_param.span, ty_param.name);
732732
walk_list!(visitor, visit_ty_param_bound, &ty_param.bounds);
733733
walk_list!(visitor, visit_ty, &ty_param.default);
734+
walk_list!(visitor, visit_attribute, ty_param.attrs.iter());
734735
}
735736
}
736737
}

‎src/librustc_resolve/macros.rs‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -206,7 +206,7 @@ impl<'a> base::Resolver for Resolver<'a> {
206206
}
207207

208208
// Resolves attribute and derive legacy macros from `#![plugin(..)]`.
209-
fn find_legacy_attr_invoc(&mut self, attrs: &mut Vec<ast::Attribute>)
209+
fn find_legacy_attr_invoc(&mut self, attrs: &mut Vec<ast::Attribute>, allow_derive: bool)
210210
-> Option<ast::Attribute> {
211211
for i in 0..attrs.len() {
212212
let name = unwrap_or!(attrs[i].name(), continue);
@@ -227,6 +227,8 @@ impl<'a> base::Resolver for Resolver<'a> {
227227
}
228228
}
229229

230+
if !allow_derive { return None }
231+
230232
// Check for legacy derives
231233
for i in 0..attrs.len() {
232234
let name = unwrap_or!(attrs[i].name(), continue);

‎src/libsyntax/ast.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -821,7 +821,7 @@ impl Stmt {
821821

822822
pub fn is_item(&self) -> bool {
823823
match self.node {
824-
StmtKind::Local(_) => true,
824+
StmtKind::Item(_) => true,
825825
_ => false,
826826
}
827827
}

‎src/libsyntax/ext/base.rs‎

Lines changed: 19 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -118,6 +118,20 @@ impl Annotatable {
118118
}
119119
}
120120

121+
pub fn expect_stmt(self) -> ast::Stmt {
122+
match self {
123+
Annotatable::Stmt(stmt) => stmt.into_inner(),
124+
_ => panic!("expected statement"),
125+
}
126+
}
127+
128+
pub fn expect_expr(self) -> P<ast::Expr> {
129+
match self {
130+
Annotatable::Expr(expr) => expr,
131+
_ => panic!("expected expression"),
132+
}
133+
}
134+
121135
pub fn derive_allowed(&self) -> bool {
122136
match *self {
123137
Annotatable::Item(ref item) => match item.node {
@@ -661,7 +675,9 @@ pub trait Resolver {
661675

662676
fn resolve_imports(&mut self);
663677
// Resolves attribute and derive legacy macros from `#![plugin(..)]`.
664-
fn find_legacy_attr_invoc(&mut self, attrs: &mut Vec<Attribute>) -> Option<Attribute>;
678+
fn find_legacy_attr_invoc(&mut self, attrs: &mut Vec<Attribute>, allow_derive: bool)
679+
-> Option<Attribute>;
680+
665681
fn resolve_invoc(&mut self, invoc: &mut Invocation, scope: Mark, force: bool)
666682
-> Result<Option<Lrc<SyntaxExtension>>, Determinacy>;
667683
fn resolve_macro(&mut self, scope: Mark, path: &ast::Path, kind: MacroKind, force: bool)
@@ -687,7 +703,8 @@ impl Resolver for DummyResolver {
687703
fn add_builtin(&mut self, _ident: ast::Ident, _ext: Lrc<SyntaxExtension>) {}
688704

689705
fn resolve_imports(&mut self) {}
690-
fn find_legacy_attr_invoc(&mut self, _attrs: &mut Vec<Attribute>) -> Option<Attribute> { None }
706+
fn find_legacy_attr_invoc(&mut self, _attrs: &mut Vec<Attribute>, _allow_derive: bool)
707+
-> Option<Attribute> { None }
691708
fn resolve_invoc(&mut self, _invoc: &mut Invocation, _scope: Mark, _force: bool)
692709
-> Result<Option<Lrc<SyntaxExtension>>, Determinacy> {
693710
Err(Determinacy::Determined)

‎src/libsyntax/ext/expand.rs‎

Lines changed: 57 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -143,7 +143,7 @@ impl ExpansionKind {
143143
}
144144

145145
fn expect_from_annotatables<I: IntoIterator<Item = Annotatable>>(self, items: I) -> Expansion {
146-
let items = items.into_iter();
146+
let mut items = items.into_iter();
147147
match self {
148148
ExpansionKind::Items =>
149149
Expansion::Items(items.map(Annotatable::expect_item).collect()),
@@ -153,7 +153,14 @@ impl ExpansionKind {
153153
Expansion::TraitItems(items.map(Annotatable::expect_trait_item).collect()),
154154
ExpansionKind::ForeignItems =>
155155
Expansion::ForeignItems(items.map(Annotatable::expect_foreign_item).collect()),
156-
_ => unreachable!(),
156+
ExpansionKind::Stmts => Expansion::Stmts(items.map(Annotatable::expect_stmt).collect()),
157+
ExpansionKind::Expr => Expansion::Expr(
158+
items.next().expect("expected exactly one expression").expect_expr()
159+
),
160+
ExpansionKind::OptExpr =>
161+
Expansion::OptExpr(items.next().map(Annotatable::expect_expr)),
162+
ExpansionKind::Pat | ExpansionKind::Ty =>
163+
panic!("patterns and types aren't annotatable"),
157164
}
158165
}
159166
}
@@ -886,14 +893,15 @@ impl<'a, 'b> InvocationCollector<'a, 'b> {
886893
self.collect(kind, InvocationKind::Attr { attr, traits, item })
887894
}
888895

889-
// If `item` is an attr invocation, remove and return the macro attribute.
896+
/// If `item` is an attr invocation, remove and return the macro attribute and derive traits.
890897
fn classify_item<T>(&mut self, mut item: T) -> (Option<ast::Attribute>, Vec<Path>, T)
891898
where T: HasAttrs,
892899
{
893900
let (mut attr, mut traits) = (None, Vec::new());
894901

895902
item = item.map_attrs(|mut attrs| {
896-
if let Some(legacy_attr_invoc) = self.cx.resolver.find_legacy_attr_invoc(&mut attrs) {
903+
if let Some(legacy_attr_invoc) = self.cx.resolver.find_legacy_attr_invoc(&mut attrs,
904+
true) {
897905
attr = Some(legacy_attr_invoc);
898906
return attrs;
899907
}
@@ -908,6 +916,28 @@ impl<'a, 'b> InvocationCollector<'a, 'b> {
908916
(attr, traits, item)
909917
}
910918

919+
/// Alternative of `classify_item()` that ignores `#[derive]` so invocations fallthrough
920+
/// to the unused-attributes lint (making it an error on statements and expressions
921+
/// is a breaking change)
922+
fn classify_nonitem<T: HasAttrs>(&mut self, mut item: T) -> (Option<ast::Attribute>, T) {
923+
let mut attr = None;
924+
925+
item = item.map_attrs(|mut attrs| {
926+
if let Some(legacy_attr_invoc) = self.cx.resolver.find_legacy_attr_invoc(&mut attrs,
927+
false) {
928+
attr = Some(legacy_attr_invoc);
929+
return attrs;
930+
}
931+
932+
if self.cx.ecfg.proc_macro_enabled() {
933+
attr = find_attr_invoc(&mut attrs);
934+
}
935+
attrs
936+
});
937+
938+
(attr, item)
939+
}
940+
911941
fn configure<T: HasAttrs>(&mut self, node: T) -> Option<T> {
912942
self.cfg.configure(node)
913943
}
@@ -918,6 +948,13 @@ impl<'a, 'b> InvocationCollector<'a, 'b> {
918948
let features = self.cx.ecfg.features.unwrap();
919949
for attr in attrs.iter() {
920950
feature_gate::check_attribute(attr, self.cx.parse_sess, features);
951+
952+
// macros are expanded before any lint passes so this warning has to be hardcoded
953+
if attr.path == "derive" {
954+
self.cx.struct_span_warn(attr.span, "`#[derive]` does nothing on macro invocations")
955+
.note("this may become a hard error in a future release")
956+
.emit();
957+
}
921958
}
922959
}
923960

@@ -938,15 +975,16 @@ impl<'a, 'b> Folder for InvocationCollector<'a, 'b> {
938975
let mut expr = self.cfg.configure_expr(expr).into_inner();
939976
expr.node = self.cfg.configure_expr_kind(expr.node);
940977

941-
let (attr, derives, expr) = self.classify_item(expr);
978+
// ignore derives so they remain unused
979+
let (attr, expr) = self.classify_nonitem(expr);
942980

943-
if attr.is_some() || !derives.is_empty() {
981+
if attr.is_some() {
944982
// collect the invoc regardless of whether or not attributes are permitted here
945983
// expansion will eat the attribute so it won't error later
946984
attr.as_ref().map(|a| self.cfg.maybe_emit_expr_attr_err(a));
947985

948986
// ExpansionKind::Expr requires the macro to emit an expression
949-
return self.collect_attr(attr, derives, Annotatable::Expr(P(expr)), ExpansionKind::Expr)
987+
return self.collect_attr(attr, vec![], Annotatable::Expr(P(expr)), ExpansionKind::Expr)
950988
.make_expr();
951989
}
952990

@@ -962,12 +1000,13 @@ impl<'a, 'b> Folder for InvocationCollector<'a, 'b> {
9621000
let mut expr = configure!(self, expr).into_inner();
9631001
expr.node = self.cfg.configure_expr_kind(expr.node);
9641002

965-
let (attr, derives, expr) = self.classify_item(expr);
1003+
// ignore derives so they remain unused
1004+
let (attr, expr) = self.classify_nonitem(expr);
9661005

967-
if attr.is_some() || !derives.is_empty() {
1006+
if attr.is_some() {
9681007
attr.as_ref().map(|a| self.cfg.maybe_emit_expr_attr_err(a));
9691008

970-
return self.collect_attr(attr, derives, Annotatable::Expr(P(expr)),
1009+
return self.collect_attr(attr, vec![], Annotatable::Expr(P(expr)),
9711010
ExpansionKind::OptExpr)
9721011
.make_opt_expr();
9731012
}
@@ -1001,7 +1040,14 @@ impl<'a, 'b> Folder for InvocationCollector<'a, 'b> {
10011040

10021041
// we'll expand attributes on expressions separately
10031042
if !stmt.is_expr() {
1004-
let (attr, derives, stmt_) = self.classify_item(stmt);
1043+
let (attr, derives, stmt_) = if stmt.is_item() {
1044+
self.classify_item(stmt)
1045+
} else {
1046+
// ignore derives on non-item statements so it falls through
1047+
// to the unused-attributes lint
1048+
let (attr, stmt) = self.classify_nonitem(stmt);
1049+
(attr, vec![], stmt)
1050+
};
10051051

10061052
if attr.is_some() || !derives.is_empty() {
10071053
return self.collect_attr(attr, derives,

‎src/test/ui/issue-49934.rs‎

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,52 @@
1+
// Copyright 2018 The Rust Project Developers. See the COPYRIGHT
2+
// file at the top-level directory of this distribution and at
3+
// http://rust-lang.org/COPYRIGHT.
4+
//
5+
// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
6+
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
7+
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
8+
// option. This file may not be copied, modified, or distributed
9+
// except according to those terms.
10+
11+
// compile-pass
12+
13+
#![feature(stmt_expr_attributes)]
14+
#![warn(unused_attributes)] //~ NOTE lint level defined here
15+
16+
fn foo<#[derive(Debug)] T>() { //~ WARN unused attribute
17+
match 0 {
18+
#[derive(Debug)] //~ WARN unused attribute
19+
_ => (),
20+
}
21+
}
22+
23+
fn main() {
24+
// fold_stmt (Item)
25+
#[allow(dead_code)]
26+
#[derive(Debug)] // should not warn
27+
struct Foo;
28+
29+
// fold_stmt (Mac)
30+
#[derive(Debug)]
31+
//~^ WARN `#[derive]` does nothing on macro invocations
32+
//~| NOTE this may become a hard error in a future release
33+
println!("Hello, world!");
34+
35+
// fold_stmt (Semi)
36+
#[derive(Debug)] //~ WARN unused attribute
37+
"Hello, world!";
38+
39+
// fold_stmt (Local)
40+
#[derive(Debug)] //~ WARN unused attribute
41+
let _ = "Hello, world!";
42+
43+
// fold_expr
44+
let _ = #[derive(Debug)] "Hello, world!";
45+
//~^ WARN unused attribute
46+
47+
let _ = [
48+
// fold_opt_expr
49+
#[derive(Debug)] //~ WARN unused attribute
50+
"Hello, world!"
51+
];
52+
}

‎src/test/ui/issue-49934.stderr‎

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,50 @@
1+
warning: `#[derive]` does nothing on macro invocations
2+
--> $DIR/issue-49934.rs:30:5
3+
|
4+
LL | #[derive(Debug)]
5+
| ^^^^^^^^^^^^^^^^
6+
|
7+
= note: this may become a hard error in a future release
8+
9+
warning: unused attribute
10+
--> $DIR/issue-49934.rs:16:8
11+
|
12+
LL | fn foo<#[derive(Debug)] T>() { //~ WARN unused attribute
13+
| ^^^^^^^^^^^^^^^^
14+
|
15+
note: lint level defined here
16+
--> $DIR/issue-49934.rs:14:9
17+
|
18+
LL | #![warn(unused_attributes)] //~ NOTE lint level defined here
19+
| ^^^^^^^^^^^^^^^^^
20+
21+
warning: unused attribute
22+
--> $DIR/issue-49934.rs:18:9
23+
|
24+
LL | #[derive(Debug)] //~ WARN unused attribute
25+
| ^^^^^^^^^^^^^^^^
26+
27+
warning: unused attribute
28+
--> $DIR/issue-49934.rs:36:5
29+
|
30+
LL | #[derive(Debug)] //~ WARN unused attribute
31+
| ^^^^^^^^^^^^^^^^
32+
33+
warning: unused attribute
34+
--> $DIR/issue-49934.rs:40:5
35+
|
36+
LL | #[derive(Debug)] //~ WARN unused attribute
37+
| ^^^^^^^^^^^^^^^^
38+
39+
warning: unused attribute
40+
--> $DIR/issue-49934.rs:44:13
41+
|
42+
LL | let _ = #[derive(Debug)] "Hello, world!";
43+
| ^^^^^^^^^^^^^^^^
44+
45+
warning: unused attribute
46+
--> $DIR/issue-49934.rs:49:9
47+
|
48+
LL | #[derive(Debug)] //~ WARN unused attribute
49+
| ^^^^^^^^^^^^^^^^
50+

0 commit comments

Comments
 (0)