-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
New lint: missing_must_use
#16919
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
Kokoro2336
wants to merge
1
commit into
rust-lang:master
Choose a base branch
from
Kokoro2336:feat/type-must-use
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
New lint: missing_must_use
#16919
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,84 @@ | ||
| use clippy_utils::diagnostics::span_lint_and_sugg; | ||
| use clippy_utils::source::snippet_indent; | ||
| use rustc_errors::Applicability; | ||
| use rustc_hir::{HirId, Item, ItemKind, find_attr}; | ||
| use rustc_lint::{LateContext, LateLintPass, LintContext}; | ||
| use rustc_session::declare_lint_pass; | ||
| use rustc_span::Span; | ||
|
|
||
| declare_clippy_lint! { | ||
| /// ### What it does | ||
| /// It finds types that are not marked with `#[must_use]`. | ||
| /// | ||
| /// ### Why restrict this? | ||
| /// Marking a type with `#[must_use]` ensures that the type cannot be silently discarded. | ||
| /// This is especially important for types that represent resources, handles, or results, | ||
| /// where ignoring the value is almost certainly a bug. | ||
| /// | ||
| /// Enabling this lint enforces that every type definition is explicitly considered for | ||
| /// `#[must_use]` annotation, rather than relying on authors to remember to add it. | ||
| /// | ||
| /// Types that genuinely do not need the attribute can be annotated with | ||
| /// `#[expect(clippy::missing_must_use)]` individually with a justifying comment. | ||
| /// | ||
| /// ### Example | ||
| /// ```no_run | ||
| /// struct S(u8); // missing `#[must_use]` and the suggestion to add `#[must_use]` will be triggered. | ||
| /// ``` | ||
| #[clippy::version = "1.97.0"] | ||
| pub MISSING_MUST_USE, | ||
| restriction, | ||
| "finding types that are not marked with `#[must_use]`" | ||
| } | ||
|
|
||
| declare_lint_pass!(MissingMustUse => [MISSING_MUST_USE]); | ||
|
|
||
| fn check_item(cx: &LateContext<'_>, item_hir_id: HirId, span: Span) { | ||
| let attrs = cx.tcx.hir_attrs(item_hir_id); | ||
| if find_attr!(attrs, MustUse { .. }) { | ||
| return; | ||
| } | ||
| let indent = snippet_indent(cx, span).unwrap_or_default(); | ||
| span_lint_and_sugg( | ||
| cx, | ||
| MISSING_MUST_USE, | ||
| span.shrink_to_lo(), | ||
| "missing `#[must_use]` attribute on this type", | ||
| "add #[must_use] to this type definition", | ||
| format!("#[must_use]\n{indent}"), | ||
| Applicability::MachineApplicable, | ||
| ); | ||
| } | ||
|
|
||
| impl LateLintPass<'_> for MissingMustUse { | ||
| fn check_item(&mut self, cx: &LateContext<'_>, item: &Item<'_>) { | ||
| if item.span.in_external_macro(cx.sess().source_map()) { | ||
| return; | ||
| } | ||
| // 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), | ||
| ItemKind::Const(..) | ||
| | ItemKind::Static(..) | ||
| | ItemKind::Fn { .. } | ||
| | ItemKind::Mod(..) | ||
| | ItemKind::Use(..) | ||
| | ItemKind::ForeignMod { .. } | ||
| | ItemKind::GlobalAsm { .. } | ||
| | ItemKind::Struct(..) | ||
| | ItemKind::Enum(..) | ||
| | ItemKind::TyAlias(..) | ||
| | ItemKind::Trait { .. } | ||
| | ItemKind::TraitAlias(..) | ||
| | ItemKind::Impl(..) | ||
| | ItemKind::Macro(..) | ||
| | ItemKind::ExternCrate(..) => {}, | ||
| } | ||
| } | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,187 @@ | ||
| #![feature(trait_alias)] | ||
| #![warn(clippy::missing_must_use)] | ||
|
|
||
| pub struct PubStructUnitNoMustUse; | ||
| struct PrivStructUnitNoMustUse; | ||
|
|
||
| pub struct PubTupleStructEmptyNoMustUse(); | ||
| pub struct PubStructEmptyNoMustUse {} | ||
|
|
||
| #[must_use] | ||
| pub struct PubTupleStructNoMustUse(u8); | ||
| //~^ missing_must_use | ||
| struct PrivTupleStructNoMustUse(u8); | ||
|
|
||
| #[must_use] | ||
| pub struct PubStructNoMustUse { | ||
| //~^ missing_must_use | ||
| pub field: u8, | ||
| } | ||
| struct PrivStructNoMustUse { | ||
| pub field: u8, | ||
| } | ||
|
|
||
| pub enum PubEnumEmptyNoMustUse {} | ||
|
|
||
| #[must_use] | ||
| pub enum PubEnumNoMustUse { | ||
| //~^ missing_must_use | ||
| Unit, | ||
| Tuple(u8), | ||
| Struct { field: u8 }, | ||
| } | ||
| enum PrivEnumNoMustUse { | ||
| Unit, | ||
| Tuple(u8), | ||
| Struct { field: u8 }, | ||
| } | ||
|
|
||
| #[must_use] | ||
| pub union PubUnionNoMustUse { | ||
| //~^ missing_must_use | ||
| f1: u8, | ||
| f2: u16, | ||
| } | ||
| union PrivUnionNoMustUse { | ||
| f1: u8, | ||
| f2: u16, | ||
| } | ||
|
|
||
| #[must_use] | ||
| pub struct PubStructUnitMustUse; | ||
| #[must_use] | ||
| struct PrivStructUnitMustUse; | ||
|
|
||
| #[must_use] | ||
| pub struct PubStructTupleMustUse(u8); | ||
| #[must_use] | ||
| struct PrivStructTupleMustUse(u8); | ||
|
|
||
| #[must_use] | ||
| pub struct PubStructMustUse { | ||
| pub field: u8, | ||
| } | ||
| #[must_use] | ||
| struct PrivStructMustUse { | ||
| pub field: u8, | ||
| } | ||
|
|
||
| #[must_use] | ||
| pub enum PubEnumMustUse { | ||
| Unit, | ||
| Tuple(u8), | ||
| Struct { field: u8 }, | ||
| } | ||
| #[must_use] | ||
| enum PrivEnumMustUse { | ||
| Unit, | ||
| Tuple(u8), | ||
| Struct { field: u8 }, | ||
| } | ||
|
|
||
| #[must_use] | ||
| pub union PubUnionMustUse { | ||
| f1: u8, | ||
| f2: u16, | ||
| } | ||
| #[must_use] | ||
| union PrivUnionMustUse { | ||
| f1: u8, | ||
| f2: u16, | ||
| } | ||
|
|
||
| const IGNORED_CONST: PubEnumMustUse = PubEnumMustUse::Unit; | ||
| static IGNORED_STATIC: PubStructUnitMustUse = PubStructUnitMustUse; | ||
|
|
||
| mod ignored_mod {} | ||
| use ignored_mod as _; | ||
|
|
||
| unsafe extern "C" { | ||
| fn ignored_foreign_fn(); | ||
| } | ||
|
|
||
| #[cfg(any(target_arch = "x86", target_arch = "x86_64"))] | ||
| std::arch::global_asm!(""); | ||
|
|
||
| type IgnoredTypeAlias = PubStructUnitMustUse; | ||
| trait IgnoredTraitAliasBase {} | ||
| trait IgnoredTraitAlias = IgnoredTraitAliasBase; | ||
|
|
||
| macro_rules! ignored_macro { | ||
| () => {}; | ||
| } | ||
|
|
||
| extern crate core as ignored_core; | ||
| use ignored_core::fmt::Debug as _; | ||
|
|
||
| pub fn pub_fn_no_must_use(_: PubStructUnitMustUse) -> u8 { | ||
| 0 | ||
| } | ||
| fn priv_fn_no_must_use(_: PubEnumMustUse) -> u8 { | ||
| 0 | ||
| } | ||
|
|
||
| #[must_use] | ||
| pub fn pub_fn_must_use(_: PubStructUnitMustUse) -> u8 { | ||
| 0 | ||
| } | ||
| #[must_use] | ||
| fn priv_fn_must_use(_: PubEnumMustUse) -> u8 { | ||
| 0 | ||
| } | ||
|
|
||
| pub trait PubTraitNoMustUse { | ||
| type Assoc; | ||
| const VALUE: u8; | ||
|
|
||
| fn pub_trait_fn_no_must_use(_: PubStructUnitMustUse) -> u8; | ||
| } | ||
| trait PrivTraitNoMustUse { | ||
| type Assoc; | ||
| const VALUE: u8; | ||
|
|
||
| fn priv_trait_fn_no_must_use(_: PubEnumMustUse) -> u8; | ||
| } | ||
| trait TraitMustUse { | ||
| type Assoc; | ||
| const VALUE: u8; | ||
|
|
||
| #[must_use] | ||
| fn trait_fn_must_use(_: PubStructUnitMustUse) -> u8; | ||
| } | ||
|
|
||
| impl PubStructUnitMustUse { | ||
| const IGNORED_IMPL_CONST: u8 = 0; | ||
|
|
||
| pub fn pub_impl_fn_no_must_use(_: PubStructUnitMustUse) -> u8 { | ||
| 0 | ||
| } | ||
|
|
||
| fn priv_impl_fn_no_must_use(_: PubEnumMustUse) -> u8 { | ||
| 0 | ||
| } | ||
|
|
||
| #[must_use] | ||
| pub fn pub_impl_fn_must_use(_: PubStructUnitMustUse) -> u8 { | ||
| 0 | ||
| } | ||
| } | ||
|
|
||
| trait ImplTrait { | ||
| type Assoc; | ||
| const VALUE: u8; | ||
|
|
||
| #[must_use] | ||
| fn implemented_fn(_: PubStructUnitMustUse) -> u8; | ||
| } | ||
| impl ImplTrait for PubStructUnitMustUse { | ||
| type Assoc = PubEnumMustUse; | ||
| const VALUE: u8 = 0; | ||
|
|
||
| fn implemented_fn(_: PubStructUnitMustUse) -> u8 { | ||
| 0 | ||
| } | ||
| } | ||
|
|
||
| // Ignore entry point function | ||
| fn main() {} |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
View changes since the review
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
restrictcheck 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While I personally like must-use (which is why I wrote the
must_use_candidatelint), 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't disagree.
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)