From 2899344087186205e3221dd55225488c680ad07e Mon Sep 17 00:00:00 2001 From: Austin Bonander <austin.bonander@gmail.com> Date: Tue, 17 Apr 2018 23:19:21 -0700 Subject: [PATCH 1/4] 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 --- src/librustc/hir/intravisit.rs | 3 +- src/librustc_resolve/macros.rs | 4 +- src/libsyntax/ast.rs | 2 +- src/libsyntax/ext/base.rs | 28 +++++++++++++- src/libsyntax/ext/expand.rs | 70 ++++++++++++++++++++++++++++------ src/test/ui/issue-49934.rs | 52 +++++++++++++++++++++++++ src/test/ui/issue-49934.stderr | 50 ++++++++++++++++++++++++ 7 files changed, 193 insertions(+), 16 deletions(-) create mode 100644 src/test/ui/issue-49934.rs create mode 100644 src/test/ui/issue-49934.stderr diff --git a/src/librustc/hir/intravisit.rs b/src/librustc/hir/intravisit.rs index 9f51eb8c35a82..ac2be5cd6df33 100644 --- 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); @@ -730,6 +730,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 index 0692a1e0d7f8a..7b5adb6871cac 100644 --- a/src/librustc_resolve/macros.rs +++ b/src/librustc_resolve/macros.rs @@ -207,7 +207,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); @@ -228,6 +228,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 index c90b0aecfc044..99f5e87d528ca 100644 --- a/src/libsyntax/ast.rs +++ b/src/libsyntax/ast.rs @@ -833,7 +833,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 index d3157af984e80..204b0922e5967 100644 --- a/src/libsyntax/ext/base.rs +++ b/src/libsyntax/ext/base.rs @@ -106,6 +106,27 @@ impl Annotatable { } } + pub fn expect_foreign_item(self) -> ast::ForeignItem { + match self { + Annotatable::ForeignItem(i) => i.into_inner(), + _ => panic!("expected foreign item") + } + } + + 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 { @@ -631,7 +652,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) @@ -657,7 +680,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 index 864969c40750b..0fba0c0337116 100644 --- a/src/libsyntax/ext/expand.rs +++ b/src/libsyntax/ext/expand.rs @@ -141,7 +141,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()), @@ -149,7 +149,16 @@ impl ExpansionKind { Expansion::ImplItems(items.map(Annotatable::expect_impl_item).collect()), ExpansionKind::TraitItems => Expansion::TraitItems(items.map(Annotatable::expect_trait_item).collect()), - _ => unreachable!(), + ExpansionKind::ForeignItems => + Expansion::ForeignItems(items.map(Annotatable::expect_foreign_item).collect()), + 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"), } } } @@ -867,14 +876,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; } @@ -889,6 +899,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) } @@ -899,6 +931,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(); + } } } @@ -919,15 +958,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(); } @@ -943,12 +983,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(); } @@ -982,7 +1023,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 new file mode 100644 index 0000000000000..3e30e7a6450fc --- /dev/null +++ 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 new file mode 100644 index 0000000000000..298230b8b29f7 --- /dev/null +++ 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 + | ^^^^^^^^^^^^^^^^ + From 214fd33b2f8712c3be7266a2a6c9b24e9a1b9d48 Mon Sep 17 00:00:00 2001 From: Shotaro Yamada <sinkuu@sinkuu.xyz> Date: Mon, 23 Apr 2018 13:54:09 +0900 Subject: [PATCH 2/4] Do not ICE on generics mismatch with non-local traits Fixes #49841 --- src/librustc_typeck/check/compare_method.rs | 3 +-- .../compile-fail/impl-trait/impl-generic-mismatch.rs | 11 +++++++++++ 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/src/librustc_typeck/check/compare_method.rs b/src/librustc_typeck/check/compare_method.rs index e1e3dea9a2a11..2003e0cc3d28f 100644 --- a/src/librustc_typeck/check/compare_method.rs +++ b/src/librustc_typeck/check/compare_method.rs @@ -730,8 +730,7 @@ fn compare_synthetic_generics<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>, if impl_ty.synthetic != trait_ty.synthetic { let impl_node_id = tcx.hir.as_local_node_id(impl_ty.def_id).unwrap(); let impl_span = tcx.hir.span(impl_node_id); - let trait_node_id = tcx.hir.as_local_node_id(trait_ty.def_id).unwrap(); - let trait_span = tcx.hir.span(trait_node_id); + let trait_span = tcx.def_span(trait_ty.def_id); let mut err = struct_span_err!(tcx.sess, impl_span, E0643, diff --git a/src/test/compile-fail/impl-trait/impl-generic-mismatch.rs b/src/test/compile-fail/impl-trait/impl-generic-mismatch.rs index eea7ca2095780..d6707f590113c 100644 --- a/src/test/compile-fail/impl-trait/impl-generic-mismatch.rs +++ b/src/test/compile-fail/impl-trait/impl-generic-mismatch.rs @@ -28,4 +28,15 @@ impl Bar for () { //~^ Error method `bar` has incompatible signature for trait } +// With non-local trait (#49841): + +use std::hash::{Hash, Hasher}; + +struct X; + +impl Hash for X { + fn hash(&self, hasher: &mut impl Hasher) {} + //~^ Error method `hash` has incompatible signature for trait +} + fn main() {} From dacebb8a0044a83b6319e9ae60ab0359f6fd3abb Mon Sep 17 00:00:00 2001 From: Pietro Albini <pietro@pietroalbini.org> Date: Mon, 30 Apr 2018 09:29:35 +0200 Subject: [PATCH 3/4] Remove new enum variant from #50092 backport ExpansionKind::ForeignItems was added in #49350, which is not included in the 1.26 beta. --- src/libsyntax/ext/base.rs | 7 ------- src/libsyntax/ext/expand.rs | 2 -- 2 files changed, 9 deletions(-) diff --git a/src/libsyntax/ext/base.rs b/src/libsyntax/ext/base.rs index 204b0922e5967..50eafe60148a8 100644 --- a/src/libsyntax/ext/base.rs +++ b/src/libsyntax/ext/base.rs @@ -106,13 +106,6 @@ impl Annotatable { } } - pub fn expect_foreign_item(self) -> ast::ForeignItem { - match self { - Annotatable::ForeignItem(i) => i.into_inner(), - _ => panic!("expected foreign item") - } - } - pub fn expect_stmt(self) -> ast::Stmt { match self { Annotatable::Stmt(stmt) => stmt.into_inner(), diff --git a/src/libsyntax/ext/expand.rs b/src/libsyntax/ext/expand.rs index 0fba0c0337116..cad51f06e0955 100644 --- a/src/libsyntax/ext/expand.rs +++ b/src/libsyntax/ext/expand.rs @@ -149,8 +149,6 @@ impl ExpansionKind { Expansion::ImplItems(items.map(Annotatable::expect_impl_item).collect()), ExpansionKind::TraitItems => Expansion::TraitItems(items.map(Annotatable::expect_trait_item).collect()), - ExpansionKind::ForeignItems => - Expansion::ForeignItems(items.map(Annotatable::expect_foreign_item).collect()), ExpansionKind::Stmts => Expansion::Stmts(items.map(Annotatable::expect_stmt).collect()), ExpansionKind::Expr => Expansion::Expr( items.next().expect("expected exactly one expression").expect_expr() From 81c564ecc40ebcb354e7fac07c0e513911fa9e40 Mon Sep 17 00:00:00 2001 From: Pietro Albini <pietro@pietroalbini.org> Date: Mon, 30 Apr 2018 10:00:30 +0200 Subject: [PATCH 4/4] Adapt ui test of #50092 to beta * Changed `// compile-pass` to `// must-compile-successfully` * Removed checks on unstable features --- src/test/ui/issue-49934.rs | 9 ++------- src/test/ui/issue-49934.stderr | 28 ++++++++-------------------- 2 files changed, 10 insertions(+), 27 deletions(-) diff --git a/src/test/ui/issue-49934.rs b/src/test/ui/issue-49934.rs index 3e30e7a6450fc..edf6fff588801 100644 --- a/src/test/ui/issue-49934.rs +++ b/src/test/ui/issue-49934.rs @@ -8,12 +8,11 @@ // option. This file may not be copied, modified, or distributed // except according to those terms. -// compile-pass +// must-compile-successfully -#![feature(stmt_expr_attributes)] #![warn(unused_attributes)] //~ NOTE lint level defined here -fn foo<#[derive(Debug)] T>() { //~ WARN unused attribute +fn foo() { match 0 { #[derive(Debug)] //~ WARN unused attribute _ => (), @@ -40,10 +39,6 @@ fn main() { #[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 diff --git a/src/test/ui/issue-49934.stderr b/src/test/ui/issue-49934.stderr index 298230b8b29f7..df39162cce909 100644 --- a/src/test/ui/issue-49934.stderr +++ b/src/test/ui/issue-49934.stderr @@ -1,5 +1,5 @@ warning: `#[derive]` does nothing on macro invocations - --> $DIR/issue-49934.rs:30:5 + --> $DIR/issue-49934.rs:29:5 | LL | #[derive(Debug)] | ^^^^^^^^^^^^^^^^ @@ -7,43 +7,31 @@ LL | #[derive(Debug)] = note: this may become a hard error in a future release warning: unused attribute - --> $DIR/issue-49934.rs:16:8 + --> $DIR/issue-49934.rs:17:9 | -LL | fn foo<#[derive(Debug)] T>() { //~ WARN unused attribute - | ^^^^^^^^^^^^^^^^ +LL | #[derive(Debug)] //~ WARN unused attribute + | ^^^^^^^^^^^^^^^^ | note: lint level defined here - --> $DIR/issue-49934.rs:14:9 + --> $DIR/issue-49934.rs:13: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 + --> $DIR/issue-49934.rs:35:5 | LL | #[derive(Debug)] //~ WARN unused attribute | ^^^^^^^^^^^^^^^^ warning: unused attribute - --> $DIR/issue-49934.rs:40:5 + --> $DIR/issue-49934.rs:39: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 + --> $DIR/issue-49934.rs:44:9 | LL | #[derive(Debug)] //~ WARN unused attribute | ^^^^^^^^^^^^^^^^