From 092c4595fef374cc10cc44ada67a14e773f1b49e Mon Sep 17 00:00:00 2001 From: Aleksei Latyshev Date: Fri, 17 Apr 2020 13:53:13 +0300 Subject: [PATCH] fix redundant_pattern_matching lint - now it handles `while let` case - better suggestions in `if let` case --- .../src/redundant_pattern_matching.rs | 30 ++++-- tests/ui/redundant_pattern_matching.fixed | 51 ++++++++-- tests/ui/redundant_pattern_matching.rs | 27 +++++- tests/ui/redundant_pattern_matching.stderr | 96 ++++++++++++------- 4 files changed, 153 insertions(+), 51 deletions(-) diff --git a/clippy_lints/src/redundant_pattern_matching.rs b/clippy_lints/src/redundant_pattern_matching.rs index bdc32dbba87..334ceed64c2 100644 --- a/clippy_lints/src/redundant_pattern_matching.rs +++ b/clippy_lints/src/redundant_pattern_matching.rs @@ -1,4 +1,5 @@ -use crate::utils::{match_qpath, paths, snippet, span_lint_and_then}; +use crate::utils::{match_qpath, match_trait_method, paths, snippet, span_lint_and_then}; +use if_chain::if_chain; use rustc_ast::ast::LitKind; use rustc_errors::Applicability; use rustc_hir::{Arm, Expr, ExprKind, MatchSource, PatKind, QPath}; @@ -48,9 +49,8 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for RedundantPatternMatching { if let ExprKind::Match(op, arms, ref match_source) = &expr.kind { match match_source { MatchSource::Normal => find_sugg_for_match(cx, expr, op, arms), - MatchSource::IfLetDesugar { contains_else_clause } => { - find_sugg_for_if_let(cx, expr, op, arms, *contains_else_clause) - }, + MatchSource::IfLetDesugar { .. } => find_sugg_for_if_let(cx, expr, op, arms, "if"), + MatchSource::WhileLetDesugar => find_sugg_for_if_let(cx, expr, op, arms, "while"), _ => return, } } @@ -62,7 +62,7 @@ fn find_sugg_for_if_let<'a, 'tcx>( expr: &'tcx Expr<'_>, op: &Expr<'_>, arms: &[Arm<'_>], - has_else: bool, + keyword: &'static str, ) { let good_method = match arms[0].pat.kind { PatKind::TupleStruct(ref path, ref patterns, _) if patterns.len() == 1 => { @@ -86,7 +86,16 @@ fn find_sugg_for_if_let<'a, 'tcx>( _ => return, }; - let maybe_semi = if has_else { "" } else { ";" }; + // check that `while_let_on_iterator` lint does not trigger + if_chain! { + if keyword == "while"; + if let ExprKind::MethodCall(method_path, _, _) = op.kind; + if method_path.ident.name == sym!(next); + if match_trait_method(cx, op, &paths::ITERATOR); + then { + return; + } + } span_lint_and_then( cx, @@ -94,12 +103,15 @@ fn find_sugg_for_if_let<'a, 'tcx>( arms[0].pat.span, &format!("redundant pattern matching, consider using `{}`", good_method), |diag| { - let span = expr.span.to(op.span); + // in the case of WhileLetDesugar expr.span == op.span incorrectly. + // this is a workaround to restore true value of expr.span + let expr_span = expr.span.to(arms[1].span); + let span = expr_span.until(op.span.shrink_to_hi()); diag.span_suggestion( span, "try this", - format!("{}.{}{}", snippet(cx, op.span, "_"), good_method, maybe_semi), - Applicability::MaybeIncorrect, // snippet + format!("{} {}.{}", keyword, snippet(cx, op.span, "_"), good_method), + Applicability::MachineApplicable, // snippet ); }, ); diff --git a/tests/ui/redundant_pattern_matching.fixed b/tests/ui/redundant_pattern_matching.fixed index 538fa1ed9cb..c58db5493ad 100644 --- a/tests/ui/redundant_pattern_matching.fixed +++ b/tests/ui/redundant_pattern_matching.fixed @@ -2,16 +2,37 @@ #![warn(clippy::all)] #![warn(clippy::redundant_pattern_matching)] -#![allow(clippy::unit_arg, unused_must_use)] +#![allow(clippy::unit_arg, unused_must_use, clippy::needless_bool)] fn main() { - Ok::(42).is_ok(); + if Ok::(42).is_ok() {} - Err::(42).is_err(); + if Err::(42).is_err() {} - None::<()>.is_none(); + if None::<()>.is_none() {} - Some(42).is_some(); + if Some(42).is_some() {} + + if Some(42).is_some() { + foo(); + } else { + bar(); + } + + while Some(42).is_some() {} + + while Some(42).is_none() {} + + while None::<()>.is_none() {} + + while Ok::(10).is_ok() {} + + while Ok::(10).is_err() {} + + let mut v = vec![1, 2, 3]; + while v.pop().is_some() { + foo(); + } if Ok::(42).is_ok() {} @@ -39,22 +60,34 @@ fn main() { let _ = None::<()>.is_none(); - let _ = Ok::(4).is_ok(); + let _ = if Ok::(4).is_ok() { true } else { false }; let _ = does_something(); let _ = returns_unit(); let opt = Some(false); - let x = opt.is_some(); + let x = if opt.is_some() { true } else { false }; takes_bool(x); } fn takes_bool(_: bool) {} +fn foo() {} + +fn bar() {} + fn does_something() -> bool { - Ok::(4).is_ok() + if Ok::(4).is_ok() { + true + } else { + false + } } fn returns_unit() { - Ok::(4).is_ok(); + if Ok::(4).is_ok() { + true + } else { + false + }; } diff --git a/tests/ui/redundant_pattern_matching.rs b/tests/ui/redundant_pattern_matching.rs index 34d2cd62e54..9a9b3fb5ca0 100644 --- a/tests/ui/redundant_pattern_matching.rs +++ b/tests/ui/redundant_pattern_matching.rs @@ -2,7 +2,7 @@ #![warn(clippy::all)] #![warn(clippy::redundant_pattern_matching)] -#![allow(clippy::unit_arg, unused_must_use)] +#![allow(clippy::unit_arg, unused_must_use, clippy::needless_bool)] fn main() { if let Ok(_) = Ok::(42) {} @@ -13,6 +13,27 @@ fn main() { if let Some(_) = Some(42) {} + if let Some(_) = Some(42) { + foo(); + } else { + bar(); + } + + while let Some(_) = Some(42) {} + + while let None = Some(42) {} + + while let None = None::<()> {} + + while let Ok(_) = Ok::(10) {} + + while let Err(_) = Ok::(10) {} + + let mut v = vec![1, 2, 3]; + while let Some(_) = v.pop() { + foo(); + } + if Ok::(42).is_ok() {} if Err::(42).is_err() {} @@ -72,6 +93,10 @@ fn main() { fn takes_bool(_: bool) {} +fn foo() {} + +fn bar() {} + fn does_something() -> bool { if let Ok(_) = Ok::(4) { true diff --git a/tests/ui/redundant_pattern_matching.stderr b/tests/ui/redundant_pattern_matching.stderr index 5a4a69b1220..6285a7f5fcd 100644 --- a/tests/ui/redundant_pattern_matching.stderr +++ b/tests/ui/redundant_pattern_matching.stderr @@ -2,7 +2,7 @@ error: redundant pattern matching, consider using `is_ok()` --> $DIR/redundant_pattern_matching.rs:8:12 | LL | if let Ok(_) = Ok::(42) {} - | -------^^^^^------------------------ help: try this: `Ok::(42).is_ok();` + | -------^^^^^--------------------- help: try this: `if Ok::(42).is_ok()` | = note: `-D clippy::redundant-pattern-matching` implied by `-D warnings` @@ -10,22 +10,64 @@ error: redundant pattern matching, consider using `is_err()` --> $DIR/redundant_pattern_matching.rs:10:12 | LL | if let Err(_) = Err::(42) {} - | -------^^^^^^------------------------- help: try this: `Err::(42).is_err();` + | -------^^^^^^---------------------- help: try this: `if Err::(42).is_err()` error: redundant pattern matching, consider using `is_none()` --> $DIR/redundant_pattern_matching.rs:12:12 | LL | if let None = None::<()> {} - | -------^^^^---------------- help: try this: `None::<()>.is_none();` + | -------^^^^------------- help: try this: `if None::<()>.is_none()` error: redundant pattern matching, consider using `is_some()` --> $DIR/redundant_pattern_matching.rs:14:12 | LL | if let Some(_) = Some(42) {} - | -------^^^^^^^-------------- help: try this: `Some(42).is_some();` + | -------^^^^^^^----------- help: try this: `if Some(42).is_some()` + +error: redundant pattern matching, consider using `is_some()` + --> $DIR/redundant_pattern_matching.rs:16:12 + | +LL | if let Some(_) = Some(42) { + | -------^^^^^^^----------- help: try this: `if Some(42).is_some()` + +error: redundant pattern matching, consider using `is_some()` + --> $DIR/redundant_pattern_matching.rs:22:15 + | +LL | while let Some(_) = Some(42) {} + | ----------^^^^^^^----------- help: try this: `while Some(42).is_some()` + +error: redundant pattern matching, consider using `is_none()` + --> $DIR/redundant_pattern_matching.rs:24:15 + | +LL | while let None = Some(42) {} + | ----------^^^^----------- help: try this: `while Some(42).is_none()` + +error: redundant pattern matching, consider using `is_none()` + --> $DIR/redundant_pattern_matching.rs:26:15 + | +LL | while let None = None::<()> {} + | ----------^^^^------------- help: try this: `while None::<()>.is_none()` error: redundant pattern matching, consider using `is_ok()` - --> $DIR/redundant_pattern_matching.rs:28:5 + --> $DIR/redundant_pattern_matching.rs:28:15 + | +LL | while let Ok(_) = Ok::(10) {} + | ----------^^^^^--------------------- help: try this: `while Ok::(10).is_ok()` + +error: redundant pattern matching, consider using `is_err()` + --> $DIR/redundant_pattern_matching.rs:30:15 + | +LL | while let Err(_) = Ok::(10) {} + | ----------^^^^^^--------------------- help: try this: `while Ok::(10).is_err()` + +error: redundant pattern matching, consider using `is_some()` + --> $DIR/redundant_pattern_matching.rs:33:15 + | +LL | while let Some(_) = v.pop() { + | ----------^^^^^^^---------- help: try this: `while v.pop().is_some()` + +error: redundant pattern matching, consider using `is_ok()` + --> $DIR/redundant_pattern_matching.rs:49:5 | LL | / match Ok::(42) { LL | | Ok(_) => true, @@ -34,7 +76,7 @@ LL | | }; | |_____^ help: try this: `Ok::(42).is_ok()` error: redundant pattern matching, consider using `is_err()` - --> $DIR/redundant_pattern_matching.rs:33:5 + --> $DIR/redundant_pattern_matching.rs:54:5 | LL | / match Ok::(42) { LL | | Ok(_) => false, @@ -43,7 +85,7 @@ LL | | }; | |_____^ help: try this: `Ok::(42).is_err()` error: redundant pattern matching, consider using `is_err()` - --> $DIR/redundant_pattern_matching.rs:38:5 + --> $DIR/redundant_pattern_matching.rs:59:5 | LL | / match Err::(42) { LL | | Ok(_) => false, @@ -52,7 +94,7 @@ LL | | }; | |_____^ help: try this: `Err::(42).is_err()` error: redundant pattern matching, consider using `is_ok()` - --> $DIR/redundant_pattern_matching.rs:43:5 + --> $DIR/redundant_pattern_matching.rs:64:5 | LL | / match Err::(42) { LL | | Ok(_) => true, @@ -61,7 +103,7 @@ LL | | }; | |_____^ help: try this: `Err::(42).is_ok()` error: redundant pattern matching, consider using `is_some()` - --> $DIR/redundant_pattern_matching.rs:48:5 + --> $DIR/redundant_pattern_matching.rs:69:5 | LL | / match Some(42) { LL | | Some(_) => true, @@ -70,7 +112,7 @@ LL | | }; | |_____^ help: try this: `Some(42).is_some()` error: redundant pattern matching, consider using `is_none()` - --> $DIR/redundant_pattern_matching.rs:53:5 + --> $DIR/redundant_pattern_matching.rs:74:5 | LL | / match None::<()> { LL | | Some(_) => false, @@ -79,7 +121,7 @@ LL | | }; | |_____^ help: try this: `None::<()>.is_none()` error: redundant pattern matching, consider using `is_none()` - --> $DIR/redundant_pattern_matching.rs:58:13 + --> $DIR/redundant_pattern_matching.rs:79:13 | LL | let _ = match None::<()> { | _____________^ @@ -89,38 +131,28 @@ LL | | }; | |_____^ help: try this: `None::<()>.is_none()` error: redundant pattern matching, consider using `is_ok()` - --> $DIR/redundant_pattern_matching.rs:63:20 + --> $DIR/redundant_pattern_matching.rs:84:20 | LL | let _ = if let Ok(_) = Ok::(4) { true } else { false }; - | -------^^^^^--------------------------------------------- help: try this: `Ok::(4).is_ok()` + | -------^^^^^--------------------- help: try this: `if Ok::(4).is_ok()` error: redundant pattern matching, consider using `is_some()` - --> $DIR/redundant_pattern_matching.rs:69:20 + --> $DIR/redundant_pattern_matching.rs:90:20 | LL | let x = if let Some(_) = opt { true } else { false }; - | -------^^^^^^^------------------------------ help: try this: `opt.is_some()` + | -------^^^^^^^------ help: try this: `if opt.is_some()` error: redundant pattern matching, consider using `is_ok()` - --> $DIR/redundant_pattern_matching.rs:76:12 + --> $DIR/redundant_pattern_matching.rs:101:12 | -LL | if let Ok(_) = Ok::(4) { - | _____- ^^^^^ -LL | | true -LL | | } else { -LL | | false -LL | | } - | |_____- help: try this: `Ok::(4).is_ok()` +LL | if let Ok(_) = Ok::(4) { + | -------^^^^^-------------------- help: try this: `if Ok::(4).is_ok()` error: redundant pattern matching, consider using `is_ok()` - --> $DIR/redundant_pattern_matching.rs:84:12 + --> $DIR/redundant_pattern_matching.rs:109:12 | -LL | if let Ok(_) = Ok::(4) { - | _____- ^^^^^ -LL | | true -LL | | } else { -LL | | false -LL | | }; - | |_____- help: try this: `Ok::(4).is_ok()` +LL | if let Ok(_) = Ok::(4) { + | -------^^^^^-------------------- help: try this: `if Ok::(4).is_ok()` -error: aborting due to 15 previous errors +error: aborting due to 22 previous errors