Rollup merge of #65252 - petrochenkov:deriveholders2, r=matthewjasper

expand: Simplify expansion of derives

And make it more uniform with other macros.
This is done by merging placeholders for future derives' outputs into the derive container's output fragment early (addressing FIXMEs from #63667).

Also, macros with names starting with `_` are no longer reported as unused, in accordance with the usual behavior of `unused` lints.

r? @matthewjasper or @mark-i-m

Author: Centril

src/librustc/hir/map/def_collector.rs

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

src/librustc_privacy/lib.rs

@@ -880,11 +880,11 @@ impl Visitor<'tcx> for EmbargoVisitor<'tcx> {
             self.tcx,
             self.tcx.hir().local_def_id(md.hir_id)
         ).unwrap();
-        let mut module_id = self.tcx.hir().as_local_hir_id(macro_module_def_id).unwrap();
-        if !self.tcx.hir().is_hir_id_module(module_id) {
-            // `module_id` doesn't correspond to a `mod`, return early (#63164).
-            return;
-        }
+        let mut module_id = match self.tcx.hir().as_local_hir_id(macro_module_def_id) {
+            Some(module_id) if self.tcx.hir().is_hir_id_module(module_id) => module_id,
+            // `module_id` doesn't correspond to a `mod`, return early (#63164, #65252).
+            _ => return,
+        };
         let level = if md.vis.node.is_pub() { self.get(module_id) } else { None };
         let new_level = self.update(md.hir_id, level);
         if new_level.is_none() {

src/librustc_resolve/build_reduced_graph.rs

@@ -163,25 +163,15 @@ 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,
-        extra_placeholders: &[NodeId],
         parent_scope: ParentScope<'a>,
     ) -> LegacyScope<'a> {
         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
     }
 
@@ -1064,8 +1054,17 @@ impl<'a, 'b> BuildReducedGraphVisitor<'a, 'b> {
         None
     }
 
+    // Mark the given macro as unused unless its name starts with `_`.
+    // Macro uses will remove items from this set, and the remaining
+    // items will be reported as `unused_macros`.
+    fn insert_unused_macro(&mut self, ident: Ident, node_id: NodeId, span: Span) {
+        if !ident.as_str().starts_with("_") {
+            self.r.unused_macros.insert(node_id, span);
+        }
+    }
+
     fn define_macro(&mut self, item: &ast::Item) -> LegacyScope<'a> {
-        let parent_scope = &self.parent_scope;
+        let parent_scope = self.parent_scope;
         let expansion = parent_scope.expansion;
         let (ext, ident, span, is_legacy) = match &item.kind {
             ItemKind::MacroDef(def) => {
@@ -1105,7 +1104,7 @@ impl<'a, 'b> BuildReducedGraphVisitor<'a, 'b> {
                             (res, vis, span, expansion, IsMacroExport));
             } else {
                 self.r.check_reserved_macro_name(ident, res);
-                self.r.unused_macros.insert(item.id, span);
+                self.insert_unused_macro(ident, item.id, span);
             }
             LegacyScope::Binding(self.r.arenas.alloc_legacy_binding(LegacyBinding {
                 parent_legacy_scope: parent_scope.legacy, binding, ident
@@ -1114,7 +1113,7 @@ impl<'a, 'b> BuildReducedGraphVisitor<'a, 'b> {
             let module = parent_scope.module;
             let vis = self.resolve_visibility(&item.vis);
             if vis != ty::Visibility::Public {
-                self.r.unused_macros.insert(item.id, span);
+                self.insert_unused_macro(ident, item.id, span);
             }
             self.r.define(module, ident, MacroNS, (res, vis, span, expansion));
             self.parent_scope.legacy

src/librustc_resolve/macros.rs

@@ -108,15 +108,11 @@ 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, extra_placeholders: &[NodeId]
-    ) {
+    fn visit_ast_fragment_with_placeholders(&mut self, expansion: ExpnId, fragment: &AstFragment) {
         // 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 output_legacy_scope =
-            self.build_reduced_graph(fragment, extra_placeholders, parent_scope);
+        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);

src/libsyntax_expand/base.rs

@@ -851,8 +851,7 @@ pub trait Resolver {
     fn next_node_id(&mut self) -> NodeId;
 
     fn resolve_dollar_crates(&mut self);
-    fn visit_ast_fragment_with_placeholders(&mut self, expn_id: ExpnId, fragment: &AstFragment,
-                                            extra_placeholders: &[NodeId]);
+    fn visit_ast_fragment_with_placeholders(&mut self, expn_id: ExpnId, fragment: &AstFragment);
     fn register_builtin_macro(&mut self, ident: ast::Ident, ext: SyntaxExtension);
 
     fn expansion_for_ast_pass(

src/libsyntax_expand/expand.rs

@@ -26,7 +26,6 @@ use errors::{Applicability, FatalError};
 use smallvec::{smallvec, SmallVec};
 use syntax_pos::{Span, DUMMY_SP, FileName};
 
-use rustc_data_structures::fx::FxHashMap;
 use rustc_data_structures::sync::Lrc;
 use std::io::ErrorKind;
 use std::{iter, mem, slice};
@@ -75,6 +74,22 @@ macro_rules! ast_fragments {
         }
 
         impl AstFragment {
+            pub fn add_placeholders(&mut self, placeholders: &[NodeId]) {
+                if placeholders.is_empty() {
+                    return;
+                }
+                match self {
+                    $($(AstFragment::$Kind(ast) => ast.extend(placeholders.iter().flat_map(|id| {
+                        // We are repeating through arguments with `many`, to do that we have to
+                        // mention some macro variable from those arguments even if it's not used.
+                        #[cfg_attr(bootstrap, allow(unused_macros))]
+                        macro _repeating($flat_map_ast_elt) {}
+                        placeholder(AstFragmentKind::$Kind, *id).$make_ast()
+                    })),)?)*
+                    _ => panic!("unexpected AST fragment kind")
+                }
+            }
+
             pub fn make_opt_expr(self) -> Option<P<ast::Expr>> {
                 match self {
                     AstFragment::OptExpr(expr) => expr,
@@ -342,7 +357,6 @@ 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 all_derive_placeholders: FxHashMap<ExpnId, Vec<_>> = FxHashMap::default();
         let mut undetermined_invocations = Vec::new();
         let (mut progress, mut force) = (false, !self.monotonic);
         loop {
@@ -420,9 +434,7 @@ impl<'a, 'b> MacroExpander<'a, 'b> {
                         self.cx.resolver.add_derives(invoc.expansion_data.id, SpecialDerives::COPY);
                     }
 
-                    let derive_placeholders =
-                        all_derive_placeholders.entry(invoc.expansion_data.id).or_default();
-                    derive_placeholders.reserve(derives.len());
+                    let mut derive_placeholders = Vec::with_capacity(derives.len());
                     invocations.reserve(derives.len());
                     for path in derives {
                         let expn_id = ExpnId::fresh(None);
@@ -438,7 +450,7 @@ impl<'a, 'b> MacroExpander<'a, 'b> {
                     }
                     let fragment = invoc.fragment_kind
                         .expect_from_annotatables(::std::iter::once(item));
-                    self.collect_invocations(fragment, derive_placeholders)
+                    self.collect_invocations(fragment, &derive_placeholders)
                 }
             };
 
@@ -457,10 +469,8 @@ impl<'a, 'b> MacroExpander<'a, 'b> {
         let mut placeholder_expander = PlaceholderExpander::new(self.cx, self.monotonic);
         while let Some(expanded_fragments) = expanded_fragments.pop() {
             for (expn_id, expanded_fragment) in expanded_fragments.into_iter().rev() {
-                let derive_placeholders =
-                    all_derive_placeholders.remove(&expn_id).unwrap_or_else(Vec::new);
                 placeholder_expander.add(NodeId::placeholder_from_expn_id(expn_id),
-                                         expanded_fragment, derive_placeholders);
+                                         expanded_fragment);
             }
         }
         fragment_with_placeholders.mut_visit_with(&mut placeholder_expander);
@@ -493,13 +503,14 @@ impl<'a, 'b> MacroExpander<'a, 'b> {
                 monotonic: self.monotonic,
             };
             fragment.mut_visit_with(&mut collector);
+            fragment.add_placeholders(extra_placeholders);
             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, extra_placeholders);
+                self.cx.current_expansion.id, &fragment
+            );
         }
 
         (fragment, invocations)

src/libsyntax_expand/lib.rs

@@ -1,4 +1,5 @@
 #![feature(crate_visibility_modifier)]
+#![feature(decl_macro)]
 #![feature(proc_macro_diagnostic)]
 #![feature(proc_macro_internals)]
 #![feature(proc_macro_span)]

src/libsyntax_expand/placeholders.rs

@@ -1,7 +1,7 @@
 use crate::base::ExtCtxt;
 use crate::expand::{AstFragment, AstFragmentKind};
 
-use syntax::ast::{self, NodeId};
+use syntax::ast;
 use syntax::source_map::{DUMMY_SP, dummy_spanned};
 use syntax::tokenstream::TokenStream;
 use syntax::mut_visit::*;
@@ -171,17 +171,8 @@ impl<'a, 'b> PlaceholderExpander<'a, 'b> {
         }
     }
 
-    pub fn add(&mut self, id: ast::NodeId, mut fragment: AstFragment, placeholders: Vec<NodeId>) {
+    pub fn add(&mut self, id: ast::NodeId, mut fragment: AstFragment) {
         fragment.mut_visit_with(self);
-        if let AstFragment::Items(mut items) = fragment {
-            for placeholder in placeholders {
-                match self.remove(placeholder) {
-                    AstFragment::Items(derived_items) => items.extend(derived_items),
-                    _ => unreachable!(),
-                }
-            }
-            fragment = AstFragment::Items(items);
-        }
         self.expanded_fragments.insert(id, fragment);
     }
 

src/test/rustdoc/macro-in-closure.rs

@@ -0,0 +1,9 @@
+// Regression issue for rustdoc ICE encountered in PR #65252.
+
+#![feature(decl_macro)]
+
+fn main() {
+    || {
+        macro m() {}
+    };
+}