Lint when shadowing a glob-reexported item with a local declaration

[obi1kenobi]

Code

mod inner {
    pub struct Foo {};
}

mod other {
    struct Foo;
}

pub use inner::*;

// Adding either of the following two lines
// would shadow `inner::Foo` and
// hide the name `Foo` from the public API.
//
// (1)
// struct Foo;
//
// (2)
// use other::Foo;

Current output

compiles without errors, warnings, or clippy lints

Desired output

struct Foo;
^^^^^^^^^^
shadows `Foo` re-exported from `pub use inner::*`
note: the shadowing prevents `Foo` from being exported by this module, which may break downstream uses

***

use other::Foo;
           ^^^
           shadows `Foo` re-exported from `pub use inner::*`
note: the shadowing prevents `Foo` from being exported by this module, which may break downstream uses

Rationale and extra context

Similar to #107563

Whereas that issue tackled the case of glob-glob name conflicts, this issue is about glob-vs-local name conflicts in which case the local name shadows the one imported from the glob. This is a footgun and can lead to semver-major breaking changes: uncommenting either line (1) or (2) in the code example above is a major breaking change for the containing crate, since it causes the name Foo to stop being exported and makes Foo become an unnamable (Voldemort) type.

This is not a hypothetical issue. Around a year ago, the opencl3 crate seems to have suffered a regression when a large number of its items were accidentally shadowed. Neither code review nor tests seem to have been effective at preventing the regression. The fixing PR for that regression adds a test for the public re-export of one of the affected items.

This seems like a perfect situation for a lint. Shadowing a glob-reexported item with a local declaration is pretty much never what you want. If you wanted an unnamable type, there's a better way, and if you didn't want the item to be reexported then tweak the module structure so it doesn't get reexported.

I plan to publish a blog post with more details about this in a few hours, it will be available at the following link: https://predr.ag/blog/breaking-semver-in-rust-by-adding-private-type-or-import/

Other cases

No response

Anything else?

No response

[jieyouxu]

I would like to take a stab at this
@rustbot claim

[scottmcm]

(Great blog post!)

I wonder if we could go even further here and start linting on any crate export that goes through a glob? (Maybe with some carveouts for, say, modules that reexport an entire other module via glob but include nothing else, or something?)

We're definitely not going to prevent glob imports in general -- especially crate-local ones -- but maybe some sort of "look, this glob escapes the crate" checking could make sense. (Like how the pub-in-priv lints are starting to get smarter about whether things are exported, not just how they're tagged in the definition.)

[obi1kenobi]

I'm not familiar with the lint system internals, but from a purely "protecting the author from unpleasantness" perspective I'd be in favor of such lints.

I continued looking into glob-related risks after publishing the blog posts, and I've come up with things that are arguably even worse than what the blog post showed. All the examples require globs somewhere, so the globs are the thing to target with lints:

• changing pub(super) to pub(crate) can be breaking on an unrelated type: https://twitter.com/PredragGruevski/status/1656745204819324928
• removing a private field on a private type can be breaking on an unrelated type: https://twitter.com/PredragGruevski/status/1656668014044155907
• vice versa is also breaking both times: adding a private field / reducing field visibility can be breaking as well

I'm adding examples like these to the test suite of trustfall-rustdoc-adapter (a component of cargo-semver-checks that allows querying a single rustdoc JSON file), to make sure cargo-semver-checks catches semver breakage caused by such issues. You're welcome to reuse any of the code examples there: obi1kenobi/trustfall-rustdoc-adapter#172