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.

Sign up for GitHub

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

Jump to bottom

Add useless_anonymous_reexport lint #109003

Merged
merged 3 commits into from
Mar 19, 2023
Merged

Add useless_anonymous_reexport lint #109003

merged 3 commits into from
Mar 19, 2023

Conversation

GuillaumeGomez
Copy link
Member

@GuillaumeGomez GuillaumeGomez commented Mar 10, 2023 •
edited
Loading

This is a follow-up of #108936. We once again show all anonymous re-exports in rustdoc, however we also wanted to add a lint to let users know that it very likely doesn't have the effect they think it has.

@rustbot
Copy link
Collaborator

rustbot commented Mar 10, 2023

r? @TaKO8Ki

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

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Mar 10, 2023
@GuillaumeGomez GuillaumeGomez force-pushed the useless-anonymous-reexport-lint branch from fd77cfe to 0884258 Compare March 10, 2023 22:17
@cjgillot
Copy link
Contributor

This lint would probably be easier to implement in rustc_resolve::check_unused, which is the place where we lint unused imports already.

@GuillaumeGomez
Copy link
Member Author

@cjgillot It doesn't seem that they are considered as potentially unused since they don't appear in Resolver::potentially_unused_imports. Are you suggesting that I should add them directly into this Resolver and modify check_unused.rs heavily?

@cjgillot
Copy link
Contributor

UnusedImportCheckVisitor is responsible for populating the list of unused imports, beyond potentially_unused_imports. The check for the visibility of the use item is in visit_item.

potentially_unused_imports is a actually a small list with only extern crate and #[macro_use].

@GuillaumeGomez
Copy link
Member Author

So unless I missed something, UnusedImportCheckVisitor is using the EarlyLintPass. However, I need to have access to TyCtxt to compute the actual visibility of the re-export, so I don't think it's a good match. Also, in here we want to warn for useless re-exports, not about unused imports.

@GuillaumeGomez GuillaumeGomez force-pushed the useless-anonymous-reexport-lint branch from 0884258 to 6d2b775 Compare March 11, 2023 10:33
@GuillaumeGomez
Copy link
Member Author

Greatly improved the detection of re-export following suggestion.

@bors
Copy link
Contributor

bors commented Mar 12, 2023

☔ The latest upstream changes (presumably #108682) made this pull request unmergeable. Please resolve the merge conflicts.

@GuillaumeGomez GuillaumeGomez force-pushed the useless-anonymous-reexport-lint branch from 6d2b775 to fcadc03 Compare March 12, 2023 14:12
cjgillot
cjgillot reviewed Mar 12, 2023
View reviewed changes
tests/ui/lint/anonymous-reexport.rs
pub use self::my_mod::Foo as _;
pub use self::my_mod::TyFoo as _;
pub use self::my_mod::Bar as _; //~ ERROR
pub use self::my_mod::TyBar as _; //~ ERROR
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we suggest to remove it?
Could you add tests with nested uses:

pub use self::my_mod::{Bar as _};
pub use self::my_mod::{Bar as _, Foo as _};
pub use self::my_mod::{Bar as _, TyBar as _};

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding tests with nested uses.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we suggest to remove it?

I'm not sure. If you prefer I add this suggestion, I can.

compiler/rustc_lint/messages.ftl Outdated Show resolved Hide resolved
@GuillaumeGomez GuillaumeGomez force-pushed the useless-anonymous-reexport-lint branch from e89c425 to 9272b69 Compare March 12, 2023 15:33
@GuillaumeGomez GuillaumeGomez force-pushed the useless-anonymous-reexport-lint branch from 9272b69 to 7b0fa08 Compare March 12, 2023 15:39
@GuillaumeGomez
Copy link
Member Author

Added what was missing.

@cjgillot
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Mar 18, 2023

📌 Commit 7b0fa08 has been approved by cjgillot

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 18, 2023
@Dylan-DPC Dylan-DPC mentioned this pull request Mar 19, 2023
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 19, 2023
@bors
Auto merge of rust-lang#109346 - Dylan-DPC:rollup-vszi5bn, r=Dylan-DPC
8826b68 
Rollup of 10 pull requests

Successful merges:

 - rust-lang#104100 (Allow using `Range` as an `Iterator` in const contexts. )
 - rust-lang#105793 (Add note for mismatched types because of circular dependencies)
 - rust-lang#108798 (move default backtrace setting to sys)
 - rust-lang#108829 (Use Edition 2021 :pat in matches macro)
 - rust-lang#108973 (Beautify pin! docs)
 - rust-lang#109003 (Add `useless_anonymous_reexport` lint)
 - rust-lang#109022 (read_buf_exact: on error, all read bytes are appended to the buffer)
 - rust-lang#109212 (fix: don't suggest similar method when unstable)
 - rust-lang#109243 (The name of NativeLib will be presented)
 - rust-lang#109324 (Implement FixedSizeEncoding for UnusedGenericParams.)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 462e7e7 into rust-lang:master Mar 19, 2023
@rustbot rustbot added this to the 1.70.0 milestone Mar 19, 2023
@GuillaumeGomez GuillaumeGomez deleted the useless-anonymous-reexport-lint branch March 19, 2023 19:10
petrochenkov
petrochenkov reviewed Mar 21, 2023
View reviewed changes
compiler/rustc_lint/src/reexports.rs
if let ItemKind::Use(path, kind) = item.kind &&
!matches!(kind, UseKind::Glob) &&
item.ident.name == kw::Underscore &&
// We only want re-exports. If it's just a `use X;`, then we ignore it.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this important?
A private use some::Struct as _ is equally useless.

The original suggestion from @cjgillot to use UnusedImportCheckVisitor and report this early looks like the right direction.

This case doesn't need a separate lint, existing unused_imports should be perfectly fine.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This lint multiplies entities without necessity, I think it is better to remove it before it hit stable.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(I noticed this while auditing uses of imports in HIR for #109176 (comment).)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This lint multiplies entities without necessity

How so?

I think it is better to remove it before it hit stable.

Replaced, sure. Otherwise please don't remove it altogether.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How so?

Is there any scenario when you'd want to enable/deny reporting this case specifically, separately from other unused imports? I don't think so.

Replaced, sure. Otherwise please don't remove it altogether.

Yeah, I mean merge into unused_imports, not remove outright.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Also, the lint name doesn't match conventions.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok I see. Well, since both you and cjgillot think so, I'll move it into unused imports.

@GuillaumeGomez GuillaumeGomez mentioned this pull request Mar 22, 2023
Merged
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 23, 2023
@matthiaskrgr
Rollup merge of rust-lang#109487 - GuillaumeGomez:move-useless-reexpo…
3961ef5 
…rt-check, r=petrochenkov

Move useless_anynous_reexport lint into unused_imports

As mentioned in rust-lang#109003, this check should have been merged with `unused_imports` in the start.

r? `@petrochenkov`
RalfJung pushed a commit to RalfJung/miri that referenced this pull request Mar 24, 2023
@matthiaskrgr
Rollup merge of #109487 - GuillaumeGomez:move-useless-reexport-check,…
b5a04bf 
… r=petrochenkov

Move useless_anynous_reexport lint into unused_imports

As mentioned in rust-lang/rust#109003, this check should have been merged with `unused_imports` in the start.

r? `@petrochenkov`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fmease fmease fmease left review comments

@petrochenkov petrochenkov petrochenkov left review comments

@cjgillot cjgillot Awaiting requested review from cjgillot

Assignees

@TaKO8Ki TaKO8Ki

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.70.0
Development

Successfully merging this pull request may close these issues.
7 participants
@GuillaumeGomez @rustbot @cjgillot @bors @petrochenkov @fmease @TaKO8Ki