Rollup merge of #63667 - petrochenkov:deriveholders, r=matthewjasper

resolve: Properly integrate derives and `macro_rules` scopes

So,
```rust
#[derive(A, B)]
struct S;

m!();
```
turns into something like
```rust
struct S;

A_placeholder!( struct S; );

B_placeholder!( struct S; );

m!();
```
during expansion.

And for `m!()` its "`macro_rules` scope" (aka "legacy scope") should point to the `B_placeholder` call rather than to the derive container `#[derive(A, B)]`.

`fn build_reduced_graph` now makes sure the legacy scope points to the right thing.
(It's still a mystery for me why this worked before https://github.com/rust-lang/rust/pull/63535.)

Unfortunately, placeholders from derives are currently treated separately from placeholders from other macros and need to be passed as `extra_placeholders` rather than a part of the AST fragment.
That's fixable, but I wanted to keep this PR more minimal to close the regression faster.

Fixes https://github.com/rust-lang/rust/issues/63651
r? @matthewjasper

src/librustc/hir/map/def_collector.rs

@@ -74,7 +74,7 @@ impl<'a> DefCollector<'a> {
         })
     }
 
-    fn visit_macro_invoc(&mut self, id: NodeId) {
+    pub fn visit_macro_invoc(&mut self, id: NodeId) {
         self.definitions.set_invocation_parent(id.placeholder_to_expn_id(), self.parent_def);
     }
 }

src/librustc_resolve/build_reduced_graph.rs

@@ -160,12 +160,25 @@ impl<'a> Resolver<'a> {
         Some(ext)
     }
 
+    // FIXME: `extra_placeholders` should be included into the `fragment` as regular placeholders.
     crate fn build_reduced_graph(
-        &mut self, fragment: &AstFragment, parent_scope: ParentScope<'a>
+        &mut self,
+        fragment: &AstFragment,
+        extra_placeholders: &[NodeId],
+        parent_scope: ParentScope<'a>,
     ) -> LegacyScope<'a> {
-        fragment.visit_with(&mut DefCollector::new(&mut self.definitions, parent_scope.expansion));
+        let mut def_collector = DefCollector::new(&mut self.definitions, parent_scope.expansion);
+        fragment.visit_with(&mut def_collector);
+        for placeholder in extra_placeholders {
+            def_collector.visit_macro_invoc(*placeholder);
+        }
+
         let mut visitor = BuildReducedGraphVisitor { r: self, parent_scope };
         fragment.visit_with(&mut visitor);
+        for placeholder in extra_placeholders {
+            visitor.parent_scope.legacy = visitor.visit_invoc(*placeholder);
+        }
+
         visitor.parent_scope.legacy
     }
 
@@ -871,7 +884,7 @@ impl<'a, 'b> BuildReducedGraphVisitor<'a, 'b> {
     }
 
     /// Builds the reduced graph for a single item in an external crate.
-    fn build_reduced_graph_for_external_crate_res(&mut self, child: Export<ast::NodeId>) {
+    fn build_reduced_graph_for_external_crate_res(&mut self, child: Export<NodeId>) {
         let parent = self.parent_scope.module;
         let Export { ident, res, vis, span } = child;
         // FIXME: We shouldn't create the gensym here, it should come from metadata,
@@ -1060,10 +1073,10 @@ impl<'a, 'b> BuildReducedGraphVisitor<'a, 'b> {
         false
     }
 
-    fn visit_invoc(&mut self, id: ast::NodeId) -> LegacyScope<'a> {
+    fn visit_invoc(&mut self, id: NodeId) -> LegacyScope<'a> {
         let invoc_id = id.placeholder_to_expn_id();
 
-        self.parent_scope.module.unresolved_invocations.borrow_mut().insert(invoc_id);
+        self.parent_scope.module.unexpanded_invocations.borrow_mut().insert(invoc_id);
 
         let old_parent_scope = self.r.invocation_parent_scopes.insert(invoc_id, self.parent_scope);
         assert!(old_parent_scope.is_none(), "invocation data is reset for an invocation");

src/librustc_resolve/lib.rs

@@ -448,7 +448,7 @@ pub struct ModuleData<'a> {
     populate_on_access: Cell<bool>,
 
     // Macro invocations that can expand into items in this module.
-    unresolved_invocations: RefCell<FxHashSet<ExpnId>>,
+    unexpanded_invocations: RefCell<FxHashSet<ExpnId>>,
 
     no_implicit_prelude: bool,
 
@@ -478,7 +478,7 @@ impl<'a> ModuleData<'a> {
             normal_ancestor_id,
             lazy_resolutions: Default::default(),
             populate_on_access: Cell::new(!normal_ancestor_id.is_local()),
-            unresolved_invocations: Default::default(),
+            unexpanded_invocations: Default::default(),
             no_implicit_prelude: false,
             glob_importers: RefCell::new(Vec::new()),
             globs: RefCell::new(Vec::new()),

src/librustc_resolve/macros.rs

@@ -10,7 +10,7 @@ use crate::resolve_imports::ImportResolver;
 use rustc::hir::def::{self, DefKind, NonMacroAttrKind};
 use rustc::middle::stability;
 use rustc::{ty, lint, span_bug};
-use syntax::ast::{self, Ident};
+use syntax::ast::{self, NodeId, Ident};
 use syntax::attr::StabilityLevel;
 use syntax::edition::Edition;
 use syntax::ext::base::{self, Indeterminate, SpecialDerives};
@@ -26,7 +26,7 @@ use syntax_pos::{Span, DUMMY_SP};
 use std::{mem, ptr};
 use rustc_data_structures::sync::Lrc;
 
-type Res = def::Res<ast::NodeId>;
+type Res = def::Res<NodeId>;
 
 /// Binding produced by a `macro_rules` item.
 /// Not modularized, can shadow previous legacy bindings, etc.
@@ -91,11 +91,11 @@ fn fast_print_path(path: &ast::Path) -> Symbol {
 }
 
 impl<'a> base::Resolver for Resolver<'a> {
-    fn next_node_id(&mut self) -> ast::NodeId {
+    fn next_node_id(&mut self) -> NodeId {
         self.session.next_node_id()
     }
 
-    fn get_module_scope(&mut self, id: ast::NodeId) -> ExpnId {
+    fn get_module_scope(&mut self, id: NodeId) -> ExpnId {
         let expn_id = ExpnId::fresh(Some(ExpnData::default(
             ExpnKind::Macro(MacroKind::Attr, sym::test_case), DUMMY_SP, self.session.edition()
         )));
@@ -115,23 +115,18 @@ impl<'a> base::Resolver for Resolver<'a> {
         });
     }
 
+    // FIXME: `extra_placeholders` should be included into the `fragment` as regular placeholders.
     fn visit_ast_fragment_with_placeholders(
-        &mut self, expansion: ExpnId, fragment: &AstFragment, derives: &[ExpnId]
+        &mut self, expansion: ExpnId, fragment: &AstFragment, extra_placeholders: &[NodeId]
     ) {
-        // Fill in some data for derives if the fragment is from a derive container.
+        // Integrate the new AST fragment into all the definition and module structures.
         // We are inside the `expansion` now, but other parent scope components are still the same.
         let parent_scope = ParentScope { expansion, ..self.invocation_parent_scopes[&expansion] };
-        let parent_def = self.definitions.invocation_parent(expansion);
+        let output_legacy_scope =
-        self.invocation_parent_scopes.extend(derives.iter().map(|&derive| (derive, parent_scope)));
+            self.build_reduced_graph(fragment, extra_placeholders, parent_scope);
-        for &derive_invoc_id in derives {
-            self.definitions.set_invocation_parent(derive_invoc_id, parent_def);
-        }
-        parent_scope.module.unresolved_invocations.borrow_mut().remove(&expansion);
-        parent_scope.module.unresolved_invocations.borrow_mut().extend(derives);
-
-        // Integrate the new AST fragment into all the definition and module structures.
-        let output_legacy_scope = self.build_reduced_graph(fragment, parent_scope);
         self.output_legacy_scopes.insert(expansion, output_legacy_scope);
+
+        parent_scope.module.unexpanded_invocations.borrow_mut().remove(&expansion);
     }
 
     fn register_builtin_macro(&mut self, ident: ast::Ident, ext: SyntaxExtension) {
@@ -485,7 +480,7 @@ impl<'a> Resolver<'a> {
                 Scope::MacroUsePrelude => match this.macro_use_prelude.get(&ident.name).cloned() {
                     Some(binding) => Ok((binding, Flags::PRELUDE | Flags::MISC_FROM_PRELUDE)),
                     None => Err(Determinacy::determined(
-                        this.graph_root.unresolved_invocations.borrow().is_empty()
+                        this.graph_root.unexpanded_invocations.borrow().is_empty()
                     ))
                 }
                 Scope::BuiltinAttrs => if is_builtin_attr_name(ident.name) {
@@ -508,7 +503,7 @@ impl<'a> Resolver<'a> {
                 Scope::ExternPrelude => match this.extern_prelude_get(ident, !record_used) {
                     Some(binding) => Ok((binding, Flags::PRELUDE)),
                     None => Err(Determinacy::determined(
-                        this.graph_root.unresolved_invocations.borrow().is_empty()
+                        this.graph_root.unexpanded_invocations.borrow().is_empty()
                     )),
                 }
                 Scope::ToolPrelude => if KNOWN_TOOLS.contains(&ident.name) {

src/librustc_resolve/resolve_imports.rs

@@ -202,7 +202,7 @@ impl<'a> Resolver<'a> {
                     Err((Determined, Weak::No))
                 } else if let Some(binding) = self.extern_prelude_get(ident, !record_used) {
                     Ok(binding)
-                } else if !self.graph_root.unresolved_invocations.borrow().is_empty() {
+                } else if !self.graph_root.unexpanded_invocations.borrow().is_empty() {
                     // Macro-expanded `extern crate` items can add names to extern prelude.
                     Err((Undetermined, Weak::No))
                 } else {
@@ -348,7 +348,7 @@ impl<'a> Resolver<'a> {
         // progress, we have to ignore those potential unresolved invocations from other modules
         // and prohibit access to macro-expanded `macro_export` macros instead (unless restricted
         // shadowing is enabled, see `macro_expanded_macro_export_errors`).
-        let unexpanded_macros = !module.unresolved_invocations.borrow().is_empty();
+        let unexpanded_macros = !module.unexpanded_invocations.borrow().is_empty();
         if let Some(binding) = resolution.binding {
             if !unexpanded_macros || ns == MacroNS || restricted_shadowing {
                 return check_usable(self, binding);

src/libsyntax/ext/base.rs

@@ -1,4 +1,4 @@
-use crate::ast::{self, Attribute, Name, PatKind};
+use crate::ast::{self, NodeId, Attribute, Name, PatKind};
 use crate::attr::{HasAttrs, Stability, Deprecation};
 use crate::source_map::SourceMap;
 use crate::edition::Edition;
@@ -671,13 +671,13 @@ bitflags::bitflags! {
 }
 
 pub trait Resolver {
-    fn next_node_id(&mut self) -> ast::NodeId;
+    fn next_node_id(&mut self) -> NodeId;
 
-    fn get_module_scope(&mut self, id: ast::NodeId) -> ExpnId;
+    fn get_module_scope(&mut self, id: NodeId) -> ExpnId;
 
     fn resolve_dollar_crates(&mut self);
     fn visit_ast_fragment_with_placeholders(&mut self, expn_id: ExpnId, fragment: &AstFragment,
-                                            derives: &[ExpnId]);
+                                            extra_placeholders: &[NodeId]);
     fn register_builtin_macro(&mut self, ident: ast::Ident, ext: SyntaxExtension);
 
     fn resolve_imports(&mut self);

src/libsyntax/ext/expand.rs

@@ -291,7 +291,7 @@ impl<'a, 'b> MacroExpander<'a, 'b> {
         // Unresolved macros produce dummy outputs as a recovery measure.
         invocations.reverse();
         let mut expanded_fragments = Vec::new();
-        let mut derives: FxHashMap<ExpnId, Vec<_>> = FxHashMap::default();
+        let mut all_derive_placeholders: FxHashMap<ExpnId, Vec<_>> = FxHashMap::default();
         let mut undetermined_invocations = Vec::new();
         let (mut progress, mut force) = (false, !self.monotonic);
         loop {
@@ -347,13 +347,14 @@ impl<'a, 'b> MacroExpander<'a, 'b> {
 
                 let mut item = self.fully_configure(item);
                 item.visit_attrs(|attrs| attrs.retain(|a| a.path != sym::derive));
-                let derives = derives.entry(invoc.expansion_data.id).or_default();
+                let derive_placeholders =
+                    all_derive_placeholders.entry(invoc.expansion_data.id).or_default();
 
-                derives.reserve(traits.len());
+                derive_placeholders.reserve(traits.len());
                 invocations.reserve(traits.len());
                 for path in traits {
                     let expn_id = ExpnId::fresh(None);
-                    derives.push(expn_id);
+                    derive_placeholders.push(NodeId::placeholder_from_expn_id(expn_id));
                     invocations.push(Invocation {
                         kind: InvocationKind::Derive { path, item: item.clone() },
                         fragment_kind: invoc.fragment_kind,
@@ -365,7 +366,7 @@ impl<'a, 'b> MacroExpander<'a, 'b> {
                 }
                 let fragment = invoc.fragment_kind
                     .expect_from_annotatables(::std::iter::once(item));
-                self.collect_invocations(fragment, derives)
+                self.collect_invocations(fragment, derive_placeholders)
             } else {
                 unreachable!()
             };
@@ -384,10 +385,11 @@ impl<'a, 'b> MacroExpander<'a, 'b> {
         // Finally incorporate all the expanded macros into the input AST fragment.
         let mut placeholder_expander = PlaceholderExpander::new(self.cx, self.monotonic);
         while let Some(expanded_fragments) = expanded_fragments.pop() {
-            for (mark, expanded_fragment) in expanded_fragments.into_iter().rev() {
+            for (expn_id, expanded_fragment) in expanded_fragments.into_iter().rev() {
-                let derives = derives.remove(&mark).unwrap_or_else(Vec::new);
+                let derive_placeholders =
-                placeholder_expander.add(NodeId::placeholder_from_expn_id(mark),
+                    all_derive_placeholders.remove(&expn_id).unwrap_or_else(Vec::new);
-                                         expanded_fragment, derives);
+                placeholder_expander.add(NodeId::placeholder_from_expn_id(expn_id),
+                                         expanded_fragment, derive_placeholders);
             }
         }
         fragment_with_placeholders.mut_visit_with(&mut placeholder_expander);
@@ -404,7 +406,7 @@ impl<'a, 'b> MacroExpander<'a, 'b> {
     /// them with "placeholders" - dummy macro invocations with specially crafted `NodeId`s.
     /// Then call into resolver that builds a skeleton ("reduced graph") of the fragment and
     /// prepares data for resolving paths of macro invocations.
-    fn collect_invocations(&mut self, mut fragment: AstFragment, derives: &[ExpnId])
+    fn collect_invocations(&mut self, mut fragment: AstFragment, extra_placeholders: &[NodeId])
                            -> (AstFragment, Vec<Invocation>) {
         // Resolve `$crate`s in the fragment for pretty-printing.
         self.cx.resolver.resolve_dollar_crates();
@@ -423,9 +425,10 @@ impl<'a, 'b> MacroExpander<'a, 'b> {
             collector.invocations
         };
 
+        // FIXME: Merge `extra_placeholders` into the `fragment` as regular placeholders.
         if self.monotonic {
             self.cx.resolver.visit_ast_fragment_with_placeholders(
-                self.cx.current_expansion.id, &fragment, derives);
+                self.cx.current_expansion.id, &fragment, extra_placeholders);
         }
 
         (fragment, invocations)

src/libsyntax/ext/placeholders.rs

@@ -2,7 +2,6 @@ use crate::ast::{self, NodeId};
 use crate::source_map::{DUMMY_SP, dummy_spanned};
 use crate::ext::base::ExtCtxt;
 use crate::ext::expand::{AstFragment, AstFragmentKind};
-use crate::ext::hygiene::ExpnId;
 use crate::tokenstream::TokenStream;
 use crate::mut_visit::*;
 use crate::ptr::P;
@@ -86,11 +85,11 @@ impl<'a, 'b> PlaceholderExpander<'a, 'b> {
         }
     }
 
-    pub fn add(&mut self, id: ast::NodeId, mut fragment: AstFragment, derives: Vec<ExpnId>) {
+    pub fn add(&mut self, id: ast::NodeId, mut fragment: AstFragment, placeholders: Vec<NodeId>) {
         fragment.mut_visit_with(self);
         if let AstFragment::Items(mut items) = fragment {
-            for derive in derives {
+            for placeholder in placeholders {
-                match self.remove(NodeId::placeholder_from_expn_id(derive)) {
+                match self.remove(placeholder) {
                     AstFragment::Items(derived_items) => items.extend(derived_items),
                     _ => unreachable!(),
                 }

src/test/ui/proc-macro/auxiliary/gen-macro-rules.rs

@@ -0,0 +1,12 @@
+// force-host
+// no-prefer-dynamic
+
+#![crate_type = "proc-macro"]
+
+extern crate proc_macro;
+use proc_macro::TokenStream;
+
+#[proc_macro_derive(repro)]
+pub fn proc_macro_hack_expr(_input: TokenStream) -> TokenStream {
+    "macro_rules! m {()=>{}}".parse().unwrap()
+}

src/test/ui/proc-macro/gen-macro-rules.rs

@@ -0,0 +1,13 @@
+// Derive macros can generate `macro_rules` items, regression test for issue #63651.
+
+// check-pass
+// aux-build:gen-macro-rules.rs
+
+extern crate gen_macro_rules as repro;
+
+#[derive(repro::repro)]
+pub struct S;
+
+m!(); // OK
+
+fn main() {}