Auto merge of #64694 - petrochenkov:reshelp, r=matthewjasper

Fully integrate derive helpers into name resolution

```rust
#[derive(Foo)]
#[foo_helper] // already goes through name resolution
struct S {
    #[foo_helper] // goes through name resolution after this PR
    field: u8
}
```
How name resolution normally works:
- We have an identifier we need to resolve, take its location (in some sense) and look what names are in scope in that location.

How derive helper attributes are "resolved" (before this PR):
- After resolving the derive `Foo` we visit the derive's input (`struct S { ... } `) as a piece of AST and mark attributes textually matching one of the derive's helper attributes (`foo_helper`) as "known", so they never go through name resolution.

This PR changes the rules for derive helpers, so they are not proactively marked as known (which is a big hack ignoring any ambiguities or hygiene), but go through regular name resolution instead.
This change was previously blocked by attributes not being resolved in some positions at all (fixed in #63468).

"Where exactly are derive helpers in scope?" is an interesting question, and I need some feedback from proc macro authors to answer it, see the post below (#64694 (comment)).

src/librustc_resolve/diagnostics.rs

@@ -368,7 +368,16 @@ impl<'a> Resolver<'a> {
         let mut suggestions = Vec::new();
         self.visit_scopes(scope_set, parent_scope, ident, |this, scope, use_prelude, _| {
             match scope {
-                Scope::DeriveHelpers => {
+                Scope::DeriveHelpers(expn_id) => {
+                    let res = Res::NonMacroAttr(NonMacroAttrKind::DeriveHelper);
+                    if filter_fn(res) {
+                        suggestions.extend(this.helper_attrs.get(&expn_id)
+                                               .into_iter().flatten().map(|ident| {
+                            TypoSuggestion::from_res(ident.name, res)
+                        }));
+                    }
+                }
+                Scope::DeriveHelpersCompat => {
                     let res = Res::NonMacroAttr(NonMacroAttrKind::DeriveHelper);
                     if filter_fn(res) {
                         for derive in parent_scope.derives {

src/librustc_resolve/lib.rs

@@ -45,7 +45,7 @@ use syntax::symbol::{kw, sym};
 use syntax::source_map::Spanned;
 use syntax::visit::{self, Visitor};
 use syntax_expand::base::SyntaxExtension;
-use syntax_pos::hygiene::{MacroKind, ExpnId, Transparency, SyntaxContext};
+use syntax_pos::hygiene::{MacroKind, ExpnId, ExpnKind, Transparency, SyntaxContext};
 use syntax_pos::{Span, DUMMY_SP};
 use errors::{Applicability, DiagnosticBuilder};
 
@@ -97,7 +97,8 @@ impl Determinacy {
 /// but not for late resolution yet.
 #[derive(Clone, Copy)]
 enum Scope<'a> {
-    DeriveHelpers,
+    DeriveHelpers(ExpnId),
+    DeriveHelpersCompat,
     MacroRules(LegacyScope<'a>),
     CrateRoot,
     Module(Module<'a>),
@@ -942,6 +943,8 @@ pub struct Resolver<'a> {
     /// Legacy scopes *produced* by expanding the macro invocations,
     /// include all the `macro_rules` items and other invocations generated by them.
     output_legacy_scopes: FxHashMap<ExpnId, LegacyScope<'a>>,
+    /// Helper attributes that are in scope for the given expansion.
+    helper_attrs: FxHashMap<ExpnId, Vec<Ident>>,
 
     /// Avoid duplicated errors for "name already defined".
     name_already_seen: FxHashMap<Name, Span>,
@@ -1219,6 +1222,7 @@ impl<'a> Resolver<'a> {
             non_macro_attrs: [non_macro_attr(false), non_macro_attr(true)],
             invocation_parent_scopes,
             output_legacy_scopes: Default::default(),
+            helper_attrs: Default::default(),
             macro_defs,
             local_macro_def_scopes: FxHashMap::default(),
             name_already_seen: FxHashMap::default(),
@@ -1467,24 +1471,27 @@ impl<'a> Resolver<'a> {
         //    in prelude, not sure where exactly (creates ambiguities with any other prelude names).
 
         let rust_2015 = ident.span.rust_2015();
-        let (ns, is_absolute_path) = match scope_set {
-            ScopeSet::All(ns, _) => (ns, false),
-            ScopeSet::AbsolutePath(ns) => (ns, true),
-            ScopeSet::Macro(_) => (MacroNS, false),
+        let (ns, macro_kind, is_absolute_path) = match scope_set {
+            ScopeSet::All(ns, _) => (ns, None, false),
+            ScopeSet::AbsolutePath(ns) => (ns, None, true),
+            ScopeSet::Macro(macro_kind) => (MacroNS, Some(macro_kind), false),
         };
         // Jump out of trait or enum modules, they do not act as scopes.
         let module = parent_scope.module.nearest_item_scope();
         let mut scope = match ns {
             _ if is_absolute_path => Scope::CrateRoot,
             TypeNS | ValueNS => Scope::Module(module),
-            MacroNS => Scope::DeriveHelpers,
+            MacroNS => Scope::DeriveHelpers(parent_scope.expansion),
         };
         let mut ident = ident.modern();
         let mut use_prelude = !module.no_implicit_prelude;
 
         loop {
             let visit = match scope {
-                Scope::DeriveHelpers => true,
+                // Derive helpers are not in scope when resolving derives in the same container.
+                Scope::DeriveHelpers(expn_id) =>
+                    !(expn_id == parent_scope.expansion && macro_kind == Some(MacroKind::Derive)),
+                Scope::DeriveHelpersCompat => true,
                 Scope::MacroRules(..) => true,
                 Scope::CrateRoot => true,
                 Scope::Module(..) => true,
@@ -1505,7 +1512,18 @@ impl<'a> Resolver<'a> {
             }
 
             scope = match scope {
-                Scope::DeriveHelpers =>
+                Scope::DeriveHelpers(expn_id) if expn_id != ExpnId::root() => {
+                    // Derive helpers are not visible to code generated by bang or derive macros.
+                    let expn_data = expn_id.expn_data();
+                    match expn_data.kind {
+                        ExpnKind::Root |
+                        ExpnKind::Macro(MacroKind::Bang, _) |
+                        ExpnKind::Macro(MacroKind::Derive, _) => Scope::DeriveHelpersCompat,
+                        _ => Scope::DeriveHelpers(expn_data.parent),
+                    }
+                }
+                Scope::DeriveHelpers(..) => Scope::DeriveHelpersCompat,
+                Scope::DeriveHelpersCompat =>
                     Scope::MacroRules(parent_scope.legacy),
                 Scope::MacroRules(legacy_scope) => match legacy_scope {
                     LegacyScope::Binding(binding) => Scope::MacroRules(

src/librustc_resolve/macros.rs

@@ -237,15 +237,26 @@ impl<'a> base::Resolver for Resolver<'a> {
                 // - Derives in the container need to know whether one of them is a built-in `Copy`.
                 // FIXME: Try to avoid repeated resolutions for derives here and in expansion.
                 let mut exts = Vec::new();
+                let mut helper_attrs = Vec::new();
                 for path in derives {
                     exts.push(match self.resolve_macro_path(
                         path, Some(MacroKind::Derive), &parent_scope, true, force
                     ) {
-                        Ok((Some(ext), _)) => ext,
+                        Ok((Some(ext), _)) => {
+                            let span = path.segments.last().unwrap().ident.span.modern();
+                            helper_attrs.extend(
+                                ext.helper_attrs.iter().map(|name| Ident::new(*name, span))
+                            );
+                            if ext.is_derive_copy {
+                                self.add_derive_copy(invoc_id);
+                            }
+                            ext
+                        }
                         Ok(_) | Err(Determinacy::Determined) => self.dummy_ext(MacroKind::Derive),
                         Err(Determinacy::Undetermined) => return Err(Indeterminate),
                     })
                 }
+                self.helper_attrs.insert(invoc_id, helper_attrs);
                 return Ok(InvocationRes::DeriveContainer(exts));
             }
         };
@@ -498,7 +509,19 @@ impl<'a> Resolver<'a> {
                 Flags::empty(),
             ));
             let result = match scope {
-                Scope::DeriveHelpers => {
+                Scope::DeriveHelpers(expn_id) => {
+                    if let Some(attr) = this.helper_attrs.get(&expn_id).and_then(|attrs| {
+                        attrs.iter().rfind(|i| ident == **i)
+                    }) {
+                        let binding = (Res::NonMacroAttr(NonMacroAttrKind::DeriveHelper),
+                                       ty::Visibility::Public, attr.span, expn_id)
+                                       .to_name_binding(this.arenas);
+                        Ok((binding, Flags::empty()))
+                    } else {
+                        Err(Determinacy::Determined)
+                    }
+                }
+                Scope::DeriveHelpersCompat => {
                     let mut result = Err(Determinacy::Determined);
                     for derive in parent_scope.derives {
                         let parent_scope = &ParentScope { derives: &[], ..*parent_scope };

src/libsyntax_expand/expand.rs

@@ -1,5 +1,5 @@
 use crate::base::*;
-use crate::proc_macro::{collect_derives, MarkAttrs};
+use crate::proc_macro::collect_derives;
 use crate::hygiene::{ExpnId, SyntaxContext, ExpnData, ExpnKind};
 use crate::mbe::macro_rules::annotate_err_with_kind;
 use crate::placeholders::{placeholder, PlaceholderExpander};
@@ -394,7 +394,9 @@ impl<'a, 'b> MacroExpander<'a, 'b> {
                     let fragment = self.expand_invoc(invoc, &ext.kind);
                     self.collect_invocations(fragment, &[])
                 }
-                InvocationRes::DeriveContainer(exts) => {
+                InvocationRes::DeriveContainer(_exts) => {
+                    // FIXME: Consider using the derive resolutions (`_exts`) immediately,
+                    // instead of enqueuing the derives to be resolved again later.
                     let (derives, item) = match invoc.kind {
                         InvocationKind::DeriveContainer { derives, item } => (derives, item),
                         _ => unreachable!(),
@@ -421,20 +423,6 @@ impl<'a, 'b> MacroExpander<'a, 'b> {
 
                     let mut item = self.fully_configure(item);
                     item.visit_attrs(|attrs| attrs.retain(|a| !a.has_name(sym::derive)));
-                    let mut helper_attrs = Vec::new();
-                    let mut has_copy = false;
-                    for ext in exts {
-                        helper_attrs.extend(&ext.helper_attrs);
-                        has_copy |= ext.is_derive_copy;
-                    }
-                    // Mark derive helpers inside this item as known and used.
-                    // FIXME: This is a hack, derive helpers should be integrated with regular name
-                    // resolution instead. For example, helpers introduced by a derive container
-                    // can be in scope for all code produced by that container's expansion.
-                    item.visit_with(&mut MarkAttrs(&helper_attrs));
-                    if has_copy {
-                        self.cx.resolver.add_derive_copy(invoc.expansion_data.id);
-                    }
 
                     let mut derive_placeholders = Vec::with_capacity(derives.len());
                     invocations.reserve(derives.len());

src/libsyntax_expand/proc_macro.rs

@@ -1,13 +1,11 @@
 use crate::base::{self, *};
 use crate::proc_macro_server;
 
-use syntax::ast::{self, ItemKind, Attribute, Mac};
-use syntax::attr::{mark_used, mark_known};
+use syntax::ast::{self, ItemKind};
 use syntax::errors::{Applicability, FatalError};
 use syntax::symbol::sym;
 use syntax::token;
 use syntax::tokenstream::{self, TokenStream};
-use syntax::visit::Visitor;
 
 use rustc_data_structures::sync::Lrc;
 use syntax_pos::{Span, DUMMY_SP};
@@ -167,21 +165,6 @@ impl MultiItemModifier for ProcMacroDerive {
     }
 }
 
-crate struct MarkAttrs<'a>(crate &'a [ast::Name]);
-
-impl<'a> Visitor<'a> for MarkAttrs<'a> {
-    fn visit_attribute(&mut self, attr: &Attribute) {
-        if let Some(ident) = attr.ident() {
-            if self.0.contains(&ident.name) {
-                mark_used(attr);
-                mark_known(attr);
-            }
-        }
-    }
-
-    fn visit_mac(&mut self, _mac: &Mac) {}
-}
-
 crate fn collect_derives(cx: &mut ExtCtxt<'_>, attrs: &mut Vec<ast::Attribute>) -> Vec<ast::Path> {
     let mut result = Vec::new();
     attrs.retain(|attr| {

src/test/ui/proc-macro/auxiliary/derive-helper-shadowing.rs

@@ -0,0 +1,15 @@
+// force-host
+// no-prefer-dynamic
+
+#![crate_type = "proc-macro"]
+
+extern crate proc_macro;
+use proc_macro::*;
+
+#[proc_macro_derive(GenHelperUse)]
+pub fn derive_a(_: TokenStream) -> TokenStream {
+    "
+    #[empty_helper]
+    struct Uwu;
+    ".parse().unwrap()
+}

src/test/ui/proc-macro/derive-helper-shadowing.rs

@@ -1,33 +1,54 @@
+// edition:2018
 // aux-build:test-macros.rs
+// aux-build:derive-helper-shadowing.rs
 
 #[macro_use]
 extern crate test_macros;
+#[macro_use]
+extern crate derive_helper_shadowing;
 
 use test_macros::empty_attr as empty_helper;
 
+macro_rules! gen_helper_use {
+    () => {
+        #[empty_helper] //~ ERROR cannot find attribute `empty_helper` in this scope
+        struct W;
+    }
+}
+
 #[empty_helper] //~ ERROR `empty_helper` is ambiguous
 #[derive(Empty)]
 struct S {
-    // FIXME No ambiguity, attributes in non-macro positions are not resolved properly
-    #[empty_helper]
+    #[empty_helper] //~ ERROR `empty_helper` is ambiguous
     field: [u8; {
-        // FIXME No ambiguity, derive helpers are not put into scope for non-attributes
-        use empty_helper;
+        use empty_helper; //~ ERROR `empty_helper` is ambiguous
 
-        // FIXME No ambiguity, derive helpers are not put into scope for inner items
-        #[empty_helper]
+        #[empty_helper] //~ ERROR `empty_helper` is ambiguous
         struct U;
 
         mod inner {
-            // FIXME No ambiguity, attributes in non-macro positions are not resolved properly
+            // OK, no ambiguity, the non-helper attribute is not in scope here, only the helper.
             #[empty_helper]
             struct V;
+
+            gen_helper_use!();
+
+            #[derive(GenHelperUse)] //~ ERROR cannot find attribute `empty_helper` in this scope
+            struct Owo;
+
+            use empty_helper as renamed;
+            #[renamed] //~ ERROR cannot use a derive helper attribute through an import
+            struct Wow;
         }
 
         0
     }]
 }
 
+// OK, no ambiguity, only the non-helper attribute is in scope.
+#[empty_helper]
+struct Z;
+
 fn main() {
     let s = S { field: [] };
 }