Skip to content

Commit

Permalink
Auto merge of #53913 - petrochenkov:biattr4, r=<try>
Browse files Browse the repository at this point in the history 
resolve: Future proof resolutions for potentially built-in attributes

Based on #53778

This is not full "pass all attributes through name resolution", but a more conservative solution.
If built-in attribute is ambiguous with any other macro in scope, then an error is reported.
TODO: Explain what complications arise with the full solution.

cc #50911 (comment)
Closes #53531
  • Loading branch information
@bors
bors committed Sep 3, 2018
2 parents 591a17d + ebb7a7b commit 5fe695a
Show file tree
Hide file tree
Showing 25 changed files with 1,578 additions and 247 deletions.
35 changes: 24 additions & 11 deletions src/librustc_resolve/build_reduced_graph.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ use syntax::ext::base::{MacroKind, SyntaxExtension};
use syntax::ext::base::Determinacy::Undetermined;
use syntax::ext::hygiene::Mark;
use syntax::ext::tt::macro_rules;
use syntax::feature_gate::is_builtin_attr;
use syntax::parse::token::{self, Token};
use syntax::std_inject::injected_crate_name;
use syntax::symbol::keywords;
Expand Down Expand Up @@ -933,7 +934,7 @@ impl<'a, 'cl> Resolver<'a, 'cl> {

pub struct BuildReducedGraphVisitor<'a, 'b: 'a, 'c: 'b> {
pub resolver: &'a mut Resolver<'b, 'c>,
pub legacy_scope: LegacyScope<'b>,
pub current_legacy_scope: LegacyScope<'b>,
pub expansion: Mark,
}

Expand All @@ -943,7 +944,8 @@ impl<'a, 'b, 'cl> BuildReducedGraphVisitor<'a, 'b, 'cl> {
self.resolver.current_module.unresolved_invocations.borrow_mut().insert(mark);
let invocation = self.resolver.invocations[&mark];
invocation.module.set(self.resolver.current_module);
invocation.legacy_scope.set(self.legacy_scope);
invocation.parent_legacy_scope.set(self.current_legacy_scope);
invocation.output_legacy_scope.set(self.current_legacy_scope);
invocation
}
}
Expand All @@ -969,29 +971,30 @@ impl<'a, 'b, 'cl> Visitor<'a> for BuildReducedGraphVisitor<'a, 'b, 'cl> {
fn visit_item(&mut self, item: &'a Item) {
let macro_use = match item.node {
ItemKind::MacroDef(..) => {
self.resolver.define_macro(item, self.expansion, &mut self.legacy_scope);
self.resolver.define_macro(item, self.expansion, &mut self.current_legacy_scope);
return
}
ItemKind::Mac(..) => {
self.legacy_scope = LegacyScope::Expansion(self.visit_invoc(item.id));
self.current_legacy_scope = LegacyScope::Invocation(self.visit_invoc(item.id));
return
}
ItemKind::Mod(..) => self.resolver.contains_macro_use(&item.attrs),
_ => false,
};

let (parent, legacy_scope) = (self.resolver.current_module, self.legacy_scope);
let orig_current_module = self.resolver.current_module;
let orig_current_legacy_scope = self.current_legacy_scope;
self.resolver.build_reduced_graph_for_item(item, self.expansion);
visit::walk_item(self, item);
self.resolver.current_module = parent;
self.resolver.current_module = orig_current_module;
if !macro_use {
self.legacy_scope = legacy_scope;
self.current_legacy_scope = orig_current_legacy_scope;
}
}

fn visit_stmt(&mut self, stmt: &'a ast::Stmt) {
if let ast::StmtKind::Mac(..) = stmt.node {
self.legacy_scope = LegacyScope::Expansion(self.visit_invoc(stmt.id));
self.current_legacy_scope = LegacyScope::Invocation(self.visit_invoc(stmt.id));
} else {
visit::walk_stmt(self, stmt);
}
Expand All @@ -1008,11 +1011,12 @@ impl<'a, 'b, 'cl> Visitor<'a> for BuildReducedGraphVisitor<'a, 'b, 'cl> {
}

fn visit_block(&mut self, block: &'a Block) {
let (parent, legacy_scope) = (self.resolver.current_module, self.legacy_scope);
let orig_current_module = self.resolver.current_module;
let orig_current_legacy_scope = self.current_legacy_scope;
self.resolver.build_reduced_graph_for_block(block, self.expansion);
visit::walk_block(self, block);
self.resolver.current_module = parent;
self.legacy_scope = legacy_scope;
self.resolver.current_module = orig_current_module;
self.current_legacy_scope = orig_current_legacy_scope;
}

fn visit_trait_item(&mut self, item: &'a TraitItem) {
Expand Down Expand Up @@ -1057,4 +1061,13 @@ impl<'a, 'b, 'cl> Visitor<'a> for BuildReducedGraphVisitor<'a, 'b, 'cl> {
}
}
}

fn visit_attribute(&mut self, attr: &'a ast::Attribute) {
if !attr.is_sugared_doc && is_builtin_attr(attr) {
self.resolver.current_module.builtin_attrs.borrow_mut().push((
attr.path.segments[0].ident, self.expansion, self.current_legacy_scope
));
}
visit::walk_attribute(self, attr);
}
}
102 changes: 54 additions & 48 deletions src/librustc_resolve/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ use std::mem::replace;
use rustc_data_structures::sync::Lrc;

use resolve_imports::{ImportDirective, ImportDirectiveSubclass, NameResolution, ImportResolver};
use macros::{InvocationData, LegacyBinding};
use macros::{InvocationData, LegacyBinding, LegacyScope};

// NB: This module needs to be declared first so diagnostics are
// registered before they are used.
Expand Down Expand Up @@ -1010,8 +1010,9 @@ pub struct ModuleData<'a> {
normal_ancestor_id: DefId,

resolutions: RefCell<FxHashMap<(Ident, Namespace), &'a RefCell<NameResolution<'a>>>>,
legacy_macro_resolutions: RefCell<Vec<(Mark, Ident, MacroKind, Option<Def>)>>,
legacy_macro_resolutions: RefCell<Vec<(Ident, MacroKind, Mark, LegacyScope<'a>, Option<Def>)>>,
macro_resolutions: RefCell<Vec<(Box<[Ident]>, Span)>>,
builtin_attrs: RefCell<Vec<(Ident, Mark, LegacyScope<'a>)>>,

// Macro invocations that can expand into items in this module.
unresolved_invocations: RefCell<FxHashSet<Mark>>,
Expand Down Expand Up @@ -1050,6 +1051,7 @@ impl<'a> ModuleData<'a> {
resolutions: RefCell::new(FxHashMap()),
legacy_macro_resolutions: RefCell::new(Vec::new()),
macro_resolutions: RefCell::new(Vec::new()),
builtin_attrs: RefCell::new(Vec::new()),
unresolved_invocations: RefCell::new(FxHashSet()),
no_implicit_prelude: false,
glob_importers: RefCell::new(Vec::new()),
Expand Down Expand Up @@ -1166,7 +1168,6 @@ struct UseError<'a> {
struct AmbiguityError<'a> {
span: Span,
name: Name,
lexical: bool,
b1: &'a NameBinding<'a>,
b2: &'a NameBinding<'a>,
}
Expand Down Expand Up @@ -1270,6 +1271,23 @@ impl<'a> NameBinding<'a> {
fn descr(&self) -> &'static str {
if self.is_extern_crate() { "extern crate" } else { self.def().kind_name() }
}

// Suppose that we resolved macro invocation with `invoc_parent_expansion` to binding `binding`
// at some expansion round `max(invoc, binding)` when they both emerged from macros.
// Then this function returns `true` if `self` may emerge from a macro *after* that
// in some later round and screw up our previously found resolution.
fn may_appear_after(&self, invoc_parent_expansion: Mark, binding: &NameBinding) -> bool {
// self > max(invoc, binding) => !(self <= invoc || self <= binding)
// Expansions are partially ordered, so "may appear after" is an inversion of
// "certainly appears before or simultaneously" and includes unordered cases.
let self_parent_expansion = self.expansion;
let other_parent_expansion = binding.expansion;
let certainly_before_other_or_simultaneously =
other_parent_expansion.is_descendant_of(self_parent_expansion);
let certainly_before_invoc_or_simultaneously =
invoc_parent_expansion.is_descendant_of(self_parent_expansion);
!(certainly_before_other_or_simultaneously || certainly_before_invoc_or_simultaneously)
}
}

/// Interns the names of the primitive types.
Expand Down Expand Up @@ -1403,8 +1421,6 @@ pub struct Resolver<'a, 'b: 'a> {
proc_mac_errors: Vec<macros::ProcMacError>,
/// crate-local macro expanded `macro_export` referred to by a module-relative path
macro_expanded_macro_export_errors: BTreeSet<(Span, Span)>,
/// macro-expanded `macro_rules` shadowing existing macros
disallowed_shadowing: Vec<&'a LegacyBinding<'a>>,

arenas: &'a ResolverArenas<'a>,
dummy_binding: &'a NameBinding<'a>,
Expand Down Expand Up @@ -1715,7 +1731,6 @@ impl<'a, 'crateloader: 'a> Resolver<'a, 'crateloader> {
ambiguity_errors: Vec::new(),
use_injections: Vec::new(),
proc_mac_errors: Vec::new(),
disallowed_shadowing: Vec::new(),
macro_expanded_macro_export_errors: BTreeSet::new(),

arenas,
Expand Down Expand Up @@ -1814,7 +1829,7 @@ impl<'a, 'crateloader: 'a> Resolver<'a, 'crateloader> {
NameBindingKind::Import { .. } => false,
NameBindingKind::Ambiguity { b1, b2 } => {
self.ambiguity_errors.push(AmbiguityError {
span, name: ident.name, lexical: false, b1, b2,
span, name: ident.name, b1, b2,
});
true
}
Expand Down Expand Up @@ -3468,6 +3483,20 @@ impl<'a, 'crateloader: 'a> Resolver<'a, 'crateloader> {
record_used: bool,
path_span: Span,
crate_lint: CrateLint,
) -> PathResult<'a> {
self.resolve_path_with_parent_expansion(base_module, path, opt_ns, Mark::root(),
record_used, path_span, crate_lint)
}

fn resolve_path_with_parent_expansion(
&mut self,
base_module: Option<ModuleOrUniformRoot<'a>>,
path: &[Ident],
opt_ns: Option<Namespace>, // `None` indicates a module path
parent_expansion: Mark,
record_used: bool,
path_span: Span,
crate_lint: CrateLint,
) -> PathResult<'a> {
let mut module = base_module;
let mut allow_super = true;
Expand Down Expand Up @@ -3557,8 +3586,9 @@ impl<'a, 'crateloader: 'a> Resolver<'a, 'crateloader> {
self.resolve_ident_in_module(module, ident, ns, record_used, path_span)
} else if opt_ns == Some(MacroNS) {
assert!(ns == TypeNS);
self.resolve_lexical_macro_path_segment(ident, ns, record_used, record_used,
false, path_span).map(|(b, _)| b)
self.resolve_lexical_macro_path_segment(ident, ns, parent_expansion, record_used,
record_used, false, path_span)
.map(|(binding, _)| binding)
} else {
let record_used_id =
if record_used { crate_lint.node_id().or(Some(CRATE_NODE_ID)) } else { None };
Expand Down Expand Up @@ -4499,35 +4529,32 @@ impl<'a, 'crateloader: 'a> Resolver<'a, 'crateloader> {
vis.is_accessible_from(module.normal_ancestor_id, self)
}

fn report_ambiguity_error(
&self, name: Name, span: Span, _lexical: bool,
def1: Def, is_import1: bool, is_glob1: bool, from_expansion1: bool, span1: Span,
def2: Def, is_import2: bool, _is_glob2: bool, _from_expansion2: bool, span2: Span,
) {
fn report_ambiguity_error(&self, name: Name, span: Span, b1: &NameBinding, b2: &NameBinding) {
let participle = |is_import: bool| if is_import { "imported" } else { "defined" };
let msg1 = format!("`{}` could refer to the name {} here", name, participle(is_import1));
let msg1 =
format!("`{}` could refer to the name {} here", name, participle(b1.is_import()));
let msg2 =
format!("`{}` could also refer to the name {} here", name, participle(is_import2));
let note = if from_expansion1 {
Some(if let Def::Macro(..) = def1 {
format!("`{}` could also refer to the name {} here", name, participle(b2.is_import()));
let note = if b1.expansion != Mark::root() {
Some(if let Def::Macro(..) = b1.def() {
format!("macro-expanded {} do not shadow",
if is_import1 { "macro imports" } else { "macros" })
if b1.is_import() { "macro imports" } else { "macros" })
} else {
format!("macro-expanded {} do not shadow when used in a macro invocation path",
if is_import1 { "imports" } else { "items" })
if b1.is_import() { "imports" } else { "items" })
})
} else if is_glob1 {
} else if b1.is_glob_import() {
Some(format!("consider adding an explicit import of `{}` to disambiguate", name))
} else {
None
};

let mut err = struct_span_err!(self.session, span, E0659, "`{}` is ambiguous", name);
err.span_note(span1, &msg1);
match def2 {
Def::Macro(..) if span2.is_dummy() =>
err.span_note(b1.span, &msg1);
match b2.def() {
Def::Macro(..) if b2.span.is_dummy() =>
err.note(&format!("`{}` is also a builtin macro", name)),
_ => err.span_note(span2, &msg2),
_ => err.span_note(b2.span, &msg2),
};
if let Some(note) = note {
err.note(&note);
Expand All @@ -4536,7 +4563,6 @@ impl<'a, 'crateloader: 'a> Resolver<'a, 'crateloader> {
}

fn report_errors(&mut self, krate: &Crate) {
self.report_shadowing_errors();
self.report_with_use_injections(krate);
self.report_proc_macro_import(krate);
let mut reported_spans = FxHashSet();
Expand All @@ -4552,15 +4578,9 @@ impl<'a, 'crateloader: 'a> Resolver<'a, 'crateloader> {
);
}

for &AmbiguityError { span, name, b1, b2, lexical } in &self.ambiguity_errors {
for &AmbiguityError { span, name, b1, b2 } in &self.ambiguity_errors {
if reported_spans.insert(span) {
self.report_ambiguity_error(
name, span, lexical,
b1.def(), b1.is_import(), b1.is_glob_import(),
b1.expansion != Mark::root(), b1.span,
b2.def(), b2.is_import(), b2.is_glob_import(),
b2.expansion != Mark::root(), b2.span,
);
self.report_ambiguity_error(name, span, b1, b2);
}
}

Expand All @@ -4580,20 +4600,6 @@ impl<'a, 'crateloader: 'a> Resolver<'a, 'crateloader> {
}
}

fn report_shadowing_errors(&mut self) {
let mut reported_errors = FxHashSet();
for binding in replace(&mut self.disallowed_shadowing, Vec::new()) {
if self.resolve_legacy_scope(&binding.parent, binding.ident, false).is_some() &&
reported_errors.insert((binding.ident, binding.span)) {
let msg = format!("`{}` is already in scope", binding.ident);
self.session.struct_span_err(binding.span, &msg)
.note("macro-expanded `macro_rules!`s may not shadow \
existing macros (see RFC 1560)")
.emit();
}
}
}

fn report_conflict<'b>(&mut self,
parent: Module,
ident: Ident,
Expand Down
Loading

0 comments on commit 5fe695a

Please sign in to comment.