Auto merge of rust-lang#44055 - zackmdavis:condensed_non-ADT_derive_error, r=jseyfried

only set non-ADT derive error once per attribute, not per trait

I found the expansion code very hard to follow, leaving me unsure as to whether this might somehow be done better, but this patch does give us the behavior requested in rust-lang#43927 (up to exact choice of span; here, it's the entire attribute, not just the `derive` token).

(Note to GitHub robots: _resolves #43927_.)

r? @jseyfried

Author: bors

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);
     }

src/libsyntax/ext/expand.rs

@@ -282,6 +282,32 @@ 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 attr = item.attrs().iter()
+                            .find(|attr| attr.check_name("derive"))
+                            .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::<Vec<_>>();
+                            let suggestion = format!("#[derive({})]", trait_list.join(", "));
+                            err.span_suggestion(span, "try an outer attribute", suggestion);
+                        }
+                        err.emit();
+                    }
+
                     let item = item
                         .map_attrs(|mut attrs| { attrs.retain(|a| a.path != "derive"); attrs });
                     let item_with_markers =

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;
             }
         }
     }

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 { }
 }

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

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 <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.
+
+#![allow(dead_code)]
+
+#![derive(Debug, PartialEq, Eq)] // should be an outer attribute!
+struct DerivedOn;
+
+fn main() {}

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!
+   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try an outer attribute: `#[derive(Debug, PartialEq, Eq)]`
+
+error: aborting due to previous error
+