resolve: Report more visibility-related early resolution ambiguities for imports

[petrochenkov]

The new ambiguities are reported when the import's visibility is ambiguous and may depend on the resolution/expansion order.

Detailed description: #149596 (comment).

[rustbot]

r? @lcnr

rustbot has assigned @lcnr.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[petrochenkov]

@bors try

[petrochenkov]

@craterbot run mode=check-only p=1 crates=https://crater-reports.s3.amazonaws.com/pr-149145/retry-regressed-list.txt

[rust-bors]

☀️ Try build successful (CI)
Build commit: ce3348c (ce3348c21531b911ca5aadbbbe7ad63a1bc4c382, parent: 8202d110516c6bd87cee375e9a9b774db499015c)

[petrochenkov]

@craterbot run mode=check-only p=1 crates=https://crater-reports.s3.amazonaws.com/pr-149145/retry-regressed-list.txt

[craterbot]

👌 Experiment pr-149596 created and queued.
🤖 Automatically detected try build ce3348c
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[craterbot]

🚧 Experiment pr-149596 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[craterbot]

🎉 Experiment pr-149596 is completed!
📊 359 regressed and 0 fixed (3583 total)
📊 218 spurious results on the retry-regessed-list.txt, consider a retry¹ if this is a significant amount.
📰 Open the summary report.

⚠️ If you notice any spurious failure please add them to the denylist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

Footnotes

1. re-run the experiment with crates=https://crater-reports.s3.amazonaws.com/pr-149596/retry-regressed-list.txt ↩

[petrochenkov]

@bors try

[rust-bors]

☀️ Try build successful (CI)
Build commit: e617cc9 (e617cc9342f64b2efdd8eeb91834ef450a15825f, parent: 29e035e1728488dce5a03daa527ab1091d1cda47)

[craterbot]

🎉 Experiment pr-149596-1 is completed!
📊 30 regressed and 0 fixed (577 total)
📊 32 spurious results on the retry-regessed-list.txt, consider a retry¹ if this is a significant amount.
📰 Open the summary report.

⚠️ If you notice any spurious failure please add them to the denylist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

Footnotes

1. re-run the experiment with crates=https://crater-reports.s3.amazonaws.com/pr-149596-1/retry-regressed-list.txt ↩

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[petrochenkov]

Change description for lang team

Early name resolution has a number of ambiguity errors that are reported when a name declaration that is "unreliable" in some sense (e.g. comes from a glob import or macro expansion) may appear during macro expansion and shadow some more reliable declaration. The rules are documented in rust-lang/reference#2055.

Currently those ambiguity errors are not always reported.
They are not reported if the two ambiguous declarations (which may be imports) point to the same end definition (Res)
In that case the ambiguity is considered "benign", even if the exact import chain that was used to resolve the name is unpredictable and depends on the algorithm internals.

In general, the benign ambiguities are not really benign, because the exact import chains are important (#141043 (comment)),
but we cannot outright make them errors because in practice it causes too much breakage to existing code.
So we need to address more targeted problematic cases, when they are discovered.

One such case related to visibilities was discovered in rust-embed crate in the import resolution parallelization PR that changed some algorithm internals and affected the import resolution order.
In pseudocode form the case looks like:

// Outer scope
pub(crate) decl;

// "Unreliable" inner scope
// Both `decl`s refer to the same definition
pub decl;

// There's an ambiguity, and the reexport will either
// - report a privacy error (cannot reexport `pub(crate)` as `pub`)
//   - or produce a `pub(crate)` declaration, if there's some other `pub` item called `decl` in a different namespace, that's what happens in `rust-embed`
// - successfully produce a `pub` declaration
// depending on unpredictable resolution internals.
// (An import's visibility is a minimum between it's own `use` item's visibility and its target declaration's visibility.)
pub use decl as rename;

This PR reports a future-incompatibility lint called ambiguous_import_visibilities for cases like this.
Specifically, if:

• two declarations are ambiguous (the criterias are the same as before, and documented in Add section on expansion-time (early) name resolution reference#2055)
• two declarations have the same definition, but different visibilities vis(decl1) and vis(decl2)
• min(vis(decl1), vis(import)) != min(vis(decl2), vis(import)), i.e. the resulting import visibility is unpredictable.

The lint is warn-by-default and not reported in dependencies for now.
Crater run showed 30 regressions from this lint - #149596 (comment).

[bors]

☔ The latest upstream changes (presumably #147984) made this pull request unmergeable. Please resolve the merge conflicts.

[tmandry]

During the lang meeting today, Niko and I batted back and forth some ideas on how this model could be simpler by adopting a model where import chains don't matter for final visibility. In this model, all that matters is the original item visibility and the final import visibility, assuming each import along the way is allowed according to the current scope visibility. (We decided to take the discussion to Zulip and didn't discuss the other reasons you gave in #141043 (comment) for why import chains matter.)

After the meeting, @Nadrieril pointed out that this is problematic when talking about glob imports (should have seen that one coming..), and would silently change the public API of existing crates:

mod internal {
  mod inner {
    pub struct Foo;
  }
  pub(crate) use inner::Foo;
  pub struct Bar;
}
pub use internal::*; // `Foo` would now be part of the public API of the crate

While I still like the idea of simplifying the story for non-glob imports, we can't change the public API of existing crates. Adopting this model is therefore going to require one of two options:

1. Changing the behavior of glob imports over an edition. This of course requires keeping the current semantics in old editions.
2. Making glob imports look at the visibility of the next link in the chain (transitively, if they hit another glob import). So in the example above, the pub(crate) on pub(crate) use inner::Foo would cause Foo not to be re-exported as pub by the glob import.
   • This can result in the same kinds of ambiguity issues addressed by this PR, because which import a glob import imports from matters – and there can be multiple possibilities. In the above example, this can arise if there was a use inner::*; inside of internal. To be honest, it seems kinda like the glob imports are colluding to make our lives more difficult.

Either way, we are going to need to handle ambiguous cases like the ones caught by this PR.

[tmandry]

@petrochenkov thanks for the writeup in #149596 (comment). This seems like a reasonable step to make our model more precise. I hope it eventually helps us unblock efforts on parallel resolution.

As I detailed in my comment above, the kind of ambiguity this PR catches is going to matter whether we like it or not. So we might as well start cleaning them up now.

@rfcbot fcp merge lang

[tmandry]

@rfcbot fcp merge lang

[rust-rfcbot]

Team member @tmandry has proposed to merge this. The next step is review by the rest of the tagged team members:

• @joshtriplett
• @nikomatsakis
• @scottmcm
• @tmandry
• @traviscross

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns.
See this document for info about what commands tagged team members can give me.

[traviscross]

Possibly-relevant rules, by the way, in the pending Reference PR, include:

r[names.resolution.expansion.imports.shadowing.shared-scope]
Shadowing of names introduced via use declarations within a single scope is permitted in the following situations:

• [use glob shadowing]
• [Macro textual scope shadowing]

r[names.resolution.expansion.expansion-order-stability]
The resolution of names must be stable. After expansion, names in the fully expanded AST must resolve to the same definition regardless of the order in which macros are expanded and imports are resolved.

r[names.resolution.expansion.speculation]
All name resolution candidates selected during macro expansion are considered speculative. Once the crate has been fully expanded, all speculative import resolutions are validated to ensure that macro expansion did not introduce any new ambiguities.

r[names.resolution.expansion.imports.ambiguity.intro]
Some situations are an error when there is an ambiguity as to which macro definition, use declaration, or module an import or macro invocation's name refers to. This happens when there are two name candidates that do not resolve to the same entity where neither candidate is [permitted] to shadow the other.

r[names.resolution.expansion.imports.ambiguity.glob-vs-outer]
Names may not be resolved through glob imports when there is another candidate available in an [outer scope].

r[names.resolution.expansion.imports.ambiguity.path-vs-textual-macro]
Names may not be resolved through ambiguous macro re-exports. Macro re-exports are ambiguous when they would shadow a textual macro candidate for the same name in an [outer scope].

r[names.resolution.expansion.macros.ambiguity.more-expanded-vs-outer]
Name bindings from macro expansions may not shadow name bindings from outside of those expansions.

(See the PR for examples for each of these rules and explanatory admonitions.)

[traviscross]

Let's talk through what we're doing here. Today, this is an error:

mod m {
    pub struct S {}
}
macro_rules! mac { () => { struct S {} }}

pub use m::*;
mac!();

pub use S as _;
//~^ error[E0659]: `S` is ambiguous

Why? What do the rules say? First:

r[names.resolution.expansion.imports.ambiguity.intro]
Some situations are an error when there is an ambiguity as to which macro definition, use declaration, or module an import or macro invocation's name refers to. This happens when there are two name candidates that do not resolve to the same entity where neither candidate is [permitted] to shadow the other.

Are there two name candidates? Yes; the struct in m and the one defined via the macro invocation.

Do they resolve to the same entity? No; they have two distinct definitions.

Is one permitted to shadow the other? Let's see.

r[names.resolution.expansion.imports.shadowing.shared-scope]
Shadowing of names introduced via use declarations within a single scope is permitted in the following situations:

• [use glob shadowing]
• [Macro textual scope shadowing]

This is indeed use glob shadowing, which says:

r[items.use.glob.shadowing]
Items and named imports are allowed to shadow names from glob imports in the same [namespace]. That is, if there is a name already defined by another item in the same namespace, the glob import will be shadowed.

So this would be allowed normally. If S were used in a function body during primary name resolution (late resolution), it would be accepted). But S is used in an import, which is resolved during expansion-time resolution (early resolution).

r[names.resolution.expansion.expansion-order-stability]
The resolution of names must be stable. After expansion, names in the fully expanded AST must resolve to the same definition regardless of the order in which macros are expanded and imports are resolved.

Because the struct S is introduced by mac!(), it might not be visible when S in pub use S as _ is first resolved. If the compiler resolves S before expanding mac!(), it would see m::S (from the glob). If it expands mac!() first, it would see the struct from the macro invocation. Since the resolution depends on the order of expansion, it violates the stability rule and reports an error.

Conversely, this is accepted today:

mod m {
    pub struct S {}
}
macro_rules! mac { () => { use m::S; }}

pub use m::*;
mac!();

pub use S as _; //~ OK

Why? Are there two name candidates? Yes. But now they resolve to the same entity. The ambiguity still exists, but because both resolution paths lead to the same definition, the compiler currently considers it benign and accepts it.

Should we lint about this? @petrochenkov gives three rules:

1. The two declarations are ambiguous.
2. The two declarations have the same definition but different visibilities.
3. min(vis(decl1), vis(import)) != min(vis(decl2), vis(import))

Taking them in turn:

One. What's ambiguous mean? As I read this (and, @petrochenkov or @yaahc, correct me if I'm wrong), this is suggesting that two declarations are ambiguous when they are candidates for the same name in the same scope and neither is permitted to shadow the other or the resolution order would make the shadowing unpredictable. I.e., they are ambiguous when we would give an error if the definitions of those declarations are not the same.

So these declarations are ambiguous.

Two. They have the same definition. Do they have different visibilities? Yes. pub (from pub use m::*) vs private (from use m::S).

Three: The visibility of the import is pub (from pub use S as _), so the minimum visibility will be the visibility of each declaration. As above, the declarations have different visibilities.

QED. We lint this case.

---

Net-net, with this FCW, we're reducing the scope of what we consider to be a benign ambiguity.

Based on that understanding...

@rfcbot reviewed

[traviscross]

What's ambiguous mean? As I read this (and, @petrochenkov or @yaahc, correct me if I'm wrong), this is suggesting that two declarations are ambiguous when they are candidates for the same name in the same scope and neither is permitted to shadow the other or the resolution order would make the shadowing unpredictable. I.e., they are ambiguous when we would give an error if the definitions of those declarations are not the same.

I do note a possible counterexample to the way I stated it here (in the i.e.). We give an error for this:

mod m1 {
    pub struct S {}
}
mod m2 {
    pub struct S {}
}

pub mod m3 {
    pub use crate::m1::*;
    use crate::m2::*;
}
pub use m3::S as _;
//~^ error[E0659]: `S` is ambiguous

But the PR branch does not warn if we make the definition for each declaration the same:

mod m1 {
    pub struct S {}
}

pub mod m3 {
    pub use crate::m1::*;
    use crate::m1::*;
}
pub use m3::S as _;

Do we not consider these declarations ambiguous, or what's the rule we're using to decide to not lint here?