wip: Anonymous Trait Import

[RuairidhWilliamson]

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]

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]

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]

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]

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]

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]

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]

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]

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

[bors]

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

[y21]

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]

@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]

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]

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

[y21]

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]

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]

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]

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

[RuairidhWilliamson]

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]

we could probably add it to Cargo.toml to be part of dogfood? I really think this is a good thing :tm: 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]

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 :)

[RuairidhWilliamson]

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

[y21]

Great, thank you for implementing this lint!

@bors r+

[bors]

:pushpin: Commit 739ef7bf0d5686ef10e7700f34e34995936e00ff has been approved by y21

It is now in the queue for this repository.

[bors]

:hourglass: Testing commit 739ef7bf0d5686ef10e7700f34e34995936e00ff with merge 7951e02a6a0f3d0eb2b18690f8ab6556662caa0b...

[bors]

:broken_heart: Test failed - checks-action_test

[y21]

PR description was missing a changelog: line, I edited it in.

@bors retry

[bors]

:hourglass: Testing commit 739ef7bf0d5686ef10e7700f34e34995936e00ff with merge abdf173ef2972033e1b5ac78e1bbc886b7f871c1...

[bors]

:sunny: Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test Approved by: y21 Pushing abdf173ef2972033e1b5ac78e1bbc886b7f871c1 to master...

[RuairidhWilliamson]

@y21 Great thank you for all your help, I'm glad we got this added!