Add new lint to warn when #[must_use] attribute should be used on a method #8071
Conversation
|
r? @xFrednet (rust-highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
GuillaumeGomez
force-pushed
the
method-must-use
branch
from
eb71297 to
8f03d3d
Compare
| hir_id: HirId, | ||
| ) { | ||
| if_chain! { | ||
| // We are only interested in methods, not in functions or associated functions. |
There was a problem hiding this comment.
Why not? What about fn new() -> Self, fn new_foo(bar: Baz) -> Self or fn from_blurb(blurb: Blurb) -> Self?
There was a problem hiding this comment.
They look like different use cases to me. What I'm trying to prevent here is cases like 0f32.rad(2.). It's much less likely (in my opinion) if it's not a method.
There was a problem hiding this comment.
Ah sure, if you don't want to cover that case here that makes sense. I also think it's quite unlikely that someone calls such a constructor function and forgets to actually do something with the result. Adding #[must_use] on all these might add quite a bit of noise to the code.
GuillaumeGomez
force-pushed
the
method-must-use
branch
from
8f03d3d to
f305926
Compare
|
Updated with @sdroege's suggestions. |
|
I'd call it For the category I'd do |
GuillaumeGomez
force-pushed
the
method-must-use
branch
2 times, most recently
from
ca04751 to
aecc38d
Compare
|
@camsteffen This is indeed much better, I renamed the lint. |
GuillaumeGomez
force-pushed
the
method-must-use
branch
from
aecc38d to
63b6787
Compare
|
And updated the category to pedantic. |
GuillaumeGomez
force-pushed
the
method-must-use
branch
from
63b6787 to
56d2ab3
Compare
GuillaumeGomez
force-pushed
the
method-must-use
branch
from
56d2ab3 to
037d52e
Compare
GuillaumeGomez
force-pushed
the
method-must-use
branch
from
037d52e to
e93767b
Compare
|
Updated with comments from @xFrednet. :) |
|
Everything looks good to me, thank you for the work! I ran the lint on 25 crates with lintcheck and got about 60 reports, the once I've looked at seem to be correct. 🙃 @bors r+ |
|
📌 Commit e93767b has been approved by |
Add new lint to warn when #[must_use] attribute should be used on a method This lint is somewhat similar to https://rust-lang.github.io/rust-clippy/master/index.html#must_use_candidate but also different: it emits a warning by default and only targets methods (so not functions nor associated functions). Someone suggested it to me after this tweet: https://twitter.com/m_ou_se/status/1466439813230477312 I think it would reduce the number of cases of API misuses quite a lot. What do you think?
|
💔 Test failed - checks-action_test |
|
Clippy created the changelog based on the PR description. I added the corresponding message 🙃 @bors r+ |
Add new lint to warn when #[must_use] attribute should be used on a method This lint is somewhat similar to https://rust-lang.github.io/rust-clippy/master/index.html#must_use_candidate but also different: it emits a warning by default and only targets methods (so not functions nor associated functions). Someone suggested it to me after this tweet: https://twitter.com/m_ou_se/status/1466439813230477312 I think it would reduce the number of cases of API misuses quite a lot. What do you think? --- changelog: Added new [`return_self_not_must_use`] lint
|
☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test |
|
Thanks! |
This lint is somewhat similar to https://rust-lang.github.io/rust-clippy/master/index.html#must_use_candidate but also different: it emits a warning by default and only targets methods (so not functions nor associated functions).
Someone suggested it to me after this tweet: https://twitter.com/m_ou_se/status/1466439813230477312
I think it would reduce the number of cases of API misuses quite a lot.
What do you think?
changelog: Added new [
return_self_not_must_use] lint