diff --git a/clippy_lints/src/matches/single_match.rs b/clippy_lints/src/matches/single_match.rs index 0c4cb45d147c..df60401aa8b9 100644 --- a/clippy_lints/src/matches/single_match.rs +++ b/clippy_lints/src/matches/single_match.rs @@ -1,14 +1,13 @@ use clippy_utils::diagnostics::span_lint_and_sugg; use clippy_utils::source::{expr_block, snippet}; -use clippy_utils::ty::{implements_trait, match_type, peel_mid_ty_refs}; -use clippy_utils::{ - is_lint_allowed, is_unit_expr, is_wild, paths, peel_blocks, peel_hir_pat_refs, peel_n_hir_expr_refs, -}; +use clippy_utils::ty::{implements_trait, peel_mid_ty_refs}; +use clippy_utils::{is_lint_allowed, is_unit_expr, is_wild, peel_blocks, peel_hir_pat_refs, peel_n_hir_expr_refs}; use core::cmp::max; use rustc_errors::Applicability; -use rustc_hir::{Arm, BindingAnnotation, Block, Expr, ExprKind, Pat, PatKind}; +use rustc_hir::{Arm, Block, Expr, ExprKind, Pat, PatField, PatKind}; use rustc_lint::LateContext; use rustc_middle::ty::{self, Ty}; +use rustc_span::sym; use super::{MATCH_BOOL, SINGLE_MATCH, SINGLE_MATCH_ELSE}; @@ -140,74 +139,133 @@ fn check_opt_like<'a>( ty: Ty<'a>, els: Option<&Expr<'_>>, ) { - // list of candidate `Enum`s we know will never get any more members - let candidates = &[ - (&paths::COW, "Borrowed"), - (&paths::COW, "Cow::Borrowed"), - (&paths::COW, "Cow::Owned"), - (&paths::COW, "Owned"), - (&paths::OPTION, "None"), - (&paths::RESULT, "Err"), - (&paths::RESULT, "Ok"), - ]; + if check_exhaustive(cx, arms[0].pat, arms[1].pat, ty) { + report_single_pattern(cx, ex, arms, expr, els); + } +} - // We want to suggest to exclude an arm that contains only wildcards or forms the exhaustive - // match with the second branch, without enum variants in matches. - if !contains_only_wilds(arms[1].pat) && !form_exhaustive_matches(arms[0].pat, arms[1].pat) { - return; +/// Resturns true if the given type is one of the standard `Enum`s we know will never get any more +/// members. +fn is_known_enum(cx: &LateContext<'_>, ty: Ty<'_>) -> bool { + if let Some(adt) = ty.ty_adt_def() { + return matches!( + cx.tcx.get_diagnostic_name(adt.did), + Some(sym::Cow | sym::Option | sym::Result) + ); } + false +} - let mut paths_and_types = Vec::new(); - if !collect_pat_paths(&mut paths_and_types, cx, arms[1].pat, ty) { - return; +fn contains_only_known_enums(cx: &LateContext<'_>, pat: &Pat<'_>) -> bool { + match pat.kind { + PatKind::Path(..) => is_known_enum(cx, cx.typeck_results().pat_ty(pat)), + PatKind::TupleStruct(..) | PatKind::Struct(..) => { + let ty = cx.typeck_results().pat_ty(pat); + (contains_only_wilds(pat) || contains_single_binding(pat)) && is_known_enum(cx, ty) + }, + _ => false, } +} - let in_candidate_enum = |path_info: &(String, Ty<'_>)| -> bool { - let (path, ty) = path_info; - for &(ty_path, pat_path) in candidates { - if path == pat_path && match_type(cx, *ty, ty_path) { - return true; - } - } - false - }; - if paths_and_types.iter().all(in_candidate_enum) { - report_single_pattern(cx, ex, arms, expr, els); +fn peel_subpatterns<'a>(pat: &'a Pat<'a>) -> &'a Pat<'a> { + if let PatKind::Binding(.., Some(sub)) = pat.kind { + return peel_subpatterns(sub); } + pat } -/// Collects paths and their types from the given patterns. Returns true if the given pattern could -/// be simplified, false otherwise. -fn collect_pat_paths<'a>(acc: &mut Vec<(String, Ty<'a>)>, cx: &LateContext<'a>, pat: &Pat<'_>, ty: Ty<'a>) -> bool { - match pat.kind { - PatKind::Wild => true, - PatKind::Tuple(inner, _) => inner.iter().all(|p| { - let p_ty = cx.typeck_results().pat_ty(p); - collect_pat_paths(acc, cx, p, p_ty) - }), - PatKind::TupleStruct(ref path, ..) => { - let path = rustc_hir_pretty::to_string(rustc_hir_pretty::NO_ANN, |s| { - s.print_qpath(path, false); - }); - acc.push((path, ty)); - true +/// Returns false if the patterns exhaustively match an enum. +fn check_exhaustive<'a>(cx: &LateContext<'a>, left: &Pat<'_>, right: &Pat<'_>, ty: Ty<'a>) -> bool { + let left = peel_subpatterns(left); + let right = peel_subpatterns(right); + match (&left.kind, &right.kind) { + (PatKind::Wild, _) | (_, PatKind::Wild) => true, + (PatKind::Struct(_, left_fields, _), PatKind::Struct(_, right_fields, _)) => { + check_exhaustive_structs(cx, left_fields, right_fields) }, - PatKind::Binding(BindingAnnotation::Unannotated, .., ident, None) => { - acc.push((ident.to_string(), ty)); - true + (PatKind::Tuple(left_in, left_pos), PatKind::Tuple(right_in, right_pos)) => { + check_exhaustive_tuples(cx, left_in, left_pos, right_in, right_pos) }, - PatKind::Path(ref path) => { - let path = rustc_hir_pretty::to_string(rustc_hir_pretty::NO_ANN, |s| { - s.print_qpath(path, false); - }); - acc.push((path, ty)); - true + (PatKind::TupleStruct(_, left_in, left_pos), PatKind::TupleStruct(_, right_in, right_pos)) + if contains_only_wilds(right) && contains_only_known_enums(cx, left) => + { + check_exhaustive_tuples(cx, left_in, left_pos, right_in, right_pos) }, + (PatKind::Binding(.., None) | PatKind::Path(_), _) if contains_only_wilds(right) => is_known_enum(cx, ty), _ => false, } } -/// Returns true if the given arm of pattern matching contains wildcard patterns. +fn could_be_simplified(cx: &LateContext<'_>, left_pat: &Pat<'_>, right_pat: &Pat<'_>) -> bool { + contains_only_wilds(left_pat) + || contains_only_wilds(right_pat) + || contains_only_known_enums(cx, left_pat) && contains_only_known_enums(cx, right_pat) +} + +/// Returns true if two tuples that contain patterns `left_in` and `right_in` and `..` operators at +/// positions `left_pos` and `right_pos` form exhaustive match. This means, that two elements of +/// these tuples located at the same positions must have at least one wildcard (`_`) pattern. +fn check_exhaustive_tuples<'a>( + cx: &LateContext<'_>, + left_in: &'a [Pat<'a>], + left_pos: &Option, + right_in: &'a [Pat<'a>], + right_pos: &Option, +) -> bool { + // We don't actually know the position and the presence of the `..` (dotdot) operator + // in the arms, so we need to evaluate the correct offsets here in order to iterate in + // both arms at the same time. + let len = max( + left_in.len() + { + if left_pos.is_some() { 1 } else { 0 } + }, + right_in.len() + { + if right_pos.is_some() { 1 } else { 0 } + }, + ); + let mut left_pos = left_pos.unwrap_or(usize::MAX); + let mut right_pos = right_pos.unwrap_or(usize::MAX); + let mut left_dot_space = 0; + let mut right_dot_space = 0; + for i in 0..len { + let mut found_dotdot = false; + if i == left_pos { + left_dot_space += 1; + if left_dot_space < len - left_in.len() { + left_pos += 1; + } + found_dotdot = true; + } + if i == right_pos { + right_dot_space += 1; + if right_dot_space < len - right_in.len() { + right_pos += 1; + } + found_dotdot = true; + } + if found_dotdot { + continue; + } + + if !could_be_simplified(cx, &left_in[i - left_dot_space], &right_in[i - right_dot_space]) { + return false; + } + } + true +} + +fn check_exhaustive_structs<'a>( + cx: &LateContext<'_>, + left_fields: &'a [PatField<'a>], + right_fields: &'a [PatField<'a>], +) -> bool { + left_fields + .iter() + .zip(right_fields.iter()) + .all(|(left_field, right_field)| could_be_simplified(cx, left_field.pat, right_field.pat)) +} + +/// Returns true if the given arm of pattern matching contains only wildcard patterns. fn contains_only_wilds(pat: &Pat<'_>) -> bool { match pat.kind { PatKind::Wild => true, @@ -216,54 +274,10 @@ fn contains_only_wilds(pat: &Pat<'_>) -> bool { } } -/// Returns true if the given patterns forms only exhaustive matches that don't contain enum -/// patterns without a wildcard. -fn form_exhaustive_matches(left: &Pat<'_>, right: &Pat<'_>) -> bool { - match (&left.kind, &right.kind) { - (PatKind::Wild, _) | (_, PatKind::Wild) => true, - (PatKind::Tuple(left_in, left_pos), PatKind::Tuple(right_in, right_pos)) => { - // We don't actually know the position and the presence of the `..` (dotdot) operator - // in the arms, so we need to evaluate the correct offsets here in order to iterate in - // both arms at the same time. - let len = max( - left_in.len() + { - if left_pos.is_some() { 1 } else { 0 } - }, - right_in.len() + { - if right_pos.is_some() { 1 } else { 0 } - }, - ); - let mut left_pos = left_pos.unwrap_or(usize::MAX); - let mut right_pos = right_pos.unwrap_or(usize::MAX); - let mut left_dot_space = 0; - let mut right_dot_space = 0; - for i in 0..len { - let mut found_dotdot = false; - if i == left_pos { - left_dot_space += 1; - if left_dot_space < len - left_in.len() { - left_pos += 1; - } - found_dotdot = true; - } - if i == right_pos { - right_dot_space += 1; - if right_dot_space < len - right_in.len() { - right_pos += 1; - } - found_dotdot = true; - } - if found_dotdot { - continue; - } - if !contains_only_wilds(&left_in[i - left_dot_space]) - && !contains_only_wilds(&right_in[i - right_dot_space]) - { - return false; - } - } - true - }, +fn contains_single_binding(pat: &Pat<'_>) -> bool { + match pat.kind { + PatKind::Binding(.., None) => true, + PatKind::TupleStruct(_, inner, ..) => inner.iter().all(contains_single_binding), _ => false, } } diff --git a/tests/ui/patterns.fixed b/tests/ui/patterns.fixed index f22388154499..61cb98edb7dc 100644 --- a/tests/ui/patterns.fixed +++ b/tests/ui/patterns.fixed @@ -2,12 +2,16 @@ #![allow(unused)] #![warn(clippy::all)] +fn dummy() { + dbg!("test"); +} + fn main() { let v = Some(true); let s = [0, 1, 2, 3, 4]; match v { - Some(x) => (), - y => (), + Some(x) => dummy(), + y => dummy(), } match v { Some(x) => (), diff --git a/tests/ui/patterns.rs b/tests/ui/patterns.rs index 5848ecd38d98..cc753f8b5eb9 100644 --- a/tests/ui/patterns.rs +++ b/tests/ui/patterns.rs @@ -2,12 +2,16 @@ #![allow(unused)] #![warn(clippy::all)] +fn dummy() { + dbg!("test"); +} + fn main() { let v = Some(true); let s = [0, 1, 2, 3, 4]; match v { - Some(x) => (), - y @ _ => (), + Some(x) => dummy(), + y @ _ => dummy(), } match v { Some(x) => (), diff --git a/tests/ui/patterns.stderr b/tests/ui/patterns.stderr index af067580688b..e414f97a95a3 100644 --- a/tests/ui/patterns.stderr +++ b/tests/ui/patterns.stderr @@ -1,19 +1,19 @@ error: the `y @ _` pattern can be written as just `y` - --> $DIR/patterns.rs:10:9 + --> $DIR/patterns.rs:14:9 | -LL | y @ _ => (), +LL | y @ _ => dummy(), | ^^^^^ help: try: `y` | = note: `-D clippy::redundant-pattern` implied by `-D warnings` error: the `x @ _` pattern can be written as just `x` - --> $DIR/patterns.rs:25:9 + --> $DIR/patterns.rs:29:9 | LL | ref mut x @ _ => { | ^^^^^^^^^^^^^ help: try: `ref mut x` error: the `x @ _` pattern can be written as just `x` - --> $DIR/patterns.rs:33:9 + --> $DIR/patterns.rs:37:9 | LL | ref x @ _ => println!("vec: {:?}", x), | ^^^^^^^^^ help: try: `ref x` diff --git a/tests/ui/single_match.rs b/tests/ui/single_match.rs index dd148edf5292..040c909c4209 100644 --- a/tests/ui/single_match.rs +++ b/tests/ui/single_match.rs @@ -159,68 +159,161 @@ fn ranges() { (Some(E::V), _) => {}, (None, _) => {}, } + match (Some(E::V), Some(E::V), Some(E::V)) { + (.., Some(E::V), _) => {}, + (.., None, _) => {}, + } + match (Some(E::V), Some(E::V), Some(E::V)) { + (Some(E::V), ..) => {}, + (None, ..) => {}, + } + match (Some(E::V), Some(E::V), Some(E::V)) { + (_, Some(E::V), ..) => {}, + (_, None, ..) => {}, + } - // lint + // Don't lint, because the second arm contains bindings. So, we cannot keep them in the `else` + // branch. + match (1, 2) { + (1, _b) => (), + (_a, ..) => (), + } + match (1, 2) { + (1, _b) => {}, + (_a, _) => {}, + } + + // Lint cases. match x { (Some(_), _) => {}, (None, _) => {}, } - - // lint match x { (Some(E::V), _) => todo!(), (_, _) => {}, } - - // lint match (Some(42), Some(E::V), Some(42)) { (.., Some(E::V), _) => {}, (..) => {}, } +} - // Don't lint, see above. - match (Some(E::V), Some(E::V), Some(E::V)) { - (.., Some(E::V), _) => {}, - (.., None, _) => {}, +fn annotated_bindings() { + struct S(i32); + let x = Some(S(1i32)); + + match Some(S(1i32)) { + Some(a) => {}, + _ => (), } - // Don't lint, see above. - match (Some(E::V), Some(E::V), Some(E::V)) { - (Some(E::V), ..) => {}, - (None, ..) => {}, + match x { + Some(ref a) => {}, + _ => (), } - // Don't lint, see above. - match (Some(E::V), Some(E::V), Some(E::V)) { - (_, Some(E::V), ..) => {}, - (_, None, ..) => {}, + match x { + Some(mut a) => {}, + _ => (), + } + + match Some(S(1i32)) { + Some(ref mut a) => {}, + _ => (), } } -fn skip_type_aliases() { - enum OptionEx { - Some(i32), - None, +#[allow(clippy::redundant_pattern)] +fn binding_subpatterns() { + // Lint. + match 1 { + _a @ 42 => {}, + _ => {}, + } + match 1 { + _a @ 42 => {}, + _a @ _ => {}, } - enum ResultEx { - Err(i32), - Ok(i32), + + // Don't lint. + match 1 { + _a @ 42 => {}, + _b => {}, } +} - use OptionEx::{None, Some}; - use ResultEx::{Err, Ok}; +fn tuple_structs() { + struct S(i32, i32); + let s = S(1, 2); + + // dont' lint + match s { + S(42, _a) => {}, + S(_, _) => {}, + } // don't lint - match Err(42) { - Ok(_) => dummy(), - Err(_) => (), - }; + match s { + S(42, _a) => {}, + S(..) => {}, + } + + enum E { + S1(i32, i32), + S2(i32, i32), + } + let s = E::S1(1, 2); // don't lint - match Some(1i32) { - Some(_) => dummy(), - None => (), + match s { + E::S1(..) => {}, + E::S2(..) => {}, + } +} + +fn structs() { + struct S { + x: i32, + y: i32, + } + let s = S { x: 1, y: 2 }; + + // lint + match s { + S { x: 42, .. } => {}, + S { .. } => {}, + } + + // lint + match s { + S { x: _x, y: 42 } => {}, + S { .. } => {}, + } + + // don't lint + match s { + S { x: i32::MIN, y: _y } => {}, + S { + x: i32::MIN..=i32::MAX, .. + } => {}, + } + + // Don't lint, because we have an annotated binding in the second arm. So we cannot replace it + // with `else` branch, keeping this binding. + match s { + S { x: _x, y: _y } => dummy(), + S { x: _x, .. } => dummy(), + } + + // Lint, because it contains only known enums. + struct S1 { + a: Option, }; + let s = S1 { a: Some(33) }; + match s { + S1 { a: Some(..) } => (), + S1 { a: None } => (), + } } macro_rules! single_match { diff --git a/tests/ui/single_match.stderr b/tests/ui/single_match.stderr index 318faf257175..0be262d7e738 100644 --- a/tests/ui/single_match.stderr +++ b/tests/ui/single_match.stderr @@ -120,7 +120,7 @@ LL | | }; | |_____^ help: try this: `if let None = x { println!() }` error: you seem to be trying to use `match` for destructuring a single pattern. Consider using `if let` - --> $DIR/single_match.rs:164:5 + --> $DIR/single_match.rs:187:5 | LL | / match x { LL | | (Some(_), _) => {}, @@ -129,7 +129,7 @@ LL | | } | |_____^ help: try this: `if let (Some(_), _) = x {}` error: you seem to be trying to use `match` for destructuring a single pattern. Consider using `if let` - --> $DIR/single_match.rs:170:5 + --> $DIR/single_match.rs:191:5 | LL | / match x { LL | | (Some(E::V), _) => todo!(), @@ -138,7 +138,7 @@ LL | | } | |_____^ help: try this: `if let (Some(E::V), _) = x { todo!() }` error: you seem to be trying to use `match` for destructuring a single pattern. Consider using `if let` - --> $DIR/single_match.rs:176:5 + --> $DIR/single_match.rs:195:5 | LL | / match (Some(42), Some(E::V), Some(42)) { LL | | (.., Some(E::V), _) => {}, @@ -146,5 +146,86 @@ LL | | (..) => {}, LL | | } | |_____^ help: try this: `if let (.., Some(E::V), _) = (Some(42), Some(E::V), Some(42)) {}` -error: aborting due to 15 previous errors +error: you seem to be trying to use `match` for destructuring a single pattern. Consider using `if let` + --> $DIR/single_match.rs:205:5 + | +LL | / match Some(S(1i32)) { +LL | | Some(a) => {}, +LL | | _ => (), +LL | | } + | |_____^ help: try this: `if let Some(a) = Some(S(1i32)) {}` + +error: you seem to be trying to use `match` for destructuring a single pattern. Consider using `if let` + --> $DIR/single_match.rs:210:5 + | +LL | / match x { +LL | | Some(ref a) => {}, +LL | | _ => (), +LL | | } + | |_____^ help: try this: `if let Some(ref a) = x {}` + +error: you seem to be trying to use `match` for destructuring a single pattern. Consider using `if let` + --> $DIR/single_match.rs:215:5 + | +LL | / match x { +LL | | Some(mut a) => {}, +LL | | _ => (), +LL | | } + | |_____^ help: try this: `if let Some(mut a) = x {}` + +error: you seem to be trying to use `match` for destructuring a single pattern. Consider using `if let` + --> $DIR/single_match.rs:220:5 + | +LL | / match Some(S(1i32)) { +LL | | Some(ref mut a) => {}, +LL | | _ => (), +LL | | } + | |_____^ help: try this: `if let Some(ref mut a) = Some(S(1i32)) {}` + +error: you seem to be trying to use `match` for destructuring a single pattern. Consider using `if let` + --> $DIR/single_match.rs:229:5 + | +LL | / match 1 { +LL | | _a @ 42 => {}, +LL | | _ => {}, +LL | | } + | |_____^ help: try this: `if let _a @ 42 = 1 {}` + +error: you seem to be trying to use `match` for destructuring a single pattern. Consider using `if let` + --> $DIR/single_match.rs:233:5 + | +LL | / match 1 { +LL | | _a @ 42 => {}, +LL | | _a @ _ => {}, +LL | | } + | |_____^ help: try this: `if let _a @ 42 = 1 {}` + +error: you seem to be trying to use `match` for destructuring a single pattern. Consider using `if let` + --> $DIR/single_match.rs:282:5 + | +LL | / match s { +LL | | S { x: 42, .. } => {}, +LL | | S { .. } => {}, +LL | | } + | |_____^ help: try this: `if let S { x: 42, .. } = s {}` + +error: you seem to be trying to use `match` for destructuring a single pattern. Consider using `if let` + --> $DIR/single_match.rs:288:5 + | +LL | / match s { +LL | | S { x: _x, y: 42 } => {}, +LL | | S { .. } => {}, +LL | | } + | |_____^ help: try this: `if let S { x: _x, y: 42 } = s {}` + +error: you seem to be trying to use `match` for destructuring a single pattern. Consider using `if let` + --> $DIR/single_match.rs:313:5 + | +LL | / match s { +LL | | S1 { a: Some(..) } => (), +LL | | S1 { a: None } => (), +LL | | } + | |_____^ help: try this: `if let S1 { a: Some(..) } = s { () }` + +error: aborting due to 24 previous errors diff --git a/tests/ui/unneeded_field_pattern.stderr b/tests/ui/unneeded_field_pattern.stderr index b8d3c2945322..18f5ea26c970 100644 --- a/tests/ui/unneeded_field_pattern.stderr +++ b/tests/ui/unneeded_field_pattern.stderr @@ -15,5 +15,26 @@ LL | Foo { a: _, b: _, c: _ } => {}, | = help: try with `Foo { .. }` instead -error: aborting due to 2 previous errors +error: you seem to be trying to use `match` for destructuring a single pattern. Consider using `if let` + --> $DIR/unneeded_field_pattern.rs:13:5 + | +LL | / match f { +LL | | Foo { a: _, b: 0, .. } => {}, +LL | | +LL | | Foo { a: _, b: _, c: _ } => {}, +LL | | } + | |_____^ help: try this: `if let Foo { a: _, b: 0, .. } = f {}` + | + = note: `-D clippy::single-match` implied by `-D warnings` + +error: you seem to be trying to use `match` for destructuring a single pattern. Consider using `if let` + --> $DIR/unneeded_field_pattern.rs:18:5 + | +LL | / match f { +LL | | Foo { b: 0, .. } => {}, // should be OK +LL | | Foo { .. } => {}, // and the Force might be with this one +LL | | } + | |_____^ help: try this: `if let Foo { b: 0, .. } = f {}` + +error: aborting due to 4 previous errors