Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consider adding a lint for ambiguous re-exports, since they are unusable #107563

Closed
obi1kenobi opened this issue Feb 1, 2023 · 4 comments · Fixed by #107880
Closed

Consider adding a lint for ambiguous re-exports, since they are unusable #107563

obi1kenobi opened this issue Feb 1, 2023 · 4 comments · Fixed by #107880
Assignees
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-lints Area: Lints (warnings about flaws in source code) such as unused_mut. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@obi1kenobi
Copy link
Member

obi1kenobi commented Feb 1, 2023

Code

pub mod a {
    pub struct Foo {}
}

pub mod b {
    // Some other unrelated stuff here,
    // for example:
    pub struct Bar {}
    
    // Later on,
    // the following struct is added:
    //
    // pub struct Foo {}
    //
    // Uncommenting the line above breaks
    // all downstream uses of `this_crate::Foo`:
    // - the name is ambiguous,
    // - ambiguous names only error if used,
    // - there's no lint or warning for this.
    //
    // Unless `this_crate` has tests that
    // *specifically* use `this_crate::Foo` itself,
    // the odds are excellent that this bug
    // only gets discovered after breaking downstream.
}

pub use a::*;
pub use b::*;

Current output

compiles without errors, warnings, or clippy lints

Desired output

pub use b::*;
        ^^^^ this includes b::Foo
note: re-exported Foo is ambiguous, since a::Foo is also re-exported
      this will cause an error when attempting to import Foo from this module

Rationale and extra context

It makes sense to defer ambiguous import errors until the ambiguous import is used. However, the same policy currently applies to ambiguous re-exports: they do not error at the time they are created, and instead error only when they are attempted to be used. But an ambiguous re-export is almost certainly unintentional and an error, as it is completely unusable.

Mistakes like in the example code above have excellent odds of being discovered only when they break downstream: there's currently no error, lint, or warning from any tool, and most crates don't write test that use the re-exported names of items so cargo test probably won't find the problem either.

A bit more discussion (including a playground link) here: https://hachyderm.io/@predrag/109786028398707576

Other cases

No response

Anything else?

I wasn't sure if this should be a rustc lint, a clippy lint, or something else. I decided to start here but I wouldn't mind moving this to the clippy issue tracker if that's the better place for this lint.

@obi1kenobi obi1kenobi added A-diagnostics Area: Messages for errors, warnings, and lints T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Feb 1, 2023
@estebank estebank added the A-lints Area: Lints (warnings about flaws in source code) such as unused_mut. label Feb 2, 2023
@jieyouxu

This comment was marked as outdated.

@jieyouxu

This comment was marked as outdated.

@jieyouxu

This comment was marked as outdated.

@jieyouxu
Copy link
Member

@rustbot claim

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Mar 20, 2023
Lint ambiguous glob re-exports

Attempts to fix rust-lang#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.
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this issue Mar 21, 2023
Lint ambiguous glob re-exports

Attempts to fix rust-lang#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.
@bors bors closed this as completed in bacf059 Mar 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-lints Area: Lints (warnings about flaws in source code) such as unused_mut. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants