Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

resolve: Relax shadowing restrictions on macro-expanded macros #53778

Merged
merged 11 commits into from
Sep 9, 2018
Merged

resolve: Relax shadowing restrictions on macro-expanded macros #53778

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
4e5e045
resolve: Partially unify bindings from macro_rules and from other items
petrochenkov Aug 27, 2018
83a51de
resolve: Model `resolve_legacy_scope` after `resolve_lexical_macro_pa…
petrochenkov Aug 27, 2018
f34ac26
resolve: Model shadowing restriction for macro_rules after modern macros
petrochenkov Aug 27, 2018
9a539ad
resolve: Cleanup two main macro resolution functions, tweak some comm…
petrochenkov Aug 28, 2018
2723569
resolve: Introduce "may appear after" abstraction for macro path reso…
petrochenkov Aug 28, 2018
c057d57
resolve: Relax shadowing restriction on macro-expanded macros
petrochenkov Aug 29, 2018
d26ae20
resolve: Rename some fields related to legacy macro scopes
petrochenkov Aug 29, 2018
e00993a
Add test cases for possible restricted shadowing configurations
petrochenkov Aug 30, 2018
ae2e5aa
resolve: Further simplify legacy scopes, add comments
petrochenkov Aug 31, 2018
9beb5c3
Add checks for expected macro output in restricted shadowing tests
petrochenkov Sep 7, 2018
2dce377
resolve: More precise spans for ambiguous resolution errors
petrochenkov Sep 7, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 14 additions & 11 deletions src/librustc_resolve/build_reduced_graph.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -950,7 +950,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 @@ -960,7 +960,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 @@ -986,29 +987,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 @@ -1025,11 +1027,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
119 changes: 62 additions & 57 deletions src/librustc_resolve/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1164,9 +1164,7 @@ struct UseError<'a> {
}

struct AmbiguityError<'a> {
span: Span,
name: Name,
lexical: bool,
ident: Ident,
b1: &'a NameBinding<'a>,
b2: &'a NameBinding<'a>,
}
Expand Down Expand Up @@ -1270,6 +1268,26 @@ 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_id` to binding `binding` at some
// expansion round `max(invoc_id, 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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, can we improve this comment a bit?

For example, I am not clear on how you get an error -- the comment makes it sounds like returning true means that the result "may screw up our previously found resolution" -- so is that an error? Or does "may" here mean "is allowed to".

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From inspecting the rest of the code, it seems like true means error. If so, may I suggest not_shadowed_by as a possible name? Alternatively, invert the sense: name it is_shadowed_by.

I'd also like some comments that explain the logic here. Here is my attempt to figure it out. =) I think some examples are helpful, too, so I'll try to supply some.

This function defines the shadowing rules across macro invocations. The idea here is that if a macro definition defines a macro and then invokes it, we should resolve to the macro defined locally. So, for example, the following macro resolves just fine:

            macro gen_inner_invoc() {
                macro m() {}  // B
                m!(); // OK -- resolves to B
            }

This is true even if that macro is defined (or invoked) in a context that already contains a macro m:

macro m() { } // macro def'n A, shadowed by B above
maco gen_inner_invoc() { /* .. as above .. */ }
gen_inner_invoc!();

The general rule is that a macro B will shadow some "earlier" macro A if:

  • B is defined in a "subinvocation" of the one that defined A (as above)
  • B is XX (see my questions below)

Copy link
Contributor Author

@petrochenkov petrochenkov Sep 4, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I think "shadowed by"/"not shadowed by" is not the right way to think about it, that also leads to confusion here and in #53778 (comment).

Macro from inner scope always shadows a macro from outer scope:

// Fully expanded
macro m() {} // Outer
{
    macro m(); // Inner, always shadows outer
}

the question is whether an instance of shadowing is "good" or "bad", which is determined entirely by their relative expansion order (earlier/later relations on expansions).
I hoped to convey that in the comments to may_appear_after, but probably not as successfully as I thought.

I'll try to reread this comment and #53778 (comment) later this evening and say something more specific.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I.. well I'm not sure I see but I may be starting to see. =) I sort of had the sense I was thinking about it wrong. I will try to revisit this with this paragraph in mind.

// See more detailed explanation in
// https://github.com/rust-lang/rust/pull/53778#issuecomment-419224049
fn may_appear_after(&self, invoc_id: Mark, binding: &NameBinding) -> bool {
// self > max(invoc_id, binding) => !(self <= invoc_id || 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 invoc_parent_expansion = invoc_id.parent();
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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I understand this. I think this is saying that the self would legally be shadowed by any resolution that happens in "a child" macro.. but is that right? I'm probably missing some bit of context here. I guess I'm imagining that this would apply in a case like:

macro m { .. }
macro foo {
    () => m!()
}
foo!() { }

Here, the invocation foo! is a "sub-expansion" of the one that defined m -- but this seems to suggest that there hence m the decision to use m cannot be shadowed. I guess that means that (e.g.) we have "definition 1" here?

macro m { .. } // definition 1

macro gen_m { () => macro m { ... } }
gen_m!(); // generates definition 2, possibly later

macro foo {
    () => m!() // resolves to definition 1
}
foo!() { }

!(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 @@ -1716,7 +1732,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 @@ -1802,7 +1817,7 @@ impl<'a, 'crateloader: 'a> Resolver<'a, 'crateloader> {
self.arenas.alloc_module(module)
}

fn record_use(&mut self, ident: Ident, ns: Namespace, binding: &'a NameBinding<'a>, span: Span)
fn record_use(&mut self, ident: Ident, ns: Namespace, binding: &'a NameBinding<'a>)
-> bool /* true if an error was reported */ {
match binding.kind {
NameBindingKind::Import { directive, binding, ref used }
Expand All @@ -1811,13 +1826,11 @@ impl<'a, 'crateloader: 'a> Resolver<'a, 'crateloader> {
directive.used.set(true);
self.used_imports.insert((directive.id, ns));
self.add_to_glob_map(directive.id, ident);
self.record_use(ident, ns, binding, span)
self.record_use(ident, ns, binding)
}
NameBindingKind::Import { .. } => false,
NameBindingKind::Ambiguity { b1, b2 } => {
self.ambiguity_errors.push(AmbiguityError {
span, name: ident.name, lexical: false, b1, b2,
});
self.ambiguity_errors.push(AmbiguityError { ident, b1, b2 });
true
}
_ => false
Expand Down Expand Up @@ -2837,7 +2850,7 @@ impl<'a, 'crateloader: 'a> Resolver<'a, 'crateloader> {
Def::Const(..) if is_syntactic_ambiguity => {
// Disambiguate in favor of a unit struct/variant
// or constant pattern.
self.record_use(ident, ValueNS, binding.unwrap(), ident.span);
self.record_use(ident, ValueNS, binding.unwrap());
Some(PathResolution::new(def))
}
Def::StructCtor(..) | Def::VariantCtor(..) |
Expand Down Expand Up @@ -3470,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_invoc_id(base_module, path, opt_ns, Mark::root(),
record_used, path_span, crate_lint)
}

fn resolve_path_with_invoc_id(
&mut self,
base_module: Option<ModuleOrUniformRoot<'a>>,
path: &[Ident],
opt_ns: Option<Namespace>, // `None` indicates a module path
invoc_id: 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 @@ -3559,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, invoc_id, 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 @@ -4501,35 +4529,33 @@ 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, ident: Ident, 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", ident, 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", ident, 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 {
Some(format!("consider adding an explicit import of `{}` to disambiguate", name))
} else if b1.is_glob_import() {
Some(format!("consider adding an explicit import of `{}` to disambiguate", ident))
} 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.note(&format!("`{}` is also a builtin macro", name)),
_ => err.span_note(span2, &msg2),
let mut err = struct_span_err!(self.session, ident.span, E0659, "`{}` is ambiguous", ident);
err.span_label(ident.span, "ambiguous name");
err.span_note(b1.span, &msg1);
match b2.def() {
Def::Macro(..) if b2.span.is_dummy() =>
err.note(&format!("`{}` is also a builtin macro", ident)),
_ => err.span_note(b2.span, &msg2),
};
if let Some(note) = note {
err.note(&note);
Expand All @@ -4538,7 +4564,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 @@ -4554,15 +4579,9 @@ impl<'a, 'crateloader: 'a> Resolver<'a, 'crateloader> {
);
}

for &AmbiguityError { span, name, b1, b2, lexical } 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,
);
for &AmbiguityError { ident, b1, b2 } in &self.ambiguity_errors {
if reported_spans.insert(ident.span) {
self.report_ambiguity_error(ident, b1, b2);
}
}

Expand All @@ -4582,20 +4601,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