Add warn-by-default lint when local binding shadows exported glob re-export item

[jieyouxu]

This PR introduces a warn-by-default rustc lint for when a local binding (a use statement, or a type declaration) produces a name which shadows an exported glob re-export item, causing the name from the exported glob re-export to be hidden (see #111336).

Unresolved Questions

• Is this approach correct? While it passes the UI tests, I'm not entirely convinced it is correct. Seems to be ok now.
• What should the lint be called / how should it be worded? I don't like calling use x::*; or struct Foo; a "local binding" but they are NameBindings internally if I'm not mistaken. The lint is called local_binding_shadows_glob_reexport for now, unless a better name is suggested. hidden_glob_reexports.

Fixes #111336.

[rustbot]

r? @b-naber

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

[petrochenkov]

r? @petrochenkov

[petrochenkov]

Similarly to #107880 it's better to run this check somewhere near finalize_imports.

At that point the main binding is available as resolution.binding and the shadowed glob binding is available as resolution.shadowed_glob.
Effective visibilities are also available there, so we'll be able to report this lint only for actually exported globs (again, similarly to #107880).

@rustbot author

[petrochenkov]

glob_binding.res().opt_def_id() is the glob import's target, of course it's shadowed and not exported.
Instead I suggest checking effective visibility of the glob import item itself.

(But if that fails too we can always fall back to vis.is_public().)

[jieyouxu]

Instead I suggest checking effective visibility of the glob import item itself.

Sorry how do I check this?

[petrochenkov]

shadowed_glob -> kind: NameBindingKind::Import { import, .. } -> import.id() -> resolver.local_def_id(id)

[petrochenkov]

but it seems self.effective_visibilities.is_exported(import_local_def_id) is still returning false

On which test case?
If you are using tests/ui/resolve/local-shadows-glob-reexport.rs then both glob imports there are indeed not exported (not usable from other crates) because upstream_a and upstream_b are private.

[jieyouxu]

If I change upstream_a and upstream_b to pub mods

#![deny(local_binding_shadows_glob_reexport)]

pub mod upstream_a {
    mod inner {
        pub struct Foo {}
    }

    pub use self::inner::*;

    struct Foo;
    //~^ ERROR local binding shadows glob re-export
}

pub mod upstream_b {
    mod inner {
        pub struct Foo {}
    }

    mod other {
        pub struct Foo;
    }

    pub use self::inner::*;

    use self::other::Foo;
    //~^ ERROR local binding shadows glob re-export
}

// Downstream crate
// mod downstream {
//     fn proof() {
//         let _ = crate::upstream_a::Foo;
//         let _ = crate::upstream_b::Foo;
//     }
// }

pub fn main() {}

then the glob re-export items should be exported right?

[petrochenkov]

They should be exported if you add any additional non-shadowed names to them, e.g. pub struct Bar {} to mod inner.

As written in #111378 (comment) they should not be exported and should be reported as unused because they don't reexport anything at all.
Although in ui tests lints from the unused group are allowed by default, so the unused warnings may be lost unless enabled explicitly.

[jieyouxu]

They should be exported if you add any additional non-shadowed names to them, e.g. pub struct Bar {} to mod inner.

Ohh right that makes sense. Yeah the check works if I add additional non-shadowed names to mod inner.

@rustbot ready

[jieyouxu]

error: local binding shadows glob re-export
   --> compiler/rustc_middle/src/ty/mod.rs:148:1
    |
73  | pub use rustc_type_ir::*;
    |         ---------------- the name `sty` in the type namespace is introduced by the glob reexport here
...
148 | mod sty;
    | ^^^^^^^^ but the local binding here shadows the name `sty` in the type namespace
    |

I think the lint correctly fires here? The glob re-export item pub use rustc_type_ir::* is exported because there are other pub items apart from sty, and rustc_type_ir::sty would be shadowed by the local mod sty in rustc_middle.

[petrochenkov]

compiler/rustc_middle/src/ty/mod.rs:148:1

Yes, looks exactly like a case which this lint is supposed to catch.

[jieyouxu]

In this case should I just band-aid a #![allow(local_binding_shadows_glob_reexport)] for the mod sty or do I expand pub use rustc_type_ir::*; into everything needed modulo sty?

[petrochenkov]

Probably rename the private mod sty to something else.

[petrochenkov]

DEBUG rustc_resolve::imports check_hidden_glob_reexports: self.effective_visibilities.is_exported(glob_import_def_id) = true

Not in case of tests/ui/shadowed/shadowed-trait-methods.rs

https://play.rust-lang.org/?version=nightly&mode=debug&edition=2015&gist=e5d5a14962010be4938af1c2eec9ac16

[petrochenkov]

Checking that the resolutions are not Res::Err helped with tests/ui/shadowed/shadowed-trait-methods.rs but not with tests/ui/imports/issue-55884-2.rs.

r=me after allowing the lint in tests/ui/imports/issue-55884-2.rs (just for the shadowing import, not for the whole test).

[jieyouxu]

@rustbot ready

[petrochenkov]

@bors r+

[bors]

📌 Commit b960658 has been approved by petrochenkov

It is now in the queue for this repository.

[bors]

⌛ Testing commit b960658 with merge b9c5fdc...

[bors]

☀️ Test successful - checks-actions
Approved by: petrochenkov
Pushing b9c5fdc to master...

[rust-timer]

Finished benchmarking commit (b9c5fdc): comparison URL.

Overall result: ❌ regressions - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	0.4%	[0.4%, 0.4%]	1
Regressions ❌ (secondary)	0.4%	[0.4%, 0.4%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.4%	[0.4%, 0.4%]	1

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	2.3%	[2.0%, 2.5%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	2.0%	[2.0%, 2.0%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	2.0%	[2.0%, 2.0%]	1

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 645.492s -> 645.767s (0.04%)

[petrochenkov]

@jieyouxu
I've just noticed that the labels here go in a span order, but they should go in a fixed order instead ("supposed to be publicly re-exported" first, and "but the private" second).

If the order is not fixed, then it looks weird, see for example https://crater-reports.s3.amazonaws.com/pr-111505/try%23590af02d982816cfb2e4e43f8c54065e816581ce/reg/assemble-std-0.2.0/log.txt

[INFO] [stdout] warning: private item shadows public glob re-export
[INFO] [stdout]   --> src/lib.rs:20:5
[INFO] [stdout]    |
[INFO] [stdout] 20 | use assemble_core::Project;
[INFO] [stdout]    |     ^^^^^^^^^^^^^^^^^^^^^^ but the private item here shadows it
[INFO] [stdout] ...
[INFO] [stdout] 26 | pub use assemble_core::*;
[INFO] [stdout]    |         ---------------- the name `Project` in the type namespace is supposed to be publicly re-exported here

[jieyouxu]

Thanks for the heads up, I'll fix this tomorrow. EDIT: Currently trying to find out how I can sort them in fixed order and not span order.