Lint on underscore variable assignment

Fix tests after no_effect update

Add a drop testcase

Don't lint _ variables in macro expansion

Address review comments and update tests

Don't shadow unnecessary operation lint if no_effect is allowed

Revert shadowing change and remove no_effect allows

Update clippy_lints/src/no_effect.rs

Co-authored-by: Takayuki Nakata <f.seasons017@gmail.com>

Update clippy_lints/src/no_effect.rs

Co-authored-by: Takayuki Nakata <f.seasons017@gmail.com>

Address review comments

11 files changed, 170 insertions, 86 deletions

diff --git a/CHANGELOG.md b/CHANGELOG.md
index b1e837d363a..b7a7a421532 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -2899,6 +2899,7 @@ Released 2018-09-13
 [`new_ret_no_self`]: https://rust-lang.github.io/rust-clippy/master/index.html#new_ret_no_self
 [`new_without_default`]: https://rust-lang.github.io/rust-clippy/master/index.html#new_without_default
 [`no_effect`]: https://rust-lang.github.io/rust-clippy/master/index.html#no_effect
+[`no_effect_underscore_binding`]: https://rust-lang.github.io/rust-clippy/master/index.html#no_effect_underscore_binding
 [`non_ascii_literal`]: https://rust-lang.github.io/rust-clippy/master/index.html#non_ascii_literal
 [`non_octal_unix_permissions`]: https://rust-lang.github.io/rust-clippy/master/index.html#non_octal_unix_permissions
 [`non_send_fields_in_send_ty`]: https://rust-lang.github.io/rust-clippy/master/index.html#non_send_fields_in_send_ty
diff --git a/clippy_lints/src/lib.register_lints.rs b/clippy_lints/src/lib.register_lints.rs
index 7d81fafe4c9..e5fe22ad394 100644
--- a/clippy_lints/src/lib.register_lints.rs
+++ b/clippy_lints/src/lib.register_lints.rs
@@ -362,6 +362,7 @@ store.register_lints(&[
     neg_multiply::NEG_MULTIPLY,
     new_without_default::NEW_WITHOUT_DEFAULT,
     no_effect::NO_EFFECT,
+    no_effect::NO_EFFECT_UNDERSCORE_BINDING,
     no_effect::UNNECESSARY_OPERATION,
     non_copy_const::BORROW_INTERIOR_MUTABLE_CONST,
     non_copy_const::DECLARE_INTERIOR_MUTABLE_CONST,
diff --git a/clippy_lints/src/lib.register_pedantic.rs b/clippy_lints/src/lib.register_pedantic.rs
index 6533b94e82b..268349d2848 100644
--- a/clippy_lints/src/lib.register_pedantic.rs
+++ b/clippy_lints/src/lib.register_pedantic.rs
@@ -72,6 +72,7 @@ store.register_group(true, "clippy::pedantic", Some("clippy_pedantic"), vec![
     LintId::of(needless_continue::NEEDLESS_CONTINUE),
     LintId::of(needless_for_each::NEEDLESS_FOR_EACH),
     LintId::of(needless_pass_by_value::NEEDLESS_PASS_BY_VALUE),
+    LintId::of(no_effect::NO_EFFECT_UNDERSCORE_BINDING),
     LintId::of(non_expressive_names::MANY_SINGLE_CHAR_NAMES),
     LintId::of(non_expressive_names::SIMILAR_NAMES),
     LintId::of(pass_by_ref_or_value::LARGE_TYPES_PASSED_BY_VALUE),
diff --git a/clippy_lints/src/no_effect.rs b/clippy_lints/src/no_effect.rs
index c5a5cde4b11..6dae8f32043 100644
--- a/clippy_lints/src/no_effect.rs
+++ b/clippy_lints/src/no_effect.rs
@@ -1,9 +1,10 @@
 use clippy_utils::diagnostics::{span_lint_hir, span_lint_hir_and_then};
+use clippy_utils::is_lint_allowed;
 use clippy_utils::source::snippet_opt;
 use clippy_utils::ty::has_drop;
 use rustc_errors::Applicability;
 use rustc_hir::def::{DefKind, Res};
-use rustc_hir::{is_range_literal, BinOpKind, BlockCheckMode, Expr, ExprKind, Stmt, StmtKind, UnsafeSource};
+use rustc_hir::{is_range_literal, BinOpKind, BlockCheckMode, Expr, ExprKind, PatKind, Stmt, StmtKind, UnsafeSource};
 use rustc_lint::{LateContext, LateLintPass};
 use rustc_session::{declare_lint_pass, declare_tool_lint};
 use std::ops::Deref;
@@ -13,7 +14,7 @@ declare_clippy_lint! {
     /// Checks for statements which have no effect.
     ///
     /// ### Why is this bad?
-    /// Similar to dead code, these statements are actually
+    /// Unlike dead code, these statements are actually
     /// executed. However, as they have no effect, all they do is make the code less
     /// readable.
     ///
@@ -28,6 +29,28 @@ declare_clippy_lint! {
 
 declare_clippy_lint! {
     /// ### What it does
+    /// Checks for binding to underscore prefixed variable without side-effects.
+    ///
+    /// ### Why is this bad?
+    /// Unlike dead code, these bindings are actually
+    /// executed. However, as they have no effect and shouldn't be used further on, all they
+    /// do is make the code less readable.
+    ///
+    /// ### Known problems
+    /// Further usage of this variable is not checked, which can lead to false positives if it is
+    /// used later in the code.
+    ///
+    /// ### Example
+    /// ```rust,ignore
+    /// let _i_serve_no_purpose = 1;
+    /// ```
+    pub NO_EFFECT_UNDERSCORE_BINDING,
+    pedantic,
+    "binding to `_` prefixed variable with no side-effect"
+}
+
+declare_clippy_lint! {
+    /// ### What it does
     /// Checks for expression statements that can be reduced to a
     /// sub-expression.
     ///
@@ -44,6 +67,46 @@ declare_clippy_lint! {
     "outer expressions with no effect"
 }
 
+declare_lint_pass!(NoEffect => [NO_EFFECT, UNNECESSARY_OPERATION, NO_EFFECT_UNDERSCORE_BINDING]);
+
+impl<'tcx> LateLintPass<'tcx> for NoEffect {
+    fn check_stmt(&mut self, cx: &LateContext<'tcx>, stmt: &'tcx Stmt<'_>) {
+        if check_no_effect(cx, stmt) {
+            return;
+        }
+        check_unnecessary_operation(cx, stmt);
+    }
+}
+
+fn check_no_effect(cx: &LateContext<'tcx>, stmt: &'tcx Stmt<'_>) -> bool {
+    if let StmtKind::Semi(expr) = stmt.kind {
+        if has_no_effect(cx, expr) {
+            span_lint_hir(cx, NO_EFFECT, expr.hir_id, stmt.span, "statement with no effect");
+            return true;
+        }
+    } else if let StmtKind::Local(local) = stmt.kind {
+        if_chain! {
+            if !is_lint_allowed(cx, NO_EFFECT_UNDERSCORE_BINDING, local.hir_id);
+            if let Some(init) = local.init;
+            if !local.pat.span.from_expansion();
+            if has_no_effect(cx, init);
+            if let PatKind::Binding(_, _, ident, _) = local.pat.kind;
+            if ident.name.to_ident_string().starts_with('_');
+            then {
+                span_lint_hir(
+                    cx,
+                    NO_EFFECT_UNDERSCORE_BINDING,
+                    init.hir_id,
+                    stmt.span,
+                    "binding to `_` prefixed variable with no side-effect"
+                );
+                return true;
+            }
+        }
+    }
+    false
+}
+
 fn has_no_effect(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool {
     if expr.span.from_expansion() {
         return false;
@@ -88,71 +151,59 @@ fn has_no_effect(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool {
     }
 }
 
-declare_lint_pass!(NoEffect => [NO_EFFECT, UNNECESSARY_OPERATION]);
-
-impl<'tcx> LateLintPass<'tcx> for NoEffect {
-    fn check_stmt(&mut self, cx: &LateContext<'tcx>, stmt: &'tcx Stmt<'_>) {
-        if let StmtKind::Semi(expr) = stmt.kind {
-            if has_no_effect(cx, expr) {
-                span_lint_hir(cx, NO_EFFECT, expr.hir_id, stmt.span, "statement with no effect");
-            } else if let Some(reduced) = reduce_expression(cx, expr) {
-                for e in &reduced {
-                    if e.span.from_expansion() {
-                        return;
-                    }
-                }
-                if let ExprKind::Index(..) = &expr.kind {
-                    let snippet;
-                    if_chain! {
-                        if let Some(arr) = snippet_opt(cx, reduced[0].span);
-                        if let Some(func) = snippet_opt(cx, reduced[1].span);
-                        then {
-                            snippet = format!("assert!({}.len() > {});", &arr, &func);
-                        } else {
-                            return;
-                        }
-                    }
-                    span_lint_hir_and_then(
-                        cx,
-                        UNNECESSARY_OPERATION,
-                        expr.hir_id,
-                        stmt.span,
-                        "unnecessary operation",
-                        |diag| {
-                            diag.span_suggestion(
-                                stmt.span,
-                                "statement can be written as",
-                                snippet,
-                                Applicability::MaybeIncorrect,
-                            );
-                        },
-                    );
+fn check_unnecessary_operation(cx: &LateContext<'tcx>, stmt: &'tcx Stmt<'_>) {
+    if_chain! {
+        if let StmtKind::Semi(expr) = stmt.kind;
+        if let Some(reduced) = reduce_expression(cx, expr);
+        if !&reduced.iter().any(|e| e.span.from_expansion());
+        then {
+            if let ExprKind::Index(..) = &expr.kind {
+                let snippet;
+                if let (Some(arr), Some(func)) = (snippet_opt(cx, reduced[0].span), snippet_opt(cx, reduced[1].span)) {
+                    snippet = format!("assert!({}.len() > {});", &arr, &func);
                 } else {
-                    let mut snippet = String::new();
-                    for e in reduced {
-                        if let Some(snip) = snippet_opt(cx, e.span) {
-                            snippet.push_str(&snip);
-                            snippet.push(';');
-                        } else {
-                            return;
-                        }
+                    return;
+                }
+                span_lint_hir_and_then(
+                    cx,
+                    UNNECESSARY_OPERATION,
+                    expr.hir_id,
+                    stmt.span,
+                    "unnecessary operation",
+                    |diag| {
+                        diag.span_suggestion(
+                            stmt.span,
+                            "statement can be written as",
+                            snippet,
+                            Applicability::MaybeIncorrect,
+                        );
+                    },
+                );
+            } else {
+                let mut snippet = String::new();
+                for e in reduced {
+                    if let Some(snip) = snippet_opt(cx, e.span) {
+                        snippet.push_str(&snip);
+                        snippet.push(';');
+                    } else {
+                        return;
                     }
-                    span_lint_hir_and_then(
-                        cx,
-                        UNNECESSARY_OPERATION,
-                        expr.hir_id,
-                        stmt.span,
-                        "unnecessary operation",
-                        |diag| {
-                            diag.span_suggestion(
-                                stmt.span,
-                                "statement can be reduced to",
-                                snippet,
-                                Applicability::MachineApplicable,
-                            );
-                        },
-                    );
                 }
+                span_lint_hir_and_then(
+                    cx,
+                    UNNECESSARY_OPERATION,
+                    expr.hir_id,
+                    stmt.span,
+                    "unnecessary operation",
+                    |diag| {
+                        diag.span_suggestion(
+                            stmt.span,
+                            "statement can be reduced to",
+                            snippet,
+                            Applicability::MachineApplicable,
+                        );
+                    },
+                );
             }
         }
     }
diff --git a/tests/ui/cfg_attr_rustfmt.fixed b/tests/ui/cfg_attr_rustfmt.fixed
index 4e583a25b94..061a4ab9b2e 100644
--- a/tests/ui/cfg_attr_rustfmt.fixed
+++ b/tests/ui/cfg_attr_rustfmt.fixed
@@ -1,7 +1,7 @@
 // run-rustfix
 #![feature(stmt_expr_attributes)]
 
-#![allow(unused, clippy::no_effect)]
+#![allow(unused, clippy::no_effect, clippy::unnecessary_operation)]
 #![warn(clippy::deprecated_cfg_attr)]
 
 // This doesn't get linted, see known problems
diff --git a/tests/ui/cfg_attr_rustfmt.rs b/tests/ui/cfg_attr_rustfmt.rs
index 9c0fcf6fb45..035169fab85 100644
--- a/tests/ui/cfg_attr_rustfmt.rs
+++ b/tests/ui/cfg_attr_rustfmt.rs
@@ -1,7 +1,7 @@
 // run-rustfix
 #![feature(stmt_expr_attributes)]
 
-#![allow(unused, clippy::no_effect)]
+#![allow(unused, clippy::no_effect, clippy::unnecessary_operation)]
 #![warn(clippy::deprecated_cfg_attr)]
 
 // This doesn't get linted, see known problems
diff --git a/tests/ui/no_effect.rs b/tests/ui/no_effect.rs
index 7ec845adfaa..7bcc4cad0d3 100644
--- a/tests/ui/no_effect.rs
+++ b/tests/ui/no_effect.rs
@@ -1,5 +1,5 @@
 #![feature(box_syntax)]
-#![warn(clippy::no_effect)]
+#![warn(clippy::no_effect_underscore_binding)]
 #![allow(dead_code)]
 #![allow(path_statements)]
 #![allow(clippy::deref_addrof)]
@@ -90,6 +90,10 @@ fn main() {
     || x += 5;
     let s: String = "foo".into();
     FooString { s: s };
+    let _unused = 1;
+    let _penguin = || println!("Some helpful closure");
+    let _duck = Struct { field: 0 };
+    let _cat = [2, 4, 6, 8][2];
 
     #[allow(clippy::no_effect)]
     0;
@@ -97,6 +101,8 @@ fn main() {
     // Do not warn
     get_number();
     unsafe { unsafe_fn() };
+    let _used = get_struct();
+    let _x = vec![1];
     DropUnit;
     DropStruct { field: 0 };
     DropTuple(0);
diff --git a/tests/ui/no_effect.stderr b/tests/ui/no_effect.stderr
index 6b24675ac2d..a5dbc9fef45 100644
--- a/tests/ui/no_effect.stderr
+++ b/tests/ui/no_effect.stderr
@@ -156,5 +156,31 @@ error: statement with no effect
 LL |     FooString { s: s };
    |     ^^^^^^^^^^^^^^^^^^^
 
-error: aborting due to 26 previous errors
+error: binding to `_` prefixed variable with no side-effect
+  --> $DIR/no_effect.rs:93:5
+   |
+LL |     let _unused = 1;
+   |     ^^^^^^^^^^^^^^^^
+   |
+   = note: `-D clippy::no-effect-underscore-binding` implied by `-D warnings`
+
+error: binding to `_` prefixed variable with no side-effect
+  --> $DIR/no_effect.rs:94:5
+   |
+LL |     let _penguin = || println!("Some helpful closure");
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+error: binding to `_` prefixed variable with no side-effect
+  --> $DIR/no_effect.rs:95:5
+   |
+LL |     let _duck = Struct { field: 0 };
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+error: binding to `_` prefixed variable with no side-effect
+  --> $DIR/no_effect.rs:96:5
+   |
+LL |     let _cat = [2, 4, 6, 8][2];
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+error: aborting due to 30 previous errors
 
diff --git a/tests/ui/option_if_let_else.fixed b/tests/ui/option_if_let_else.fixed
index a3ebe5d0703..4077f1920a3 100644
--- a/tests/ui/option_if_let_else.fixed
+++ b/tests/ui/option_if_let_else.fixed
@@ -1,8 +1,7 @@
 // edition:2018
 // run-rustfix
 #![warn(clippy::option_if_let_else)]
-#![allow(clippy::redundant_closure)]
-#![allow(clippy::ref_option_ref, clippy::equatable_if_let)]
+#![allow(clippy::redundant_closure, clippy::ref_option_ref, clippy::equatable_if_let)]
 
 fn bad1(string: Option<&str>) -> (bool, &str) {
     string.map_or((false, "hello"), |x| (true, x))
diff --git a/tests/ui/option_if_let_else.rs b/tests/ui/option_if_let_else.rs
index b11df3db60f..2f414e129d5 100644
--- a/tests/ui/option_if_let_else.rs
+++ b/tests/ui/option_if_let_else.rs
@@ -1,8 +1,7 @@
 // edition:2018
 // run-rustfix
 #![warn(clippy::option_if_let_else)]
-#![allow(clippy::redundant_closure)]
-#![allow(clippy::ref_option_ref, clippy::equatable_if_let)]
+#![allow(clippy::redundant_closure, clippy::ref_option_ref, clippy::equatable_if_let)]
 
 fn bad1(string: Option<&str>) -> (bool, &str) {
     if let Some(x) = string {
diff --git a/tests/ui/option_if_let_else.stderr b/tests/ui/option_if_let_else.stderr
index ed748ee8b39..803d941c36d 100644
--- a/tests/ui/option_if_let_else.stderr
+++ b/tests/ui/option_if_let_else.stderr
@@ -1,5 +1,5 @@
 error: use Option::map_or instead of an if let/else
-  --> $DIR/option_if_let_else.rs:8:5
+  --> $DIR/option_if_let_else.rs:7:5
    |
 LL | /     if let Some(x) = string {
 LL | |         (true, x)
@@ -11,19 +11,19 @@ LL | |     }
    = note: `-D clippy::option-if-let-else` implied by `-D warnings`
 
 error: use Option::map_or instead of an if let/else
-  --> $DIR/option_if_let_else.rs:26:13
+  --> $DIR/option_if_let_else.rs:25:13
    |
 LL |     let _ = if let Some(s) = *string { s.len() } else { 0 };
    |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `string.map_or(0, |s| s.len())`
 
 error: use Option::map_or instead of an if let/else
-  --> $DIR/option_if_let_else.rs:27:13
+  --> $DIR/option_if_let_else.rs:26:13
    |
 LL |     let _ = if let Some(s) = &num { s } else { &0 };
    |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `num.as_ref().map_or(&0, |s| s)`
 
 error: use Option::map_or instead of an if let/else
-  --> $DIR/option_if_let_else.rs:28:13
+  --> $DIR/option_if_let_else.rs:27:13
    |
 LL |       let _ = if let Some(s) = &mut num {
    |  _____________^
@@ -43,13 +43,13 @@ LL ~     });
    |
 
 error: use Option::map_or instead of an if let/else
-  --> $DIR/option_if_let_else.rs:34:13
+  --> $DIR/option_if_let_else.rs:33:13
    |
 LL |     let _ = if let Some(ref s) = num { s } else { &0 };
    |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `num.as_ref().map_or(&0, |s| s)`
 
 error: use Option::map_or instead of an if let/else
-  --> $DIR/option_if_let_else.rs:35:13
+  --> $DIR/option_if_let_else.rs:34:13
    |
 LL |       let _ = if let Some(mut s) = num {
    |  _____________^
@@ -69,7 +69,7 @@ LL ~     });
    |
 
 error: use Option::map_or instead of an if let/else
-  --> $DIR/option_if_let_else.rs:41:13
+  --> $DIR/option_if_let_else.rs:40:13
    |
 LL |       let _ = if let Some(ref mut s) = num {
    |  _____________^
@@ -89,7 +89,7 @@ LL ~     });
    |
 
 error: use Option::map_or instead of an if let/else
-  --> $DIR/option_if_let_else.rs:50:5
+  --> $DIR/option_if_let_else.rs:49:5
    |
 LL | /     if let Some(x) = arg {
 LL | |         let y = x * x;
@@ -108,7 +108,7 @@ LL +     })
    |
 
 error: use Option::map_or_else instead of an if let/else
-  --> $DIR/option_if_let_else.rs:63:13
+  --> $DIR/option_if_let_else.rs:62:13
    |
 LL |       let _ = if let Some(x) = arg {
    |  _____________^
@@ -120,7 +120,7 @@ LL | |     };
    | |_____^ help: try: `arg.map_or_else(|| side_effect(), |x| x)`
 
 error: use Option::map_or_else instead of an if let/else
-  --> $DIR/option_if_let_else.rs:72:13
+  --> $DIR/option_if_let_else.rs:71:13
    |
 LL |       let _ = if let Some(x) = arg {
    |  _____________^
@@ -143,13 +143,13 @@ LL ~     }, |x| x * x * x * x);
    |
 
 error: use Option::map_or instead of an if let/else
-  --> $DIR/option_if_let_else.rs:101:13
+  --> $DIR/option_if_let_else.rs:100:13
    |
 LL |     let _ = if let Some(x) = optional { x + 2 } else { 5 };
    |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `optional.map_or(5, |x| x + 2)`
 
 error: use Option::map_or instead of an if let/else
-  --> $DIR/option_if_let_else.rs:110:13
+  --> $DIR/option_if_let_else.rs:109:13
    |
 LL |       let _ = if let Some(x) = Some(0) {
    |  _____________^
@@ -171,13 +171,13 @@ LL ~         });
    |
 
 error: use Option::map_or_else instead of an if let/else
-  --> $DIR/option_if_let_else.rs:138:13
+  --> $DIR/option_if_let_else.rs:137:13
    |
 LL |     let _ = if let Some(x) = Some(0) { s.len() + x } else { s.len() };
    |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Some(0).map_or_else(|| s.len(), |x| s.len() + x)`
 
 error: use Option::map_or instead of an if let/else
-  --> $DIR/option_if_let_else.rs:142:13
+  --> $DIR/option_if_let_else.rs:141:13
    |
 LL |       let _ = if let Some(x) = Some(0) {
    |  _____________^