From 821b32bd406e9c29b2e9ca2a647d30021cff653d Mon Sep 17 00:00:00 2001 From: Aaron Kofsky Date: Sun, 29 May 2022 14:35:00 -0400 Subject: [PATCH 01/25] Add `let_underscore_drop` lint. This lint checks for statements similar to `let _ = foo`, where `foo` is a type that implements `Drop`. These types of let statements cause the expression in them to be dropped immediately, instead of at the end of the scope. Such behavior can be surprizing, especially if you are relying on the value to be dropped at the end of the scope. Instead, the binding should be an underscore prefixed name (like `_unused`) or the value should explicitly be passed to `std::mem::drop()` if the value really should be dropped immediately. --- compiler/rustc_lint/src/let_underscore.rs | 66 +++++++++++++++++++++++ compiler/rustc_lint/src/lib.rs | 5 ++ src/test/ui/let_underscore_drop.rs | 13 +++++ src/test/ui/let_underscore_drop.stderr | 12 +++++ 4 files changed, 96 insertions(+) create mode 100644 compiler/rustc_lint/src/let_underscore.rs create mode 100644 src/test/ui/let_underscore_drop.rs create mode 100644 src/test/ui/let_underscore_drop.stderr diff --git a/compiler/rustc_lint/src/let_underscore.rs b/compiler/rustc_lint/src/let_underscore.rs new file mode 100644 index 0000000000000..44242173c008b --- /dev/null +++ b/compiler/rustc_lint/src/let_underscore.rs @@ -0,0 +1,66 @@ +use crate::{LateContext, LateLintPass, LintContext}; +use rustc_hir as hir; + +declare_lint! { + /// The `let_underscore_drop` lint checks for statements which don't bind + /// an expression which has a non-trivial Drop implementation to anything, + /// causing the expression to be dropped immediately instead of at end of + /// scope. + /// + /// ### Example + /// ```rust + /// struct SomeStruct; + /// impl Drop for SomeStruct { + /// fn drop(&mut self) { + /// println!("Dropping SomeStruct"); + /// } + /// } + /// + /// fn main() { + /// // SomeStuct is dropped immediately instead of at end of scope, + /// // so "Dropping SomeStruct" is printed before "end of main". + /// // The order of prints would be reversed if SomeStruct was bound to + /// // a name (such as "_foo"). + /// let _ = SomeStruct; + /// println!("end of main"); + /// } + /// ``` + /// ### Explanation + /// + /// Statements which assign an expression to an underscore causes the + /// expression to immediately drop instead of extending the expression's + /// lifetime to the end of the scope. This is usually unintended, + /// especially for types like `MutexGuard`, which are typically used to + /// lock a mutex for the duration of an entire scope. + /// + /// If you want to extend the expression's lifetime to the end of the scope, + /// assign an underscore-prefixed name (such as `_foo`) to the expression. + /// If you do actually want to drop the expression immediately, then + /// calling `std::mem::drop` on the expression is clearer and helps convey + /// intent. + pub LET_UNDERSCORE_DROP, + Warn, + "non-binding let on a type that implements `Drop`" +} + +declare_lint_pass!(LetUnderscore => [LET_UNDERSCORE_DROP]); + +impl<'tcx> LateLintPass<'tcx> for LetUnderscore { + fn check_local(&mut self, cx: &LateContext<'_>, local: &hir::Local<'_>) { + if !matches!(local.pat.kind, hir::PatKind::Wild) { + return; + } + if let Some(init) = local.init { + let init_ty = cx.typeck_results().expr_ty(init); + let needs_drop = init_ty.needs_drop(cx.tcx, cx.param_env); + if needs_drop { + cx.struct_span_lint(LET_UNDERSCORE_DROP, local.span, |lint| { + lint.build("non-binding let on a type that implements `Drop`") + .help("consider binding to an unused variable") + .help("consider explicitly droping with `std::mem::drop`") + .emit(); + }) + } + } + } +} diff --git a/compiler/rustc_lint/src/lib.rs b/compiler/rustc_lint/src/lib.rs index 0a0f292fe7a4d..55396b36dbc2e 100644 --- a/compiler/rustc_lint/src/lib.rs +++ b/compiler/rustc_lint/src/lib.rs @@ -54,6 +54,7 @@ mod expect; pub mod hidden_unicode_codepoints; mod internal; mod late; +mod let_underscore; mod levels; mod methods; mod non_ascii_idents; @@ -85,6 +86,7 @@ use builtin::*; use enum_intrinsics_non_enums::EnumIntrinsicsNonEnums; use hidden_unicode_codepoints::*; use internal::*; +use let_underscore::*; use methods::*; use non_ascii_idents::*; use non_fmt_panic::NonPanicFmt; @@ -199,6 +201,7 @@ macro_rules! late_lint_mod_passes { VariantSizeDifferences: VariantSizeDifferences, BoxPointers: BoxPointers, PathStatements: PathStatements, + LetUnderscore: LetUnderscore, // Depends on referenced function signatures in expressions UnusedResults: UnusedResults, NonUpperCaseGlobals: NonUpperCaseGlobals, @@ -314,6 +317,8 @@ fn register_builtins(store: &mut LintStore, no_interleave_lints: bool) { REDUNDANT_SEMICOLONS ); + add_lint_group!("let_underscore", LET_UNDERSCORE_DROP); + add_lint_group!( "rust_2018_idioms", BARE_TRAIT_OBJECTS, diff --git a/src/test/ui/let_underscore_drop.rs b/src/test/ui/let_underscore_drop.rs new file mode 100644 index 0000000000000..c1c5207d0fe87 --- /dev/null +++ b/src/test/ui/let_underscore_drop.rs @@ -0,0 +1,13 @@ +// run-pass + +struct NontrivialDrop; + +impl Drop for NontrivialDrop { + fn drop(&mut self) { + println!("Dropping!"); + } +} + +fn main() { + let _ = NontrivialDrop; //~WARNING non-binding let on a type that implements `Drop` +} diff --git a/src/test/ui/let_underscore_drop.stderr b/src/test/ui/let_underscore_drop.stderr new file mode 100644 index 0000000000000..40ed1abd8dc6f --- /dev/null +++ b/src/test/ui/let_underscore_drop.stderr @@ -0,0 +1,12 @@ +warning: non-binding let on a type that implements `Drop` + --> $DIR/let_underscore_drop.rs:12:5 + | +LL | let _ = NontrivialDrop; + | ^^^^^^^^^^^^^^^^^^^^^^^ + | + = note: `#[warn(let_underscore_drop)]` on by default + = help: consider binding to an unused variable + = help: consider explicitly droping with `std::mem::drop` + +warning: 1 warning emitted + From ad7587fedcf29a6629e0218f0e180ddf8960461d Mon Sep 17 00:00:00 2001 From: Aaron Kofsky Date: Tue, 31 May 2022 14:05:04 -0400 Subject: [PATCH 02/25] Add `let_underscore_lock` lint. Similar to `let_underscore_drop`, this lint checks for statements similar to `let _ = foo`, where `foo` is a lock guard. These types of let statements are especially problematic because the lock gets released immediately, instead of at the end of the scope. This behavior is almost always the wrong thing. --- compiler/rustc_lint/src/let_underscore.rs | 72 ++++++++++++++++++++++- compiler/rustc_lint/src/lib.rs | 2 +- src/test/ui/let_underscore_lock.rs | 8 +++ src/test/ui/let_underscore_lock.stderr | 12 ++++ 4 files changed, 91 insertions(+), 3 deletions(-) create mode 100644 src/test/ui/let_underscore_lock.rs create mode 100644 src/test/ui/let_underscore_lock.stderr diff --git a/compiler/rustc_lint/src/let_underscore.rs b/compiler/rustc_lint/src/let_underscore.rs index 44242173c008b..81906a24d9029 100644 --- a/compiler/rustc_lint/src/let_underscore.rs +++ b/compiler/rustc_lint/src/let_underscore.rs @@ -1,5 +1,7 @@ use crate::{LateContext, LateLintPass, LintContext}; use rustc_hir as hir; +use rustc_middle::ty::{self, subst::GenericArgKind}; +use rustc_span::Symbol; declare_lint! { /// The `let_underscore_drop` lint checks for statements which don't bind @@ -43,7 +45,53 @@ declare_lint! { "non-binding let on a type that implements `Drop`" } -declare_lint_pass!(LetUnderscore => [LET_UNDERSCORE_DROP]); +declare_lint! { + /// The `let_underscore_lock` lint checks for statements which don't bind + /// a mutex to anything, causing the lock to be released immediately instead + /// of at end of scope, which is typically incorrect. + /// + /// ### Example + /// ```rust + /// use std::sync::{Arc, Mutex}; + /// use std::thread; + /// let data = Arc::new(Mutex::new(0)); + /// + /// thread::spawn(move || { + /// // The lock is immediately released instead of at the end of the + /// // scope, which is probably not intended. + /// let _ = data.lock().unwrap(); + /// println!("doing some work"); + /// let mut lock = data.lock().unwrap(); + /// *lock += 1; + /// }); + /// ``` + /// ### Explanation + /// + /// Statements which assign an expression to an underscore causes the + /// expression to immediately drop instead of extending the expression's + /// lifetime to the end of the scope. This is usually unintended, + /// especially for types like `MutexGuard`, which are typically used to + /// lock a mutex for the duration of an entire scope. + /// + /// If you want to extend the expression's lifetime to the end of the scope, + /// assign an underscore-prefixed name (such as `_foo`) to the expression. + /// If you do actually want to drop the expression immediately, then + /// calling `std::mem::drop` on the expression is clearer and helps convey + /// intent. + pub LET_UNDERSCORE_LOCK, + Warn, + "non-binding let on a synchronization lock" +} + +declare_lint_pass!(LetUnderscore => [LET_UNDERSCORE_DROP, LET_UNDERSCORE_LOCK]); + +const SYNC_GUARD_PATHS: [&[&str]; 5] = [ + &["std", "sync", "mutex", "MutexGuard"], + &["std", "sync", "rwlock", "RwLockReadGuard"], + &["std", "sync", "rwlock", "RwLockWriteGuard"], + &["parking_lot", "raw_mutex", "RawMutex"], + &["parking_lot", "raw_rwlock", "RawRwLock"], +]; impl<'tcx> LateLintPass<'tcx> for LetUnderscore { fn check_local(&mut self, cx: &LateContext<'_>, local: &hir::Local<'_>) { @@ -53,7 +101,27 @@ impl<'tcx> LateLintPass<'tcx> for LetUnderscore { if let Some(init) = local.init { let init_ty = cx.typeck_results().expr_ty(init); let needs_drop = init_ty.needs_drop(cx.tcx, cx.param_env); - if needs_drop { + let is_sync_lock = init_ty.walk().any(|inner| match inner.unpack() { + GenericArgKind::Type(inner_ty) => { + SYNC_GUARD_PATHS.iter().any(|guard_path| match inner_ty.kind() { + ty::Adt(adt, _) => { + let ty_path = cx.get_def_path(adt.did()); + guard_path.iter().map(|x| Symbol::intern(x)).eq(ty_path.iter().copied()) + } + _ => false, + }) + } + + GenericArgKind::Lifetime(_) | GenericArgKind::Const(_) => false, + }); + if is_sync_lock { + cx.struct_span_lint(LET_UNDERSCORE_LOCK, local.span, |lint| { + lint.build("non-binding let on a synchronization lock") + .help("consider binding to an unused variable") + .help("consider explicitly droping with `std::mem::drop`") + .emit(); + }) + } else if needs_drop { cx.struct_span_lint(LET_UNDERSCORE_DROP, local.span, |lint| { lint.build("non-binding let on a type that implements `Drop`") .help("consider binding to an unused variable") diff --git a/compiler/rustc_lint/src/lib.rs b/compiler/rustc_lint/src/lib.rs index 55396b36dbc2e..79661c0fefe8d 100644 --- a/compiler/rustc_lint/src/lib.rs +++ b/compiler/rustc_lint/src/lib.rs @@ -317,7 +317,7 @@ fn register_builtins(store: &mut LintStore, no_interleave_lints: bool) { REDUNDANT_SEMICOLONS ); - add_lint_group!("let_underscore", LET_UNDERSCORE_DROP); + add_lint_group!("let_underscore", LET_UNDERSCORE_DROP, LET_UNDERSCORE_LOCK); add_lint_group!( "rust_2018_idioms", diff --git a/src/test/ui/let_underscore_lock.rs b/src/test/ui/let_underscore_lock.rs new file mode 100644 index 0000000000000..774b610db2fb8 --- /dev/null +++ b/src/test/ui/let_underscore_lock.rs @@ -0,0 +1,8 @@ +// run-pass + +use std::sync::{Arc, Mutex}; + +fn main() { + let data = Arc::new(Mutex::new(0)); + let _ = data.lock().unwrap(); //~WARNING non-binding let on a synchronization lock +} diff --git a/src/test/ui/let_underscore_lock.stderr b/src/test/ui/let_underscore_lock.stderr new file mode 100644 index 0000000000000..77379d8c3db2c --- /dev/null +++ b/src/test/ui/let_underscore_lock.stderr @@ -0,0 +1,12 @@ +warning: non-binding let on a synchronization lock + --> $DIR/let_underscore_lock.rs:7:5 + | +LL | let _ = data.lock().unwrap(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = note: `#[warn(let_underscore_lock)]` on by default + = help: consider binding to an unused variable + = help: consider explicitly droping with `std::mem::drop` + +warning: 1 warning emitted + From 758a9fd0f9ac191f0ea99215e07816631f402b7e Mon Sep 17 00:00:00 2001 From: Aaron Kofsky Date: Thu, 2 Jun 2022 14:58:44 -0400 Subject: [PATCH 03/25] Add `let_underscore_must_use` lint. Similar to `let_underscore_drop`, this lint checks for statements similar to `let _ = foo`, where `foo` is an expression marked `must_use`. --- compiler/rustc_lint/src/let_underscore.rs | 106 ++++++++++++++++++++- compiler/rustc_lint/src/lib.rs | 7 +- src/test/ui/let_underscore_must_use.rs | 12 +++ src/test/ui/let_underscore_must_use.stderr | 21 ++++ 4 files changed, 143 insertions(+), 3 deletions(-) create mode 100644 src/test/ui/let_underscore_must_use.rs create mode 100644 src/test/ui/let_underscore_must_use.stderr diff --git a/compiler/rustc_lint/src/let_underscore.rs b/compiler/rustc_lint/src/let_underscore.rs index 81906a24d9029..40e6d12abf910 100644 --- a/compiler/rustc_lint/src/let_underscore.rs +++ b/compiler/rustc_lint/src/let_underscore.rs @@ -1,6 +1,6 @@ use crate::{LateContext, LateLintPass, LintContext}; use rustc_hir as hir; -use rustc_middle::ty::{self, subst::GenericArgKind}; +use rustc_middle::ty::{self, subst::GenericArgKind, Ty}; use rustc_span::Symbol; declare_lint! { @@ -83,7 +83,32 @@ declare_lint! { "non-binding let on a synchronization lock" } -declare_lint_pass!(LetUnderscore => [LET_UNDERSCORE_DROP, LET_UNDERSCORE_LOCK]); +declare_lint! { + /// The `let_underscore_must_use` lint checks for statements which don't bind + /// a `must_use` expression to anything, causing the lock to be released + /// immediately instead of at end of scope, which is typically incorrect. + /// + /// ### Example + /// ```rust + /// #[must_use] + /// struct SomeStruct; + /// + /// fn main() { + /// // SomeStuct is dropped immediately instead of at end of scope. + /// let _ = SomeStruct; + /// } + /// ``` + /// ### Explanation + /// + /// Statements which assign an expression to an underscore causes the + /// expression to immediately drop. Usually, it's better to explicitly handle + /// the `must_use` expression. + pub LET_UNDERSCORE_MUST_USE, + Warn, + "non-binding let on a expression marked `must_use`" +} + +declare_lint_pass!(LetUnderscore => [LET_UNDERSCORE_DROP, LET_UNDERSCORE_LOCK, LET_UNDERSCORE_MUST_USE]); const SYNC_GUARD_PATHS: [&[&str]; 5] = [ &["std", "sync", "mutex", "MutexGuard"], @@ -114,6 +139,8 @@ impl<'tcx> LateLintPass<'tcx> for LetUnderscore { GenericArgKind::Lifetime(_) | GenericArgKind::Const(_) => false, }); + let is_must_use_ty = is_must_use_ty(cx, cx.typeck_results().expr_ty(init)); + let is_must_use_func_call = is_must_use_func_call(cx, init); if is_sync_lock { cx.struct_span_lint(LET_UNDERSCORE_LOCK, local.span, |lint| { lint.build("non-binding let on a synchronization lock") @@ -121,6 +148,13 @@ impl<'tcx> LateLintPass<'tcx> for LetUnderscore { .help("consider explicitly droping with `std::mem::drop`") .emit(); }) + } else if is_must_use_ty || is_must_use_func_call { + cx.struct_span_lint(LET_UNDERSCORE_MUST_USE, local.span, |lint| { + lint.build("non-binding let on a expression marked `must_use`") + .help("consider binding to an unused variable") + .help("consider explicitly droping with `std::mem::drop`") + .emit(); + }) } else if needs_drop { cx.struct_span_lint(LET_UNDERSCORE_DROP, local.span, |lint| { lint.build("non-binding let on a type that implements `Drop`") @@ -130,5 +164,73 @@ impl<'tcx> LateLintPass<'tcx> for LetUnderscore { }) } } + + fn is_must_use_ty<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> bool { + match ty.kind() { + ty::Adt(adt, _) => has_must_use_attr(cx, adt.did()), + ty::Foreign(ref did) => has_must_use_attr(cx, *did), + ty::Slice(ty) + | ty::Array(ty, _) + | ty::RawPtr(ty::TypeAndMut { ty, .. }) + | ty::Ref(_, ty, _) => { + // for the Array case we don't need to care for the len == 0 case + // because we don't want to lint functions returning empty arrays + is_must_use_ty(cx, *ty) + } + ty::Tuple(substs) => substs.iter().any(|ty| is_must_use_ty(cx, ty)), + ty::Opaque(ref def_id, _) => { + for (predicate, _) in cx.tcx.explicit_item_bounds(*def_id) { + if let ty::PredicateKind::Trait(trait_predicate) = + predicate.kind().skip_binder() + { + if has_must_use_attr(cx, trait_predicate.trait_ref.def_id) { + return true; + } + } + } + false + } + ty::Dynamic(binder, _) => { + for predicate in binder.iter() { + if let ty::ExistentialPredicate::Trait(ref trait_ref) = + predicate.skip_binder() + { + if has_must_use_attr(cx, trait_ref.def_id) { + return true; + } + } + } + false + } + _ => false, + } + } + + // check if expr is calling method or function with #[must_use] attribute + fn is_must_use_func_call(cx: &LateContext<'_>, expr: &hir::Expr<'_>) -> bool { + let did = match expr.kind { + hir::ExprKind::Call(path, _) if let hir::ExprKind::Path(ref qpath) = path.kind => { + if let hir::def::Res::Def(_, did) = cx.qpath_res(qpath, path.hir_id) { + Some(did) + } else { + None + } + }, + hir::ExprKind::MethodCall(..) => { + cx.typeck_results().type_dependent_def_id(expr.hir_id) + } + _ => None, + }; + + did.map_or(false, |did| has_must_use_attr(cx, did)) + } + + // returns true if DefId contains a `#[must_use]` attribute + fn has_must_use_attr(cx: &LateContext<'_>, did: hir::def_id::DefId) -> bool { + cx.tcx + .get_attrs(did, rustc_span::sym::must_use) + .find(|a| a.has_name(rustc_span::sym::must_use)) + .is_some() + } } } diff --git a/compiler/rustc_lint/src/lib.rs b/compiler/rustc_lint/src/lib.rs index 79661c0fefe8d..4359a54b698dc 100644 --- a/compiler/rustc_lint/src/lib.rs +++ b/compiler/rustc_lint/src/lib.rs @@ -317,7 +317,12 @@ fn register_builtins(store: &mut LintStore, no_interleave_lints: bool) { REDUNDANT_SEMICOLONS ); - add_lint_group!("let_underscore", LET_UNDERSCORE_DROP, LET_UNDERSCORE_LOCK); + add_lint_group!( + "let_underscore", + LET_UNDERSCORE_DROP, + LET_UNDERSCORE_LOCK, + LET_UNDERSCORE_MUST_USE + ); add_lint_group!( "rust_2018_idioms", diff --git a/src/test/ui/let_underscore_must_use.rs b/src/test/ui/let_underscore_must_use.rs new file mode 100644 index 0000000000000..6a78e3fc4b402 --- /dev/null +++ b/src/test/ui/let_underscore_must_use.rs @@ -0,0 +1,12 @@ +// run-pass + +#[must_use] +struct MustUseType; + +#[must_use] +fn must_use_function() -> () {} + +fn main() { + let _ = MustUseType; //~WARNING non-binding let on a expression marked `must_use` + let _ = must_use_function(); //~WARNING non-binding let on a expression marked `must_use` +} diff --git a/src/test/ui/let_underscore_must_use.stderr b/src/test/ui/let_underscore_must_use.stderr new file mode 100644 index 0000000000000..0b840385e5dfa --- /dev/null +++ b/src/test/ui/let_underscore_must_use.stderr @@ -0,0 +1,21 @@ +warning: non-binding let on a expression marked `must_use` + --> $DIR/let_underscore_must_use.rs:10:5 + | +LL | let _ = MustUseType; + | ^^^^^^^^^^^^^^^^^^^^ + | + = note: `#[warn(let_underscore_must_use)]` on by default + = help: consider binding to an unused variable + = help: consider explicitly droping with `std::mem::drop` + +warning: non-binding let on a expression marked `must_use` + --> $DIR/let_underscore_must_use.rs:11:5 + | +LL | let _ = must_use_function(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = help: consider binding to an unused variable + = help: consider explicitly droping with `std::mem::drop` + +warning: 2 warnings emitted + From 36b6309c6556e5486138057e8c654b40e005327d Mon Sep 17 00:00:00 2001 From: Aaron Kofsky Date: Fri, 3 Jun 2022 14:37:14 -0400 Subject: [PATCH 04/25] Move let_underscore tests to their own subfolder. This was done to pass `tidy`. --- src/test/ui/{ => let_underscore}/let_underscore_drop.rs | 0 src/test/ui/{ => let_underscore}/let_underscore_drop.stderr | 0 src/test/ui/{ => let_underscore}/let_underscore_lock.rs | 0 src/test/ui/{ => let_underscore}/let_underscore_lock.stderr | 0 src/test/ui/{ => let_underscore}/let_underscore_must_use.rs | 0 src/test/ui/{ => let_underscore}/let_underscore_must_use.stderr | 0 6 files changed, 0 insertions(+), 0 deletions(-) rename src/test/ui/{ => let_underscore}/let_underscore_drop.rs (100%) rename src/test/ui/{ => let_underscore}/let_underscore_drop.stderr (100%) rename src/test/ui/{ => let_underscore}/let_underscore_lock.rs (100%) rename src/test/ui/{ => let_underscore}/let_underscore_lock.stderr (100%) rename src/test/ui/{ => let_underscore}/let_underscore_must_use.rs (100%) rename src/test/ui/{ => let_underscore}/let_underscore_must_use.stderr (100%) diff --git a/src/test/ui/let_underscore_drop.rs b/src/test/ui/let_underscore/let_underscore_drop.rs similarity index 100% rename from src/test/ui/let_underscore_drop.rs rename to src/test/ui/let_underscore/let_underscore_drop.rs diff --git a/src/test/ui/let_underscore_drop.stderr b/src/test/ui/let_underscore/let_underscore_drop.stderr similarity index 100% rename from src/test/ui/let_underscore_drop.stderr rename to src/test/ui/let_underscore/let_underscore_drop.stderr diff --git a/src/test/ui/let_underscore_lock.rs b/src/test/ui/let_underscore/let_underscore_lock.rs similarity index 100% rename from src/test/ui/let_underscore_lock.rs rename to src/test/ui/let_underscore/let_underscore_lock.rs diff --git a/src/test/ui/let_underscore_lock.stderr b/src/test/ui/let_underscore/let_underscore_lock.stderr similarity index 100% rename from src/test/ui/let_underscore_lock.stderr rename to src/test/ui/let_underscore/let_underscore_lock.stderr diff --git a/src/test/ui/let_underscore_must_use.rs b/src/test/ui/let_underscore/let_underscore_must_use.rs similarity index 100% rename from src/test/ui/let_underscore_must_use.rs rename to src/test/ui/let_underscore/let_underscore_must_use.rs diff --git a/src/test/ui/let_underscore_must_use.stderr b/src/test/ui/let_underscore/let_underscore_must_use.stderr similarity index 100% rename from src/test/ui/let_underscore_must_use.stderr rename to src/test/ui/let_underscore/let_underscore_must_use.stderr From ae2ac3b4c568d2d20cb395a57a163e1e298d6d6c Mon Sep 17 00:00:00 2001 From: Aaron Kofsky Date: Fri, 3 Jun 2022 16:19:55 -0400 Subject: [PATCH 05/25] Allow `let_underscore_drop` and `let_underscore_must_use` by default. These lints are very noisy and are allow-by-default in clippy anyways. Hence, setting them to allow-by-default here makes more sense than warning constantly on these cases. --- compiler/rustc_lint/src/let_underscore.rs | 4 ++-- src/test/ui/let_underscore/let_underscore_drop.rs | 1 + src/test/ui/let_underscore/let_underscore_drop.stderr | 4 ++-- src/test/ui/let_underscore/let_underscore_must_use.rs | 1 + src/test/ui/let_underscore/let_underscore_must_use.stderr | 6 +++--- 5 files changed, 9 insertions(+), 7 deletions(-) diff --git a/compiler/rustc_lint/src/let_underscore.rs b/compiler/rustc_lint/src/let_underscore.rs index 40e6d12abf910..985c7300efa95 100644 --- a/compiler/rustc_lint/src/let_underscore.rs +++ b/compiler/rustc_lint/src/let_underscore.rs @@ -41,7 +41,7 @@ declare_lint! { /// calling `std::mem::drop` on the expression is clearer and helps convey /// intent. pub LET_UNDERSCORE_DROP, - Warn, + Allow, "non-binding let on a type that implements `Drop`" } @@ -104,7 +104,7 @@ declare_lint! { /// expression to immediately drop. Usually, it's better to explicitly handle /// the `must_use` expression. pub LET_UNDERSCORE_MUST_USE, - Warn, + Allow, "non-binding let on a expression marked `must_use`" } diff --git a/src/test/ui/let_underscore/let_underscore_drop.rs b/src/test/ui/let_underscore/let_underscore_drop.rs index c1c5207d0fe87..75c6ecec3ee53 100644 --- a/src/test/ui/let_underscore/let_underscore_drop.rs +++ b/src/test/ui/let_underscore/let_underscore_drop.rs @@ -1,4 +1,5 @@ // run-pass +// compile-flags: -W let_underscore_drop struct NontrivialDrop; diff --git a/src/test/ui/let_underscore/let_underscore_drop.stderr b/src/test/ui/let_underscore/let_underscore_drop.stderr index 40ed1abd8dc6f..f4fd663c7f9ea 100644 --- a/src/test/ui/let_underscore/let_underscore_drop.stderr +++ b/src/test/ui/let_underscore/let_underscore_drop.stderr @@ -1,10 +1,10 @@ warning: non-binding let on a type that implements `Drop` - --> $DIR/let_underscore_drop.rs:12:5 + --> $DIR/let_underscore_drop.rs:13:5 | LL | let _ = NontrivialDrop; | ^^^^^^^^^^^^^^^^^^^^^^^ | - = note: `#[warn(let_underscore_drop)]` on by default + = note: requested on the command line with `-W let-underscore-drop` = help: consider binding to an unused variable = help: consider explicitly droping with `std::mem::drop` diff --git a/src/test/ui/let_underscore/let_underscore_must_use.rs b/src/test/ui/let_underscore/let_underscore_must_use.rs index 6a78e3fc4b402..8efaad51ea877 100644 --- a/src/test/ui/let_underscore/let_underscore_must_use.rs +++ b/src/test/ui/let_underscore/let_underscore_must_use.rs @@ -1,4 +1,5 @@ // run-pass +// compile-flags: -W let_underscore_must_use #[must_use] struct MustUseType; diff --git a/src/test/ui/let_underscore/let_underscore_must_use.stderr b/src/test/ui/let_underscore/let_underscore_must_use.stderr index 0b840385e5dfa..ea1de45e17b57 100644 --- a/src/test/ui/let_underscore/let_underscore_must_use.stderr +++ b/src/test/ui/let_underscore/let_underscore_must_use.stderr @@ -1,15 +1,15 @@ warning: non-binding let on a expression marked `must_use` - --> $DIR/let_underscore_must_use.rs:10:5 + --> $DIR/let_underscore_must_use.rs:11:5 | LL | let _ = MustUseType; | ^^^^^^^^^^^^^^^^^^^^ | - = note: `#[warn(let_underscore_must_use)]` on by default + = note: requested on the command line with `-W let-underscore-must-use` = help: consider binding to an unused variable = help: consider explicitly droping with `std::mem::drop` warning: non-binding let on a expression marked `must_use` - --> $DIR/let_underscore_must_use.rs:11:5 + --> $DIR/let_underscore_must_use.rs:12:5 | LL | let _ = must_use_function(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ From eba6c789dcfca7065d1c292c06eb447ac9895db3 Mon Sep 17 00:00:00 2001 From: Aaron Kofsky Date: Fri, 3 Jun 2022 21:33:13 -0400 Subject: [PATCH 06/25] Show code suggestions in `let_undescore` lint messages. This commit uses `span_suggestion_verbose` to add what specific code changes can be done as suggested by the lint--in this case, either binding the expression to an unused variable or using `std::mem::drop` to drop the value explicitly. --- compiler/rustc_lint/src/let_underscore.rs | 60 +++++++++++++++---- .../let_underscore/let_underscore_drop.stderr | 10 +++- .../let_underscore/let_underscore_lock.stderr | 10 +++- .../let_underscore_must_use.stderr | 20 +++++-- 4 files changed, 79 insertions(+), 21 deletions(-) diff --git a/compiler/rustc_lint/src/let_underscore.rs b/compiler/rustc_lint/src/let_underscore.rs index 985c7300efa95..4e4cedaeb7821 100644 --- a/compiler/rustc_lint/src/let_underscore.rs +++ b/compiler/rustc_lint/src/let_underscore.rs @@ -1,6 +1,10 @@ use crate::{LateContext, LateLintPass, LintContext}; +use rustc_errors::Applicability; use rustc_hir as hir; -use rustc_middle::ty::{self, subst::GenericArgKind, Ty}; +use rustc_middle::{ + lint::LintDiagnosticBuilder, + ty::{self, subst::GenericArgKind, Ty}, +}; use rustc_span::Symbol; declare_lint! { @@ -141,30 +145,60 @@ impl<'tcx> LateLintPass<'tcx> for LetUnderscore { }); let is_must_use_ty = is_must_use_ty(cx, cx.typeck_results().expr_ty(init)); let is_must_use_func_call = is_must_use_func_call(cx, init); + if is_sync_lock { cx.struct_span_lint(LET_UNDERSCORE_LOCK, local.span, |lint| { - lint.build("non-binding let on a synchronization lock") - .help("consider binding to an unused variable") - .help("consider explicitly droping with `std::mem::drop`") - .emit(); + build_and_emit_lint( + lint, + local, + init.span, + "non-binding let on a synchronization lock", + ) }) } else if is_must_use_ty || is_must_use_func_call { cx.struct_span_lint(LET_UNDERSCORE_MUST_USE, local.span, |lint| { - lint.build("non-binding let on a expression marked `must_use`") - .help("consider binding to an unused variable") - .help("consider explicitly droping with `std::mem::drop`") - .emit(); + build_and_emit_lint( + lint, + local, + init.span, + "non-binding let on a expression marked `must_use`", + ); }) } else if needs_drop { cx.struct_span_lint(LET_UNDERSCORE_DROP, local.span, |lint| { - lint.build("non-binding let on a type that implements `Drop`") - .help("consider binding to an unused variable") - .help("consider explicitly droping with `std::mem::drop`") - .emit(); + build_and_emit_lint( + lint, + local, + init.span, + "non-binding let on a type that implements `Drop`", + ); }) } } + fn build_and_emit_lint( + lint: LintDiagnosticBuilder<'_, ()>, + local: &hir::Local<'_>, + init_span: rustc_span::Span, + msg: &str, + ) { + lint.build(msg) + .span_suggestion_verbose( + local.pat.span, + "consider binding to an unused variable", + "_unused", + Applicability::MachineApplicable, + ) + .span_suggestion_verbose( + init_span, + "consider explicitly droping with `std::mem::drop`", + "drop(...)", + Applicability::HasPlaceholders, + ) + .emit(); + } + + // return true if `ty` is a type that is marked as `must_use` fn is_must_use_ty<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> bool { match ty.kind() { ty::Adt(adt, _) => has_must_use_attr(cx, adt.did()), diff --git a/src/test/ui/let_underscore/let_underscore_drop.stderr b/src/test/ui/let_underscore/let_underscore_drop.stderr index f4fd663c7f9ea..5034f682bb76b 100644 --- a/src/test/ui/let_underscore/let_underscore_drop.stderr +++ b/src/test/ui/let_underscore/let_underscore_drop.stderr @@ -5,8 +5,14 @@ LL | let _ = NontrivialDrop; | ^^^^^^^^^^^^^^^^^^^^^^^ | = note: requested on the command line with `-W let-underscore-drop` - = help: consider binding to an unused variable - = help: consider explicitly droping with `std::mem::drop` +help: consider binding to an unused variable + | +LL | let _unused = NontrivialDrop; + | ~~~~~~~ +help: consider explicitly droping with `std::mem::drop` + | +LL | let _ = drop(...); + | ~~~~~~~~~ warning: 1 warning emitted diff --git a/src/test/ui/let_underscore/let_underscore_lock.stderr b/src/test/ui/let_underscore/let_underscore_lock.stderr index 77379d8c3db2c..08f81962f3c65 100644 --- a/src/test/ui/let_underscore/let_underscore_lock.stderr +++ b/src/test/ui/let_underscore/let_underscore_lock.stderr @@ -5,8 +5,14 @@ LL | let _ = data.lock().unwrap(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | = note: `#[warn(let_underscore_lock)]` on by default - = help: consider binding to an unused variable - = help: consider explicitly droping with `std::mem::drop` +help: consider binding to an unused variable + | +LL | let _unused = data.lock().unwrap(); + | ~~~~~~~ +help: consider explicitly droping with `std::mem::drop` + | +LL | let _ = drop(...); + | ~~~~~~~~~ warning: 1 warning emitted diff --git a/src/test/ui/let_underscore/let_underscore_must_use.stderr b/src/test/ui/let_underscore/let_underscore_must_use.stderr index ea1de45e17b57..959572edd7c0d 100644 --- a/src/test/ui/let_underscore/let_underscore_must_use.stderr +++ b/src/test/ui/let_underscore/let_underscore_must_use.stderr @@ -5,8 +5,14 @@ LL | let _ = MustUseType; | ^^^^^^^^^^^^^^^^^^^^ | = note: requested on the command line with `-W let-underscore-must-use` - = help: consider binding to an unused variable - = help: consider explicitly droping with `std::mem::drop` +help: consider binding to an unused variable + | +LL | let _unused = MustUseType; + | ~~~~~~~ +help: consider explicitly droping with `std::mem::drop` + | +LL | let _ = drop(...); + | ~~~~~~~~~ warning: non-binding let on a expression marked `must_use` --> $DIR/let_underscore_must_use.rs:12:5 @@ -14,8 +20,14 @@ warning: non-binding let on a expression marked `must_use` LL | let _ = must_use_function(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | - = help: consider binding to an unused variable - = help: consider explicitly droping with `std::mem::drop` +help: consider binding to an unused variable + | +LL | let _unused = must_use_function(); + | ~~~~~~~ +help: consider explicitly droping with `std::mem::drop` + | +LL | let _ = drop(...); + | ~~~~~~~~~ warning: 2 warnings emitted From 6b179e3a67a2242b63892a1831ba217d968c2f62 Mon Sep 17 00:00:00 2001 From: Aaron Kofsky Date: Sat, 4 Jun 2022 13:50:12 -0400 Subject: [PATCH 07/25] Set `let_underscore_lock` to Deny by default. Clippy sets this lint to Deny by default, and it having the lint be Deny is useful for when we test the lint against a Crater run. --- compiler/rustc_lint/src/let_underscore.rs | 4 ++-- src/test/ui/let_underscore/let_underscore_lock.rs | 4 +--- src/test/ui/let_underscore/let_underscore_lock.stderr | 8 ++++---- 3 files changed, 7 insertions(+), 9 deletions(-) diff --git a/compiler/rustc_lint/src/let_underscore.rs b/compiler/rustc_lint/src/let_underscore.rs index 4e4cedaeb7821..306c6197c24e2 100644 --- a/compiler/rustc_lint/src/let_underscore.rs +++ b/compiler/rustc_lint/src/let_underscore.rs @@ -55,7 +55,7 @@ declare_lint! { /// of at end of scope, which is typically incorrect. /// /// ### Example - /// ```rust + /// ```compile_fail /// use std::sync::{Arc, Mutex}; /// use std::thread; /// let data = Arc::new(Mutex::new(0)); @@ -83,7 +83,7 @@ declare_lint! { /// calling `std::mem::drop` on the expression is clearer and helps convey /// intent. pub LET_UNDERSCORE_LOCK, - Warn, + Deny, "non-binding let on a synchronization lock" } diff --git a/src/test/ui/let_underscore/let_underscore_lock.rs b/src/test/ui/let_underscore/let_underscore_lock.rs index 774b610db2fb8..c37264136ef7f 100644 --- a/src/test/ui/let_underscore/let_underscore_lock.rs +++ b/src/test/ui/let_underscore/let_underscore_lock.rs @@ -1,8 +1,6 @@ -// run-pass - use std::sync::{Arc, Mutex}; fn main() { let data = Arc::new(Mutex::new(0)); - let _ = data.lock().unwrap(); //~WARNING non-binding let on a synchronization lock + let _ = data.lock().unwrap(); //~ERROR non-binding let on a synchronization lock } diff --git a/src/test/ui/let_underscore/let_underscore_lock.stderr b/src/test/ui/let_underscore/let_underscore_lock.stderr index 08f81962f3c65..b7e14e8c7b5c5 100644 --- a/src/test/ui/let_underscore/let_underscore_lock.stderr +++ b/src/test/ui/let_underscore/let_underscore_lock.stderr @@ -1,10 +1,10 @@ -warning: non-binding let on a synchronization lock - --> $DIR/let_underscore_lock.rs:7:5 +error: non-binding let on a synchronization lock + --> $DIR/let_underscore_lock.rs:5:5 | LL | let _ = data.lock().unwrap(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | - = note: `#[warn(let_underscore_lock)]` on by default + = note: `#[deny(let_underscore_lock)]` on by default help: consider binding to an unused variable | LL | let _unused = data.lock().unwrap(); @@ -14,5 +14,5 @@ help: consider explicitly droping with `std::mem::drop` LL | let _ = drop(...); | ~~~~~~~~~ -warning: 1 warning emitted +error: aborting due to previous error From a7e2b3e879790284b64529e3fb7d659274420a90 Mon Sep 17 00:00:00 2001 From: Aaron Kofsky Date: Sat, 4 Jun 2022 19:52:12 -0400 Subject: [PATCH 08/25] Move local functions to outer scope. --- compiler/rustc_lint/src/let_underscore.rs | 134 +++++++++++----------- 1 file changed, 65 insertions(+), 69 deletions(-) diff --git a/compiler/rustc_lint/src/let_underscore.rs b/compiler/rustc_lint/src/let_underscore.rs index 306c6197c24e2..c2d1cff313b87 100644 --- a/compiler/rustc_lint/src/let_underscore.rs +++ b/compiler/rustc_lint/src/let_underscore.rs @@ -175,74 +175,72 @@ impl<'tcx> LateLintPass<'tcx> for LetUnderscore { }) } } + } +} - fn build_and_emit_lint( - lint: LintDiagnosticBuilder<'_, ()>, - local: &hir::Local<'_>, - init_span: rustc_span::Span, - msg: &str, - ) { - lint.build(msg) - .span_suggestion_verbose( - local.pat.span, - "consider binding to an unused variable", - "_unused", - Applicability::MachineApplicable, - ) - .span_suggestion_verbose( - init_span, - "consider explicitly droping with `std::mem::drop`", - "drop(...)", - Applicability::HasPlaceholders, - ) - .emit(); - } +fn build_and_emit_lint( + lint: LintDiagnosticBuilder<'_, ()>, + local: &hir::Local<'_>, + init_span: rustc_span::Span, + msg: &str, +) { + lint.build(msg) + .span_suggestion_verbose( + local.pat.span, + "consider binding to an unused variable", + "_unused", + Applicability::MachineApplicable, + ) + .span_suggestion_verbose( + init_span, + "consider explicitly droping with `std::mem::drop`", + "drop(...)", + Applicability::HasPlaceholders, + ) + .emit(); +} - // return true if `ty` is a type that is marked as `must_use` - fn is_must_use_ty<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> bool { - match ty.kind() { - ty::Adt(adt, _) => has_must_use_attr(cx, adt.did()), - ty::Foreign(ref did) => has_must_use_attr(cx, *did), - ty::Slice(ty) - | ty::Array(ty, _) - | ty::RawPtr(ty::TypeAndMut { ty, .. }) - | ty::Ref(_, ty, _) => { - // for the Array case we don't need to care for the len == 0 case - // because we don't want to lint functions returning empty arrays - is_must_use_ty(cx, *ty) - } - ty::Tuple(substs) => substs.iter().any(|ty| is_must_use_ty(cx, ty)), - ty::Opaque(ref def_id, _) => { - for (predicate, _) in cx.tcx.explicit_item_bounds(*def_id) { - if let ty::PredicateKind::Trait(trait_predicate) = - predicate.kind().skip_binder() - { - if has_must_use_attr(cx, trait_predicate.trait_ref.def_id) { - return true; - } - } +// return true if `ty` is a type that is marked as `must_use` +fn is_must_use_ty<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> bool { + match ty.kind() { + ty::Adt(adt, _) => has_must_use_attr(cx, adt.did()), + ty::Foreign(ref did) => has_must_use_attr(cx, *did), + ty::Slice(ty) + | ty::Array(ty, _) + | ty::RawPtr(ty::TypeAndMut { ty, .. }) + | ty::Ref(_, ty, _) => { + // for the Array case we don't need to care for the len == 0 case + // because we don't want to lint functions returning empty arrays + is_must_use_ty(cx, *ty) + } + ty::Tuple(substs) => substs.iter().any(|ty| is_must_use_ty(cx, ty)), + ty::Opaque(ref def_id, _) => { + for (predicate, _) in cx.tcx.explicit_item_bounds(*def_id) { + if let ty::PredicateKind::Trait(trait_predicate) = predicate.kind().skip_binder() { + if has_must_use_attr(cx, trait_predicate.trait_ref.def_id) { + return true; } - false } - ty::Dynamic(binder, _) => { - for predicate in binder.iter() { - if let ty::ExistentialPredicate::Trait(ref trait_ref) = - predicate.skip_binder() - { - if has_must_use_attr(cx, trait_ref.def_id) { - return true; - } - } + } + false + } + ty::Dynamic(binder, _) => { + for predicate in binder.iter() { + if let ty::ExistentialPredicate::Trait(ref trait_ref) = predicate.skip_binder() { + if has_must_use_attr(cx, trait_ref.def_id) { + return true; } - false } - _ => false, } + false } + _ => false, + } +} - // check if expr is calling method or function with #[must_use] attribute - fn is_must_use_func_call(cx: &LateContext<'_>, expr: &hir::Expr<'_>) -> bool { - let did = match expr.kind { +// check if expr is calling method or function with #[must_use] attribute +fn is_must_use_func_call(cx: &LateContext<'_>, expr: &hir::Expr<'_>) -> bool { + let did = match expr.kind { hir::ExprKind::Call(path, _) if let hir::ExprKind::Path(ref qpath) = path.kind => { if let hir::def::Res::Def(_, did) = cx.qpath_res(qpath, path.hir_id) { Some(did) @@ -256,15 +254,13 @@ impl<'tcx> LateLintPass<'tcx> for LetUnderscore { _ => None, }; - did.map_or(false, |did| has_must_use_attr(cx, did)) - } + did.map_or(false, |did| has_must_use_attr(cx, did)) +} - // returns true if DefId contains a `#[must_use]` attribute - fn has_must_use_attr(cx: &LateContext<'_>, did: hir::def_id::DefId) -> bool { - cx.tcx - .get_attrs(did, rustc_span::sym::must_use) - .find(|a| a.has_name(rustc_span::sym::must_use)) - .is_some() - } - } +// returns true if DefId contains a `#[must_use]` attribute +fn has_must_use_attr(cx: &LateContext<'_>, did: hir::def_id::DefId) -> bool { + cx.tcx + .get_attrs(did, rustc_span::sym::must_use) + .find(|a| a.has_name(rustc_span::sym::must_use)) + .is_some() } From 7e485bf4ddd3f56c76a572757d60c24dcfaa7dbe Mon Sep 17 00:00:00 2001 From: Aaron Kofsky Date: Sat, 4 Jun 2022 20:04:01 -0400 Subject: [PATCH 09/25] Move `let_underscore` tests into the `lint` subfolder. --- src/test/ui/{ => lint}/let_underscore/let_underscore_drop.rs | 0 src/test/ui/{ => lint}/let_underscore/let_underscore_drop.stderr | 0 src/test/ui/{ => lint}/let_underscore/let_underscore_lock.rs | 0 src/test/ui/{ => lint}/let_underscore/let_underscore_lock.stderr | 0 src/test/ui/{ => lint}/let_underscore/let_underscore_must_use.rs | 0 .../ui/{ => lint}/let_underscore/let_underscore_must_use.stderr | 0 6 files changed, 0 insertions(+), 0 deletions(-) rename src/test/ui/{ => lint}/let_underscore/let_underscore_drop.rs (100%) rename src/test/ui/{ => lint}/let_underscore/let_underscore_drop.stderr (100%) rename src/test/ui/{ => lint}/let_underscore/let_underscore_lock.rs (100%) rename src/test/ui/{ => lint}/let_underscore/let_underscore_lock.stderr (100%) rename src/test/ui/{ => lint}/let_underscore/let_underscore_must_use.rs (100%) rename src/test/ui/{ => lint}/let_underscore/let_underscore_must_use.stderr (100%) diff --git a/src/test/ui/let_underscore/let_underscore_drop.rs b/src/test/ui/lint/let_underscore/let_underscore_drop.rs similarity index 100% rename from src/test/ui/let_underscore/let_underscore_drop.rs rename to src/test/ui/lint/let_underscore/let_underscore_drop.rs diff --git a/src/test/ui/let_underscore/let_underscore_drop.stderr b/src/test/ui/lint/let_underscore/let_underscore_drop.stderr similarity index 100% rename from src/test/ui/let_underscore/let_underscore_drop.stderr rename to src/test/ui/lint/let_underscore/let_underscore_drop.stderr diff --git a/src/test/ui/let_underscore/let_underscore_lock.rs b/src/test/ui/lint/let_underscore/let_underscore_lock.rs similarity index 100% rename from src/test/ui/let_underscore/let_underscore_lock.rs rename to src/test/ui/lint/let_underscore/let_underscore_lock.rs diff --git a/src/test/ui/let_underscore/let_underscore_lock.stderr b/src/test/ui/lint/let_underscore/let_underscore_lock.stderr similarity index 100% rename from src/test/ui/let_underscore/let_underscore_lock.stderr rename to src/test/ui/lint/let_underscore/let_underscore_lock.stderr diff --git a/src/test/ui/let_underscore/let_underscore_must_use.rs b/src/test/ui/lint/let_underscore/let_underscore_must_use.rs similarity index 100% rename from src/test/ui/let_underscore/let_underscore_must_use.rs rename to src/test/ui/lint/let_underscore/let_underscore_must_use.rs diff --git a/src/test/ui/let_underscore/let_underscore_must_use.stderr b/src/test/ui/lint/let_underscore/let_underscore_must_use.stderr similarity index 100% rename from src/test/ui/let_underscore/let_underscore_must_use.stderr rename to src/test/ui/lint/let_underscore/let_underscore_must_use.stderr From 1421cffca17f8904664d036996768d5880604f71 Mon Sep 17 00:00:00 2001 From: Aaron Kofsky Date: Sat, 4 Jun 2022 20:12:26 -0400 Subject: [PATCH 10/25] Add `let_underscore` lint group to `GROUP_DESCRIPTIONS`. --- src/tools/lint-docs/src/groups.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/tools/lint-docs/src/groups.rs b/src/tools/lint-docs/src/groups.rs index 9696e35b7963f..2a923a61b0a70 100644 --- a/src/tools/lint-docs/src/groups.rs +++ b/src/tools/lint-docs/src/groups.rs @@ -8,6 +8,7 @@ use std::process::Command; /// Descriptions of rustc lint groups. static GROUP_DESCRIPTIONS: &[(&str, &str)] = &[ ("unused", "Lints that detect things being declared but not used, or excess syntax"), + ("let-underscore", "Lints that detect wildcard let bindings that are likely to be invalid"), ("rustdoc", "Rustdoc-specific lints"), ("rust-2018-idioms", "Lints to nudge you toward idiomatic features of Rust 2018"), ("nonstandard-style", "Violation of standard naming conventions"), From 30e8adb1a766e8444586fef3953bf7715fe588e1 Mon Sep 17 00:00:00 2001 From: Aaron Kofsky Date: Sat, 4 Jun 2022 20:16:56 -0400 Subject: [PATCH 11/25] Use `has_attr` instead of `get_attrs` in `has_must_use_attr` --- compiler/rustc_lint/src/let_underscore.rs | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/compiler/rustc_lint/src/let_underscore.rs b/compiler/rustc_lint/src/let_underscore.rs index c2d1cff313b87..d33ea450b3e89 100644 --- a/compiler/rustc_lint/src/let_underscore.rs +++ b/compiler/rustc_lint/src/let_underscore.rs @@ -259,8 +259,5 @@ fn is_must_use_func_call(cx: &LateContext<'_>, expr: &hir::Expr<'_>) -> bool { // returns true if DefId contains a `#[must_use]` attribute fn has_must_use_attr(cx: &LateContext<'_>, did: hir::def_id::DefId) -> bool { - cx.tcx - .get_attrs(did, rustc_span::sym::must_use) - .find(|a| a.has_name(rustc_span::sym::must_use)) - .is_some() + cx.tcx.has_attr(did, rustc_span::sym::must_use) } From e6b66784aca8564557485d902968ff7523cf30ca Mon Sep 17 00:00:00 2001 From: Aaron Kofsky Date: Sat, 4 Jun 2022 20:19:19 -0400 Subject: [PATCH 12/25] Bail out early if the type does not has a trivial Drop implementation. If the type has a trivial Drop implementation, then it is probably irrelevant that the type was dropped immediately, since nothing important happens on drop. Hence, we can bail out early instead of doing some expensive checks. --- compiler/rustc_lint/src/let_underscore.rs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/compiler/rustc_lint/src/let_underscore.rs b/compiler/rustc_lint/src/let_underscore.rs index d33ea450b3e89..1e4565a226c7a 100644 --- a/compiler/rustc_lint/src/let_underscore.rs +++ b/compiler/rustc_lint/src/let_underscore.rs @@ -129,7 +129,11 @@ impl<'tcx> LateLintPass<'tcx> for LetUnderscore { } if let Some(init) = local.init { let init_ty = cx.typeck_results().expr_ty(init); - let needs_drop = init_ty.needs_drop(cx.tcx, cx.param_env); + // If the type has a trivial Drop implementation, then it doesn't + // matter that we drop the value immediately. + if !init_ty.needs_drop(cx.tcx, cx.param_env) { + return; + } let is_sync_lock = init_ty.walk().any(|inner| match inner.unpack() { GenericArgKind::Type(inner_ty) => { SYNC_GUARD_PATHS.iter().any(|guard_path| match inner_ty.kind() { @@ -164,7 +168,7 @@ impl<'tcx> LateLintPass<'tcx> for LetUnderscore { "non-binding let on a expression marked `must_use`", ); }) - } else if needs_drop { + } else { cx.struct_span_lint(LET_UNDERSCORE_DROP, local.span, |lint| { build_and_emit_lint( lint, From 6342b58ef0f71ca0284dced4b1d22f9726c1c74a Mon Sep 17 00:00:00 2001 From: Aaron Kofsky Date: Sat, 4 Jun 2022 22:27:32 -0400 Subject: [PATCH 13/25] Use diagnostic items instead of hard coded paths for `let_underscore_lock` Using diagnostic items avoids having to update the paths if the guard types ever get moved around for some reason. Additionally, it also greatly simplifies the `is_sync_lock` check. --- compiler/rustc_lint/src/let_underscore.rs | 31 ++++++++--------------- compiler/rustc_span/src/symbol.rs | 3 +++ 2 files changed, 14 insertions(+), 20 deletions(-) diff --git a/compiler/rustc_lint/src/let_underscore.rs b/compiler/rustc_lint/src/let_underscore.rs index 1e4565a226c7a..2ad09312d9d56 100644 --- a/compiler/rustc_lint/src/let_underscore.rs +++ b/compiler/rustc_lint/src/let_underscore.rs @@ -3,7 +3,7 @@ use rustc_errors::Applicability; use rustc_hir as hir; use rustc_middle::{ lint::LintDiagnosticBuilder, - ty::{self, subst::GenericArgKind, Ty}, + ty::{self, Ty}, }; use rustc_span::Symbol; @@ -114,12 +114,10 @@ declare_lint! { declare_lint_pass!(LetUnderscore => [LET_UNDERSCORE_DROP, LET_UNDERSCORE_LOCK, LET_UNDERSCORE_MUST_USE]); -const SYNC_GUARD_PATHS: [&[&str]; 5] = [ - &["std", "sync", "mutex", "MutexGuard"], - &["std", "sync", "rwlock", "RwLockReadGuard"], - &["std", "sync", "rwlock", "RwLockWriteGuard"], - &["parking_lot", "raw_mutex", "RawMutex"], - &["parking_lot", "raw_rwlock", "RawRwLock"], +const SYNC_GUARD_SYMBOLS: [Symbol; 3] = [ + rustc_span::sym::MutexGuard, + rustc_span::sym::RwLockReadGuard, + rustc_span::sym::RwLockWriteGuard, ]; impl<'tcx> LateLintPass<'tcx> for LetUnderscore { @@ -134,19 +132,12 @@ impl<'tcx> LateLintPass<'tcx> for LetUnderscore { if !init_ty.needs_drop(cx.tcx, cx.param_env) { return; } - let is_sync_lock = init_ty.walk().any(|inner| match inner.unpack() { - GenericArgKind::Type(inner_ty) => { - SYNC_GUARD_PATHS.iter().any(|guard_path| match inner_ty.kind() { - ty::Adt(adt, _) => { - let ty_path = cx.get_def_path(adt.did()); - guard_path.iter().map(|x| Symbol::intern(x)).eq(ty_path.iter().copied()) - } - _ => false, - }) - } - - GenericArgKind::Lifetime(_) | GenericArgKind::Const(_) => false, - }); + let is_sync_lock = match init_ty.kind() { + ty::Adt(adt, _) => SYNC_GUARD_SYMBOLS + .iter() + .any(|guard_symbol| cx.tcx.is_diagnostic_item(*guard_symbol, adt.did())), + _ => false, + }; let is_must_use_ty = is_must_use_ty(cx, cx.typeck_results().expr_ty(init)); let is_must_use_func_call = is_must_use_func_call(cx, init); diff --git a/compiler/rustc_span/src/symbol.rs b/compiler/rustc_span/src/symbol.rs index 5c9c16350e469..bc9f5c910df54 100644 --- a/compiler/rustc_span/src/symbol.rs +++ b/compiler/rustc_span/src/symbol.rs @@ -213,6 +213,7 @@ symbols! { LinkedList, LintPass, Mutex, + MutexGuard, N, None, Ok, @@ -250,6 +251,8 @@ symbols! { Right, RustcDecodable, RustcEncodable, + RwLockReadGuard, + RwLockWriteGuard, Send, SeqCst, SliceIndex, From 11663b1d78cd4e9bc68cf13e55ee73189f1d1cf5 Mon Sep 17 00:00:00 2001 From: Aaron Kofsky Date: Sat, 4 Jun 2022 23:31:47 -0400 Subject: [PATCH 14/25] Use `check-pass` instead of `run-pass` We don't actually care about running these programs, only checking the warnings they generate. --- src/test/ui/lint/let_underscore/let_underscore_drop.rs | 2 +- src/test/ui/lint/let_underscore/let_underscore_lock.rs | 1 + src/test/ui/lint/let_underscore/let_underscore_must_use.rs | 2 +- 3 files changed, 3 insertions(+), 2 deletions(-) diff --git a/src/test/ui/lint/let_underscore/let_underscore_drop.rs b/src/test/ui/lint/let_underscore/let_underscore_drop.rs index 75c6ecec3ee53..85ca5dd2880a4 100644 --- a/src/test/ui/lint/let_underscore/let_underscore_drop.rs +++ b/src/test/ui/lint/let_underscore/let_underscore_drop.rs @@ -1,4 +1,4 @@ -// run-pass +// check-pass // compile-flags: -W let_underscore_drop struct NontrivialDrop; diff --git a/src/test/ui/lint/let_underscore/let_underscore_lock.rs b/src/test/ui/lint/let_underscore/let_underscore_lock.rs index c37264136ef7f..7423862cdf05d 100644 --- a/src/test/ui/lint/let_underscore/let_underscore_lock.rs +++ b/src/test/ui/lint/let_underscore/let_underscore_lock.rs @@ -1,3 +1,4 @@ +// check-fail use std::sync::{Arc, Mutex}; fn main() { diff --git a/src/test/ui/lint/let_underscore/let_underscore_must_use.rs b/src/test/ui/lint/let_underscore/let_underscore_must_use.rs index 8efaad51ea877..3f79dc7a096b6 100644 --- a/src/test/ui/lint/let_underscore/let_underscore_must_use.rs +++ b/src/test/ui/lint/let_underscore/let_underscore_must_use.rs @@ -1,4 +1,4 @@ -// run-pass +// check-pass // compile-flags: -W let_underscore_must_use #[must_use] From b5b5b5471ba324f6b4d5a53957a800ad44126964 Mon Sep 17 00:00:00 2001 From: Aaron Kofsky Date: Sun, 5 Jun 2022 00:05:50 -0400 Subject: [PATCH 15/25] Remove `let_underscore_must_use` The `let_underscore_must_use` lint was really only added because clippy included it, but it doesn't actually seem very useful. --- compiler/rustc_lint/src/let_underscore.rs | 105 +----------------- compiler/rustc_lint/src/lib.rs | 7 +- .../let_underscore/let_underscore_must_use.rs | 13 --- .../let_underscore_must_use.stderr | 33 ------ 4 files changed, 3 insertions(+), 155 deletions(-) delete mode 100644 src/test/ui/lint/let_underscore/let_underscore_must_use.rs delete mode 100644 src/test/ui/lint/let_underscore/let_underscore_must_use.stderr diff --git a/compiler/rustc_lint/src/let_underscore.rs b/compiler/rustc_lint/src/let_underscore.rs index 2ad09312d9d56..dea878c4460b9 100644 --- a/compiler/rustc_lint/src/let_underscore.rs +++ b/compiler/rustc_lint/src/let_underscore.rs @@ -1,10 +1,7 @@ use crate::{LateContext, LateLintPass, LintContext}; use rustc_errors::Applicability; use rustc_hir as hir; -use rustc_middle::{ - lint::LintDiagnosticBuilder, - ty::{self, Ty}, -}; +use rustc_middle::{lint::LintDiagnosticBuilder, ty}; use rustc_span::Symbol; declare_lint! { @@ -87,32 +84,7 @@ declare_lint! { "non-binding let on a synchronization lock" } -declare_lint! { - /// The `let_underscore_must_use` lint checks for statements which don't bind - /// a `must_use` expression to anything, causing the lock to be released - /// immediately instead of at end of scope, which is typically incorrect. - /// - /// ### Example - /// ```rust - /// #[must_use] - /// struct SomeStruct; - /// - /// fn main() { - /// // SomeStuct is dropped immediately instead of at end of scope. - /// let _ = SomeStruct; - /// } - /// ``` - /// ### Explanation - /// - /// Statements which assign an expression to an underscore causes the - /// expression to immediately drop. Usually, it's better to explicitly handle - /// the `must_use` expression. - pub LET_UNDERSCORE_MUST_USE, - Allow, - "non-binding let on a expression marked `must_use`" -} - -declare_lint_pass!(LetUnderscore => [LET_UNDERSCORE_DROP, LET_UNDERSCORE_LOCK, LET_UNDERSCORE_MUST_USE]); +declare_lint_pass!(LetUnderscore => [LET_UNDERSCORE_DROP, LET_UNDERSCORE_LOCK]); const SYNC_GUARD_SYMBOLS: [Symbol; 3] = [ rustc_span::sym::MutexGuard, @@ -138,8 +110,6 @@ impl<'tcx> LateLintPass<'tcx> for LetUnderscore { .any(|guard_symbol| cx.tcx.is_diagnostic_item(*guard_symbol, adt.did())), _ => false, }; - let is_must_use_ty = is_must_use_ty(cx, cx.typeck_results().expr_ty(init)); - let is_must_use_func_call = is_must_use_func_call(cx, init); if is_sync_lock { cx.struct_span_lint(LET_UNDERSCORE_LOCK, local.span, |lint| { @@ -150,15 +120,6 @@ impl<'tcx> LateLintPass<'tcx> for LetUnderscore { "non-binding let on a synchronization lock", ) }) - } else if is_must_use_ty || is_must_use_func_call { - cx.struct_span_lint(LET_UNDERSCORE_MUST_USE, local.span, |lint| { - build_and_emit_lint( - lint, - local, - init.span, - "non-binding let on a expression marked `must_use`", - ); - }) } else { cx.struct_span_lint(LET_UNDERSCORE_DROP, local.span, |lint| { build_and_emit_lint( @@ -194,65 +155,3 @@ fn build_and_emit_lint( ) .emit(); } - -// return true if `ty` is a type that is marked as `must_use` -fn is_must_use_ty<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> bool { - match ty.kind() { - ty::Adt(adt, _) => has_must_use_attr(cx, adt.did()), - ty::Foreign(ref did) => has_must_use_attr(cx, *did), - ty::Slice(ty) - | ty::Array(ty, _) - | ty::RawPtr(ty::TypeAndMut { ty, .. }) - | ty::Ref(_, ty, _) => { - // for the Array case we don't need to care for the len == 0 case - // because we don't want to lint functions returning empty arrays - is_must_use_ty(cx, *ty) - } - ty::Tuple(substs) => substs.iter().any(|ty| is_must_use_ty(cx, ty)), - ty::Opaque(ref def_id, _) => { - for (predicate, _) in cx.tcx.explicit_item_bounds(*def_id) { - if let ty::PredicateKind::Trait(trait_predicate) = predicate.kind().skip_binder() { - if has_must_use_attr(cx, trait_predicate.trait_ref.def_id) { - return true; - } - } - } - false - } - ty::Dynamic(binder, _) => { - for predicate in binder.iter() { - if let ty::ExistentialPredicate::Trait(ref trait_ref) = predicate.skip_binder() { - if has_must_use_attr(cx, trait_ref.def_id) { - return true; - } - } - } - false - } - _ => false, - } -} - -// check if expr is calling method or function with #[must_use] attribute -fn is_must_use_func_call(cx: &LateContext<'_>, expr: &hir::Expr<'_>) -> bool { - let did = match expr.kind { - hir::ExprKind::Call(path, _) if let hir::ExprKind::Path(ref qpath) = path.kind => { - if let hir::def::Res::Def(_, did) = cx.qpath_res(qpath, path.hir_id) { - Some(did) - } else { - None - } - }, - hir::ExprKind::MethodCall(..) => { - cx.typeck_results().type_dependent_def_id(expr.hir_id) - } - _ => None, - }; - - did.map_or(false, |did| has_must_use_attr(cx, did)) -} - -// returns true if DefId contains a `#[must_use]` attribute -fn has_must_use_attr(cx: &LateContext<'_>, did: hir::def_id::DefId) -> bool { - cx.tcx.has_attr(did, rustc_span::sym::must_use) -} diff --git a/compiler/rustc_lint/src/lib.rs b/compiler/rustc_lint/src/lib.rs index 4359a54b698dc..79661c0fefe8d 100644 --- a/compiler/rustc_lint/src/lib.rs +++ b/compiler/rustc_lint/src/lib.rs @@ -317,12 +317,7 @@ fn register_builtins(store: &mut LintStore, no_interleave_lints: bool) { REDUNDANT_SEMICOLONS ); - add_lint_group!( - "let_underscore", - LET_UNDERSCORE_DROP, - LET_UNDERSCORE_LOCK, - LET_UNDERSCORE_MUST_USE - ); + add_lint_group!("let_underscore", LET_UNDERSCORE_DROP, LET_UNDERSCORE_LOCK); add_lint_group!( "rust_2018_idioms", diff --git a/src/test/ui/lint/let_underscore/let_underscore_must_use.rs b/src/test/ui/lint/let_underscore/let_underscore_must_use.rs deleted file mode 100644 index 3f79dc7a096b6..0000000000000 --- a/src/test/ui/lint/let_underscore/let_underscore_must_use.rs +++ /dev/null @@ -1,13 +0,0 @@ -// check-pass -// compile-flags: -W let_underscore_must_use - -#[must_use] -struct MustUseType; - -#[must_use] -fn must_use_function() -> () {} - -fn main() { - let _ = MustUseType; //~WARNING non-binding let on a expression marked `must_use` - let _ = must_use_function(); //~WARNING non-binding let on a expression marked `must_use` -} diff --git a/src/test/ui/lint/let_underscore/let_underscore_must_use.stderr b/src/test/ui/lint/let_underscore/let_underscore_must_use.stderr deleted file mode 100644 index 959572edd7c0d..0000000000000 --- a/src/test/ui/lint/let_underscore/let_underscore_must_use.stderr +++ /dev/null @@ -1,33 +0,0 @@ -warning: non-binding let on a expression marked `must_use` - --> $DIR/let_underscore_must_use.rs:11:5 - | -LL | let _ = MustUseType; - | ^^^^^^^^^^^^^^^^^^^^ - | - = note: requested on the command line with `-W let-underscore-must-use` -help: consider binding to an unused variable - | -LL | let _unused = MustUseType; - | ~~~~~~~ -help: consider explicitly droping with `std::mem::drop` - | -LL | let _ = drop(...); - | ~~~~~~~~~ - -warning: non-binding let on a expression marked `must_use` - --> $DIR/let_underscore_must_use.rs:12:5 - | -LL | let _ = must_use_function(); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ - | -help: consider binding to an unused variable - | -LL | let _unused = must_use_function(); - | ~~~~~~~ -help: consider explicitly droping with `std::mem::drop` - | -LL | let _ = drop(...); - | ~~~~~~~~~ - -warning: 2 warnings emitted - From 321a598b7519733aff7dea8b4ebd3631626d656b Mon Sep 17 00:00:00 2001 From: Aaron Kofsky Date: Sun, 5 Jun 2022 01:01:54 -0400 Subject: [PATCH 16/25] Add diagnostic items to MutexGuard and RwLock Guards I forgot to add the diagnostic to the actual types in `std` earlier. --- library/std/src/sync/mutex.rs | 1 + library/std/src/sync/rwlock.rs | 2 ++ src/test/ui/lint/let_underscore/let_underscore_lock.stderr | 2 +- 3 files changed, 4 insertions(+), 1 deletion(-) diff --git a/library/std/src/sync/mutex.rs b/library/std/src/sync/mutex.rs index 3d8281fe59389..d976725c2761a 100644 --- a/library/std/src/sync/mutex.rs +++ b/library/std/src/sync/mutex.rs @@ -193,6 +193,7 @@ unsafe impl Sync for Mutex {} and cause Futures to not implement `Send`"] #[stable(feature = "rust1", since = "1.0.0")] #[clippy::has_significant_drop] +#[cfg_attr(not(test), rustc_diagnostic_item = "MutexGuard")] pub struct MutexGuard<'a, T: ?Sized + 'a> { lock: &'a Mutex, poison: poison::Guard, diff --git a/library/std/src/sync/rwlock.rs b/library/std/src/sync/rwlock.rs index 4f1b4bedaab25..e956f00a12fd3 100644 --- a/library/std/src/sync/rwlock.rs +++ b/library/std/src/sync/rwlock.rs @@ -100,6 +100,7 @@ unsafe impl Sync for RwLock {} and cause Futures to not implement `Send`"] #[stable(feature = "rust1", since = "1.0.0")] #[clippy::has_significant_drop] +#[cfg_attr(not(test), rustc_diagnostic_item = "RwLockReadGuard")] pub struct RwLockReadGuard<'a, T: ?Sized + 'a> { lock: &'a RwLock, } @@ -124,6 +125,7 @@ unsafe impl Sync for RwLockReadGuard<'_, T> {} and cause Future's to not implement `Send`"] #[stable(feature = "rust1", since = "1.0.0")] #[clippy::has_significant_drop] +#[cfg_attr(not(test), rustc_diagnostic_item = "RwLockWriteGuard")] pub struct RwLockWriteGuard<'a, T: ?Sized + 'a> { lock: &'a RwLock, poison: poison::Guard, diff --git a/src/test/ui/lint/let_underscore/let_underscore_lock.stderr b/src/test/ui/lint/let_underscore/let_underscore_lock.stderr index b7e14e8c7b5c5..7ff42cb152441 100644 --- a/src/test/ui/lint/let_underscore/let_underscore_lock.stderr +++ b/src/test/ui/lint/let_underscore/let_underscore_lock.stderr @@ -1,5 +1,5 @@ error: non-binding let on a synchronization lock - --> $DIR/let_underscore_lock.rs:5:5 + --> $DIR/let_underscore_lock.rs:6:5 | LL | let _ = data.lock().unwrap(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ From 211feb106a188fef698bb36d19402808f9930154 Mon Sep 17 00:00:00 2001 From: Aaron Kofsky Date: Sun, 5 Jun 2022 12:47:19 -0400 Subject: [PATCH 17/25] Add `{{produces}}` tag to lint doc comments. --- compiler/rustc_lint/src/let_underscore.rs | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/compiler/rustc_lint/src/let_underscore.rs b/compiler/rustc_lint/src/let_underscore.rs index dea878c4460b9..520c865cc1922 100644 --- a/compiler/rustc_lint/src/let_underscore.rs +++ b/compiler/rustc_lint/src/let_underscore.rs @@ -11,7 +11,7 @@ declare_lint! { /// scope. /// /// ### Example - /// ```rust + /// ``` /// struct SomeStruct; /// impl Drop for SomeStruct { /// fn drop(&mut self) { @@ -20,6 +20,7 @@ declare_lint! { /// } /// /// fn main() { + /// #[warn(let_underscore_drop)] /// // SomeStuct is dropped immediately instead of at end of scope, /// // so "Dropping SomeStruct" is printed before "end of main". /// // The order of prints would be reversed if SomeStruct was bound to @@ -28,6 +29,9 @@ declare_lint! { /// println!("end of main"); /// } /// ``` + /// + /// {{produces}} + /// /// ### Explanation /// /// Statements which assign an expression to an underscore causes the @@ -66,6 +70,9 @@ declare_lint! { /// *lock += 1; /// }); /// ``` + /// + /// {{produces}} + /// /// ### Explanation /// /// Statements which assign an expression to an underscore causes the From cdf66060666fa53cbca9647ac7434bdfd2cc37a5 Mon Sep 17 00:00:00 2001 From: Aaron Kofsky Date: Thu, 9 Jun 2022 14:03:35 -0400 Subject: [PATCH 18/25] Use `multipart_suggestion` to create an applicable suggestion. The "consider explicitly droping" can now suggest a machine applicable suggestion now. --- compiler/rustc_lint/src/let_underscore.rs | 10 ++++++---- .../ui/lint/let_underscore/let_underscore_drop.stderr | 4 ++-- .../ui/lint/let_underscore/let_underscore_lock.stderr | 4 ++-- 3 files changed, 10 insertions(+), 8 deletions(-) diff --git a/compiler/rustc_lint/src/let_underscore.rs b/compiler/rustc_lint/src/let_underscore.rs index 520c865cc1922..18661e8c5059c 100644 --- a/compiler/rustc_lint/src/let_underscore.rs +++ b/compiler/rustc_lint/src/let_underscore.rs @@ -154,11 +154,13 @@ fn build_and_emit_lint( "_unused", Applicability::MachineApplicable, ) - .span_suggestion_verbose( - init_span, + .multipart_suggestion( "consider explicitly droping with `std::mem::drop`", - "drop(...)", - Applicability::HasPlaceholders, + vec![ + (init_span.shrink_to_lo(), "drop(".to_string()), + (init_span.shrink_to_hi(), ")".to_string()), + ], + Applicability::MachineApplicable, ) .emit(); } diff --git a/src/test/ui/lint/let_underscore/let_underscore_drop.stderr b/src/test/ui/lint/let_underscore/let_underscore_drop.stderr index 5034f682bb76b..dfac6d3f74185 100644 --- a/src/test/ui/lint/let_underscore/let_underscore_drop.stderr +++ b/src/test/ui/lint/let_underscore/let_underscore_drop.stderr @@ -11,8 +11,8 @@ LL | let _unused = NontrivialDrop; | ~~~~~~~ help: consider explicitly droping with `std::mem::drop` | -LL | let _ = drop(...); - | ~~~~~~~~~ +LL | let _ = drop(NontrivialDrop); + | +++++ + warning: 1 warning emitted diff --git a/src/test/ui/lint/let_underscore/let_underscore_lock.stderr b/src/test/ui/lint/let_underscore/let_underscore_lock.stderr index 7ff42cb152441..f37483ddd96d8 100644 --- a/src/test/ui/lint/let_underscore/let_underscore_lock.stderr +++ b/src/test/ui/lint/let_underscore/let_underscore_lock.stderr @@ -11,8 +11,8 @@ LL | let _unused = data.lock().unwrap(); | ~~~~~~~ help: consider explicitly droping with `std::mem::drop` | -LL | let _ = drop(...); - | ~~~~~~~~~ +LL | let _ = drop(data.lock().unwrap()); + | +++++ + error: aborting due to previous error From 7237e8635dc0611a051de8d398759fe7eae90d0a Mon Sep 17 00:00:00 2001 From: Aaron Kofsky Date: Sat, 11 Jun 2022 10:19:53 -0400 Subject: [PATCH 19/25] Reword suggestion messages. --- compiler/rustc_lint/src/let_underscore.rs | 4 ++-- src/test/ui/lint/let_underscore/let_underscore_drop.stderr | 4 ++-- src/test/ui/lint/let_underscore/let_underscore_lock.stderr | 4 ++-- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/compiler/rustc_lint/src/let_underscore.rs b/compiler/rustc_lint/src/let_underscore.rs index 18661e8c5059c..fe0e0511f6b49 100644 --- a/compiler/rustc_lint/src/let_underscore.rs +++ b/compiler/rustc_lint/src/let_underscore.rs @@ -150,12 +150,12 @@ fn build_and_emit_lint( lint.build(msg) .span_suggestion_verbose( local.pat.span, - "consider binding to an unused variable", + "consider binding to an unused variable to avoid immediately dropping the value", "_unused", Applicability::MachineApplicable, ) .multipart_suggestion( - "consider explicitly droping with `std::mem::drop`", + "consider immediately dropping the value", vec![ (init_span.shrink_to_lo(), "drop(".to_string()), (init_span.shrink_to_hi(), ")".to_string()), diff --git a/src/test/ui/lint/let_underscore/let_underscore_drop.stderr b/src/test/ui/lint/let_underscore/let_underscore_drop.stderr index dfac6d3f74185..b6ff9d1a27cd8 100644 --- a/src/test/ui/lint/let_underscore/let_underscore_drop.stderr +++ b/src/test/ui/lint/let_underscore/let_underscore_drop.stderr @@ -5,11 +5,11 @@ LL | let _ = NontrivialDrop; | ^^^^^^^^^^^^^^^^^^^^^^^ | = note: requested on the command line with `-W let-underscore-drop` -help: consider binding to an unused variable +help: consider binding to an unused variable to avoid immediately dropping the value | LL | let _unused = NontrivialDrop; | ~~~~~~~ -help: consider explicitly droping with `std::mem::drop` +help: consider immediately dropping the value | LL | let _ = drop(NontrivialDrop); | +++++ + diff --git a/src/test/ui/lint/let_underscore/let_underscore_lock.stderr b/src/test/ui/lint/let_underscore/let_underscore_lock.stderr index f37483ddd96d8..1e49b89c5a874 100644 --- a/src/test/ui/lint/let_underscore/let_underscore_lock.stderr +++ b/src/test/ui/lint/let_underscore/let_underscore_lock.stderr @@ -5,11 +5,11 @@ LL | let _ = data.lock().unwrap(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | = note: `#[deny(let_underscore_lock)]` on by default -help: consider binding to an unused variable +help: consider binding to an unused variable to avoid immediately dropping the value | LL | let _unused = data.lock().unwrap(); | ~~~~~~~ -help: consider explicitly droping with `std::mem::drop` +help: consider immediately dropping the value | LL | let _ = drop(data.lock().unwrap()); | +++++ + From b040666e052a94241d2598b06d5e5dc19c77257b Mon Sep 17 00:00:00 2001 From: Aaron Kofsky Date: Sat, 11 Jun 2022 10:36:48 -0400 Subject: [PATCH 20/25] Have the drop code suggestion not include `let _ =` --- compiler/rustc_lint/src/let_underscore.rs | 2 +- src/test/ui/lint/let_underscore/let_underscore_drop.stderr | 4 ++-- src/test/ui/lint/let_underscore/let_underscore_lock.stderr | 4 ++-- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/compiler/rustc_lint/src/let_underscore.rs b/compiler/rustc_lint/src/let_underscore.rs index fe0e0511f6b49..2ba79aacace83 100644 --- a/compiler/rustc_lint/src/let_underscore.rs +++ b/compiler/rustc_lint/src/let_underscore.rs @@ -157,7 +157,7 @@ fn build_and_emit_lint( .multipart_suggestion( "consider immediately dropping the value", vec![ - (init_span.shrink_to_lo(), "drop(".to_string()), + (local.span.until(init_span), "drop(".to_string()), (init_span.shrink_to_hi(), ")".to_string()), ], Applicability::MachineApplicable, diff --git a/src/test/ui/lint/let_underscore/let_underscore_drop.stderr b/src/test/ui/lint/let_underscore/let_underscore_drop.stderr index b6ff9d1a27cd8..cf7b882e946d9 100644 --- a/src/test/ui/lint/let_underscore/let_underscore_drop.stderr +++ b/src/test/ui/lint/let_underscore/let_underscore_drop.stderr @@ -11,8 +11,8 @@ LL | let _unused = NontrivialDrop; | ~~~~~~~ help: consider immediately dropping the value | -LL | let _ = drop(NontrivialDrop); - | +++++ + +LL | drop(NontrivialDrop); + | ~~~~~ + warning: 1 warning emitted diff --git a/src/test/ui/lint/let_underscore/let_underscore_lock.stderr b/src/test/ui/lint/let_underscore/let_underscore_lock.stderr index 1e49b89c5a874..7aa119003b4ba 100644 --- a/src/test/ui/lint/let_underscore/let_underscore_lock.stderr +++ b/src/test/ui/lint/let_underscore/let_underscore_lock.stderr @@ -11,8 +11,8 @@ LL | let _unused = data.lock().unwrap(); | ~~~~~~~ help: consider immediately dropping the value | -LL | let _ = drop(data.lock().unwrap()); - | +++++ + +LL | drop(data.lock().unwrap()); + | ~~~~~ + error: aborting due to previous error From 8807c2d8e512668a6f09ddb14c6742bf28e66816 Mon Sep 17 00:00:00 2001 From: Aaron Kofsky Date: Sat, 11 Jun 2022 10:58:04 -0400 Subject: [PATCH 21/25] Make `let_underscore_drop` Deny by default. This is done so that we can check the noisiness of this lint in a Crater run. Note that when I built the compiler, I actually encountered lots of places where this lint will trigger and fail compilation, so I had to also set `RUSTFLAGS_NOT_BOOSTRAP` to `-A let_underscore_drop` when compiling to prevent that. --- compiler/rustc_lint/src/let_underscore.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler/rustc_lint/src/let_underscore.rs b/compiler/rustc_lint/src/let_underscore.rs index 2ba79aacace83..d40c83e311f96 100644 --- a/compiler/rustc_lint/src/let_underscore.rs +++ b/compiler/rustc_lint/src/let_underscore.rs @@ -46,7 +46,7 @@ declare_lint! { /// calling `std::mem::drop` on the expression is clearer and helps convey /// intent. pub LET_UNDERSCORE_DROP, - Allow, + Deny, "non-binding let on a type that implements `Drop`" } From a9095ff2139fb305b15006d1f2a1ff16da0b6c29 Mon Sep 17 00:00:00 2001 From: Aaron Kofsky Date: Thu, 16 Jun 2022 21:07:00 -0400 Subject: [PATCH 22/25] Re-allow `let_underscore_drop` by default. This lint is way way too noisy to have it be `Deny` by default. --- compiler/rustc_lint/src/let_underscore.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler/rustc_lint/src/let_underscore.rs b/compiler/rustc_lint/src/let_underscore.rs index d40c83e311f96..2ba79aacace83 100644 --- a/compiler/rustc_lint/src/let_underscore.rs +++ b/compiler/rustc_lint/src/let_underscore.rs @@ -46,7 +46,7 @@ declare_lint! { /// calling `std::mem::drop` on the expression is clearer and helps convey /// intent. pub LET_UNDERSCORE_DROP, - Deny, + Allow, "non-binding let on a type that implements `Drop`" } From a9f1b7bd2a25e34de29eb88f81550690f4fec5dc Mon Sep 17 00:00:00 2001 From: Aaron Kofsky Date: Thu, 4 Aug 2022 17:00:48 -0400 Subject: [PATCH 23/25] Explain why let-underscoring a lock guard is incorrect. Currently, the let_underscore_lock lint simply tells what is wrong, but not why it is wrong. We fix this by using a `MultiSpan` to explain specifically that doing `let _ = ` immediately drops the lock guard because it does not assign the lock guard to a binding. --- compiler/rustc_lint/src/let_underscore.rs | 13 +++++++++++-- .../lint/let_underscore/let_underscore_lock.stderr | 6 ++++-- 2 files changed, 15 insertions(+), 4 deletions(-) diff --git a/compiler/rustc_lint/src/let_underscore.rs b/compiler/rustc_lint/src/let_underscore.rs index 2ba79aacace83..79d1443dc353c 100644 --- a/compiler/rustc_lint/src/let_underscore.rs +++ b/compiler/rustc_lint/src/let_underscore.rs @@ -1,5 +1,5 @@ use crate::{LateContext, LateLintPass, LintContext}; -use rustc_errors::Applicability; +use rustc_errors::{Applicability, MultiSpan}; use rustc_hir as hir; use rustc_middle::{lint::LintDiagnosticBuilder, ty}; use rustc_span::Symbol; @@ -119,7 +119,16 @@ impl<'tcx> LateLintPass<'tcx> for LetUnderscore { }; if is_sync_lock { - cx.struct_span_lint(LET_UNDERSCORE_LOCK, local.span, |lint| { + let mut span = MultiSpan::from_spans(vec![local.pat.span, init.span]); + span.push_span_label( + local.pat.span, + "this lock is not assigned to a binding and is immediately dropped".to_string(), + ); + span.push_span_label( + init.span, + "this binding will immediately drop the value assigned to it".to_string(), + ); + cx.struct_span_lint(LET_UNDERSCORE_LOCK, span, |lint| { build_and_emit_lint( lint, local, diff --git a/src/test/ui/lint/let_underscore/let_underscore_lock.stderr b/src/test/ui/lint/let_underscore/let_underscore_lock.stderr index 7aa119003b4ba..fb58af0a42f81 100644 --- a/src/test/ui/lint/let_underscore/let_underscore_lock.stderr +++ b/src/test/ui/lint/let_underscore/let_underscore_lock.stderr @@ -1,8 +1,10 @@ error: non-binding let on a synchronization lock - --> $DIR/let_underscore_lock.rs:6:5 + --> $DIR/let_underscore_lock.rs:6:9 | LL | let _ = data.lock().unwrap(); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | ^ ^^^^^^^^^^^^^^^^^^^^ this binding will immediately drop the value assigned to it + | | + | this lock is not assigned to a binding and is immediately dropped | = note: `#[deny(let_underscore_lock)]` on by default help: consider binding to an unused variable to avoid immediately dropping the value From d355ec94ff0ef409bfa953f715987a98de13fd64 Mon Sep 17 00:00:00 2001 From: Aaron Kofsky Date: Thu, 4 Aug 2022 17:31:08 -0400 Subject: [PATCH 24/25] Fix imports. I'm not really sure why this is nessecary to do, but the checks on the PR do not seem to work if do not do this. --- compiler/rustc_lint/src/let_underscore.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/compiler/rustc_lint/src/let_underscore.rs b/compiler/rustc_lint/src/let_underscore.rs index 79d1443dc353c..7e885e6c51aad 100644 --- a/compiler/rustc_lint/src/let_underscore.rs +++ b/compiler/rustc_lint/src/let_underscore.rs @@ -1,7 +1,7 @@ use crate::{LateContext, LateLintPass, LintContext}; -use rustc_errors::{Applicability, MultiSpan}; +use rustc_errors::{Applicability, LintDiagnosticBuilder, MultiSpan}; use rustc_hir as hir; -use rustc_middle::{lint::LintDiagnosticBuilder, ty}; +use rustc_middle::ty; use rustc_span::Symbol; declare_lint! { From 76c90c3015a7e3ad6f0e6b807839ff59f17eba89 Mon Sep 17 00:00:00 2001 From: Aaron Kofsky Date: Fri, 5 Aug 2022 13:20:43 -0400 Subject: [PATCH 25/25] Use `#![warn(let_underscore_drop)]` in tests. --- src/test/ui/lint/let_underscore/let_underscore_drop.rs | 2 +- src/test/ui/lint/let_underscore/let_underscore_drop.stderr | 6 +++++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/src/test/ui/lint/let_underscore/let_underscore_drop.rs b/src/test/ui/lint/let_underscore/let_underscore_drop.rs index 85ca5dd2880a4..f298871f122d3 100644 --- a/src/test/ui/lint/let_underscore/let_underscore_drop.rs +++ b/src/test/ui/lint/let_underscore/let_underscore_drop.rs @@ -1,5 +1,5 @@ // check-pass -// compile-flags: -W let_underscore_drop +#![warn(let_underscore_drop)] struct NontrivialDrop; diff --git a/src/test/ui/lint/let_underscore/let_underscore_drop.stderr b/src/test/ui/lint/let_underscore/let_underscore_drop.stderr index cf7b882e946d9..7b7de202e4626 100644 --- a/src/test/ui/lint/let_underscore/let_underscore_drop.stderr +++ b/src/test/ui/lint/let_underscore/let_underscore_drop.stderr @@ -4,7 +4,11 @@ warning: non-binding let on a type that implements `Drop` LL | let _ = NontrivialDrop; | ^^^^^^^^^^^^^^^^^^^^^^^ | - = note: requested on the command line with `-W let-underscore-drop` +note: the lint level is defined here + --> $DIR/let_underscore_drop.rs:2:9 + | +LL | #![warn(let_underscore_drop)] + | ^^^^^^^^^^^^^^^^^^^ help: consider binding to an unused variable to avoid immediately dropping the value | LL | let _unused = NontrivialDrop;