New lint: missing_must_use - #16919
Conversation
missing_must_use]: new lintmissing_must_use]: new restriction-type lint
|
Lintcheck changes for c8f3e8f
This comment will be updated if you push new changes |
missing_must_use]: new restriction-type lintmissing_must_use
|
r? @llogiq rustbot has assigned @llogiq. Use Why was this reviewer chosen?The reviewer was selected based on:
|
This comment has been minimized.
This comment has been minimized.
c8e2a9e to
79cdff5
Compare
This comment has been minimized.
This comment has been minimized.
|
@LebedevRI I've added support for |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
045bdb1 to
91fc83e
Compare
|
Some changes occurred in clippy_lints/src/doc cc @notriddle |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Oh no, it seems that there're some problems in rebasing. I'll fix it later. |
…bute case (rust-lang#17144) changelog: [`double_must_use`]: make the lint machine-applicable in single-attribute case After rust-lang#16919, not having this fix-it will be unfortunate, since that lint will cause many new instances of this diag. Refs rust-lang#16856
|
The PR description should also Close #16859, Looking at clippy_lints/src/functions/must_use.rs,
I would suggest having tests for those cases. I would also suggest adding tests with macros (especially external ones), |
91fc83e to
4b239de
Compare
|
This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
4b239de to
933f21d
Compare
|
@LebedevRI I've fixed the rebasing |
|
Reminder, once the PR becomes ready for a review, use |
f231267 to
e14357c
Compare
refactor: renaming. chore: cargo dev fmt chore: cargo dev update_lints feat: add help. fix: ignore external macro. test: add test. doc: update documentation. fix: lint message format. enhance: add suggestion. fix: lint Impl/Trait/Fn.
e14357c to
c8f3e8f
Compare
| // Check if the item is public. If it's not public, we don't need to check it for `#[must_use]`. | ||
| if !cx.tcx.visibility(item.owner_id).is_public() { | ||
| return; | ||
| } | ||
|
|
||
| match item.kind { | ||
| ItemKind::Struct(_, _, data) if !data.fields().is_empty() => check_item(cx, item.hir_id(), item.span), | ||
| ItemKind::Enum(_, _, def) if !def.variants.is_empty() => check_item(cx, item.hir_id(), item.span), | ||
| ItemKind::Union(..) => check_item(cx, item.hir_id(), item.span), |
There was a problem hiding this comment.
FWIW, this doesn't solve #16858.
I've very explicitly noted that it should diagnose everything.
It's fine if such a behaviour can be toggled off (on?) via config, but it should be possible.
There was a problem hiding this comment.
Just so I understand your use case better, why do you want the lint to mark every type?
There was a problem hiding this comment.
Thank you for taking a look!
I do think this check is an improvement even if it only diagnoses non-empty public types,
however, i really do feel like such dichotomy should be configurable,
since then it obviously serves more use-cases that way.
We can of course argue about the defaults,
i'd think it should be more noisy by default, since it's easier to opt-out of something
that you know is there than opt-in into something you don't nessesairly know about.
And it's a restrict check anyways.
Likewise, i've intentionally filed #16858 and #16859 as a separate issues,
so i don't nessesairly think that this check should deal with the fns,
only type/trait definitions.
But to answer the question, mainly, i'm simply of an opinion that Rust got this default wrong,
must-use-ness should have been the other way around.
How often does one have fns that return something that is always discarded at all callsites?
I think that is not a very common pattern...
There was a problem hiding this comment.
While I personally like must-use (which is why I wrote the must_use_candidate lint), I disagree about your assertion that the default is wrong. I believe that defaulting to must-use would have led to the false sense of security with regards to the running of drop impls.
Given that those annotations carry a cost, even if it is just one more thing to ignore while reading the code, we should aim to make the most of them. Therefore my aim was to restrict the lint to cases that matter. That is why I asked for those changes. We can always make the lint more configurable, but the default should carefully balance costs and benefits.
There was a problem hiding this comment.
I don't disagree.
Given that those annotations carry a cost, even if it is just one more thing to ignore while reading the code, we should aim to make the most of them. Therefore my aim was to restrict the lint to cases that matter. That is why I asked for those changes. We can always make the lint more configurable, but the default should carefully balance costs and benefits.
Note: in previous comment, by opt-out of, i was of course talking about the config option. Of course, silencing each particular lint site would be a problematic solution, given that they'd all have the same root-cause (=empty type, etc)
Just please don't make the current behaviour (no diag) non-configurable.
I would say, discovering that half of a lint is off by default and is hidden
behing a config option is a bit hard to discover, though.
But i don't suppose there's much appetite for issuing those edge-cases
under their own lint names either?
(public-sized; public-empty, non-public-sized, non-public-empty)
|
☔ The latest upstream changes (possibly #17329) made this pull request unmergeable. Please resolve the merge conflicts. |
View all comments
Closes #16858
changelog: New lint:
missing_must_usechecklist:
.stderrfile)cargo testpasses locallycargo dev update_lintscargo dev fmt