Skip to content

Fix false positives of needless_match #9092

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Aug 21, 2022
Merged

Fix false positives of needless_match #9092

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
459821b
fix
tamaroning Jul 2, 2022
45084ee
give up when gurad has side effects
tamaroning Aug 11, 2022
f7a376e
Update needless_match.stderr
tamaroning Aug 21, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 20 additions & 2 deletions clippy_lints/src/matches/needless_match.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ use clippy_utils::{
};
use rustc_errors::Applicability;
use rustc_hir::LangItem::OptionNone;
use rustc_hir::{Arm, BindingAnnotation, Expr, ExprKind, FnRetTy, Node, Pat, PatKind, Path, QPath};
use rustc_hir::{Arm, BindingAnnotation, Expr, ExprKind, FnRetTy, Guard, Node, Pat, PatKind, Path, QPath};
use rustc_lint::LateContext;
use rustc_span::sym;
use rustc_typeck::hir_ty_to_ty;
Expand Down Expand Up @@ -65,8 +65,26 @@ pub(crate) fn check_if_let<'tcx>(cx: &LateContext<'tcx>, ex: &Expr<'_>, if_let:
fn check_all_arms(cx: &LateContext<'_>, match_expr: &Expr<'_>, arms: &[Arm<'_>]) -> bool {
for arm in arms {
let arm_expr = peel_blocks_with_stmt(arm.body);

if let Some(guard_expr) = &arm.guard {
match guard_expr {
// gives up if `pat if expr` can have side effects
Guard::If(if_cond) => {
if if_cond.can_have_side_effects() {
return false;
}
},
// gives up `pat if let ...` arm
Guard::IfLet(_) => {
return false;
},
};
}

if let PatKind::Wild = arm.pat.kind {
Copy link
Contributor

@Jarcho Jarcho Jul 18, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should also return false for arm.guard.is_some()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Jarcho
I think my code already works for arms with if guards.
If not, could you raise some cases my code dosen't catch?

  • pat => expr_same_as_pat
  • pat if some_expr => expr_same_as_pat
  • _ => scrutinee
  • _ if some_expr => scrutinee
    For now, other cases than these four returns false.

Copy link
Contributor

@Jarcho Jarcho Aug 11, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Match guards can have side-effects in them. Horrible example:

match 0 {
    0 if { println!("guard"); false } => 0,
    _ => 0,
}

Expr::can_have_side_effects would also solve the problem.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ohhh...I forgot side effects.
OKay. I will use Expr::can_have_side_effects.

return eq_expr_value(cx, match_expr, strip_return(arm_expr));
if !eq_expr_value(cx, match_expr, strip_return(arm_expr)) {
return false;
}
} else if !pat_same_as_expr(arm.pat, arm_expr) {
return false;
}
Expand Down
39 changes: 39 additions & 0 deletions tests/ui/needless_match.fixed
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -207,4 +207,43 @@ impl Tr for Result<i32, i32> {
}
}

mod issue9084 {
fn wildcard_if() {
let mut some_bool = true;
let e = Some(1);

// should lint
let _ = e;

// should lint
let _ = e;

// should not lint
let _ = match e {
_ if some_bool => e,
_ => Some(2),
};

// should not lint
let _ = match e {
Some(i) => Some(i + 1),
_ if some_bool => e,
_ => e,
};

// should not lint (guard has side effects)
let _ = match e {
Some(i) => Some(i),
_ if {
some_bool = false;
some_bool
} =>
{
e
},
_ => e,
};
}
}

fn main() {}
46 changes: 46 additions & 0 deletions tests/ui/needless_match.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -244,4 +244,50 @@ impl Tr for Result<i32, i32> {
}
}

mod issue9084 {
fn wildcard_if() {
let mut some_bool = true;
let e = Some(1);

// should lint
let _ = match e {
_ if some_bool => e,
_ => e,
};

// should lint
let _ = match e {
Some(i) => Some(i),
_ if some_bool => e,
_ => e,
};

// should not lint
let _ = match e {
_ if some_bool => e,
_ => Some(2),
};

// should not lint
let _ = match e {
Some(i) => Some(i + 1),
_ if some_bool => e,
_ => e,
};

// should not lint (guard has side effects)
let _ = match e {
Some(i) => Some(i),
_ if {
some_bool = false;
some_bool
} =>
{
e
},
_ => e,
};
}
}

fn main() {}
23 changes: 22 additions & 1 deletion tests/ui/needless_match.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,5 +109,26 @@ LL | | Complex::D(E::VariantB(ea, eb), b) => Complex::D(E::VariantB(
LL | | };
| |_________^ help: replace it with: `ce`

error: aborting due to 11 previous errors
error: this match expression is unnecessary
--> $DIR/needless_match.rs:253:17
|
LL | let _ = match e {
| _________________^
LL | | _ if some_bool => e,
LL | | _ => e,
LL | | };
| |_________^ help: replace it with: `e`

error: this match expression is unnecessary
--> $DIR/needless_match.rs:259:17
|
LL | let _ = match e {
| _________________^
LL | | Some(i) => Some(i),
LL | | _ if some_bool => e,
LL | | _ => e,
LL | | };
| |_________^ help: replace it with: `e`

error: aborting due to 13 previous errors