Skip to content
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

Uplift the let_underscore lints from clippy into rustc. #97739

Merged
merged 25 commits into from
Sep 2, 2022
Merged
Show file tree
Hide file tree
Changes from 18 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
821b32b
Add `let_underscore_drop` lint.
a2aaron May 29, 2022
ad7587f
Add `let_underscore_lock` lint.
a2aaron May 31, 2022
758a9fd
Add `let_underscore_must_use` lint.
a2aaron Jun 2, 2022
36b6309
Move let_underscore tests to their own subfolder.
a2aaron Jun 3, 2022
ae2ac3b
Allow `let_underscore_drop` and `let_underscore_must_use` by default.
a2aaron Jun 3, 2022
eba6c78
Show code suggestions in `let_undescore` lint messages.
a2aaron Jun 4, 2022
6b179e3
Set `let_underscore_lock` to Deny by default.
a2aaron Jun 4, 2022
a7e2b3e
Move local functions to outer scope.
a2aaron Jun 4, 2022
7e485bf
Move `let_underscore` tests into the `lint` subfolder.
a2aaron Jun 5, 2022
1421cff
Add `let_underscore` lint group to `GROUP_DESCRIPTIONS`.
a2aaron Jun 5, 2022
30e8adb
Use `has_attr` instead of `get_attrs` in `has_must_use_attr`
a2aaron Jun 5, 2022
e6b6678
Bail out early if the type does not has a trivial Drop implementation.
a2aaron Jun 5, 2022
6342b58
Use diagnostic items instead of hard coded paths for `let_underscore_…
a2aaron Jun 5, 2022
11663b1
Use `check-pass` instead of `run-pass`
a2aaron Jun 5, 2022
b5b5b54
Remove `let_underscore_must_use`
a2aaron Jun 5, 2022
321a598
Add diagnostic items to MutexGuard and RwLock Guards
a2aaron Jun 5, 2022
211feb1
Add `{{produces}}` tag to lint doc comments.
a2aaron Jun 5, 2022
cdf6606
Use `multipart_suggestion` to create an applicable suggestion.
a2aaron Jun 9, 2022
7237e86
Reword suggestion messages.
a2aaron Jun 11, 2022
b040666
Have the drop code suggestion not include `let _ =`
a2aaron Jun 11, 2022
8807c2d
Make `let_underscore_drop` Deny by default.
a2aaron Jun 11, 2022
a9095ff
Re-allow `let_underscore_drop` by default.
a2aaron Jun 17, 2022
a9f1b7b
Explain why let-underscoring a lock guard is incorrect.
a2aaron Aug 4, 2022
d355ec9
Fix imports.
a2aaron Aug 4, 2022
76c90c3
Use `#![warn(let_underscore_drop)]` in tests.
a2aaron Aug 5, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
166 changes: 166 additions & 0 deletions compiler/rustc_lint/src/let_underscore.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,166 @@
use crate::{LateContext, LateLintPass, LintContext};
use rustc_errors::Applicability;
use rustc_hir as hir;
use rustc_middle::{lint::LintDiagnosticBuilder, 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,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah! Can you push a new commit with this being Deny? That way we can do a crater run to see how bad this heuristic would actually be in the wild. Depending on the results of that run, we'd make this either Warn or Allow, as you have here.

@joshtriplett this is necessary to catch all cases of the RAII pattern, but we need to verify what the S/N ratio is for the simplistic heuristic. Otherwise we'd need to extend LET_UNDERSCORE_LOCK to also check for the mutexes in parkinglot and tokio, at least.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done! I will note that this ends up triggering on code in the compiler itself, which I'm guessing means it's going to be extremely noisy.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@estebank I would hypothesize that let _ = returns_type_implementing_drop() is quite common, too much so to turn on by default.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Case in point:

   Compiling rustc-demangle v0.1.21
error: non-binding let on a type that implements `Drop`
   --> library/alloc/src/vec/into_iter.rs:324:21
    |
324 |                     let _ = RawVec::from_raw_parts_in(self.0.buf.as_ptr(), self.0.cap, alloc);
    |
    |
    = note: `#[deny(let_underscore_drop)]` on by default
help: consider binding to an unused variable to avoid immediately dropping the value
    |
324 |                     let _unused = RawVec::from_raw_parts_in(self.0.buf.as_ptr(), self.0.cap, alloc);
help: consider immediately dropping the value
    |
    |
324 |                     drop(RawVec::from_raw_parts_in(self.0.buf.as_ptr(), self.0.cap, alloc));

error: could not compile `alloc` due to previous error
warning: build failed, waiting for other jobs to finish...
Build completed unsuccessfully in 0:04:32

:-/

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@a2aaron this is a problem for what I wanted to do. Did you see how many cases are in the rustc repo? Because if rustc doesn't build, then we can't run crater to collect numbers on how many crates would suddenly start warning on this. I guess path inspecting against a denylist of common crates (or better, stabilizing an attribute that they can annotate with that rustc checks for) might be warranted in the end :-/

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@estebank There's a lot. Setting let_underscore_drop to Warn and building with --warnings warn turned on, I got 28 warnings. Most of these were in std. It seems like 18 of them are caused by one macro (rtprintpanic!).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's actually not as many as I thought there would be! 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should I try manually fixing up all of those cases?

Copy link
Contributor

@estebank estebank Jun 16, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only if you have time, otherwise we can defer on that and focus on the specific denylist of std mutexes and leaving the "implicit drop" one as allow-by-default.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'll defer on it--the lint is too noisy and annoying to fix up right now, and every instance of it firing has been on types where it really doesn't matter that it gets dropped earlier (it's firing on a lot of Boxs or various Result types). I've pushed a commit making let_underscore_drop allow by default for now.

"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())),
Comment on lines +115 to +117
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we were ever to introduce 3rd party checks, it would be an extra arm here.

_ => false,
};

if is_sync_lock {
cx.struct_span_lint(LET_UNDERSCORE_LOCK, local.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",
a2aaron marked this conversation as resolved.
Show resolved Hide resolved
"_unused",
Applicability::MachineApplicable,
)
.multipart_suggestion(
"consider explicitly droping with `std::mem::drop`",
a2aaron marked this conversation as resolved.
Show resolved Hide resolved
vec![
(init_span.shrink_to_lo(), "drop(".to_string()),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the span is not empty, multipart_suggestion will suggest to replace the current text with the string, instead of just inserting it.
The span of the let _ = starts at the beginning of the statement, and ends at init_span.

(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
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -314,6 +317,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,
Expand Down
3 changes: 3 additions & 0 deletions compiler/rustc_span/src/symbol.rs
Original file line number Diff line number Diff line change
Expand Up @@ -213,6 +213,7 @@ symbols! {
LinkedList,
LintPass,
Mutex,
MutexGuard,
N,
None,
Ok,
Expand Down Expand Up @@ -250,6 +251,8 @@ symbols! {
Right,
RustcDecodable,
RustcEncodable,
RwLockReadGuard,
RwLockWriteGuard,
Send,
SeqCst,
SliceIndex,
Expand Down
1 change: 1 addition & 0 deletions library/std/src/sync/mutex.rs
Original file line number Diff line number Diff line change
Expand Up @@ -193,6 +193,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,
Expand Down
2 changes: 2 additions & 0 deletions library/std/src/sync/rwlock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,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> {
lock: &'a RwLock<T>,
}
Expand All @@ -124,6 +125,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,
Expand Down
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
// compile-flags: -W let_underscore_drop
a2aaron marked this conversation as resolved.
Show resolved Hide resolved

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`
}
18 changes: 18 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,18 @@
warning: non-binding let on a type that implements `Drop`
--> $DIR/let_underscore_drop.rs:13:5
|
LL | let _ = NontrivialDrop;
| ^^^^^^^^^^^^^^^^^^^^^^^
|
= note: requested on the command line with `-W let-underscore-drop`
help: consider binding to an unused variable
|
LL | let _unused = NontrivialDrop;
| ~~~~~~~
help: consider explicitly droping with `std::mem::drop`
|
LL | let _ = drop(NontrivialDrop);
| +++++ +
a2aaron marked this conversation as resolved.
Show resolved Hide resolved

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
}
18 changes: 18 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,18 @@
error: non-binding let on a synchronization lock
--> $DIR/let_underscore_lock.rs:6:5
|
LL | let _ = data.lock().unwrap();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= note: `#[deny(let_underscore_lock)]` on by default
help: consider binding to an unused variable
|
LL | let _unused = data.lock().unwrap();
| ~~~~~~~
help: consider explicitly droping with `std::mem::drop`
|
LL | let _ = 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
Expand Up @@ -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"),
Expand Down