Skip to content

Commit

Permalink
Fix needless_match FP on if-lets (#13646)
Browse files Browse the repository at this point in the history
Closes #13574

Make sure that `needless_match` doesn't simplify:

```
if let Some(_) = a() {
// ..
} else let Some(_) = b() {
// ..
}
```

to:

```
a()
```

changelog: [`needless_match`]: Fix false-positive on if lets
  • Loading branch information
Centri3 authored Dec 2, 2024
2 parents 0a1ba2c + febe549 commit 278e316
Show file tree
Hide file tree
Showing 4 changed files with 131 additions and 3 deletions.
6 changes: 4 additions & 2 deletions clippy_lints/src/matches/needless_match.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ use clippy_utils::diagnostics::span_lint_and_sugg;
use clippy_utils::source::snippet_with_applicability;
use clippy_utils::ty::{is_type_diagnostic_item, same_type_and_consts};
use clippy_utils::{
eq_expr_value, get_parent_expr_for_hir, higher, is_else_clause, is_res_lang_ctor, over, path_res,
SpanlessEq, eq_expr_value, get_parent_expr_for_hir, higher, is_else_clause, is_res_lang_ctor, over, path_res,
peel_blocks_with_stmt,
};
use rustc_errors::Applicability;
Expand Down Expand Up @@ -90,7 +90,9 @@ fn check_if_let_inner(cx: &LateContext<'_>, if_let: &higher::IfLet<'_>) -> bool
}

// Recursively check for each `else if let` phrase,
if let Some(ref nested_if_let) = higher::IfLet::hir(cx, if_else) {
if let Some(ref nested_if_let) = higher::IfLet::hir(cx, if_else)
&& SpanlessEq::new(cx).eq_expr(nested_if_let.let_expr, if_let.let_expr)
{
return check_if_let_inner(cx, nested_if_let);
}

Expand Down
53 changes: 53 additions & 0 deletions tests/ui/needless_match.fixed
Original file line number Diff line number Diff line change
Expand Up @@ -245,4 +245,57 @@ mod issue9084 {
}
}

fn a() -> Option<()> {
Some(())
}
fn b() -> Option<()> {
Some(())
}
fn c() -> Option<()> {
Some(())
}

#[allow(clippy::ifs_same_cond)]
pub fn issue13574() -> Option<()> {
// Do not lint.
// The right hand of all these arms are different functions.
let _ = {
if let Some(a) = a() {
Some(a)
} else if let Some(b) = b() {
Some(b)
} else if let Some(c) = c() {
Some(c)
} else {
None
}
};

const A: Option<()> = Some(());
const B: Option<()> = Some(());
const C: Option<()> = Some(());
const D: Option<()> = Some(());

let _ = {
if let Some(num) = A {
Some(num)
} else if let Some(num) = B {
Some(num)
} else if let Some(num) = C {
Some(num)
} else if let Some(num) = D {
Some(num)
} else {
None
}
};

// Same const, should lint
let _ = {
A
};

None
}

fn main() {}
61 changes: 61 additions & 0 deletions tests/ui/needless_match.rs
Original file line number Diff line number Diff line change
Expand Up @@ -289,4 +289,65 @@ mod issue9084 {
}
}

fn a() -> Option<()> {
Some(())
}
fn b() -> Option<()> {
Some(())
}
fn c() -> Option<()> {
Some(())
}

#[allow(clippy::ifs_same_cond)]
pub fn issue13574() -> Option<()> {
// Do not lint.
// The right hand of all these arms are different functions.
let _ = {
if let Some(a) = a() {
Some(a)
} else if let Some(b) = b() {
Some(b)
} else if let Some(c) = c() {
Some(c)
} else {
None
}
};

const A: Option<()> = Some(());
const B: Option<()> = Some(());
const C: Option<()> = Some(());
const D: Option<()> = Some(());

let _ = {
if let Some(num) = A {
Some(num)
} else if let Some(num) = B {
Some(num)
} else if let Some(num) = C {
Some(num)
} else if let Some(num) = D {
Some(num)
} else {
None
}
};

// Same const, should lint
let _ = {
if let Some(num) = A {
Some(num)
} else if let Some(num) = A {
Some(num)
} else if let Some(num) = A {
Some(num)
} else {
None
}
};

None
}

fn main() {}
14 changes: 13 additions & 1 deletion tests/ui/needless_match.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -131,5 +131,17 @@ LL | | _ => e,
LL | | };
| |_________^ help: replace it with: `e`

error: aborting due to 13 previous errors
error: this if-let expression is unnecessary
--> tests/ui/needless_match.rs:339:9
|
LL | / if let Some(num) = A {
LL | | Some(num)
LL | | } else if let Some(num) = A {
LL | | Some(num)
... |
LL | | None
LL | | }
| |_________^ help: replace it with: `A`

error: aborting due to 14 previous errors

0 comments on commit 278e316

Please sign in to comment.