Skip to content

Commit

Permalink
Rollup merge of #97739 - a2aaron:let_underscore, r=estebank
Browse files Browse the repository at this point in the history
Uplift the `let_underscore` lints from clippy into rustc.

This PR resolves #97241.

This PR adds three lints from clippy--`let_underscore_drop`, `let_underscore_lock`, and `let_underscore_must_use`, which are meant to capture likely-incorrect uses of `let _ = ...` bindings (in particular, doing this on a type with a non-trivial `Drop` causes the `Drop` to occur immediately, instead of at the end of the scope. For a type like `MutexGuard`, this effectively releases the lock immediately, which is almost certainly the wrong behavior)

In porting the lints from clippy I had to copy over a bunch of utility functions from `clippy_util` that these lints also relied upon. Is that the right approach?

Note that I've set the `must_use` and `drop` lints to Allow by default and set `lock` to Deny by default (this matches the same settings that clippy has). In talking with `@estebank` he informed me to do a Crater run (I am not sure what type of Crater run to request here--I think it's just "check only"?)

On the linked issue, there's some discussion about using `must_use` and `Drop` together as a heuristic for when to warn--I did not implement this yet.

r? `@estebank`
  • Loading branch information
GuillaumeGomez authored Sep 2, 2022
2 parents b10aed0 + 76c90c3 commit 07f43a1
Showing 10 changed files with 250 additions and 0 deletions.
175 changes: 175 additions & 0 deletions compiler/rustc_lint/src/let_underscore.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,175 @@
use crate::{LateContext, LateLintPass, LintContext};
use rustc_errors::{Applicability, LintDiagnosticBuilder, MultiSpan};
use rustc_hir as hir;
use rustc_middle::ty;
use rustc_span::Symbol;

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
/// ```
/// struct SomeStruct;
/// impl Drop for SomeStruct {
/// fn drop(&mut self) {
/// println!("Dropping SomeStruct");
/// }
/// }
///
/// 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
/// // a name (such as "_foo").
/// let _ = SomeStruct;
/// println!("end of main");
/// }
/// ```
///
/// {{produces}}
///
/// ### 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,
Allow,
"non-binding let on a type that implements `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
/// ```compile_fail
/// 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;
/// });
/// ```
///
/// {{produces}}
///
/// ### 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,
Deny,
"non-binding let on a synchronization lock"
}

declare_lint_pass!(LetUnderscore => [LET_UNDERSCORE_DROP, LET_UNDERSCORE_LOCK]);

const SYNC_GUARD_SYMBOLS: [Symbol; 3] = [
rustc_span::sym::MutexGuard,
rustc_span::sym::RwLockReadGuard,
rustc_span::sym::RwLockWriteGuard,
];

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);
// 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 = match init_ty.kind() {
ty::Adt(adt, _) => SYNC_GUARD_SYMBOLS
.iter()
.any(|guard_symbol| cx.tcx.is_diagnostic_item(*guard_symbol, adt.did())),
_ => false,
};

if is_sync_lock {
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,
init.span,
"non-binding let on a synchronization lock",
)
})
} else {
cx.struct_span_lint(LET_UNDERSCORE_DROP, local.span, |lint| {
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 to avoid immediately dropping the value",
"_unused",
Applicability::MachineApplicable,
)
.multipart_suggestion(
"consider immediately dropping the value",
vec![
(local.span.until(init_span), "drop(".to_string()),
(init_span.shrink_to_hi(), ")".to_string()),
],
Applicability::MachineApplicable,
)
.emit();
}
5 changes: 5 additions & 0 deletions compiler/rustc_lint/src/lib.rs
Original file line number Diff line number Diff line change
@@ -55,6 +55,7 @@ mod expect;
pub mod hidden_unicode_codepoints;
mod internal;
mod late;
mod let_underscore;
mod levels;
mod methods;
mod non_ascii_idents;
@@ -86,6 +87,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;
@@ -189,6 +191,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,
@@ -315,6 +318,8 @@ 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!(
"rust_2018_idioms",
BARE_TRAIT_OBJECTS,
3 changes: 3 additions & 0 deletions compiler/rustc_span/src/symbol.rs
Original file line number Diff line number Diff line change
@@ -223,6 +223,7 @@ symbols! {
LinkedList,
LintPass,
Mutex,
MutexGuard,
N,
NonZeroI128,
NonZeroI16,
@@ -271,6 +272,8 @@ symbols! {
Rust,
RustcDecodable,
RustcEncodable,
RwLockReadGuard,
RwLockWriteGuard,
Send,
SeqCst,
SessionDiagnostic,
1 change: 1 addition & 0 deletions library/std/src/sync/mutex.rs
Original file line number Diff line number Diff line change
@@ -192,6 +192,7 @@ unsafe impl<T: ?Sized + Send> Sync for Mutex<T> {}
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<T>,
poison: poison::Guard,
2 changes: 2 additions & 0 deletions library/std/src/sync/rwlock.rs
Original file line number Diff line number Diff line change
@@ -101,6 +101,7 @@ unsafe impl<T: ?Sized + Send + Sync> Sync for RwLock<T> {}
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> {
// NB: we use a pointer instead of `&'a T` to avoid `noalias` violations, because a
// `Ref` argument doesn't hold immutability for its whole scope, only until it drops.
@@ -130,6 +131,7 @@ unsafe impl<T: ?Sized + Sync> 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<T>,
poison: poison::Guard,
14 changes: 14 additions & 0 deletions src/test/ui/lint/let_underscore/let_underscore_drop.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
// check-pass
#![warn(let_underscore_drop)]

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`
}
22 changes: 22 additions & 0 deletions src/test/ui/lint/let_underscore/let_underscore_drop.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
warning: non-binding let on a type that implements `Drop`
--> $DIR/let_underscore_drop.rs:13:5
|
LL | let _ = NontrivialDrop;
| ^^^^^^^^^^^^^^^^^^^^^^^
|
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;
| ~~~~~~~
help: consider immediately dropping the value
|
LL | drop(NontrivialDrop);
| ~~~~~ +

warning: 1 warning emitted

7 changes: 7 additions & 0 deletions src/test/ui/lint/let_underscore/let_underscore_lock.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
// check-fail
use std::sync::{Arc, Mutex};

fn main() {
let data = Arc::new(Mutex::new(0));
let _ = data.lock().unwrap(); //~ERROR non-binding let on a synchronization lock
}
20 changes: 20 additions & 0 deletions src/test/ui/lint/let_underscore/let_underscore_lock.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
error: non-binding let on a synchronization lock
--> $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
|
LL | let _unused = data.lock().unwrap();
| ~~~~~~~
help: consider immediately dropping the value
|
LL | drop(data.lock().unwrap());
| ~~~~~ +

error: aborting due to previous error

1 change: 1 addition & 0 deletions src/tools/lint-docs/src/groups.rs
Original file line number Diff line number Diff line change
@@ -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"),

0 comments on commit 07f43a1

Please sign in to comment.