Lint ambiguous glob re-exports

[jieyouxu]

Attempts to fix #107563.

We currently already emit errors for ambiguous re-exports when two names are re-exported specifically, i.e. not from glob exports. This PR attempts to emit deny-by-default lints for ambiguous glob re-exports.

[rustbot]

r? @wesleywiser

(rustbot has picked a reviewer for you, use r? to override)

[petrochenkov]

The right place to catch this is probably fn finalize_resolutions_in.
(fn is_reexport can be inlined and removed.)

r? @petrochenkov
@rustbot author

[jieyouxu]

Hmm, I don't know if this lint is possible given https://github.com/rust-lang/rfcs/blob/master/text/1560-name-resolution.md#multiple-unused-imports, namely

A name may be imported multiple times, it is only a name resolution error if that name is used. E.g.,

Unless there's a way to liberally interpret this to constrain public re-exports at the crate boundary such that there cannot be ambiguous names even in glob exports?

As in, is there a way to limit this lint to only be active for public glob re-exports with the ambiguous names at the crate boundary? Since I would like to avoid it firing for imports if possible...

[petrochenkov]

A name may be imported multiple times, it is only a name resolution error if that name is used.

Yes, right now glob vs glob conflicts are explicitly allowed by the language, regardless of their publicity, to avoid breaking third-party code when adding new items to modules as much as possible.
I think we can try making this a lint for a start, just to check what breaks.

You can start with reporting the lint only for pub imports (import.expect_vis().is_pub()).
If that gives false positives then it can be switched to effective_visibilities.is_exported(import.id()).

[petrochenkov]

@jieyouxu
Maybe the effective visibility table is not populated yet when you code is run.

[jieyouxu]

@jieyouxu Maybe the effective visibility table is not populated yet when you code is run.

I think you are right...

DEBUG rustc_resolve::imports this.effective_visibilities=EffectiveVisibilities { map: {} }

I think effective visibilities is computed after finalize_imports is called...

[petrochenkov]

Yes, compute_effective_visibilities should be run after finalize_imports.

It shouldn't be a problem though, finalize_resolutions_in to which you add the code is run in a very simple loop.

        for module in self.r.arenas.local_modules().iter() {
            self.finalize_resolutions_in(module);
        }

So a similar loop can be run at some later point when effective visibilities are ready.

[petrochenkov]

If something suspicious is happening to stdarch, then it's better to report it to stdarch.

[petrochenkov]

I also suggest changing this diagnostic from a hard error to a deny-by-default lint so we could run it through crater and look at the results.
(See uses of buffer_lint in rustc_resolve for examples of lint reporting.)

[jieyouxu]

I also suggest changing this diagnostic from a hard error to a deny-by-default lint so we could run it through crater and look at the results. (See uses of buffer_lint in rustc_resolve for examples of lint reporting.)

I made it into a deny-by-default lint now. I'm not sure what changed, but CI isn't failing on core_arch::arm anymore...? Is it because it's not a hard error anymore?

[petrochenkov]

@bors try

[petrochenkov]

Meta: I'm not sure what is the process for adding new lints right now, people have been adding lints casually, with one reviewer approval, but that's not the official policy, from what I remember, and probably not a good idea in general.

Change description for the lang team.

Conflicts (ambiguities) between glob imports are not reported eagerly, but only if the ambiguous name is actually used.

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

use m1::*;
use m2::*; // OK, not an error

fn main() {
    let _s = S {}; // ERROR, `S` is ambiguous
}

This PR adds a lint that reports warning for cases in which this ambiguous name is visible from other crates.
(In that case it also produces an error on use, possibly with a different wording.)
That happens if we replace the imports from the previous example with

pub use m1::*;
pub use m2::*; // OK, not an error, but will error if `S` is actually used from a different crate,
                        // as a result `S` is effectively not accessible to other crates.

This is a lint and not a hard error because "visible from other crates" is not entirely precise, especially with macros 2.0, and also because this is a breakage that would be too large to land.
This lint is in rustc and not in clippy because it requires access to some name resolver internals to detect exported ambiguities, and I'd better keep those internals private.

We warn on this case, because people may expect some names be available to other crates due to pub use, but they unexpectedly end up unavailable (possibly introducing breaking changes to libraries). cc #107563

The fix is to disambiguate the name explicitly with a single import.

pub use m1::S;
pub use m1::*;
pub use m2::*;

I propose merging this lint as warn-by-default.

[scottmcm]

Linting ambiguous globs sounds good to me.

@rfcbot fcp merge

See petrochenkov's comment #107880 (comment).

[rfcbot]

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

• @joshtriplett
• @nikomatsakis
• @pnkfelix
• @scottmcm
• @tmandry

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.

[rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[rfcbot]

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

[jieyouxu]

@rustbot ready

[petrochenkov]

@bors r+

[bors]

📌 Commit 1f67949 has been approved by petrochenkov

It is now in the queue for this repository.

[petrochenkov]

@bors rollup=maybe