From 8d7417d8079b7f942e3a116ede6d36dc7a219e71 Mon Sep 17 00:00:00 2001 From: Jason Newcomb Date: Sun, 10 Jan 2021 23:32:23 -0500 Subject: [PATCH 1/3] Add: single_match will suggest using if .. == .. instead of if let when applicable --- clippy_lints/src/matches.rs | 79 ++++++++++++++++++++++++++++++------ tests/ui/single_match.rs | 43 ++++++++++++++++++++ tests/ui/single_match.stderr | 38 ++++++++++++++++- 3 files changed, 146 insertions(+), 14 deletions(-) diff --git a/clippy_lints/src/matches.rs b/clippy_lints/src/matches.rs index 04b35835c6b8..2239b519632b 100644 --- a/clippy_lints/src/matches.rs +++ b/clippy_lints/src/matches.rs @@ -2,10 +2,10 @@ use crate::consts::{constant, miri_to_const, Constant}; use crate::utils::sugg::Sugg; use crate::utils::usage::is_unused; use crate::utils::{ - expr_block, get_arg_name, get_parent_expr, in_macro, indent_of, is_allowed, is_expn_of, is_refutable, - is_type_diagnostic_item, is_wild, match_qpath, match_type, match_var, meets_msrv, multispan_sugg, remove_blocks, - snippet, snippet_block, snippet_opt, snippet_with_applicability, span_lint_and_help, span_lint_and_note, - span_lint_and_sugg, span_lint_and_then, + expr_block, get_arg_name, get_parent_expr, implements_trait, in_macro, indent_of, is_allowed, is_expn_of, + is_refutable, is_type_diagnostic_item, is_wild, match_qpath, match_type, match_var, meets_msrv, multispan_sugg, + remove_blocks, snippet, snippet_block, snippet_opt, snippet_with_applicability, span_lint_and_help, + span_lint_and_note, span_lint_and_sugg, span_lint_and_then, }; use crate::utils::{paths, search_same, SpanlessEq, SpanlessHash}; use if_chain::if_chain; @@ -717,6 +717,28 @@ fn check_single_match_single_pattern( } } +fn peel_pat_refs(pat: &'a Pat<'a>) -> (&'a Pat<'a>, usize) { + fn peel(pat: &'a Pat<'a>, count: usize) -> (&'a Pat<'a>, usize) { + if let PatKind::Ref(pat, _) = pat.kind { + peel(pat, count + 1) + } else { + (pat, count) + } + } + peel(pat, 0) +} + +fn peel_ty_refs(ty: Ty<'_>) -> (Ty<'_>, usize) { + fn peel(ty: Ty<'_>, count: usize) -> (Ty<'_>, usize) { + if let ty::Ref(_, ty, _) = ty.kind() { + peel(ty, count + 1) + } else { + (ty, count) + } + } + peel(ty, 0) +} + fn report_single_match_single_pattern( cx: &LateContext<'_>, ex: &Expr<'_>, @@ -728,20 +750,51 @@ fn report_single_match_single_pattern( let els_str = els.map_or(String::new(), |els| { format!(" else {}", expr_block(cx, els, None, "..", Some(expr.span))) }); + + let (msg, sugg) = if_chain! { + let (pat, pat_ref_count) = peel_pat_refs(arms[0].pat); + if let PatKind::Path(_) | PatKind::Lit(_) = pat.kind; + let (ty, ty_ref_count) = peel_ty_refs(cx.typeck_results().expr_ty(ex)); + if let Some(trait_id) = cx.tcx.lang_items().structural_peq_trait(); + if ty.is_integral() || ty.is_char() || ty.is_str() || implements_trait(cx, ty, trait_id, &[]); + then { + // scrutinee derives PartialEq and the pattern is a constant. + let pat_ref_count = match pat.kind { + // string literals are already a reference. + PatKind::Lit(Expr { kind: ExprKind::Lit(lit), .. }) if lit.node.is_str() => pat_ref_count + 1, + _ => pat_ref_count, + }; + let msg = "you seem to be trying to use match for an equality check. Consider using `if`"; + let sugg = format!( + "if {} == {}{} {}{}", + snippet(cx, ex.span, ".."), + // PartialEq for different reference counts may not exist. + "&".repeat(ty_ref_count - pat_ref_count), + snippet(cx, arms[0].pat.span, ".."), + expr_block(cx, &arms[0].body, None, "..", Some(expr.span)), + els_str, + ); + (msg, sugg) + } else { + let msg = "you seem to be trying to use match for destructuring a single pattern. Consider using `if let`"; + let sugg = format!( + "if let {} = {} {}{}", + snippet(cx, arms[0].pat.span, ".."), + snippet(cx, ex.span, ".."), + expr_block(cx, &arms[0].body, None, "..", Some(expr.span)), + els_str, + ); + (msg, sugg) + } + }; + span_lint_and_sugg( cx, lint, expr.span, - "you seem to be trying to use match for destructuring a single pattern. Consider using `if \ - let`", + msg, "try this", - format!( - "if let {} = {} {}{}", - snippet(cx, arms[0].pat.span, ".."), - snippet(cx, ex.span, ".."), - expr_block(cx, &arms[0].body, None, "..", Some(expr.span)), - els_str, - ), + sugg, Applicability::HasPlaceholders, ); } diff --git a/tests/ui/single_match.rs b/tests/ui/single_match.rs index 1c55af5dfb67..02266308fba2 100644 --- a/tests/ui/single_match.rs +++ b/tests/ui/single_match.rs @@ -81,6 +81,49 @@ fn single_match_know_enum() { } } +fn issue_173() { + let x = "test"; + match x { + "test" => println!(), + _ => (), + } + + #[derive(PartialEq, Eq)] + enum Foo { + A, + B, + C(u32), + } + + let x = Foo::A; + match x { + Foo::A => println!(), + _ => (), + } + + const FOO_C: Foo = Foo::C(0); + match x { + FOO_C => println!(), + _ => (), + } + enum Bar { + A, + B, + } + impl PartialEq for Bar { + fn eq(&self, rhs: &Self) -> bool { + matches!((self, rhs), (Self::A, Self::A) | (Self::B, Self::B)) + } + } + impl Eq for Bar {} + + let x = Bar::A; + match x { + Bar::A => println!(), + _ => (), + } +} + macro_rules! single_match { ($num:literal) => { match $num { diff --git a/tests/ui/single_match.stderr b/tests/ui/single_match.stderr index f69554d75f9b..5eca07ab1095 100644 --- a/tests/ui/single_match.stderr +++ b/tests/ui/single_match.stderr @@ -65,5 +65,41 @@ LL | | Cow::Owned(..) => (), LL | | }; | |_____^ help: try this: `if let Cow::Borrowed(..) = c { dummy() }` -error: aborting due to 6 previous errors +error: you seem to be trying to use match for an equality check. Consider using `if` + --> $DIR/single_match.rs:86:5 + | +LL | / match x { +LL | | "test" => println!(), +LL | | _ => (), +LL | | } + | |_____^ help: try this: `if x == "test" { println!() }` + +error: you seem to be trying to use match for an equality check. Consider using `if` + --> $DIR/single_match.rs:99:5 + | +LL | / match x { +LL | | Foo::A => println!(), +LL | | _ => (), +LL | | } + | |_____^ help: try this: `if x == Foo::A { println!() }` + +error: you seem to be trying to use match for an equality check. Consider using `if` + --> $DIR/single_match.rs:105:5 + | +LL | / match x { +LL | | FOO_C => println!(), +LL | | _ => (), +LL | | } + | |_____^ help: try this: `if x == FOO_C { println!() }` + +error: you seem to be trying to use match for destructuring a single pattern. Consider using `if let` + --> $DIR/single_match.rs:121:5 + | +LL | / match x { +LL | | Bar::A => println!(), +LL | | _ => (), +LL | | } + | |_____^ help: try this: `if let Bar::A = x { println!() }` + +error: aborting due to 10 previous errors From 85edd65bf69266dd7cec2ca6d7bb6941b6f85444 Mon Sep 17 00:00:00 2001 From: Jason Newcomb Date: Thu, 14 Jan 2021 14:26:26 -0500 Subject: [PATCH 2/3] Address review comments Add: attempt to remove address of expressions from the scrutinee expression before adding references to the pattern --- clippy_lints/src/matches.rs | 45 +++++++++++------------------ clippy_lints/src/utils/mod.rs | 38 ++++++++++++++++++++++++ tests/ui/single_match.rs | 15 +++++++++- tests/ui/single_match.stderr | 48 +++++++++++++++++++++---------- tests/ui/single_match_else.stderr | 6 ++-- 5 files changed, 104 insertions(+), 48 deletions(-) diff --git a/clippy_lints/src/matches.rs b/clippy_lints/src/matches.rs index 2239b519632b..6ecd738d2f0b 100644 --- a/clippy_lints/src/matches.rs +++ b/clippy_lints/src/matches.rs @@ -4,8 +4,8 @@ use crate::utils::usage::is_unused; use crate::utils::{ expr_block, get_arg_name, get_parent_expr, implements_trait, in_macro, indent_of, is_allowed, is_expn_of, is_refutable, is_type_diagnostic_item, is_wild, match_qpath, match_type, match_var, meets_msrv, multispan_sugg, - remove_blocks, snippet, snippet_block, snippet_opt, snippet_with_applicability, span_lint_and_help, - span_lint_and_note, span_lint_and_sugg, span_lint_and_then, + peel_hir_pat_refs, peel_mid_ty_refs, peeln_hir_expr_refs, remove_blocks, snippet, snippet_block, snippet_opt, + snippet_with_applicability, span_lint_and_help, span_lint_and_note, span_lint_and_sugg, span_lint_and_then, }; use crate::utils::{paths, search_same, SpanlessEq, SpanlessHash}; use if_chain::if_chain; @@ -717,28 +717,6 @@ fn check_single_match_single_pattern( } } -fn peel_pat_refs(pat: &'a Pat<'a>) -> (&'a Pat<'a>, usize) { - fn peel(pat: &'a Pat<'a>, count: usize) -> (&'a Pat<'a>, usize) { - if let PatKind::Ref(pat, _) = pat.kind { - peel(pat, count + 1) - } else { - (pat, count) - } - } - peel(pat, 0) -} - -fn peel_ty_refs(ty: Ty<'_>) -> (Ty<'_>, usize) { - fn peel(ty: Ty<'_>, count: usize) -> (Ty<'_>, usize) { - if let ty::Ref(_, ty, _) = ty.kind() { - peel(ty, count + 1) - } else { - (ty, count) - } - } - peel(ty, 0) -} - fn report_single_match_single_pattern( cx: &LateContext<'_>, ex: &Expr<'_>, @@ -752,9 +730,9 @@ fn report_single_match_single_pattern( }); let (msg, sugg) = if_chain! { - let (pat, pat_ref_count) = peel_pat_refs(arms[0].pat); + let (pat, pat_ref_count) = peel_hir_pat_refs(arms[0].pat); if let PatKind::Path(_) | PatKind::Lit(_) = pat.kind; - let (ty, ty_ref_count) = peel_ty_refs(cx.typeck_results().expr_ty(ex)); + let (ty, ty_ref_count) = peel_mid_ty_refs(cx.typeck_results().expr_ty(ex)); if let Some(trait_id) = cx.tcx.lang_items().structural_peq_trait(); if ty.is_integral() || ty.is_char() || ty.is_str() || implements_trait(cx, ty, trait_id, &[]); then { @@ -764,19 +742,28 @@ fn report_single_match_single_pattern( PatKind::Lit(Expr { kind: ExprKind::Lit(lit), .. }) if lit.node.is_str() => pat_ref_count + 1, _ => pat_ref_count, }; - let msg = "you seem to be trying to use match for an equality check. Consider using `if`"; + // References are only implicitly added to the pattern, so no overflow here. + // e.g. will work: match &Some(_) { Some(_) => () } + // will not: match Some(_) { &Some(_) => () } + let ref_count_diff = ty_ref_count - pat_ref_count; + + // Try to remove address of expressions first. + let (ex, removed) = peeln_hir_expr_refs(ex, ref_count_diff); + let ref_count_diff = ref_count_diff - removed; + + let msg = "you seem to be trying to use `match` for an equality check. Consider using `if`"; let sugg = format!( "if {} == {}{} {}{}", snippet(cx, ex.span, ".."), // PartialEq for different reference counts may not exist. - "&".repeat(ty_ref_count - pat_ref_count), + "&".repeat(ref_count_diff), snippet(cx, arms[0].pat.span, ".."), expr_block(cx, &arms[0].body, None, "..", Some(expr.span)), els_str, ); (msg, sugg) } else { - let msg = "you seem to be trying to use match for destructuring a single pattern. Consider using `if let`"; + let msg = "you seem to be trying to use `match` for destructuring a single pattern. Consider using `if let`"; let sugg = format!( "if let {} = {} {}{}", snippet(cx, arms[0].pat.span, ".."), diff --git a/clippy_lints/src/utils/mod.rs b/clippy_lints/src/utils/mod.rs index 8f54cad77283..8f8c681ecb71 100644 --- a/clippy_lints/src/utils/mod.rs +++ b/clippy_lints/src/utils/mod.rs @@ -1668,6 +1668,44 @@ where match_expr_list } +/// Peels off all references on the pattern. Returns the underlying pattern and the number of +/// references removed. +pub fn peel_hir_pat_refs(pat: &'a Pat<'a>) -> (&'a Pat<'a>, usize) { + fn peel(pat: &'a Pat<'a>, count: usize) -> (&'a Pat<'a>, usize) { + if let PatKind::Ref(pat, _) = pat.kind { + peel(pat, count + 1) + } else { + (pat, count) + } + } + peel(pat, 0) +} + +/// Peels off up to the given number of references on the expression. Returns the underlying +/// expression and the number of references removed. +pub fn peeln_hir_expr_refs(expr: &'a Expr<'a>, count: usize) -> (&'a Expr<'a>, usize) { + fn f(expr: &'a Expr<'a>, count: usize, target: usize) -> (&'a Expr<'a>, usize) { + match expr.kind { + ExprKind::AddrOf(_, _, expr) if count != target => f(expr, count + 1, target), + _ => (expr, count), + } + } + f(expr, 0, count) +} + +/// Peels off all references on the type. Returns the underlying type and the number of references +/// removed. +pub fn peel_mid_ty_refs(ty: Ty<'_>) -> (Ty<'_>, usize) { + fn peel(ty: Ty<'_>, count: usize) -> (Ty<'_>, usize) { + if let ty::Ref(_, ty, _) = ty.kind() { + peel(ty, count + 1) + } else { + (ty, count) + } + } + peel(ty, 0) +} + #[macro_export] macro_rules! unwrap_cargo_metadata { ($cx: ident, $lint: ident, $deps: expr) => {{ diff --git a/tests/ui/single_match.rs b/tests/ui/single_match.rs index 02266308fba2..ca884b41c457 100644 --- a/tests/ui/single_match.rs +++ b/tests/ui/single_match.rs @@ -81,7 +81,8 @@ fn single_match_know_enum() { } } -fn issue_173() { +// issue #173 +fn if_suggestion() { let x = "test"; match x { "test" => println!(), @@ -106,6 +107,18 @@ fn issue_173() { FOO_C => println!(), _ => (), } + + match &&x { + Foo::A => println!(), + _ => (), + } + + let x = &x; + match &x { + Foo::A => println!(), + _ => (), + } + enum Bar { A, B, diff --git a/tests/ui/single_match.stderr b/tests/ui/single_match.stderr index 5eca07ab1095..7ea6955b7401 100644 --- a/tests/ui/single_match.stderr +++ b/tests/ui/single_match.stderr @@ -1,4 +1,4 @@ -error: you seem to be trying to use match for destructuring a single pattern. Consider using `if let` +error: you seem to be trying to use `match` for destructuring a single pattern. Consider using `if let` --> $DIR/single_match.rs:8:5 | LL | / match x { @@ -17,7 +17,7 @@ LL | println!("{:?}", y); LL | }; | -error: you seem to be trying to use match for destructuring a single pattern. Consider using `if let` +error: you seem to be trying to use `match` for destructuring a single pattern. Consider using `if let` --> $DIR/single_match.rs:16:5 | LL | / match x { @@ -29,7 +29,7 @@ LL | | _ => (), LL | | } | |_____^ help: try this: `if let Some(y) = x { println!("{:?}", y) }` -error: you seem to be trying to use match for destructuring a single pattern. Consider using `if let` +error: you seem to be trying to use `match` for destructuring a single pattern. Consider using `if let` --> $DIR/single_match.rs:25:5 | LL | / match z { @@ -38,7 +38,7 @@ LL | | _ => {}, LL | | }; | |_____^ help: try this: `if let (2..=3, 7..=9) = z { dummy() }` -error: you seem to be trying to use match for destructuring a single pattern. Consider using `if let` +error: you seem to be trying to use `match` for destructuring a single pattern. Consider using `if let` --> $DIR/single_match.rs:54:5 | LL | / match x { @@ -47,7 +47,7 @@ LL | | None => (), LL | | }; | |_____^ help: try this: `if let Some(y) = x { dummy() }` -error: you seem to be trying to use match for destructuring a single pattern. Consider using `if let` +error: you seem to be trying to use `match` for destructuring a single pattern. Consider using `if let` --> $DIR/single_match.rs:59:5 | LL | / match y { @@ -56,7 +56,7 @@ LL | | Err(..) => (), LL | | }; | |_____^ help: try this: `if let Ok(y) = y { dummy() }` -error: you seem to be trying to use match for destructuring a single pattern. Consider using `if let` +error: you seem to be trying to use `match` for destructuring a single pattern. Consider using `if let` --> $DIR/single_match.rs:66:5 | LL | / match c { @@ -65,8 +65,8 @@ LL | | Cow::Owned(..) => (), LL | | }; | |_____^ help: try this: `if let Cow::Borrowed(..) = c { dummy() }` -error: you seem to be trying to use match for an equality check. Consider using `if` - --> $DIR/single_match.rs:86:5 +error: you seem to be trying to use `match` for an equality check. Consider using `if` + --> $DIR/single_match.rs:87:5 | LL | / match x { LL | | "test" => println!(), @@ -74,8 +74,8 @@ LL | | _ => (), LL | | } | |_____^ help: try this: `if x == "test" { println!() }` -error: you seem to be trying to use match for an equality check. Consider using `if` - --> $DIR/single_match.rs:99:5 +error: you seem to be trying to use `match` for an equality check. Consider using `if` + --> $DIR/single_match.rs:100:5 | LL | / match x { LL | | Foo::A => println!(), @@ -83,8 +83,8 @@ LL | | _ => (), LL | | } | |_____^ help: try this: `if x == Foo::A { println!() }` -error: you seem to be trying to use match for an equality check. Consider using `if` - --> $DIR/single_match.rs:105:5 +error: you seem to be trying to use `match` for an equality check. Consider using `if` + --> $DIR/single_match.rs:106:5 | LL | / match x { LL | | FOO_C => println!(), @@ -92,8 +92,26 @@ LL | | _ => (), LL | | } | |_____^ help: try this: `if x == FOO_C { println!() }` -error: you seem to be trying to use match for destructuring a single pattern. Consider using `if let` - --> $DIR/single_match.rs:121:5 +error: you seem to be trying to use `match` for an equality check. Consider using `if` + --> $DIR/single_match.rs:111:5 + | +LL | / match &&x { +LL | | Foo::A => println!(), +LL | | _ => (), +LL | | } + | |_____^ help: try this: `if x == Foo::A { println!() }` + +error: you seem to be trying to use `match` for an equality check. Consider using `if` + --> $DIR/single_match.rs:117:5 + | +LL | / match &x { +LL | | Foo::A => println!(), +LL | | _ => (), +LL | | } + | |_____^ help: try this: `if x == &Foo::A { println!() }` + +error: you seem to be trying to use `match` for destructuring a single pattern. Consider using `if let` + --> $DIR/single_match.rs:134:5 | LL | / match x { LL | | Bar::A => println!(), @@ -101,5 +119,5 @@ LL | | _ => (), LL | | } | |_____^ help: try this: `if let Bar::A = x { println!() }` -error: aborting due to 10 previous errors +error: aborting due to 12 previous errors diff --git a/tests/ui/single_match_else.stderr b/tests/ui/single_match_else.stderr index 3a07c2ec5426..20be4fa226cf 100644 --- a/tests/ui/single_match_else.stderr +++ b/tests/ui/single_match_else.stderr @@ -1,4 +1,4 @@ -error: you seem to be trying to use match for destructuring a single pattern. Consider using `if let` +error: you seem to be trying to use `match` for destructuring a single pattern. Consider using `if let` --> $DIR/single_match_else.rs:14:5 | LL | / match ExprNode::Butterflies { @@ -19,7 +19,7 @@ LL | None LL | } | -error: you seem to be trying to use match for destructuring a single pattern. Consider using `if let` +error: you seem to be trying to use `match` for destructuring a single pattern. Consider using `if let` --> $DIR/single_match_else.rs:70:5 | LL | / match Some(1) { @@ -39,7 +39,7 @@ LL | return LL | } | -error: you seem to be trying to use match for destructuring a single pattern. Consider using `if let` +error: you seem to be trying to use `match` for destructuring a single pattern. Consider using `if let` --> $DIR/single_match_else.rs:79:5 | LL | / match Some(1) { From 36ff2f739c62f81d5ecc1850d9f3354d15de928d Mon Sep 17 00:00:00 2001 From: Jason Newcomb Date: Thu, 14 Jan 2021 22:02:04 -0500 Subject: [PATCH 3/3] Rename function --- clippy_lints/src/matches.rs | 4 ++-- clippy_lints/src/utils/mod.rs | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/clippy_lints/src/matches.rs b/clippy_lints/src/matches.rs index 6ecd738d2f0b..02021b873695 100644 --- a/clippy_lints/src/matches.rs +++ b/clippy_lints/src/matches.rs @@ -4,7 +4,7 @@ use crate::utils::usage::is_unused; use crate::utils::{ expr_block, get_arg_name, get_parent_expr, implements_trait, in_macro, indent_of, is_allowed, is_expn_of, is_refutable, is_type_diagnostic_item, is_wild, match_qpath, match_type, match_var, meets_msrv, multispan_sugg, - peel_hir_pat_refs, peel_mid_ty_refs, peeln_hir_expr_refs, remove_blocks, snippet, snippet_block, snippet_opt, + peel_hir_pat_refs, peel_mid_ty_refs, peel_n_hir_expr_refs, remove_blocks, snippet, snippet_block, snippet_opt, snippet_with_applicability, span_lint_and_help, span_lint_and_note, span_lint_and_sugg, span_lint_and_then, }; use crate::utils::{paths, search_same, SpanlessEq, SpanlessHash}; @@ -748,7 +748,7 @@ fn report_single_match_single_pattern( let ref_count_diff = ty_ref_count - pat_ref_count; // Try to remove address of expressions first. - let (ex, removed) = peeln_hir_expr_refs(ex, ref_count_diff); + let (ex, removed) = peel_n_hir_expr_refs(ex, ref_count_diff); let ref_count_diff = ref_count_diff - removed; let msg = "you seem to be trying to use `match` for an equality check. Consider using `if`"; diff --git a/clippy_lints/src/utils/mod.rs b/clippy_lints/src/utils/mod.rs index 8f8c681ecb71..f81bf088ec4c 100644 --- a/clippy_lints/src/utils/mod.rs +++ b/clippy_lints/src/utils/mod.rs @@ -1683,7 +1683,7 @@ pub fn peel_hir_pat_refs(pat: &'a Pat<'a>) -> (&'a Pat<'a>, usize) { /// Peels off up to the given number of references on the expression. Returns the underlying /// expression and the number of references removed. -pub fn peeln_hir_expr_refs(expr: &'a Expr<'a>, count: usize) -> (&'a Expr<'a>, usize) { +pub fn peel_n_hir_expr_refs(expr: &'a Expr<'a>, count: usize) -> (&'a Expr<'a>, usize) { fn f(expr: &'a Expr<'a>, count: usize, target: usize) -> (&'a Expr<'a>, usize) { match expr.kind { ExprKind::AddrOf(_, _, expr) if count != target => f(expr, count + 1, target),