Skip to content
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

redundant_pattern_matching #10753

Merged
merged 10 commits into from
May 18, 2023
Merged

redundant_pattern_matching #10753

merged 10 commits into from
May 18, 2023

Conversation

disco07
Copy link
Contributor

@disco07 disco07 commented May 5, 2023

This PR try to solve this issue #10726,
but it enter in conflict with another test.

changelog: none

Try to test this:

let _w = match x {
     Some(_) => true,
     _ => false,
};

this happen:

 error: match expression looks like `matches!` macro
   --> $DIR/match_expr_like_matches_macro.rs:21:14
    |
 LL |       let _w = match x {
    |  ______________^
 LL | |         Some(_) => true,
 LL | |         _ => false,
 LL | |     };
    | |_____^ help: try this: `matches!(x, Some(_))`
 
+error: redundant pattern matching, consider using `is_some()`
+  --> $DIR/match_expr_like_matches_macro.rs:21:14
+   |
+LL |       let _w = match x {
+   |  ______________^
+LL | |         Some(_) => true,
+LL | |         _ => false,
+LL | |     };
+   | |_____^ help: try this: `x.is_some()`
+   |
+   = note: `-D clippy::redundant-pattern-matching` implied by `-D warnings`
+

I need some help to fix this. Thanks

@rustbot
Copy link
Collaborator

rustbot commented May 5, 2023

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @dswij (or someone else) soon.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

  • @rustbot author: the review is finished, PR author should check the comments and take action accordingly
  • @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label May 5, 2023
@disco07
Copy link
Contributor Author

disco07 commented May 5, 2023

@xFrednet can you help me please ?

@xFrednet
Copy link
Member

xFrednet commented May 5, 2023

@xFrednet can you help me please ?

Sure, I'm guessing, your question is how to prevent the other lint from triggering in this case? In the test files, you could just use #[allow(other_lint)], but that wouldn't be the ideal solution here, as the user would still see two lints. Instead, you can take a look at is_lint_allowed. In this case, I recommend adding a check to match_expr_like_matches_macro, that suppresses the lint, if redundant_pattern_matching is currently allowed.

Does this explanation make sense? :)

@disco07
Copy link
Contributor Author

disco07 commented May 5, 2023

@xFrednet can you help me please ?

Sure, I'm guessing, your question is how to prevent the other lint from triggering in this case? In the test files, you could just use #[allow(other_lint)], but that wouldn't be the ideal solution here, as the user would still see two lints. Instead, you can take a look at is_lint_allowed. In this case, I recommend adding a check to match_expr_like_matches_macro, that suppresses the lint, if redundant_pattern_matching is currently allowed.

Does this explanation make sense? :)

Thank you so much I will try this :-)

@disco07
Copy link
Contributor Author

disco07 commented May 8, 2023

@xFrednet Can you check my changes if you have some time please :)
Thanks in advance

@xFrednet
Copy link
Member

xFrednet commented May 9, 2023

Sure, I'll take a look at it later today or tomorrow. :)

r? @xFrednet

@rustbot rustbot assigned xFrednet and unassigned dswij May 9, 2023
Copy link
Member

@xFrednet xFrednet left a comment

Choose a reason for hiding this comment

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

This already looks good overall, I have a few NITs, where I would appreciate it if you could update them. But then it'll be ready to be merged :)

tests/ui/redundant_pattern_matching_option.rs Outdated Show resolved Hide resolved
tests/ui/redundant_pattern_matching_option.rs Outdated Show resolved Hide resolved
tests/ui/redundant_pattern_matching_result.rs Outdated Show resolved Hide resolved
clippy_lints/src/matches/match_like_matches.rs Outdated Show resolved Hide resolved
clippy_lints/src/matches/match_like_matches.rs Outdated Show resolved Hide resolved
clippy_lints/src/matches/redundant_pattern_match.rs Outdated Show resolved Hide resolved
@disco07
Copy link
Contributor Author

disco07 commented May 9, 2023

Thanks @xFrednet :)

Copy link
Member

@xFrednet xFrednet left a comment

Choose a reason for hiding this comment

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

Thank you for the swift update. I still have a NIT when it comes to the tests, but that should be a simple update. Once that is done, we can merge it. Could you also squash your fix commits, to only have one or two?


PS, I'll be gone over the weekend, so the next review will be done some time next week :)

tests/ui/redundant_pattern_matching_result.rs Outdated Show resolved Hide resolved
Copy link
Member

@xFrednet xFrednet left a comment

Choose a reason for hiding this comment

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

The Option tests still need some adjustments. Then it should be ready.

Have you seen my request to squash your commits, from the last review?

Could you also squash your fix commits, to only have one or two?

tests/ui/redundant_pattern_matching_option.rs Outdated Show resolved Hide resolved
tests/ui/redundant_pattern_matching_result.rs Show resolved Hide resolved
@disco07
Copy link
Contributor Author

disco07 commented May 15, 2023

The Option tests still need some adjustments. Then it should be ready.

Have you seen my request to squash your commits, from the last review?

Could you also squash your fix commits, to only have one or two?

I hope I made the right changes this time ;)

@xFrednet
Copy link
Member

This version looks good to me, thank you for the PR. I hope you had fun working on Clippy :)

@bors r+

@bors
Copy link
Contributor

bors commented May 16, 2023

📌 Commit 79a8867 has been approved by xFrednet

It is now in the queue for this repository.

bors added a commit that referenced this pull request May 18, 2023
redundant_pattern_matching

This PR try to solve this issue #10726,
but it enter in conflict with another test.

Try to test this:
```
let _w = match x {
     Some(_) => true,
     _ => false,
};
```

this happen:
```
 error: match expression looks like `matches!` macro
   --> $DIR/match_expr_like_matches_macro.rs:21:14
    |
 LL |       let _w = match x {
    |  ______________^
 LL | |         Some(_) => true,
 LL | |         _ => false,
 LL | |     };
    | |_____^ help: try this: `matches!(x, Some(_))`

+error: redundant pattern matching, consider using `is_some()`
+  --> $DIR/match_expr_like_matches_macro.rs:21:14
+   |
+LL |       let _w = match x {
+   |  ______________^
+LL | |         Some(_) => true,
+LL | |         _ => false,
+LL | |     };
+   | |_____^ help: try this: `x.is_some()`
+   |
+   = note: `-D clippy::redundant-pattern-matching` implied by `-D warnings`
+
```
I need some help to fix this. Thanks
@bors
Copy link
Contributor

bors commented May 18, 2023

⌛ Testing commit 79a8867 with merge 9aa84ba...

@bors
Copy link
Contributor

bors commented May 18, 2023

💔 Test failed - checks-action_test

@bors
Copy link
Contributor

bors commented May 18, 2023

@disco07: 🔑 Insufficient privileges: not in try users

@disco07
Copy link
Contributor Author

disco07 commented May 18, 2023

@xFrednet I update my PR to add changelog: none

@xFrednet
Copy link
Member

No problem :)

@bors retry

@bors
Copy link
Contributor

bors commented May 18, 2023

⌛ Testing commit 79a8867 with merge 22b3196...

@bors
Copy link
Contributor

bors commented May 18, 2023

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: xFrednet
Pushing 22b3196 to master...

@bors bors merged commit 22b3196 into rust-lang:master May 18, 2023
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.

5 participants