Unused trait imports (formerly anonymous trait import) #13322

RuairidhWilliamson · 2024-08-29T18:29:28Z

For #11969

I'm looking for help and feedback on implementing a new lint for suggesting use ... as _ for traits where possible.

I have had a go at implementing this but I don't know if this is the best way to do it as I am new to clippy.

There are some edge cases I can think of where this doesn't work but have aired on the side of false negatives instead of false positives.

An example of a false negative. I couldn't figure out the best way to resolve an import from within clippy. The sub module imports MyAny so that cannot be anonymized but use std::any::Any could be. In this case it is not caught because Any and MyAny have the same DefId.

mod nested_mod_used_bad1 {
    use std::any::Any;
    use std::any::Any as MyAny;
    mod foo {
        use crate::nested_mod_used_bad1::MyAny;
        fn foo() {
            println!("{:?}", MyAny::type_id("foo"));
        }
    }
}

Any feedback is much appreciated.

rustbot · 2024-08-29T18:29:34Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @y21 (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

@rustbot author: the review is finished, PR author should check the comments and take action accordingly
@rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

y21 · 2024-08-30T08:35:57Z

I've not properly looked at the changes here in full detail but some initial comments:

There are some edge cases I can think of where this doesn't work but have aired on the side of false negatives instead of false positives.

I'd say that this is fine and even preferred (false negatives over false positives), especially for a lint that's going to be as trigger-heavy as this one.

I wonder though, instead of reimplementing a part of resolution, could this use tcx.maybe_unused_trait_imports().contains(use_def_id)? If I remember right, that set is created in early name resolution for imports that aren't referred to by path but may be used for method resolution in typeck, which sounds like it might be of use here?
The current way of walking the whole module and all its contents for every single import seems like it could get expensive, but that might just be premature

RuairidhWilliamson · 2024-08-30T19:41:28Z

Thank you for the response.

You are right. maybe_unused_trait_imports solves exactly this problem and I was able to remove the import resolution. I am not sure how I missed that query but it has made the code much cleaner.

I had to change some of the tests but I think they are correct now and fixed the false negatives.

nyurik · 2024-09-01T17:04:15Z

Awesome lint, thanks!!! I would even suggest removing the wip from the subject.

You used Applicability::MaybeIncorrect - are there any such known bad false positive cases? If so, you may want to add them, at least as a disabled code block, to the tests - so that in the future they can be handled. Also, do add all the false negatives as tests - even though they will be ignored for now, others may improve their handling.

Lastly, a tiny minor nit - you may want to move the main() {} to the top of the test file - this way new test cases will be added at the end, and the line numbering will stay intact in the blessed results.

RuairidhWilliamson · 2024-09-01T20:36:46Z

Thanks @nyurik :)

Yes I agree it shouldn't be wip anymore.

You used Applicability::MaybeIncorrect - are there any such known bad false positive cases? If so, you may want to add them, at least as a disabled code block, to the tests - so that in the future they can be handled. Also, do add all the false negatives as tests - even though they will be ignored for now, others may improve their handling.

Yes, good point. The change to using maybe_unused_trait_imports() has cleared up the edge cases I can think of so I will make it MachineApplicable.

Lastly, a tiny minor nit - you may want to move the main() {} to the top of the test file - this way new test cases will be added at the end, and the line numbering will stay intact in the blessed results.

Ah that is a good point. I have moved main to the top.

y21

A lot of the comments that I had before when I looked at this first are no longer relevant and fixed now with the extra visitor removed so that's nice :)

clippy_lints/src/anon_trait_import.rs

y21 · 2024-09-01T21:09:40Z

clippy_lints/src/anon_trait_import.rs

+            && let module = cx.tcx.parent_module_from_def_id(item.owner_id.def_id)
+            && cx.tcx.visibility(item.owner_id.def_id) == Visibility::Restricted(module.to_def_id())
+            && let Some(last_segment) = path.segments.last()
+            && let Some(snip) = snippet_opt(cx, last_segment.ident.span)


Would be good to have some macro checks to prevent cases where the lint is coming from an external macro.
For example the first 100 results here are almost all from within a bitflags! macro expansion where the user can't change anything: https://github.com/rust-lang/rust-clippy/actions/runs/10657161173

Can use !in_external_macro(..)&&!is_from_proc_macro(..) for that

For some reason using is_from_proc_macro(cx, item) wouldn't trigger when testing with with_span!. I managed to get it working by using is_from_proc_macro(cx, &last_segment.ident) but I don't understand why that worked and the former didn't.

tests/ui/anon_trait_import.rs

clippy_lints/src/anon_trait_import.rs

y21 · 2024-09-01T22:32:46Z

You used Applicability::MaybeIncorrect - are there any such known bad false positive cases?

FWIW, something like use path::to::Trait::{self}; currently has a wrong/breaking suggestion (suggests use path::to::Trait as _::{self}), but it seems like a weird thing to write, considering that trait-relative items can't be imported, so {self} would always be on its own. So I'd say it's fine to ignore that and not block machine-applicability on it

RuairidhWilliamson · 2024-09-03T00:13:43Z

Thanks for the review.

You used Applicability::MaybeIncorrect - are there any such known bad false positive cases?

FWIW, something like use path::to::Trait::{self}; currently has a wrong/breaking suggestion (suggests use path::to::Trait as _::{self}), but it seems like a weird thing to write, considering that trait-relative items can't be imported, so {self} would always be on its own. So I'd say it's fine to ignore that and not block machine-applicability on it

Ah yes that is an interesting edge case. I have had a look at the HIR to see if it is easy to ignore this pattern but it seems to be hard to distinguish at that level.

use std::any::Any::{self};

use ::{};
use std::any::Any;

I guess maybe I could use the span to check the source manually but that feels messy.

If you are happy that it is not an issue that needs to be addressed then I am too :)

bors · 2024-09-03T17:46:07Z

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

bors · 2024-09-05T23:29:15Z

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

y21

Looks good to me aside from a few small things

y21 · 2024-09-08T23:27:28Z

clippy_lints/src/anon_trait_import.rs

+    /// }
+    /// ```
+    #[clippy::version = "1.82.0"]
+    pub ANON_TRAIT_IMPORT,


Suggested change

pub ANON_TRAIT_IMPORT,

pub ANON_TRAIT_IMPORTS,

Lint names should generally be in plural so it makes sense when read as "allow anon trait imports"

y21 · 2024-09-08T23:46:35Z

clippy_lints/src/anon_trait_import.rs

+    /// Traits imported that aren't used directly can be imported anonymously `use Trait as _`.
+    /// It is more explicit, avoids polutting the current scope with unused names and can be useful to show which imports are required for traits.


Suggested change

/// Traits imported that aren't used directly can be imported anonymously `use Trait as _`.

/// It is more explicit, avoids polutting the current scope with unused names and can be useful to show which imports are required for traits.

/// Traits imported that aren't used directly can be imported anonymously with `use Trait as _`.

/// It is more explicit, avoids polluting the current scope with unused names and can be useful to show which imports are required for traits.

y21 · 2024-09-08T23:49:48Z

clippy_lints/src/anon_trait_import.rs

+    /// ```
+    #[clippy::version = "1.82.0"]
+    pub ANON_TRAIT_IMPORT,
+    nursery,


Do you intend to move this to a different category before the merge or keep it in the nursery for now?

Usually new lints start out in a more defined category. Nursery is typically for lints that are known to be buggy

Good point, I have moved it to pedantic.

y21 · 2024-09-08T23:53:20Z

This might be more or less tangential to this lint but we might also want to adjust rustc's suggestion for importing traits for methods. I imagine it would be somewhat annoying to apply the compiler's suggestion to import a trait only to immediately trigger a clippy lint.

nyurik · 2024-09-09T18:14:10Z

@y21 i agree, rustc should suggest use Foo as _; rather than use Foo when the trait method is being used. This would both prevent inconsistencies with this lint, as well as teach users about namespace pollution. Would you like to create an issue for that?

RuairidhWilliamson · 2024-09-09T21:07:21Z

I have found another sub optimal scenario but don't think it is bad enough to prevent merge.

It would be nice if multiple renames that import the same trait gets suggested to combine into a single import. Currently this produces a suggestion for each import meaning running --fix produces the second output.

use std::any::{Any, Any as MyAny};

After running --fix:

use std::any::{Any as _, Any as _};

Rustc will lint one of the imports as unused so should highlight the issue.

bors · 2024-09-11T15:58:45Z

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

y21 · 2024-09-11T16:24:36Z

It would be nice if multiple renames that import the same trait gets suggested to combine into a single import. Currently this produces a suggestion for each import meaning running --fix produces the second output.

I think that's fine. Or at least, I don't think this situation would be common enough to be an actual problem. It seems odd to import the same trait twice to begin with IMO.
We have a few other lints that could theoretically run into this, where the suggestion would trigger more warnings in specific edge cases, which is ok as long as there's no contradictory warnings.

y21 · 2024-09-11T16:25:08Z

tests/ui/anon_trait_imports.rs

Please also add a test for making sure that #[allow(clippy::anon_trait_imports)] on the use item works. You might have to include anon_trait_imports here to avoid a false positive on the useless_attribute lint:

rust-clippy/clippy_lints/src/attrs/useless_attribute.rs

Lines 44 to 46 in 131681b

matches!(

symbol.as_str(),

"wildcard_imports"

There's tests for #[allow] on the enclosing module and function now, but not one for the use item directly as far as I can tell. I mean something like

#[allow(clippy::anon_trait_imports)] use std::any::Any; "".type_id();

Can you also add one for that?

Good point, added.

There is a false positive for useless_attribute in this case

Being able to allow the lint on the item that it warns on is something that should really work for all lints (without triggering other lints), so it would be good if you could fix that false positive for this lint. It should be fairly straightforward, you just need to add it to the matches! macro (see initial comment)

Sorry, yeah I forgot that you explained how to fix that. Fixed 👍

y21 · 2024-09-13T14:08:25Z

Opened a thread on zulip for the FCP process that new lints need to go through: https://rust-lang.zulipchat.com/#narrow/stream/257328-clippy/topic/FCP.3A.20anon_trait_imports

Implementation looks good to me aside from the test change that I commented on, but that shouldn't involve any major changes in a way that would block the FCP.

kpreid · 2024-09-13T15:16:46Z

Isn't this lint named backwards? Lints are named for what they report, not what they suggest you do instead, and this lint is objecting to unused names and suggesting an anonymous import.

How about naming it unused_trait_names?

bors · 2024-09-13T17:03:59Z

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

RuairidhWilliamson · 2024-09-19T20:15:02Z

Based on feedback in zulip I've renamed the lint from anon_trait_imports -> unused_trait_names and moved the lint from pedantic to restriction (which means it doesn't get run as dogfood).

nyurik · 2024-09-19T20:17:35Z

we could probably add it to Cargo.toml to be part of dogfood? I really think this is a good thing ™️ to have ... so if we make this more visible, and if we make the compiler suggest code in this format, eventually it could make it to pedantic and then to style...

RuairidhWilliamson · 2024-09-19T22:03:09Z

we could probably add it to Cargo.toml to be part of dogfood? I really think this is a good thing ™️ to have ... so if we make this more visible, and if we make the compiler suggest code in this format, eventually it could make it to pedantic and then to style...

yeah I agree but maybe it is best to do it in a follow up PR as it adds a lot of changes across clippy :)

y21

Looks good to me now aside from two small things. Can you also squash the commits here a bit?

I also agree that if we're going to apply this style to the code base and enable the lint it should be done in a separate PR since it'll probably involve a discussion, which doesn't really need to block the lint itself

y21 · 2024-09-20T21:17:01Z

clippy_lints/src/unused_trait_names.rs

+}
+
+pub struct UnusedTraitNames {
+    msrv: Msrv,


The lint also needs to be added to the list here now that it respects the MSRV configuration (forgot to mention that earlier)

rust-clippy/clippy_config/src/conf.rs

Line 510 in 9be28b1

#[lints(

y21 · 2024-09-20T21:19:39Z

clippy_lints/src/unused_trait_names.rs

+    ///     println!("{s}");
+    /// }
+    /// ```
+    #[clippy::version = "1.82.0"]


Suggested change

#[clippy::version = "1.82.0"]

#[clippy::version = "1.83.0"]

RuairidhWilliamson · 2024-09-21T00:06:54Z

I've addressed your review and squashed everything into two commits :)

rustbot assigned y21 Aug 29, 2024

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Aug 29, 2024

RuairidhWilliamson force-pushed the anon-trait-import branch from 20376b5 to 01856a3 Compare September 1, 2024 20:33

RuairidhWilliamson changed the title ~~wip: Anonymous Trait Import~~ Anonymous Trait Import Sep 1, 2024

y21 reviewed Sep 1, 2024

View reviewed changes

This was linked to issues Sep 2, 2024

Unname method-only trait imports #9021

Open

New lint: trait_not_impl #11570

Open

Suggest use ... as _ when importing traits only #11969

Open

Suggestion to anonymise trait imports that are not used directly #12072

Open

RuairidhWilliamson force-pushed the anon-trait-import branch from 8d6272b to 34db62c Compare September 3, 2024 18:31

y21 approved these changes Sep 8, 2024

View reviewed changes

RuairidhWilliamson force-pushed the anon-trait-import branch from 34db62c to c63465f Compare September 9, 2024 20:59

y21 reviewed Sep 11, 2024

View reviewed changes

RuairidhWilliamson force-pushed the anon-trait-import branch from c63465f to a287de0 Compare September 11, 2024 20:11

y21 added S-final-comment-period Status: final comment period it will be merged unless new objections are raised (~1 week) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Sep 15, 2024

RuairidhWilliamson force-pushed the anon-trait-import branch from 694ebb5 to dfc5a52 Compare September 19, 2024 20:12

y21 approved these changes Sep 20, 2024

View reviewed changes

RuairidhWilliamson added 2 commits September 21, 2024 00:56

Add lint unused_trait_names

05ebce8

Add unused_trait_names tests

739ef7b

RuairidhWilliamson force-pushed the anon-trait-import branch from dfc5a52 to 739ef7b Compare September 21, 2024 00:04

RuairidhWilliamson changed the title ~~Anonymous Trait Import~~ Unused trait imports (formerly anonymous trait import) Sep 21, 2024

		/// Traits imported that aren't used directly can be imported anonymously `use Trait as _`.
		/// It is more explicit, avoids polutting the current scope with unused names and can be useful to show which imports are required for traits.

Unused trait imports (formerly anonymous trait import) #13322

Are you sure you want to change the base?

Unused trait imports (formerly anonymous trait import) #13322

Conversation

RuairidhWilliamson commented Aug 29, 2024

rustbot commented Aug 29, 2024

y21 commented Aug 30, 2024

RuairidhWilliamson commented Aug 30, 2024

nyurik commented Sep 1, 2024

RuairidhWilliamson commented Sep 1, 2024

y21 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

y21 commented Sep 1, 2024

RuairidhWilliamson commented Sep 3, 2024

bors commented Sep 3, 2024

bors commented Sep 5, 2024

y21 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

y21 commented Sep 8, 2024

nyurik commented Sep 9, 2024

RuairidhWilliamson commented Sep 9, 2024

bors commented Sep 11, 2024

y21 commented Sep 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

y21 commented Sep 13, 2024

kpreid commented Sep 13, 2024

bors commented Sep 13, 2024

RuairidhWilliamson commented Sep 19, 2024

nyurik commented Sep 19, 2024

RuairidhWilliamson commented Sep 19, 2024

y21 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RuairidhWilliamson commented Sep 21, 2024

y21 left a comment •

edited

Loading

y21 commented Sep 11, 2024 •

edited

Loading