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.

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

Jump to bottom

Fix redundant_pattern_matching lint #5511

Merged
merged 1 commit into from
Apr 25, 2020
Merged

Fix redundant_pattern_matching lint #5511

merged 1 commit into from
Apr 25, 2020

Conversation

alex-700
Copy link
Contributor

fixes #5504

changelog: Fix suggestion in redundant_pattern_matching for macros.

@alex-700 alex-700 force-pushed the fix-redundant-pattern-matching branch from 856bb34 to ef9c38c Compare April 23, 2020 08:54
@phansch phansch self-requested a review April 23, 2020 11:16
@phansch
Copy link
Member

phansch commented Apr 23, 2020

@bors r+ thanks!

@bors
Copy link
Contributor

bors commented Apr 23, 2020

📌 Commit ef9c38c has been approved by phansch

@bors
Copy link
Contributor

bors commented Apr 23, 2020

⌛ Testing commit ef9c38c with merge af9212c...

bors added a commit that referenced this pull request Apr 23, 2020
@bors
Auto merge of #5511 - alex-700:fix-redundant-pattern-matching, r=phansch
af9212c 
Fix redundant_pattern_matching lint

fixes #5504

changelog: Fix suggestion in `redundant_pattern_matching` for macros.
@bors
Copy link
Contributor

bors commented Apr 23, 2020

💔 Test failed - checks-action_test

@alex-700
Copy link
Contributor Author

github was unavailable, so smb need to retry. Hm, interesting, do I have rights to do it.
@bors retry

@phansch
Copy link
Member

phansch commented Apr 23, 2020

@bors retry

@bors
Copy link
Contributor

bors commented Apr 23, 2020

@alex-700: 🔑 Insufficient privileges: not in try users

@bors
Copy link
Contributor

bors commented Apr 23, 2020

⌛ Testing commit ef9c38c with merge cce8841...

bors added a commit that referenced this pull request Apr 23, 2020
@bors
Auto merge of #5511 - alex-700:fix-redundant-pattern-matching, r=phansch
cce8841 
Fix redundant_pattern_matching lint

fixes #5504

changelog: Fix suggestion in `redundant_pattern_matching` for macros.
@bors
Copy link
Contributor

bors commented Apr 23, 2020

💔 Test failed - checks-action_dev_test

@alex-700 alex-700 requested a review from phansch April 23, 2020 15:28
@phansch
Copy link
Member

phansch commented Apr 23, 2020

Let's try again =)

@bors retry

@bors
Copy link
Contributor

bors commented Apr 23, 2020

⌛ Testing commit ef9c38c with merge 241cc58...

bors added a commit that referenced this pull request Apr 23, 2020
@bors
Auto merge of #5511 - alex-700:fix-redundant-pattern-matching, r=phansch
241cc58 
Fix redundant_pattern_matching lint

fixes #5504

changelog: Fix suggestion in `redundant_pattern_matching` for macros.
@flip1995 flip1995 added the S-waiting-on-bors Status: The marked PR was approved and is only waiting bors label Apr 23, 2020
flip1995
flip1995 requested changes Apr 23, 2020
View reviewed changes
Copy link
Member

@flip1995 flip1995 left a comment

Choose a reason for hiding this comment

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

I have to take a closer look at the span situation. I think I finally understood what @alex-700 meant in the discussion in the issue and that there is indeed a bug in the lowering.

clippy_lints/src/redundant_pattern_matching.rs Outdated
Comment on lines 106 to 108
// while let ... = ... { ... }
// ^^^^^^^^^^^^^^^^^^^^^^^^^^^
let expr_span = arms[1].pat.span;
Copy link
Member

Choose a reason for hiding this comment

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

Wait, what? I don't trust this at all. There is no test for if let ... {} else if let {}. I think this could break.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this could break.

I don't think so. I'll add this test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@flip1995
Copy link
Member

@bors r-

@alex-700 alex-700 force-pushed the fix-redundant-pattern-matching branch 2 times, most recently from 8fa9d63 to 6b256aa Compare April 23, 2020 17:03
@flip1995
Copy link
Member

rust-lang/rust#71494

@flip1995
Copy link
Member

@alex-700 The expr.span should now be as expected, can you change the code accordingly? (you may have to update your master toolchain locally)

@flip1995 flip1995 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-bors Status: The marked PR was approved and is only waiting bors labels Apr 25, 2020
@alex-700
fix redundant_pattern_matching lint
69fe6b4 
- now it gives correct suggestion in case of macros
- better tests
- remove a couple of non-relevant tests
@alex-700 alex-700 force-pushed the fix-redundant-pattern-matching branch from 6b256aa to 69fe6b4 Compare April 25, 2020 20:51
@alex-700 alex-700 requested a review from flip1995 April 25, 2020 21:34
@flip1995
Copy link
Member

Now that suggestion building looks more trustworthy and stable. Thanks! :)

@bors r+

@bors
Copy link
Contributor

bors commented Apr 25, 2020

📌 Commit 69fe6b4 has been approved by flip1995

@bors
Copy link
Contributor

bors commented Apr 25, 2020

⌛ Testing commit 69fe6b4 with merge 07dd5fa...

@bors
Copy link
Contributor

bors commented Apr 25, 2020

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: flip1995
Pushing 07dd5fa to master...

@bors bors merged commit 07dd5fa into rust-lang:master Apr 25, 2020
@alex-700 alex-700 deleted the fix-redundant-pattern-matching branch December 29, 2020 11:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@flip1995 flip1995 flip1995 approved these changes

@phansch phansch Awaiting requested review from phansch

Assignees
No one assigned
Labels
S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

FP redundant_pattern_matching: try! macro
4 participants
@alex-700 @phansch @bors @flip1995