new lint: needless traits in scope #10503

woshilapin · 2023-03-14T22:03:44Z

changelog: [needless_traits_in_scope]: trait is needlessly imported in trait's namespace

This is my first contribution to rust-clippy.

I propose a lint to find traits that are imported with a use but trait's name is not used afterwards (only methods). Importing traits with as _ gives 2 advantages:

not keeping an unused name in the trait namespace
helps identifying traits (compared to struct) in the use statements>

I do not consider this PR finished, because I'm looking for first feedbacks... and a little help for the FIXME in the UI test case (how to check the trait's name is actually not used in the rest of the scope).

And don't worry about telling me the lint is dumb, the journey trying stuff with rust-clippy so far has been enlightening anyway.

rustbot · 2023-03-14T22:03:49Z

r? @llogiq

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

llogiq

I don't think the lint is stupid. It will be useful for those who like their namespaces tidy. I left a few comments, hope they are helpful. Please comment here or ping us on zulip if you need further assistance.

@rustbot author

llogiq · 2023-03-15T09:48:25Z

clippy_lints/src/needless_traits_in_scope.rs

+    /// ```
+    #[clippy::version = "1.69.0"]
+    pub NEEDLESS_TRAITS_IN_SCOPE,
+    pedantic,


I think this should be a restriction lint. It's nothing that should be frowned upon in normal Rust code and makes the code longer, but it could bring benefits in certain situations (those you mentioned).

Indeed, better than pedantic. Fixed in 02db985.

llogiq · 2023-03-15T09:49:15Z

clippy_lints/src/needless_traits_in_scope.rs

+    /// It also helps identify the traits in `use` statements.
+    ///
+    /// ### Why is this bad?
+    /// This needlessly brings a trait in trait's namespace.


Suggested change

/// This needlessly brings a trait in trait's namespace.

/// This needlessly brings a trait in trait's namespace, where it could

/// shadow other things. This is not really a problem, this lint is just

/// for those who like to keep things tidy.

Done in ef7fac2, thank you for the improvement.

llogiq · 2023-03-15T09:55:03Z

clippy_lints/src/needless_traits_in_scope.rs

+    pedantic,
+    "trait is needlessly imported in trait's namespace, and can be anonymously imported"
+}
+declare_lint_pass!(NeedlessTraitsInScope => [NEEDLESS_TRAITS_IN_SCOPE]);


You'll want to write your own struct so you can impl_lint_pass! here instead. Then you can add a Visitor to your LintPass that you use on check_crate to run through the code and collect any trait use. Please leave a comment or ping me on zulip if you need help with that.

bors · 2023-03-16T10:51:56Z

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

clippy_lints/src/needless_traits_in_scope.rs

Co-authored-by: Ruby Lazuli <[email protected]>

llogiq · 2023-03-24T22:35:06Z

If you need further assistance for implementing the visitor, feel free to ping us here or on zulip.

woshilapin · 2023-03-25T05:53:43Z

If you need further assistance for implementing the visitor, feel free to ping us here or on zulip.

Thanks for getting back to me. To be honest, I've had little time since I opened the PR to explore the visitor solution. But the little I looked, I'm sure some help might help me.

Let met start with what I (think I) understood. check_crate will run before check_item which means I can do a first parsing to store stuff in the struct NeedlessTraitsInScope that I will use later in check_item to decide if I need to yield the lint or not.

I also understand that instead of

declare_lint_pass!(NeedlessTraitsInScope => [NEEDLESS_TRAITS_IN_SCOPE]);

I can write the following

struct NeedlessTraitsInScope {
  // some HashMap ?
}
impl_lint_pass!(NeedlessTraitsInScope => [NEEDLESS_TRAITS_IN_SCOPE]);

Now, it's fuzzy what I'm missing, but I'll try to make some questions:

I'm not sure how to parse the entire crate with a visitor?
- I've seen a for_each_expr, but I'm not sure how to detect what I need (should I only focus on ExprKind::Call and ExprKind::MethodCall)
- since check_crate only give me a LateContext, I'm not sure how to use for_each_expr which seems to take a impl Visitable
I'm not even sure how to store what I found during check_crate parsing
- because I need to remember that I found a Path used... but I also need to remember where it was used (in which module of which file)

In any case, I'll probably be slow since I don't have a lot of personal time these days to work on this. I hope it's not a problem ☺️

llogiq · 2023-03-25T12:29:21Z

No problem. That's a good start; everything you wrote seems correct to me. Now the way using a Visitor works is: in check_crate(..) you construct your visitor, call its visit_crate method that will walk the whole crate and can collect the data. After calling that, you'll deconstruct the visitor to get the collected data, which you store in your LintPass.

For the visitor itself, you'll want to override the visit_expr method to check Calls and MethodCalls.

woshilapin · 2023-03-26T14:12:57Z

I tried implementing the Visitor. With the hands on it, most of the help you provided now make sense (much more that when I read first). But I'm having some problems with the Visitor.

I started implementing rustc_hir::intravisit::Visitor for struct UsedTraitVisitor (a new struct for visiting)... but then, I couldn't find the visit_crate() for this Visitor trait.

Parsing through the documentation, I actually found a visit_crate on rustc_ast::visit::Visitor, so I tried to implement this instead (not really sure of the difference between the two). With rustc_ast::visit::Visitor, I now have a difficulty to call visit_crate that needs a rustc_ast::ast::Crate. I'm not sure how to access rustc_ast::ast::Crate from LateContext. I did found cx.tcx.crate_for_resolver() that returns a Steal<(rustc_ast::ast::Crate, _)> but using steal.borrow().0 seems to panic at runtime (even looks like an ICE, asking to report to the compiler).

Note: I usually do not like pushing broken code, but tell me if that can help.

llogiq · 2023-03-28T19:35:05Z

Sorry, my bad. I hadn't looked into the intravisit code in a while. cx.tcx().visit_all_item_likes_in_crate(&mut visitor) should do the crate walking for you.

woshilapin · 2023-04-04T06:06:16Z

Sorry, my bad. I hadn't looked into the intravisit code in a while. cx.tcx().visit_all_item_likes_in_crate(&mut visitor) should do the crate walking for you.

Hi, thanks for the help. I actually didn't mention it in my previous comment, but I did already explore visit_all_item_likes_in_crate: it looked like a dead end when I did. It does explore all rustc_hir::Item, but I'm interested in all rustc_hir::Expr (the Visitor::visit_expr is never called, only Visitor::visit_item). I also found out about clippy_utils::visitors::for_each_expr, but it does only take a impl Visitable which, at the time, didn't seem to be implemented by the things I care (the entire crate or at least, each module 🤷).

After digging, I'll try to explore the following option:

visit_all_item_likes_in_crate with a visitor implementing visit_item
when visiting ItemKind::Fn, use for_each_expr on the body (I should be able to get BodyId from ItemKind::Fn, then get the Body<'tcx> from cx.tcx.hir().body(body_id))

That said, this does not look like a general solution. If that does work, I might also need to explore other ItemKind (Macro and Impl for example ? 🤔 )

llogiq · 2023-04-04T18:50:34Z

You can just implement visit_expr, the default implementations will take care of the rest

J-ZhengLi · 2024-03-29T02:33:42Z

Hi @woshilapin , just a friendly reminder that this PR is being inactive for a while now (just pass its anniversary 😄).

So, if you're still interested in working on this, plz let us know. And if you have any further questions, don't hesitate to ask people in this zulip channel, cheers!

woshilapin · 2024-04-02T08:47:52Z

@J-ZhengLi Thank you for following-up. I don't really plan to work on it anymore, I don't really have the time. And the complexity of the task is a bit more than I had in mind when I started this. I'll close it. Feel free to re-open if you prefer to keep it open.

woshilapin · 2024-11-28T22:44:22Z

For those interested in this work, someone else actually did land a new clippy lint in 1.83 (see #13322).

rustbot assigned llogiq Mar 14, 2023

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Mar 14, 2023

new lint: needless traits in scope

fb4bacb

woshilapin force-pushed the needless_traits_in_scope branch from 2642bb4 to fb4bacb Compare March 14, 2023 22:05

llogiq reviewed Mar 15, 2023

View reviewed changes

rustbot added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Mar 15, 2023

woshilapin added 3 commits March 15, 2023 13:05

fix: check if the used path is a trait

2e082ba

fix: needless_traits_in_scope should be a 'restriction' lint

02db985

docs: improve description for needless_traits_in_scope

ef7fac2

PatchMixolydic suggested changes Mar 16, 2023

View reviewed changes

clippy_lints/src/needless_traits_in_scope.rs Outdated Show resolved Hide resolved

clippy_lints/src/needless_traits_in_scope.rs Outdated Show resolved Hide resolved

clippy_lints/src/needless_traits_in_scope.rs Outdated Show resolved Hide resolved

docs: trait are in type namespace (not trait namespace)

a6e854e

Co-authored-by: Ruby Lazuli <[email protected]>

woshilapin closed this Apr 2, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

new lint: needless traits in scope #10503

new lint: needless traits in scope #10503

woshilapin commented Mar 14, 2023

rustbot commented Mar 14, 2023

llogiq left a comment

llogiq Mar 15, 2023

woshilapin Mar 15, 2023

llogiq Mar 15, 2023

woshilapin Mar 15, 2023 •

edited

Loading

llogiq Mar 15, 2023

bors commented Mar 16, 2023

llogiq commented Mar 24, 2023

woshilapin commented Mar 25, 2023

llogiq commented Mar 25, 2023

woshilapin commented Mar 26, 2023

llogiq commented Mar 28, 2023

woshilapin commented Apr 4, 2023

llogiq commented Apr 4, 2023

J-ZhengLi commented Mar 29, 2024

woshilapin commented Apr 2, 2024 •

edited

Loading

woshilapin commented Nov 28, 2024

-    /// This needlessly brings a trait in trait's namespace.
+    /// This needlessly brings a trait in trait's namespace, where it could
+    /// shadow other things. This is not really a problem, this lint is just
+    /// for those who like to keep things tidy.

new lint: needless traits in scope #10503

new lint: needless traits in scope #10503

Conversation

woshilapin commented Mar 14, 2023

rustbot commented Mar 14, 2023

llogiq left a comment

Choose a reason for hiding this comment

llogiq Mar 15, 2023

Choose a reason for hiding this comment

woshilapin Mar 15, 2023

Choose a reason for hiding this comment

llogiq Mar 15, 2023

Choose a reason for hiding this comment

woshilapin Mar 15, 2023 • edited Loading

Choose a reason for hiding this comment

llogiq Mar 15, 2023

Choose a reason for hiding this comment

bors commented Mar 16, 2023

llogiq commented Mar 24, 2023

woshilapin commented Mar 25, 2023

llogiq commented Mar 25, 2023

woshilapin commented Mar 26, 2023

llogiq commented Mar 28, 2023

woshilapin commented Apr 4, 2023

llogiq commented Apr 4, 2023

J-ZhengLi commented Mar 29, 2024

woshilapin commented Apr 2, 2024 • edited Loading

woshilapin commented Nov 28, 2024

woshilapin Mar 15, 2023 •

edited

Loading

woshilapin commented Apr 2, 2024 •

edited

Loading