Skip to content

Commit

Permalink
Rollup merge of rust-lang#65252 - petrochenkov:deriveholders2, r=matt…
Browse files Browse the repository at this point in the history
…hewjasper

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 rust-lang#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
  • Loading branch information
Centril authored Oct 14, 2019
2 parents 9611993 + 1270140 commit 9326cd4
Show file tree
Hide file tree
Showing 7 changed files with 41 additions and 44 deletions.
2 changes: 1 addition & 1 deletion src/librustc/hir/map/def_collector.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}
Expand Down
25 changes: 12 additions & 13 deletions src/librustc_resolve/build_reduced_graph.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

Expand Down Expand Up @@ -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) => {
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand Down
8 changes: 2 additions & 6 deletions src/librustc_resolve/macros.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
3 changes: 1 addition & 2 deletions src/libsyntax/ext/base.rs
Original file line number Diff line number Diff line change
Expand Up @@ -849,8 +849,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(
Expand Down
33 changes: 22 additions & 11 deletions src/libsyntax/ext/expand.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,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};
Expand Down Expand Up @@ -73,6 +72,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,
Expand Down Expand Up @@ -340,7 +355,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 {
Expand Down Expand Up @@ -418,9 +432,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);
Expand All @@ -436,7 +448,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)
}
};

Expand All @@ -455,10 +467,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);
Expand Down Expand Up @@ -491,13 +501,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)
Expand Down
13 changes: 2 additions & 11 deletions src/libsyntax/ext/placeholders.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use crate::ast::{self, NodeId};
use crate::ast;
use crate::source_map::{DUMMY_SP, dummy_spanned};
use crate::ext::base::ExtCtxt;
use crate::ext::expand::{AstFragment, AstFragmentKind};
Expand Down Expand Up @@ -170,17 +170,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);
}

Expand Down
1 change: 1 addition & 0 deletions src/libsyntax/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
#![feature(const_fn)]
#![feature(const_transmute)]
#![feature(crate_visibility_modifier)]
#![feature(decl_macro)]
#![feature(label_break_value)]
#![feature(nll)]
#![feature(proc_macro_diagnostic)]
Expand Down

0 comments on commit 9326cd4

Please sign in to comment.