From 35176867f62f76b9bc27267878f2d74d9c776221 Mon Sep 17 00:00:00 2001 From: "Zack M. Davis" Date: Tue, 22 Aug 2017 19:22:52 -0700 Subject: [PATCH 1/2] only set non-ADT derive error once per attribute, not per trait A slight eccentricity of this change is that now non-ADT-derive errors prevent derive-macro-not-found errors from surfacing (see changes to the gating-of-derive compile-fail tests). Resolves #43927. --- src/libsyntax/ext/expand.rs | 18 ++++++++++++++ src/libsyntax_ext/deriving/generic/mod.rs | 11 +++++---- .../issue-43106-gating-of-derive-2.rs | 24 +------------------ .../issue-43106-gating-of-derive.rs | 5 ++-- .../ui/span/issue-43927-non-ADT-derive.rs | 16 +++++++++++++ .../ui/span/issue-43927-non-ADT-derive.stderr | 8 +++++++ 6 files changed, 52 insertions(+), 30 deletions(-) create mode 100644 src/test/ui/span/issue-43927-non-ADT-derive.rs create mode 100644 src/test/ui/span/issue-43927-non-ADT-derive.stderr diff --git a/src/libsyntax/ext/expand.rs b/src/libsyntax/ext/expand.rs index de9c085cc78..3a1b9342530 100644 --- a/src/libsyntax/ext/expand.rs +++ b/src/libsyntax/ext/expand.rs @@ -282,6 +282,24 @@ impl<'a, 'b> MacroExpander<'a, 'b> { let expansion = self.expand_invoc(invoc, ext); self.collect_invocations(expansion, &[]) } else if let InvocationKind::Attr { attr: None, traits, item } = invoc.kind { + let derive_allowed = match item { + Annotatable::Item(ref item) => match item.node { + ast::ItemKind::Struct(..) | + ast::ItemKind::Enum(..) | + ast::ItemKind::Union(..) => true, + _ => false, + }, + _ => false, + }; + if !derive_allowed { + let span = item.attrs().iter() + .find(|attr| attr.check_name("derive")) + .expect("`derive` attribute should exist").span; + self.cx.span_err(span, + "`derive` may only be applied to structs, enums \ + and unions"); + } + let item = item .map_attrs(|mut attrs| { attrs.retain(|a| a.path != "derive"); attrs }); let item_with_markers = diff --git a/src/libsyntax_ext/deriving/generic/mod.rs b/src/libsyntax_ext/deriving/generic/mod.rs index d701810e2e9..5c1ca19d635 100644 --- a/src/libsyntax_ext/deriving/generic/mod.rs +++ b/src/libsyntax_ext/deriving/generic/mod.rs @@ -428,8 +428,9 @@ impl<'a> TraitDef<'a> { } } _ => { - cx.span_err(mitem.span, - "`derive` may only be applied to structs, enums and unions"); + // Non-ADT derive is an error, but it should have been + // set earlier; see + // libsyntax/ext/expand.rs:MacroExpander::expand() return; } }; @@ -448,8 +449,10 @@ impl<'a> TraitDef<'a> { push(Annotatable::Item(P(ast::Item { attrs: attrs, ..(*newitem).clone() }))) } _ => { - cx.span_err(mitem.span, - "`derive` may only be applied to structs and enums"); + // Non-Item derive is an error, but it should have been + // set earlier; see + // libsyntax/ext/expand.rs:MacroExpander::expand() + return; } } } diff --git a/src/test/compile-fail/feature-gate/issue-43106-gating-of-derive-2.rs b/src/test/compile-fail/feature-gate/issue-43106-gating-of-derive-2.rs index be82d0a5f6d..2dbc6cb140d 100644 --- a/src/test/compile-fail/feature-gate/issue-43106-gating-of-derive-2.rs +++ b/src/test/compile-fail/feature-gate/issue-43106-gating-of-derive-2.rs @@ -8,23 +8,9 @@ // option. This file may not be copied, modified, or distributed // except according to those terms. -// `#![derive]` is interpreted (and raises errors) when it occurs at -// contexts other than ADT definitions. This test checks cases where -// the derive-macro does not exist. +// This test checks cases where the derive-macro does not exist. -#![derive(x3300)] -//~^ ERROR cannot find derive macro `x3300` in this scope - -#[derive(x3300)] -//~^ ERROR cannot find derive macro `x3300` in this scope mod derive { - mod inner { #![derive(x3300)] } - //~^ ERROR cannot find derive macro `x3300` in this scope - - #[derive(x3300)] - //~^ ERROR cannot find derive macro `x3300` in this scope - fn derive() { } - #[derive(x3300)] //~^ ERROR cannot find derive macro `x3300` in this scope union U { f: i32 } @@ -36,12 +22,4 @@ mod derive { #[derive(x3300)] //~^ ERROR cannot find derive macro `x3300` in this scope struct S; - - #[derive(x3300)] - //~^ ERROR cannot find derive macro `x3300` in this scope - type T = S; - - #[derive(x3300)] - //~^ ERROR cannot find derive macro `x3300` in this scope - impl S { } } diff --git a/src/test/compile-fail/feature-gate/issue-43106-gating-of-derive.rs b/src/test/compile-fail/feature-gate/issue-43106-gating-of-derive.rs index 41c3d0ef561..e5293ebb94d 100644 --- a/src/test/compile-fail/feature-gate/issue-43106-gating-of-derive.rs +++ b/src/test/compile-fail/feature-gate/issue-43106-gating-of-derive.rs @@ -8,9 +8,8 @@ // option. This file may not be copied, modified, or distributed // except according to those terms. -// `#![derive]` is interpreted (and raises errors) when it occurs at -// contexts other than ADT definitions. This test checks cases where -// the derive-macro exists. +// `#![derive]` raises errors when it occurs at contexts other than ADT +// definitions. #![derive(Debug)] //~^ ERROR `derive` may only be applied to structs, enums and unions diff --git a/src/test/ui/span/issue-43927-non-ADT-derive.rs b/src/test/ui/span/issue-43927-non-ADT-derive.rs new file mode 100644 index 00000000000..cf2a4b8d037 --- /dev/null +++ b/src/test/ui/span/issue-43927-non-ADT-derive.rs @@ -0,0 +1,16 @@ +// Copyright 2017 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 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +#![allow(dead_code)] + +#![derive(Debug, PartialEq, Eq)] // should be an outer attribute! +struct DerivedOn; + +fn main() {} diff --git a/src/test/ui/span/issue-43927-non-ADT-derive.stderr b/src/test/ui/span/issue-43927-non-ADT-derive.stderr new file mode 100644 index 00000000000..4cfbb6d6af0 --- /dev/null +++ b/src/test/ui/span/issue-43927-non-ADT-derive.stderr @@ -0,0 +1,8 @@ +error: `derive` may only be applied to structs, enums and unions + --> $DIR/issue-43927-non-ADT-derive.rs:13:1 + | +13 | #![derive(Debug, PartialEq, Eq)] // should be an outer attribute! + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: aborting due to previous error + From 083f053294b062e12bacfea17a07f83e1b4c3732 Mon Sep 17 00:00:00 2001 From: "Zack M. Davis" Date: Thu, 21 Sep 2017 20:29:29 -0700 Subject: [PATCH 2/2] suggest an outer attribute when `#![derive(...)]` (predictably) fails --- src/libsyntax/ext/base.rs | 4 ++++ src/libsyntax/ext/expand.rs | 18 +++++++++++++----- .../ui/span/issue-43927-non-ADT-derive.stderr | 2 +- 3 files changed, 18 insertions(+), 6 deletions(-) diff --git a/src/libsyntax/ext/base.rs b/src/libsyntax/ext/base.rs index c139cfeaebf..0e05cce35e2 100644 --- a/src/libsyntax/ext/base.rs +++ b/src/libsyntax/ext/base.rs @@ -783,6 +783,10 @@ impl<'a> ExtCtxt<'a> { pub fn span_err(&self, sp: Span, msg: &str) { self.parse_sess.span_diagnostic.span_err(sp, msg); } + pub fn mut_span_err(&self, sp: Span, msg: &str) + -> DiagnosticBuilder<'a> { + self.parse_sess.span_diagnostic.mut_span_err(sp, msg) + } pub fn span_warn(&self, sp: Span, msg: &str) { self.parse_sess.span_diagnostic.span_warn(sp, msg); } diff --git a/src/libsyntax/ext/expand.rs b/src/libsyntax/ext/expand.rs index 3a1b9342530..5deb4c3cc00 100644 --- a/src/libsyntax/ext/expand.rs +++ b/src/libsyntax/ext/expand.rs @@ -292,12 +292,20 @@ impl<'a, 'b> MacroExpander<'a, 'b> { _ => false, }; if !derive_allowed { - let span = item.attrs().iter() + let attr = item.attrs().iter() .find(|attr| attr.check_name("derive")) - .expect("`derive` attribute should exist").span; - self.cx.span_err(span, - "`derive` may only be applied to structs, enums \ - and unions"); + .expect("`derive` attribute should exist"); + let span = attr.span; + let mut err = self.cx.mut_span_err(span, + "`derive` may only be applied to \ + structs, enums and unions"); + if let ast::AttrStyle::Inner = attr.style { + let trait_list = traits.iter() + .map(|t| format!("{}", t)).collect::>(); + let suggestion = format!("#[derive({})]", trait_list.join(", ")); + err.span_suggestion(span, "try an outer attribute", suggestion); + } + err.emit(); } let item = item diff --git a/src/test/ui/span/issue-43927-non-ADT-derive.stderr b/src/test/ui/span/issue-43927-non-ADT-derive.stderr index 4cfbb6d6af0..a0485bed2f4 100644 --- a/src/test/ui/span/issue-43927-non-ADT-derive.stderr +++ b/src/test/ui/span/issue-43927-non-ADT-derive.stderr @@ -2,7 +2,7 @@ error: `derive` may only be applied to structs, enums and unions --> $DIR/issue-43927-non-ADT-derive.rs:13:1 | 13 | #![derive(Debug, PartialEq, Eq)] // should be an outer attribute! - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try an outer attribute: `#[derive(Debug, PartialEq, Eq)]` error: aborting due to previous error