Skip to content

Commit

Permalink
Auto merge of #6574 - Jarcho:single_match_eq, r=Manishearth
Browse files Browse the repository at this point in the history
single_match: suggest `if` over `if let` when possible

fixes: #173
changelog: single_match: suggest `if` over `if let` when possible
  • Loading branch information
bors committed Jan 15, 2021
2 parents 9490fdc + 36ff2f7 commit 2d1e129
Show file tree
Hide file tree
Showing 5 changed files with 211 additions and 23 deletions.
66 changes: 53 additions & 13 deletions clippy_lints/src/matches.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
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};
use if_chain::if_chain;
Expand Down Expand Up @@ -728,20 +728,60 @@ 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_hir_pat_refs(arms[0].pat);
if let PatKind::Path(_) | PatKind::Lit(_) = pat.kind;
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 {
// 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,
};
// 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) = 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`";
let sugg = format!(
"if {} == {}{} {}{}",
snippet(cx, ex.span, ".."),
// PartialEq for different reference counts may not exist.
"&".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 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,
);
}
Expand Down
38 changes: 38 additions & 0 deletions clippy_lints/src/utils/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1669,6 +1669,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 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),
_ => (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) => {{
Expand Down
56 changes: 56 additions & 0 deletions tests/ui/single_match.rs
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,62 @@ fn single_match_know_enum() {
}
}

// issue #173
fn if_suggestion() {
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!(),
_ => (),
}

match &&x {
Foo::A => println!(),
_ => (),
}

let x = &x;
match &x {
Foo::A => 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 {
Expand Down
68 changes: 61 additions & 7 deletions tests/ui/single_match.stderr
Original file line number Diff line number Diff line change
@@ -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 {
Expand All @@ -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 {
Expand All @@ -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 {
Expand All @@ -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 {
Expand All @@ -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 {
Expand All @@ -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 {
Expand All @@ -65,5 +65,59 @@ 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:87: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:100: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:106: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 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!(),
LL | | _ => (),
LL | | }
| |_____^ help: try this: `if let Bar::A = x { println!() }`

error: aborting due to 12 previous errors

6 changes: 3 additions & 3 deletions tests/ui/single_match_else.stderr
Original file line number Diff line number Diff line change
@@ -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 {
Expand All @@ -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) {
Expand All @@ -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) {
Expand Down

0 comments on commit 2d1e129

Please sign in to comment.