Skip to content

Commit

Permalink
Auto merge of #6305 - smoelius:master, r=flip1995
Browse files Browse the repository at this point in the history
Add `let_underscore_drop`

This line generalizes `let_underscore_lock` (#5101) to warn about any initializer expression that implements `Drop`.

So, for example, the following would generate a warning:
```rust
struct Droppable;
impl Drop for Droppable {
    fn drop(&mut self) {}
}
let _ = Droppable;
```

I tried to preserve the original `let_underscore_lock` functionality in the sense that the warning generated for
```rust
let _ = mutex.lock();
```
should be unchanged.

*Please keep the line below*
changelog: Add lint [`let_underscore_drop`]
  • Loading branch information
bors committed Nov 9, 2020
2 parents d212c38 + 4852cca commit dd826b4
Show file tree
Hide file tree
Showing 14 changed files with 139 additions and 18 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -1787,6 +1787,7 @@ Released 2018-09-13
[`len_without_is_empty`]: https://rust-lang.github.io/rust-clippy/master/index.html#len_without_is_empty
[`len_zero`]: https://rust-lang.github.io/rust-clippy/master/index.html#len_zero
[`let_and_return`]: https://rust-lang.github.io/rust-clippy/master/index.html#let_and_return
[`let_underscore_drop`]: https://rust-lang.github.io/rust-clippy/master/index.html#let_underscore_drop
[`let_underscore_lock`]: https://rust-lang.github.io/rust-clippy/master/index.html#let_underscore_lock
[`let_underscore_must_use`]: https://rust-lang.github.io/rust-clippy/master/index.html#let_underscore_must_use
[`let_unit_value`]: https://rust-lang.github.io/rust-clippy/master/index.html#let_unit_value
Expand Down
64 changes: 62 additions & 2 deletions clippy_lints/src/let_underscore.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ use rustc_middle::lint::in_external_macro;
use rustc_middle::ty::subst::GenericArgKind;
use rustc_session::{declare_lint_pass, declare_tool_lint};

use crate::utils::{is_must_use_func_call, is_must_use_ty, match_type, paths, span_lint_and_help};
use crate::utils::{implements_trait, is_must_use_func_call, is_must_use_ty, match_type, paths, span_lint_and_help};

declare_clippy_lint! {
/// **What it does:** Checks for `let _ = <expr>`
Expand Down Expand Up @@ -58,7 +58,48 @@ declare_clippy_lint! {
"non-binding let on a synchronization lock"
}

declare_lint_pass!(LetUnderscore => [LET_UNDERSCORE_MUST_USE, LET_UNDERSCORE_LOCK]);
declare_clippy_lint! {
/// **What it does:** Checks for `let _ = <expr>`
/// where expr has a type that implements `Drop`
///
/// **Why is this bad?** This statement immediately drops the initializer
/// expression instead of extending its lifetime to the end of the scope, which
/// is often not intended. To extend the expression's lifetime to the end of the
/// scope, use an underscore-prefixed name instead (i.e. _var). If you want to
/// explicitly drop the expression, `std::mem::drop` conveys your intention
/// better and is less error-prone.
///
/// **Known problems:** None.
///
/// **Example:**
///
/// Bad:
/// ```rust,ignore
/// struct Droppable;
/// impl Drop for Droppable {
/// fn drop(&mut self) {}
/// }
/// {
/// let _ = Droppable;
/// // ^ dropped here
/// /* more code */
/// }
/// ```
///
/// Good:
/// ```rust,ignore
/// {
/// let _droppable = Droppable;
/// /* more code */
/// // dropped at end of scope
/// }
/// ```
pub LET_UNDERSCORE_DROP,
pedantic,
"non-binding let on a type that implements `Drop`"
}

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

const SYNC_GUARD_PATHS: [&[&str]; 3] = [
&paths::MUTEX_GUARD,
Expand All @@ -84,6 +125,15 @@ impl<'tcx> LateLintPass<'tcx> for LetUnderscore {

GenericArgKind::Lifetime(_) | GenericArgKind::Const(_) => false,
});
let implements_drop = cx.tcx.lang_items().drop_trait().map_or(false, |drop_trait|
init_ty.walk().any(|inner| match inner.unpack() {
GenericArgKind::Type(inner_ty) => {
implements_trait(cx, inner_ty, drop_trait, &[])
},

GenericArgKind::Lifetime(_) | GenericArgKind::Const(_) => false,
})
);
if contains_sync_guard {
span_lint_and_help(
cx,
Expand All @@ -94,6 +144,16 @@ impl<'tcx> LateLintPass<'tcx> for LetUnderscore {
"consider using an underscore-prefixed named \
binding or dropping explicitly with `std::mem::drop`"
)
} else if implements_drop {
span_lint_and_help(
cx,
LET_UNDERSCORE_DROP,
local.span,
"non-binding `let` on a type that implements `Drop`",
None,
"consider using an underscore-prefixed named \
binding or dropping explicitly with `std::mem::drop`"
)
} else if is_must_use_ty(cx, cx.typeck_results().expr_ty(init)) {
span_lint_and_help(
cx,
Expand Down
2 changes: 2 additions & 0 deletions clippy_lints/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -622,6 +622,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
&len_zero::LEN_WITHOUT_IS_EMPTY,
&len_zero::LEN_ZERO,
&let_if_seq::USELESS_LET_IF_SEQ,
&let_underscore::LET_UNDERSCORE_DROP,
&let_underscore::LET_UNDERSCORE_LOCK,
&let_underscore::LET_UNDERSCORE_MUST_USE,
&lifetimes::EXTRA_UNUSED_LIFETIMES,
Expand Down Expand Up @@ -1240,6 +1241,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
LintId::of(&infinite_iter::MAYBE_INFINITE_ITER),
LintId::of(&items_after_statements::ITEMS_AFTER_STATEMENTS),
LintId::of(&large_stack_arrays::LARGE_STACK_ARRAYS),
LintId::of(&let_underscore::LET_UNDERSCORE_DROP),
LintId::of(&literal_representation::LARGE_DIGIT_GROUPS),
LintId::of(&literal_representation::UNREADABLE_LITERAL),
LintId::of(&loops::EXPLICIT_INTO_ITER_LOOP),
Expand Down
7 changes: 7 additions & 0 deletions src/lintlist/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1117,6 +1117,13 @@ vec![
deprecation: None,
module: "returns",
},
Lint {
name: "let_underscore_drop",
group: "pedantic",
desc: "non-binding let on a type that implements `Drop`",
deprecation: None,
module: "let_underscore",
},
Lint {
name: "let_underscore_lock",
group: "correctness",
Expand Down
1 change: 1 addition & 0 deletions tests/ui/filter_methods.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
#![warn(clippy::all, clippy::pedantic)]
#![allow(clippy::clippy::let_underscore_drop)]
#![allow(clippy::missing_docs_in_private_items)]

fn main() {
Expand Down
8 changes: 4 additions & 4 deletions tests/ui/filter_methods.stderr
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
error: called `filter(..).map(..)` on an `Iterator`
--> $DIR/filter_methods.rs:5:21
--> $DIR/filter_methods.rs:6:21
|
LL | let _: Vec<_> = vec![5; 6].into_iter().filter(|&x| x == 0).map(|x| x * 2).collect();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Expand All @@ -8,7 +8,7 @@ LL | let _: Vec<_> = vec![5; 6].into_iter().filter(|&x| x == 0).map(|x| x *
= help: this is more succinctly expressed by calling `.filter_map(..)` instead

error: called `filter(..).flat_map(..)` on an `Iterator`
--> $DIR/filter_methods.rs:7:21
--> $DIR/filter_methods.rs:8:21
|
LL | let _: Vec<_> = vec![5_i8; 6]
| _____________________^
Expand All @@ -20,7 +20,7 @@ LL | | .flat_map(|x| x.checked_mul(2))
= help: this is more succinctly expressed by calling `.flat_map(..)` and filtering by returning `iter::empty()`

error: called `filter_map(..).flat_map(..)` on an `Iterator`
--> $DIR/filter_methods.rs:13:21
--> $DIR/filter_methods.rs:14:21
|
LL | let _: Vec<_> = vec![5_i8; 6]
| _____________________^
Expand All @@ -32,7 +32,7 @@ LL | | .flat_map(|x| x.checked_mul(2))
= help: this is more succinctly expressed by calling `.flat_map(..)` and filtering by returning `iter::empty()`

error: called `filter_map(..).map(..)` on an `Iterator`
--> $DIR/filter_methods.rs:19:21
--> $DIR/filter_methods.rs:20:21
|
LL | let _: Vec<_> = vec![5_i8; 6]
| _____________________^
Expand Down
19 changes: 19 additions & 0 deletions tests/ui/let_underscore_drop.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
#![warn(clippy::let_underscore_drop)]

struct Droppable;

impl Drop for Droppable {
fn drop(&mut self) {}
}

fn main() {
let unit = ();
let boxed = Box::new(());
let droppable = Droppable;
let optional = Some(Droppable);

let _ = ();
let _ = Box::new(());
let _ = Droppable;
let _ = Some(Droppable);
}
27 changes: 27 additions & 0 deletions tests/ui/let_underscore_drop.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
error: non-binding `let` on a type that implements `Drop`
--> $DIR/let_underscore_drop.rs:16:5
|
LL | let _ = Box::new(());
| ^^^^^^^^^^^^^^^^^^^^^
|
= note: `-D clippy::let-underscore-drop` implied by `-D warnings`
= help: consider using an underscore-prefixed named binding or dropping explicitly with `std::mem::drop`

error: non-binding `let` on a type that implements `Drop`
--> $DIR/let_underscore_drop.rs:17:5
|
LL | let _ = Droppable;
| ^^^^^^^^^^^^^^^^^^
|
= help: consider using an underscore-prefixed named binding or dropping explicitly with `std::mem::drop`

error: non-binding `let` on a type that implements `Drop`
--> $DIR/let_underscore_drop.rs:18:5
|
LL | let _ = Some(Droppable);
| ^^^^^^^^^^^^^^^^^^^^^^^^
|
= help: consider using an underscore-prefixed named binding or dropping explicitly with `std::mem::drop`

error: aborting due to 3 previous errors

1 change: 1 addition & 0 deletions tests/ui/map_clone.fixed
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
#![warn(clippy::all, clippy::pedantic)]
#![allow(clippy::iter_cloned_collect)]
#![allow(clippy::clone_on_copy, clippy::redundant_clone)]
#![allow(clippy::let_underscore_drop)]
#![allow(clippy::missing_docs_in_private_items)]
#![allow(clippy::redundant_closure_for_method_calls)]
#![allow(clippy::many_single_char_names)]
Expand Down
1 change: 1 addition & 0 deletions tests/ui/map_clone.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
#![warn(clippy::all, clippy::pedantic)]
#![allow(clippy::iter_cloned_collect)]
#![allow(clippy::clone_on_copy, clippy::redundant_clone)]
#![allow(clippy::let_underscore_drop)]
#![allow(clippy::missing_docs_in_private_items)]
#![allow(clippy::redundant_closure_for_method_calls)]
#![allow(clippy::many_single_char_names)]
Expand Down
12 changes: 6 additions & 6 deletions tests/ui/map_clone.stderr
Original file line number Diff line number Diff line change
@@ -1,37 +1,37 @@
error: you are using an explicit closure for copying elements
--> $DIR/map_clone.rs:10:22
--> $DIR/map_clone.rs:11:22
|
LL | let _: Vec<i8> = vec![5_i8; 6].iter().map(|x| *x).collect();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider calling the dedicated `copied` method: `vec![5_i8; 6].iter().copied()`
|
= note: `-D clippy::map-clone` implied by `-D warnings`

error: you are using an explicit closure for cloning elements
--> $DIR/map_clone.rs:11:26
--> $DIR/map_clone.rs:12:26
|
LL | let _: Vec<String> = vec![String::new()].iter().map(|x| x.clone()).collect();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider calling the dedicated `cloned` method: `vec![String::new()].iter().cloned()`

error: you are using an explicit closure for copying elements
--> $DIR/map_clone.rs:12:23
--> $DIR/map_clone.rs:13:23
|
LL | let _: Vec<u32> = vec![42, 43].iter().map(|&x| x).collect();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider calling the dedicated `copied` method: `vec![42, 43].iter().copied()`

error: you are using an explicit closure for copying elements
--> $DIR/map_clone.rs:14:26
--> $DIR/map_clone.rs:15:26
|
LL | let _: Option<u64> = Some(&16).map(|b| *b);
| ^^^^^^^^^^^^^^^^^^^^^ help: consider calling the dedicated `copied` method: `Some(&16).copied()`

error: you are using an explicit closure for copying elements
--> $DIR/map_clone.rs:15:25
--> $DIR/map_clone.rs:16:25
|
LL | let _: Option<u8> = Some(&1).map(|x| x.clone());
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider calling the dedicated `copied` method: `Some(&1).copied()`

error: you are needlessly cloning iterator elements
--> $DIR/map_clone.rs:26:29
--> $DIR/map_clone.rs:27:29
|
LL | let _ = std::env::args().map(|v| v.clone());
| ^^^^^^^^^^^^^^^^^^^ help: remove the `map` call
Expand Down
1 change: 1 addition & 0 deletions tests/ui/map_flatten.fixed
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
// run-rustfix

#![warn(clippy::all, clippy::pedantic)]
#![allow(clippy::let_underscore_drop)]
#![allow(clippy::missing_docs_in_private_items)]
#![allow(clippy::map_identity)]

Expand Down
1 change: 1 addition & 0 deletions tests/ui/map_flatten.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
// run-rustfix

#![warn(clippy::all, clippy::pedantic)]
#![allow(clippy::let_underscore_drop)]
#![allow(clippy::missing_docs_in_private_items)]
#![allow(clippy::map_identity)]

Expand Down
12 changes: 6 additions & 6 deletions tests/ui/map_flatten.stderr
Original file line number Diff line number Diff line change
@@ -1,37 +1,37 @@
error: called `map(..).flatten()` on an `Iterator`
--> $DIR/map_flatten.rs:14:46
--> $DIR/map_flatten.rs:15:46
|
LL | let _: Vec<_> = vec![5_i8; 6].into_iter().map(option_id).flatten().collect();
| ^^^^^^^^^^^^^^^^^^^^^^^^^ help: try using `filter_map` instead: `.filter_map(option_id)`
|
= note: `-D clippy::map-flatten` implied by `-D warnings`

error: called `map(..).flatten()` on an `Iterator`
--> $DIR/map_flatten.rs:15:46
--> $DIR/map_flatten.rs:16:46
|
LL | let _: Vec<_> = vec![5_i8; 6].into_iter().map(option_id_ref).flatten().collect();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try using `filter_map` instead: `.filter_map(option_id_ref)`

error: called `map(..).flatten()` on an `Iterator`
--> $DIR/map_flatten.rs:16:46
--> $DIR/map_flatten.rs:17:46
|
LL | let _: Vec<_> = vec![5_i8; 6].into_iter().map(option_id_closure).flatten().collect();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try using `filter_map` instead: `.filter_map(option_id_closure)`

error: called `map(..).flatten()` on an `Iterator`
--> $DIR/map_flatten.rs:17:46
--> $DIR/map_flatten.rs:18:46
|
LL | let _: Vec<_> = vec![5_i8; 6].into_iter().map(|x| x.checked_add(1)).flatten().collect();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try using `filter_map` instead: `.filter_map(|x| x.checked_add(1))`

error: called `map(..).flatten()` on an `Iterator`
--> $DIR/map_flatten.rs:20:46
--> $DIR/map_flatten.rs:21:46
|
LL | let _: Vec<_> = vec![5_i8; 6].into_iter().map(|x| 0..x).flatten().collect();
| ^^^^^^^^^^^^^^^^^^^^^^^^ help: try using `flat_map` instead: `.flat_map(|x| 0..x)`

error: called `map(..).flatten()` on an `Option`
--> $DIR/map_flatten.rs:23:39
--> $DIR/map_flatten.rs:24:39
|
LL | let _: Option<_> = (Some(Some(1))).map(|x| x).flatten();
| ^^^^^^^^^^^^^^^^^^^^^ help: try using `and_then` instead: `.and_then(|x| x)`
Expand Down

0 comments on commit dd826b4

Please sign in to comment.