Add extra_unused_type_parameters lint

[mkrasnitski]

Closes #9240. Keeping this as draft for now, because of a bug that I don't know how to fix. It seems that opaque return types are not walked properly, for some unknown reason. As in, the following:

fn used_ret_opaque<A>() -> impl Iterator<Item = A> {
    std::iter::empty()
}

This triggers the lint even though it shouldn't. Discussion on Zulip didn't illuminate any possible reasons why, so PR-ing this now to increase visibility.

---

changelog: new lint: [extra_unused_type_parameters]
#10028

[rustbot]

r? @dswij

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

[mkrasnitski]

After some digging, it seems the corresponding hir type was added all the way back in rust-lang/rust#54741 (since renamed to TyKind::OpaqueDef), and has only ever contained lifetime bounds on the impl Trait, not any parameter bounds. Does the Item = A count as a parameter bound in this case?

cc-ing @oli-obk

[oli-obk]

When you seen an OpaqueDef you basically need to fetch the HIR of the item it references and walk that, too. We're mangling the HIR quite a bit here to make the queries like generics_of and predicates_of simpler. This is in the process of being changed and I don't know what the end result will be. Maybe for now just mark all generic params as used if there's an OpaqueDef?

[mkrasnitski]

When you seen an OpaqueDef you basically need to fetch the HIR of the item it references and walk that, too.

Turns out walk_ty already does this by calling visit_nested_item on the id. However, that involves a check that Self::NestedFilter::INTER is set, and I'm using OnlyBodies which keeps it false. That's the reason the walk was stopping short of descending down into the OpaqueDef. Maybe that behavior should be changed, since the filter check seems not to have been intended for this edge-case?

[mkrasnitski]

This PR is ready for review/merge, I think.

[flip1995]

r? @flip1995

[flip1995]

Assigning myself, because we talked about this on Zulip a lot. I will probably be busy with release work in the next week, so review might take a bit.

[flip1995]

Here's an early review. I still have to look at the tests and think a little bit about all the cases I can come up with. Impl looks already really clean AFAICT from a quick glance.

[mkrasnitski]

I've found an edge case where the lint doesn't behave quite as expected:

fn nested_bounds<A, B, C, D>()
where
    B: Iterator<Item = A>,
    C: Iterator<Item = B>,
    D: Iterator<Item = C>,
{
}

This produces the following lint output:

warning: type parameter goes unused in function definition
  --> tests/ui/extra_unused_type_parameters.rs:61:27
   |
61 | fn nested_bounds<A, B, C, D>()
   |                           ^
   |
   = help: consider removing the parameter
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#extra_unused_type_parameters

Whereas I would expect the following:

warning: type parameters go unused in function definition
  --> tests/ui/extra_unused_type_parameters.rs:61:17
   |
61 | fn nested_bounds<A, B, C, D>()
   |                 ^^^^^^^^^^^^
   |
   = help: consider removing the parameters
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#extra_unused_type_parameters

This is because I mark parameters used if they appear in the right-hand side of a where-clause. What I failed to consider, was if the left side also goes unused, then I shouldn't mark the nested parameter as unused. What is the consensus here, should this be fixed? Subsequent applications of the lint will eventually get rid of all of the parameters, but it would be nice if they were all detected at once. However, I feel like that would require a fair bit of engineering effort.

[bors]

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

[mkrasnitski]

This is ready for re-review, when you have the time @flip1995

[bors]

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

[bors]

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

[flip1995]

Sorry for taking so long. Last week before vacation in $day_job is a bit stressful. I'll get back to this during the holidays.

[bors]

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

[mkrasnitski]

Will this still be in time to get merged into 1.67? Unsure, since beta has been cut for a while already.

[flip1995]

Sadly not. It would've to get merged today to make it in 1.68. I'm so sorry that I didn't get to review this in time.

[bors]

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

[bors]

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

[flip1995]

I finally managed to get to this. Looks really good overall. I have some style questions left.

You're concern about those cascading bounds is not an issue we should address. Just applying this lint multiple times is totally fine and pretty much how Clippy works. It's not worth putting in the effort to try and fix this and then potentially introducing FPs or FNs.

[flip1995]

You've put so much effort in crafting this awesome spans and message, I think this should be span_lint_and_sugg. The non-multispan cases can then be tested with // run-rustfix in the test file. You'll have to split up the test files for that though: fixable and unfixable.

[mkrasnitski]

I've pushed some changes that hopefully address your concerns. It may be a few days before I can get to changing to using span_lint_and_sugg, as that would require changes in how I'd form the suggestion, and overall it will take a bit of effort.

Also, I'd like to at some point just squash all these commits before the PR is merged, as I don't think they're super informative on their own.

Let me know what you think of the new changes.

[flip1995]

Only one comment left.

[flip1995]

It may be a few days before I can get to changing to using span_lint_and_sugg, as that would require changes in how I'd form the suggestion, and overall it will take a bit of effort.

We can also just do this in a follow up PR. This is not blocking for this PR.

Also, I'd like to at some point just squash all these commits before the PR is merged

Sure, I'll wait for that after approving this PR 👍

[mkrasnitski]

I've gone ahead and squashed everything down. Should be ready for merge now.

[flip1995]

Awesome work! Easy to understand code, really concise and well commented+documented. And great suggestion building (one of my favorite parts when working on diagnostics).

[flip1995]

@bors r+

[bors]

📌 Commit fba16e2 has been approved by flip1995

It is now in the queue for this repository.

[bors]

⌛ Testing commit fba16e2 with merge 8a98609...

[bors]

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: flip1995
Pushing 8a98609 to master...

[bors]

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: flip1995
Pushing 8a98609 to master...

[mkrasnitski]

@flip1995 Thanks for all your help on this work, I really appreciate it!

[flip1995]

Thank you for sticking with it and patiently waiting for my review!

[mkrasnitski]

I now realize it's been pretty much 6 months since I opened the tracking issue for this - I went in with basically zero knowledge about compiler internals, but now I feel a lot better equipped to go off and tackle more work :)

[flip1995]

Sounds great! I would be happy to see more contributions from you to Clippy! But if you want to tackle bigger challenges also have a look at rustc :)