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

[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