Warn on pointless #[derive] in more places

abonander · abonander · commit f16d2ff7ec18 · 2018-04-29T16:01:41.000-07:00
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
diff --git a/src/librustc/hir/intravisit.rs b/src/librustc/hir/intravisit.rs
@@ -404,7 +404,7 @@ pub fn walk_local<'v, V: Visitor<'v>>(visitor: &mut V, local: &'v Local) {
     // Intentionally visiting the expr first - the initialization expr
     // dominates the local's definition.
     walk_list!(visitor, visit_expr, &local.init);
-
+    walk_list!(visitor, visit_attribute, local.attrs.iter());
     visitor.visit_id(local.id);
     visitor.visit_pat(&local.pat);
     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
             visitor.visit_name(ty_param.span, ty_param.name);
             walk_list!(visitor, visit_ty_param_bound, &ty_param.bounds);
             walk_list!(visitor, visit_ty, &ty_param.default);
+            walk_list!(visitor, visit_attribute, ty_param.attrs.iter());
         }
     }
 }
diff --git a/src/librustc_resolve/macros.rs b/src/librustc_resolve/macros.rs
@@ -206,7 +206,7 @@ impl<'a> base::Resolver for Resolver<'a> {
     }
 
     // Resolves attribute and derive legacy macros from `#![plugin(..)]`.
-    fn find_legacy_attr_invoc(&mut self, attrs: &mut Vec<ast::Attribute>)
+    fn find_legacy_attr_invoc(&mut self, attrs: &mut Vec<ast::Attribute>, allow_derive: bool)
                               -> Option<ast::Attribute> {
         for i in 0..attrs.len() {
             let name = unwrap_or!(attrs[i].name(), continue);
@@ -227,6 +227,8 @@ impl<'a> base::Resolver for Resolver<'a> {
             }
         }
 
+        if !allow_derive { return None }
+
         // Check for legacy derives
         for i in 0..attrs.len() {
             let name = unwrap_or!(attrs[i].name(), continue);
diff --git a/src/libsyntax/ast.rs b/src/libsyntax/ast.rs
@@ -821,7 +821,7 @@ impl Stmt {
 
     pub fn is_item(&self) -> bool {
         match self.node {
-            StmtKind::Local(_) => true,
+            StmtKind::Item(_) => true,
             _ => false,
         }
     }
diff --git a/src/libsyntax/ext/base.rs b/src/libsyntax/ext/base.rs
@@ -118,6 +118,20 @@ impl Annotatable {
         }
     }
 
+    pub fn expect_stmt(self) -> ast::Stmt {
+        match self {
+            Annotatable::Stmt(stmt) => stmt.into_inner(),
+            _ => panic!("expected statement"),
+        }
+    }
+
+    pub fn expect_expr(self) -> P<ast::Expr> {
+        match self {
+            Annotatable::Expr(expr) => expr,
+            _ => panic!("expected expression"),
+        }
+    }
+
     pub fn derive_allowed(&self) -> bool {
         match *self {
             Annotatable::Item(ref item) => match item.node {
@@ -661,7 +675,9 @@ pub trait Resolver {
 
     fn resolve_imports(&mut self);
     // Resolves attribute and derive legacy macros from `#![plugin(..)]`.
-    fn find_legacy_attr_invoc(&mut self, attrs: &mut Vec<Attribute>) -> Option<Attribute>;
+    fn find_legacy_attr_invoc(&mut self, attrs: &mut Vec<Attribute>, allow_derive: bool)
+                              -> Option<Attribute>;
+
     fn resolve_invoc(&mut self, invoc: &mut Invocation, scope: Mark, force: bool)
                      -> Result<Option<Lrc<SyntaxExtension>>, Determinacy>;
     fn resolve_macro(&mut self, scope: Mark, path: &ast::Path, kind: MacroKind, force: bool)
@@ -687,7 +703,8 @@ impl Resolver for DummyResolver {
     fn add_builtin(&mut self, _ident: ast::Ident, _ext: Lrc<SyntaxExtension>) {}
 
     fn resolve_imports(&mut self) {}
-    fn find_legacy_attr_invoc(&mut self, _attrs: &mut Vec<Attribute>) -> Option<Attribute> { None }
+    fn find_legacy_attr_invoc(&mut self, _attrs: &mut Vec<Attribute>, _allow_derive: bool)
+                              -> Option<Attribute> { None }
     fn resolve_invoc(&mut self, _invoc: &mut Invocation, _scope: Mark, _force: bool)
                      -> Result<Option<Lrc<SyntaxExtension>>, Determinacy> {
         Err(Determinacy::Determined)
diff --git a/src/libsyntax/ext/expand.rs b/src/libsyntax/ext/expand.rs
@@ -143,7 +143,7 @@ impl ExpansionKind {
     }
 
     fn expect_from_annotatables<I: IntoIterator<Item = Annotatable>>(self, items: I) -> Expansion {
-        let items = items.into_iter();
+        let mut items = items.into_iter();
         match self {
             ExpansionKind::Items =>
                 Expansion::Items(items.map(Annotatable::expect_item).collect()),
@@ -153,7 +153,14 @@ impl ExpansionKind {
                 Expansion::TraitItems(items.map(Annotatable::expect_trait_item).collect()),
             ExpansionKind::ForeignItems =>
                 Expansion::ForeignItems(items.map(Annotatable::expect_foreign_item).collect()),
-            _ => unreachable!(),
+            ExpansionKind::Stmts => Expansion::Stmts(items.map(Annotatable::expect_stmt).collect()),
+            ExpansionKind::Expr => Expansion::Expr(
+                items.next().expect("expected exactly one expression").expect_expr()
+            ),
+            ExpansionKind::OptExpr =>
+                Expansion::OptExpr(items.next().map(Annotatable::expect_expr)),
+            ExpansionKind::Pat | ExpansionKind::Ty =>
+                panic!("patterns and types aren't annotatable"),
         }
     }
 }
@@ -886,14 +893,15 @@ impl<'a, 'b> InvocationCollector<'a, 'b> {
         self.collect(kind, InvocationKind::Attr { attr, traits, item })
     }
 
-    // If `item` is an attr invocation, remove and return the macro attribute.
+    /// If `item` is an attr invocation, remove and return the macro attribute and derive traits.
     fn classify_item<T>(&mut self, mut item: T) -> (Option<ast::Attribute>, Vec<Path>, T)
         where T: HasAttrs,
     {
         let (mut attr, mut traits) = (None, Vec::new());
 
         item = item.map_attrs(|mut attrs| {
-            if let Some(legacy_attr_invoc) = self.cx.resolver.find_legacy_attr_invoc(&mut attrs) {
+            if let Some(legacy_attr_invoc) = self.cx.resolver.find_legacy_attr_invoc(&mut attrs,
+                                                                                     true) {
                 attr = Some(legacy_attr_invoc);
                 return attrs;
             }
@@ -908,6 +916,28 @@ impl<'a, 'b> InvocationCollector<'a, 'b> {
         (attr, traits, item)
     }
 
+    /// Alternative of `classify_item()` that ignores `#[derive]` so invocations fallthrough
+    /// to the unused-attributes lint (making it an error on statements and expressions
+    /// is a breaking change)
+    fn classify_nonitem<T: HasAttrs>(&mut self, mut item: T) -> (Option<ast::Attribute>, T) {
+        let mut attr = None;
+
+        item = item.map_attrs(|mut attrs| {
+            if let Some(legacy_attr_invoc) = self.cx.resolver.find_legacy_attr_invoc(&mut attrs,
+                                                                                     false) {
+                attr = Some(legacy_attr_invoc);
+                return attrs;
+            }
+
+            if self.cx.ecfg.proc_macro_enabled() {
+                attr = find_attr_invoc(&mut attrs);
+            }
+            attrs
+        });
+
+        (attr, item)
+    }
+
     fn configure<T: HasAttrs>(&mut self, node: T) -> Option<T> {
         self.cfg.configure(node)
     }
@@ -918,6 +948,13 @@ impl<'a, 'b> InvocationCollector<'a, 'b> {
         let features = self.cx.ecfg.features.unwrap();
         for attr in attrs.iter() {
             feature_gate::check_attribute(attr, self.cx.parse_sess, features);
+
+            // macros are expanded before any lint passes so this warning has to be hardcoded
+            if attr.path == "derive" {
+                self.cx.struct_span_warn(attr.span, "`#[derive]` does nothing on macro invocations")
+                    .note("this may become a hard error in a future release")
+                    .emit();
+            }
         }
     }
 
@@ -938,15 +975,16 @@ impl<'a, 'b> Folder for InvocationCollector<'a, 'b> {
         let mut expr = self.cfg.configure_expr(expr).into_inner();
         expr.node = self.cfg.configure_expr_kind(expr.node);
 
-        let (attr, derives, expr) = self.classify_item(expr);
+        // ignore derives so they remain unused
+        let (attr, expr) = self.classify_nonitem(expr);
 
-        if attr.is_some() || !derives.is_empty() {
+        if attr.is_some() {
             // collect the invoc regardless of whether or not attributes are permitted here
             // expansion will eat the attribute so it won't error later
             attr.as_ref().map(|a| self.cfg.maybe_emit_expr_attr_err(a));
 
             // ExpansionKind::Expr requires the macro to emit an expression
-            return self.collect_attr(attr, derives, Annotatable::Expr(P(expr)), ExpansionKind::Expr)
+            return self.collect_attr(attr, vec![], Annotatable::Expr(P(expr)), ExpansionKind::Expr)
                 .make_expr();
         }
 
@@ -962,12 +1000,13 @@ impl<'a, 'b> Folder for InvocationCollector<'a, 'b> {
         let mut expr = configure!(self, expr).into_inner();
         expr.node = self.cfg.configure_expr_kind(expr.node);
 
-        let (attr, derives, expr) = self.classify_item(expr);
+        // ignore derives so they remain unused
+        let (attr, expr) = self.classify_nonitem(expr);
 
-        if attr.is_some() || !derives.is_empty() {
+        if attr.is_some() {
             attr.as_ref().map(|a| self.cfg.maybe_emit_expr_attr_err(a));
 
-            return self.collect_attr(attr, derives, Annotatable::Expr(P(expr)),
+            return self.collect_attr(attr, vec![], Annotatable::Expr(P(expr)),
                                      ExpansionKind::OptExpr)
                 .make_opt_expr();
         }
@@ -1001,7 +1040,14 @@ impl<'a, 'b> Folder for InvocationCollector<'a, 'b> {
 
         // we'll expand attributes on expressions separately
         if !stmt.is_expr() {
-            let (attr, derives, stmt_) = self.classify_item(stmt);
+            let (attr, derives, stmt_) = if stmt.is_item() {
+                self.classify_item(stmt)
+            } else {
+                // ignore derives on non-item statements so it falls through
+                // to the unused-attributes lint
+                let (attr, stmt) = self.classify_nonitem(stmt);
+                (attr, vec![], stmt)
+            };
 
             if attr.is_some() || !derives.is_empty() {
                 return self.collect_attr(attr, derives,
diff --git a/src/test/ui/issue-49934.rs b/src/test/ui/issue-49934.rs
@@ -0,0 +1,52 @@
+// Copyright 2018 The Rust Project Developers. See the COPYRIGHT
+// file at the top-level directory of this distribution and at
+// http://rust-lang.org/COPYRIGHT.
+//
+// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
+// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
+// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
+// option. This file may not be copied, modified, or distributed
+// except according to those terms.
+
+// compile-pass
+
+#![feature(stmt_expr_attributes)]
+#![warn(unused_attributes)] //~ NOTE lint level defined here
+
+fn foo<#[derive(Debug)] T>() { //~ WARN unused attribute
+    match 0 {
+        #[derive(Debug)] //~ WARN unused attribute
+        _ => (),
+    }
+}
+
+fn main() {
+    // fold_stmt (Item)
+    #[allow(dead_code)]
+    #[derive(Debug)] // should not warn
+    struct Foo;
+
+    // fold_stmt (Mac)
+    #[derive(Debug)]
+    //~^ WARN `#[derive]` does nothing on macro invocations
+    //~| NOTE this may become a hard error in a future release
+    println!("Hello, world!");
+
+    // fold_stmt (Semi)
+    #[derive(Debug)] //~ WARN unused attribute
+    "Hello, world!";
+
+    // fold_stmt (Local)
+    #[derive(Debug)] //~ WARN unused attribute
+    let _ = "Hello, world!";
+
+    // fold_expr
+    let _ = #[derive(Debug)] "Hello, world!";
+    //~^ WARN unused attribute
+
+    let _ = [
+        // fold_opt_expr
+        #[derive(Debug)] //~ WARN unused attribute
+        "Hello, world!"
+    ];
+}
diff --git a/src/test/ui/issue-49934.stderr b/src/test/ui/issue-49934.stderr
@@ -0,0 +1,50 @@
+warning: `#[derive]` does nothing on macro invocations
+  --> $DIR/issue-49934.rs:30:5
+   |
+LL |     #[derive(Debug)]
+   |     ^^^^^^^^^^^^^^^^
+   |
+   = note: this may become a hard error in a future release
+
+warning: unused attribute
+  --> $DIR/issue-49934.rs:16:8
+   |
+LL | fn foo<#[derive(Debug)] T>() { //~ WARN unused attribute
+   |        ^^^^^^^^^^^^^^^^
+   |
+note: lint level defined here
+  --> $DIR/issue-49934.rs:14:9
+   |
+LL | #![warn(unused_attributes)] //~ NOTE lint level defined here
+   |         ^^^^^^^^^^^^^^^^^
+
+warning: unused attribute
+  --> $DIR/issue-49934.rs:18:9
+   |
+LL |         #[derive(Debug)] //~ WARN unused attribute
+   |         ^^^^^^^^^^^^^^^^
+
+warning: unused attribute
+  --> $DIR/issue-49934.rs:36:5
+   |
+LL |     #[derive(Debug)] //~ WARN unused attribute
+   |     ^^^^^^^^^^^^^^^^
+
+warning: unused attribute
+  --> $DIR/issue-49934.rs:40:5
+   |
+LL |     #[derive(Debug)] //~ WARN unused attribute
+   |     ^^^^^^^^^^^^^^^^
+
+warning: unused attribute
+  --> $DIR/issue-49934.rs:44:13
+   |
+LL |     let _ = #[derive(Debug)] "Hello, world!";
+   |             ^^^^^^^^^^^^^^^^
+
+warning: unused attribute
+  --> $DIR/issue-49934.rs:49:9
+   |
+LL |         #[derive(Debug)] //~ WARN unused attribute
+   |         ^^^^^^^^^^^^^^^^
+

Original file line number	Diff line number	Diff line change
`@@ -404,7 +404,7 @@ pub fn walk_local<'v, V: Visitor<'v>>(visitor: &mut V, local: &'v Local) {`
`404`	`404`	`// Intentionally visiting the expr first - the initialization expr`
`405`	`405`	`// dominates the local's definition.`
`406`	`406`	`walk_list!(visitor, visit_expr, &local.init);`
`407`		`-`
	`407`	`+ walk_list!(visitor, visit_attribute, local.attrs.iter());`
`408`	`408`	`visitor.visit_id(local.id);`
`409`	`409`	`visitor.visit_pat(&local.pat);`
`410`	`410`	`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`
`731`	`731`	`visitor.visit_name(ty_param.span, ty_param.name);`
`732`	`732`	`walk_list!(visitor, visit_ty_param_bound, &ty_param.bounds);`
`733`	`733`	`walk_list!(visitor, visit_ty, &ty_param.default);`
	`734`	`+ walk_list!(visitor, visit_attribute, ty_param.attrs.iter());`
`734`	`735`	`}`
`735`	`736`	`}`
`736`	`737`	`}`
Original file line number	Diff line number	Diff line change
`@@ -206,7 +206,7 @@ impl<'a> base::Resolver for Resolver<'a> {`
`206`	`206`	`}`
`207`	`207`
`208`	`208`	// 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)`
`210`	`210`	`-> Option<ast::Attribute> {`
`211`	`211`	`for i in 0..attrs.len() {`
`212`	`212`	`let name = unwrap_or!(attrs[i].name(), continue);`
`@@ -227,6 +227,8 @@ impl<'a> base::Resolver for Resolver<'a> {`
`227`	`227`	`}`
`228`	`228`	`}`
`229`	`229`
	`230`	`+ if !allow_derive { return None }`
	`231`	`+`
`230`	`232`	`// Check for legacy derives`
`231`	`233`	`for i in 0..attrs.len() {`
`232`	`234`	`let name = unwrap_or!(attrs[i].name(), continue);`
Original file line number	Diff line number	Diff line change
`@@ -821,7 +821,7 @@ impl Stmt {`
`821`	`821`
`822`	`822`	`pub fn is_item(&self) -> bool {`
`823`	`823`	`match self.node {`
`824`		`- StmtKind::Local(_) => true,`
	`824`	`+ StmtKind::Item(_) => true,`
`825`	`825`	`_ => false,`
`826`	`826`	`}`
`827`	`827`	`}`