new lint: only_used_in_recursion
#8422
Conversation
- fix code that have variables that is "only used in recursion" - add test
…rsion # Conflicts: # clippy_lints/src/lib.rs
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @llogiq (or someone else) soon. Please see the contribution instructions for more information. |
rust-highfive
added
the
S-waiting-on-review
There was a problem hiding this comment.
This is a good lint 👍! I'd like to see some improvements for the documentation, and perhaps a few comments in the code to clarify things. Also if there are multiple ways to deal with a lint, users will be better served by help messages instead of suggestions.
| /// The could contain a useless calculation and can make function simpler. | ||
| /// | ||
| /// ### Known Issues | ||
| /// It could not catch the variable that has no side effects but only used in recursion. |
There was a problem hiding this comment.
This is unclear to me. Following the tests it does catch arguments that are side-effect free and only used in recursion. So what is not caught? Is there an example I have overlooked?
There was a problem hiding this comment.
I've changed the document for clear explanation and an example that can help understand.
| ONLY_USED_IN_RECURSION, | ||
| span, | ||
| "parameter is only used in recursion with no side-effects", | ||
| "if this is intentional, prefix with an underscore", |
There was a problem hiding this comment.
Wouldn't the optimal way to deal with this be to remove the argument?
There was a problem hiding this comment.
If the function is from trait implementation, the argument needs to be there. Also, I thought since unused_variables suggests prefixing with an underscore, this way would be better for consistency.
There was a problem hiding this comment.
This looks mostly good. Small documentation nits from my side, and I'd like to see a test case with a trait method / trait method impl (the latter should not lint).
Also I'd add the drawback that if the method is publicly visible, following the lint would be API-breaking.
| /// The arguments can be involved in calculations and assignments but as long as | ||
| /// the calculations have no side-effects (function calls or mutating dereference) |
There was a problem hiding this comment.
This sentence belongs to "why is this bad?".
| /// and the assigned variables are also only in recursion, it is useless. | ||
| /// | ||
| /// ### Why is this bad? | ||
| /// The could contain a useless calculation and can make function simpler. |
There was a problem hiding this comment.
| /// The could contain a useless calculation and can make function simpler. | |
| /// The could contain a useless calculation and can make the function simpler. |
|
@llogiq I forgot about traits. I'll add some code to support trait impls. |
|
Please note that we should not lint trait impls (because the argument is part of the trait definition and the impl cannot change that), but may lint definitions. |
| } | ||
| } | ||
|
|
||
| trait B { |
There was a problem hiding this comment.
How about adding an implemented method (which should be linted) here?
There was a problem hiding this comment.
Since trait definition can be in other crate and only some of impls are linted and others are not, I think that current way is better in my opinion.
There was a problem hiding this comment.
We only lint the current crate. So if it contains a trait B { fn rec(a: usize) { if a > 0 { rec(a - 1) } else { 42 } } .. }, we can opt to lint B::rec, as the current crate defines and implements it.
There was a problem hiding this comment.
I see. I'll work on it.
| } | ||
|
|
||
| trait C { | ||
| fn hello(a: usize, b: usize) -> usize { |
There was a problem hiding this comment.
My bad. I'll fix it.
|
Thank you for this PR and sorry for drawing out this process for so long. @bors r+ |
|
📌 Commit 1ad7e70 has been approved by |
new lint: `only_used_in_recursion` changelog: - added `only_used_in_recursion`. - fixed code that variables are only used in recursion. - this would not lint when `unused_variable` Issue: #8390
|
💔 Test failed - checks-action_test |
|
@llogiq I've changed the PR body due to error. Could you try again? |
|
Wouldn't you need to push something first? |
|
It failed on changelog test. |
|
Should I need to push a commit even though it failed on changelog test? |
|
I see. @bors r+ |
|
💡 This pull request was already approved, no need to approve it again.
|
|
📌 Commit 1ad7e70 has been approved by |
new lint: `only_used_in_recursion` changed: - added `only_used_in_recursion`. - fixed code that variables are only used in recursion. - this would not lint when `unused_variable` This fixes: #8390 ----- changelog: add lint [`only_used_in_recursion`]
|
💔 Test failed - checks-action_test |
|
@llogiq I fixed the code. Could you try again? |
|
Sure! @bors r+ |
|
📌 Commit 800f66d has been approved by |
|
☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test |
changed:
only_used_in_recursion.unused_variableThis fixes: #8390
changelog: add lint [
only_used_in_recursion]