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.

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

Conversation

tamaroning
Copy link
Contributor

@tamaroning tamaroning commented Jul 2, 2022

closes: #9084
made needless_match take into account arm in the form of _ if => ...

changelog: none

@rust-highfive
Copy link

r? @llogiq

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Jul 2, 2022
@@ -66,7 +66,9 @@ fn check_all_arms(cx: &LateContext<'_>, match_expr: &Expr<'_>, arms: &[Arm<'_>])
for arm in arms {
let arm_expr = peel_blocks_with_stmt(arm.body);
if let PatKind::Wild = arm.pat.kind {
Copy link
Contributor

@Jarcho Jarcho Jul 18, 2022

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

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.

@tamaroning
Copy link
Contributor Author

thank you for your review @Jarcho .
I am a little busy until 8/1 around so I will fix this later.

@llogiq llogiq added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Jul 25, 2022
@tamaroning
Copy link
Contributor Author

@rustbot label -S-waiting-on-author +S-waiting-on-review

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties and removed S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Aug 11, 2022
@tamaroning
Copy link
Contributor Author

hmm..test failed though it passes in the local environment.

@Jarcho
Copy link
Contributor

Jarcho commented Aug 11, 2022

Try rebasing. Possibly a conflict with #8356.

@llogiq
Copy link
Contributor

llogiq commented Aug 21, 2022

I think you just need to bless the needless_match ui test again ( cargo dev bless) and commit+push the resulting diff.

(And sorry for being away for so long, thanks to @Jarcho for picking up the slack)

@tamaroning
Copy link
Contributor Author

Thank you. My commit somehow does not contained .stderr but fixed :)

@llogiq
Copy link
Contributor

llogiq commented Aug 21, 2022

Thank you!

@bors r+

@bors
Copy link
Contributor

bors commented Aug 21, 2022

📌 Commit f7a376e has been approved by llogiq

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Aug 21, 2022

⌛ Testing commit f7a376e with merge cc637ba...

@bors
Copy link
Contributor

bors commented Aug 21, 2022

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: llogiq
Pushing cc637ba to master...

@bors bors merged commit cc637ba into rust-lang:master Aug 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

needless_match false positive
6 participants