Skip to content

Commit 2899344

Browse files
abonanderpietroalbini
abonander
authored and
pietroalbini
committed
Warn on pointless #[derive] in more places
This fixes the regression in rust-lang#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 rust-lang#49934
1 parent ceaeabf commit 2899344

File tree

7 files changed

+193
-16
lines changed

7 files changed

+193
-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);
@@ -730,6 +730,7 @@ pub fn walk_generic_param<'v, V: Visitor<'v>>(visitor: &mut V, param: &'v Generi
730730
visitor.visit_name(ty_param.span, ty_param.name);
731731
walk_list!(visitor, visit_ty_param_bound, &ty_param.bounds);
732732
walk_list!(visitor, visit_ty, &ty_param.default);
733+
walk_list!(visitor, visit_attribute, ty_param.attrs.iter());
733734
}
734735
}
735736
}

‎src/librustc_resolve/macros.rs

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

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

231+
if !allow_derive { return None }
232+
231233
// Check for legacy derives
232234
for i in 0..attrs.len() {
233235
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
@@ -833,7 +833,7 @@ impl Stmt {
833833

834834
pub fn is_item(&self) -> bool {
835835
match self.node {
836-
StmtKind::Local(_) => true,
836+
StmtKind::Item(_) => true,
837837
_ => false,
838838
}
839839
}

‎src/libsyntax/ext/base.rs

Lines changed: 26 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -106,6 +106,27 @@ impl Annotatable {
106106
}
107107
}
108108

109+
pub fn expect_foreign_item(self) -> ast::ForeignItem {
110+
match self {
111+
Annotatable::ForeignItem(i) => i.into_inner(),
112+
_ => panic!("expected foreign item")
113+
}
114+
}
115+
116+
pub fn expect_stmt(self) -> ast::Stmt {
117+
match self {
118+
Annotatable::Stmt(stmt) => stmt.into_inner(),
119+
_ => panic!("expected statement"),
120+
}
121+
}
122+
123+
pub fn expect_expr(self) -> P<ast::Expr> {
124+
match self {
125+
Annotatable::Expr(expr) => expr,
126+
_ => panic!("expected expression"),
127+
}
128+
}
129+
109130
pub fn derive_allowed(&self) -> bool {
110131
match *self {
111132
Annotatable::Item(ref item) => match item.node {
@@ -631,7 +652,9 @@ pub trait Resolver {
631652

632653
fn resolve_imports(&mut self);
633654
// Resolves attribute and derive legacy macros from `#![plugin(..)]`.
634-
fn find_legacy_attr_invoc(&mut self, attrs: &mut Vec<Attribute>) -> Option<Attribute>;
655+
fn find_legacy_attr_invoc(&mut self, attrs: &mut Vec<Attribute>, allow_derive: bool)
656+
-> Option<Attribute>;
657+
635658
fn resolve_invoc(&mut self, invoc: &mut Invocation, scope: Mark, force: bool)
636659
-> Result<Option<Lrc<SyntaxExtension>>, Determinacy>;
637660
fn resolve_macro(&mut self, scope: Mark, path: &ast::Path, kind: MacroKind, force: bool)
@@ -657,7 +680,8 @@ impl Resolver for DummyResolver {
657680
fn add_builtin(&mut self, _ident: ast::Ident, _ext: Lrc<SyntaxExtension>) {}
658681

659682
fn resolve_imports(&mut self) {}
660-
fn find_legacy_attr_invoc(&mut self, _attrs: &mut Vec<Attribute>) -> Option<Attribute> { None }
683+
fn find_legacy_attr_invoc(&mut self, _attrs: &mut Vec<Attribute>, _allow_derive: bool)
684+
-> Option<Attribute> { None }
661685
fn resolve_invoc(&mut self, _invoc: &mut Invocation, _scope: Mark, _force: bool)
662686
-> Result<Option<Lrc<SyntaxExtension>>, Determinacy> {
663687
Err(Determinacy::Determined)

‎src/libsyntax/ext/expand.rs

Lines changed: 59 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -141,15 +141,24 @@ impl ExpansionKind {
141141
}
142142

143143
fn expect_from_annotatables<I: IntoIterator<Item = Annotatable>>(self, items: I) -> Expansion {
144-
let items = items.into_iter();
144+
let mut items = items.into_iter();
145145
match self {
146146
ExpansionKind::Items =>
147147
Expansion::Items(items.map(Annotatable::expect_item).collect()),
148148
ExpansionKind::ImplItems =>
149149
Expansion::ImplItems(items.map(Annotatable::expect_impl_item).collect()),
150150
ExpansionKind::TraitItems =>
151151
Expansion::TraitItems(items.map(Annotatable::expect_trait_item).collect()),
152-
_ => unreachable!(),
152+
ExpansionKind::ForeignItems =>
153+
Expansion::ForeignItems(items.map(Annotatable::expect_foreign_item).collect()),
154+
ExpansionKind::Stmts => Expansion::Stmts(items.map(Annotatable::expect_stmt).collect()),
155+
ExpansionKind::Expr => Expansion::Expr(
156+
items.next().expect("expected exactly one expression").expect_expr()
157+
),
158+
ExpansionKind::OptExpr =>
159+
Expansion::OptExpr(items.next().map(Annotatable::expect_expr)),
160+
ExpansionKind::Pat | ExpansionKind::Ty =>
161+
panic!("patterns and types aren't annotatable"),
153162
}
154163
}
155164
}
@@ -867,14 +876,15 @@ impl<'a, 'b> InvocationCollector<'a, 'b> {
867876
self.collect(kind, InvocationKind::Attr { attr, traits, item })
868877
}
869878

870-
// If `item` is an attr invocation, remove and return the macro attribute.
879+
/// If `item` is an attr invocation, remove and return the macro attribute and derive traits.
871880
fn classify_item<T>(&mut self, mut item: T) -> (Option<ast::Attribute>, Vec<Path>, T)
872881
where T: HasAttrs,
873882
{
874883
let (mut attr, mut traits) = (None, Vec::new());
875884

876885
item = item.map_attrs(|mut attrs| {
877-
if let Some(legacy_attr_invoc) = self.cx.resolver.find_legacy_attr_invoc(&mut attrs) {
886+
if let Some(legacy_attr_invoc) = self.cx.resolver.find_legacy_attr_invoc(&mut attrs,
887+
true) {
878888
attr = Some(legacy_attr_invoc);
879889
return attrs;
880890
}
@@ -889,6 +899,28 @@ impl<'a, 'b> InvocationCollector<'a, 'b> {
889899
(attr, traits, item)
890900
}
891901

902+
/// Alternative of `classify_item()` that ignores `#[derive]` so invocations fallthrough
903+
/// to the unused-attributes lint (making it an error on statements and expressions
904+
/// is a breaking change)
905+
fn classify_nonitem<T: HasAttrs>(&mut self, mut item: T) -> (Option<ast::Attribute>, T) {
906+
let mut attr = None;
907+
908+
item = item.map_attrs(|mut attrs| {
909+
if let Some(legacy_attr_invoc) = self.cx.resolver.find_legacy_attr_invoc(&mut attrs,
910+
false) {
911+
attr = Some(legacy_attr_invoc);
912+
return attrs;
913+
}
914+
915+
if self.cx.ecfg.proc_macro_enabled() {
916+
attr = find_attr_invoc(&mut attrs);
917+
}
918+
attrs
919+
});
920+
921+
(attr, item)
922+
}
923+
892924
fn configure<T: HasAttrs>(&mut self, node: T) -> Option<T> {
893925
self.cfg.configure(node)
894926
}
@@ -899,6 +931,13 @@ impl<'a, 'b> InvocationCollector<'a, 'b> {
899931
let features = self.cx.ecfg.features.unwrap();
900932
for attr in attrs.iter() {
901933
feature_gate::check_attribute(attr, self.cx.parse_sess, features);
934+
935+
// macros are expanded before any lint passes so this warning has to be hardcoded
936+
if attr.path == "derive" {
937+
self.cx.struct_span_warn(attr.span, "`#[derive]` does nothing on macro invocations")
938+
.note("this may become a hard error in a future release")
939+
.emit();
940+
}
902941
}
903942
}
904943

@@ -919,15 +958,16 @@ impl<'a, 'b> Folder for InvocationCollector<'a, 'b> {
919958
let mut expr = self.cfg.configure_expr(expr).into_inner();
920959
expr.node = self.cfg.configure_expr_kind(expr.node);
921960

922-
let (attr, derives, expr) = self.classify_item(expr);
961+
// ignore derives so they remain unused
962+
let (attr, expr) = self.classify_nonitem(expr);
923963

924-
if attr.is_some() || !derives.is_empty() {
964+
if attr.is_some() {
925965
// collect the invoc regardless of whether or not attributes are permitted here
926966
// expansion will eat the attribute so it won't error later
927967
attr.as_ref().map(|a| self.cfg.maybe_emit_expr_attr_err(a));
928968

929969
// ExpansionKind::Expr requires the macro to emit an expression
930-
return self.collect_attr(attr, derives, Annotatable::Expr(P(expr)), ExpansionKind::Expr)
970+
return self.collect_attr(attr, vec![], Annotatable::Expr(P(expr)), ExpansionKind::Expr)
931971
.make_expr();
932972
}
933973

@@ -943,12 +983,13 @@ impl<'a, 'b> Folder for InvocationCollector<'a, 'b> {
943983
let mut expr = configure!(self, expr).into_inner();
944984
expr.node = self.cfg.configure_expr_kind(expr.node);
945985

946-
let (attr, derives, expr) = self.classify_item(expr);
986+
// ignore derives so they remain unused
987+
let (attr, expr) = self.classify_nonitem(expr);
947988

948-
if attr.is_some() || !derives.is_empty() {
989+
if attr.is_some() {
949990
attr.as_ref().map(|a| self.cfg.maybe_emit_expr_attr_err(a));
950991

951-
return self.collect_attr(attr, derives, Annotatable::Expr(P(expr)),
992+
return self.collect_attr(attr, vec![], Annotatable::Expr(P(expr)),
952993
ExpansionKind::OptExpr)
953994
.make_opt_expr();
954995
}
@@ -982,7 +1023,14 @@ impl<'a, 'b> Folder for InvocationCollector<'a, 'b> {
9821023

9831024
// we'll expand attributes on expressions separately
9841025
if !stmt.is_expr() {
985-
let (attr, derives, stmt_) = self.classify_item(stmt);
1026+
let (attr, derives, stmt_) = if stmt.is_item() {
1027+
self.classify_item(stmt)
1028+
} else {
1029+
// ignore derives on non-item statements so it falls through
1030+
// to the unused-attributes lint
1031+
let (attr, stmt) = self.classify_nonitem(stmt);
1032+
(attr, vec![], stmt)
1033+
};
9861034

9871035
if attr.is_some() || !derives.is_empty() {
9881036
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)