Add `useless_anonymous_reexport` lint by GuillaumeGomez · Pull Request #109003 · rust-lang/rust

GuillaumeGomez · 2023-03-10T21:22:52Z

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 · 2023-03-10T21:22:58Z

r? @TaKO8Ki

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

compiler/rustc_lint/src/reexports.rs

cjgillot · 2023-03-10T22:25:45Z

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

GuillaumeGomez · 2023-03-10T22:40:59Z

@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 · 2023-03-10T23:11:11Z

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 · 2023-03-11T10:26:42Z

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 · 2023-03-11T10:34:22Z

Greatly improved the detection of re-export following suggestion.

bors · 2023-03-12T07:41:12Z

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

cjgillot · 2023-03-12T15:06:11Z

tests/ui/lint/anonymous-reexport.rs

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 _};

Adding tests with nested uses.

Should we suggest to remove it?

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

compiler/rustc_lint/messages.ftl

GuillaumeGomez · 2023-03-15T13:45:12Z

Added what was missing.

cjgillot · 2023-03-18T14:26:21Z

@bors r+

bors · 2023-03-18T14:26:23Z

📌 Commit 7b0fa08 has been approved by cjgillot

It is now in the queue for this repository.

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

petrochenkov · 2023-03-21T15:15:52Z

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.


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.

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

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

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.

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.

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

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

…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`

… 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`

rustbot assigned TaKO8Ki Mar 10, 2023

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 commented Mar 10, 2023

View reviewed changes

compiler/rustc_lint/src/reexports.rs Outdated Show resolved Hide resolved

GuillaumeGomez force-pushed the useless-anonymous-reexport-lint branch from fd77cfe to 0884258 Compare March 10, 2023 22:17

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

Add lint for useless anonymous reexports

2df7770

GuillaumeGomez force-pushed the useless-anonymous-reexport-lint branch from 6d2b775 to fcadc03 Compare March 12, 2023 14:12

cjgillot reviewed Mar 12, 2023

View reviewed changes

Add test for useless_anonymous_reexport lint

ac4ea52

GuillaumeGomez force-pushed the useless-anonymous-reexport-lint branch from e89c425 to 9272b69 Compare March 12, 2023 15:33

Update failing ui tests

7b0fa08

GuillaumeGomez force-pushed the useless-anonymous-reexport-lint branch from 9272b69 to 7b0fa08 Compare March 12, 2023 15:39

GuillaumeGomez requested a review from cjgillot March 16, 2023 12:56

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 mentioned this pull request Mar 19, 2023

Rollup of 10 pull requests #109346

Merged

bors merged commit 462e7e7 into rust-lang:master Mar 19, 2023

rustbot added this to the 1.70.0 milestone Mar 19, 2023

GuillaumeGomez deleted the useless-anonymous-reexport-lint branch March 19, 2023 19:10

petrochenkov reviewed Mar 21, 2023

View reviewed changes

GuillaumeGomez mentioned this pull request Mar 22, 2023

Move useless_anynous_reexport lint into unused_imports #109487

Merged

Uh oh!

Conversation

GuillaumeGomez commented Mar 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented Mar 10, 2023

Uh oh!

Uh oh!

cjgillot commented Mar 10, 2023

Uh oh!

GuillaumeGomez commented Mar 10, 2023

Uh oh!

cjgillot commented Mar 10, 2023

Uh oh!

GuillaumeGomez commented Mar 11, 2023

Uh oh!

GuillaumeGomez commented Mar 11, 2023

Uh oh!

bors commented Mar 12, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

GuillaumeGomez commented Mar 15, 2023

Uh oh!

cjgillot commented Mar 18, 2023

Uh oh!

bors commented Mar 18, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

GuillaumeGomez commented Mar 10, 2023 •

edited

Loading