diff --git a/CHANGELOG.md b/CHANGELOG.md index b1e837d363a2..b7a7a421532b 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 7d81fafe4c93..e5fe22ad3944 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 6533b94e82bd..268349d28481 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 c5a5cde4b110..6dae8f320436 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. /// @@ -26,6 +27,28 @@ declare_clippy_lint! { "statements with no effect" } +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 @@ -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 4e583a25b94c..061a4ab9b2ef 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 9c0fcf6fb454..035169fab85b 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 7ec845adfaac..7bcc4cad0d36 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 6b24675ac2d4..a5dbc9fef455 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 a3ebe5d07038..4077f1920a38 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 b11df3db60f5..2f414e129d5a 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 ed748ee8b39e..803d941c36df 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) { | _____________^