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 drop order #6568

Merged
merged 2 commits into from
Apr 16, 2021
Merged

Fix: redundant_pattern_matching drop order #6568

merged 2 commits into from
Apr 16, 2021

Conversation

Jarcho
Copy link
Contributor

@Jarcho Jarcho commented Jan 8, 2021

Fixes #5746

A note about the change in drop order is added when the scrutinee (or any temporary in the expression) isn't known to be safe to drop in any order (i.e. doesn't implement the Drop trait, or contain such a type). There is a whitelist for some std types, but it's incomplete. Currently just Vec<_>, Box<_>, Rc<_> and Arc<_>, but only if the contained type is also safe to drop in any order.

Another lint for when the drop order changes could be added as allowed by default, but the drop order requirement is pretty subtle in this case. I think the note added to the lint should be enough to make someone think before applying the change.

changelog: Added a note to redundant_pattern_matching when the change in drop order might matter

@rust-highfive
Copy link

r? @flip1995

(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 Jan 8, 2021
@Jarcho Jarcho changed the title Redundant pattern matching Fix: redundant_pattern_matching drop order Jan 8, 2021
@giraffate
Copy link
Contributor

The test seems to fail because redundant_pattern_matching_result.stderr file is too long. How about splitting the test file into two?

flip1995
flip1995 reviewed Jan 17, 2021
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'm totally lost. I don't understand what any of this code does. Please add more (doc) comments to the functions better describing it.

The only thing I found reviewing this is that the get_temporary_tys may be better implemented by a visitor.

@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-review Status: Awaiting review from the assignee but also interested parties labels Jan 17, 2021
@giraffate
Copy link
Contributor

ping from triage @Jarcho. Could you have any update on this?

@giraffate
Copy link
Contributor

ping from triage @Jarcho. According to the triage procedure, I'm closing this because 2 weeks have passed with no activity. If you have more time to work on this, feel free to reopen this.

@giraffate giraffate closed this Feb 19, 2021
@giraffate giraffate added S-inactive-closed Status: Closed due to inactivity and removed S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Feb 19, 2021
@Jarcho
Copy link
Contributor Author

Jarcho commented Feb 19, 2021

Great timing, I just went through and cleaned it up. I don't have the ability to re-open.

@giraffate giraffate reopened this Feb 19, 2021
@giraffate giraffate added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-inactive-closed Status: Closed due to inactivity labels Feb 19, 2021
@giraffate
Copy link
Contributor

@Jarcho Thanks! I reopened this.

@giraffate
Copy link
Contributor

ping from triage @Jarcho. Could you have any update on this?

@bors
Copy link
Contributor

bors commented Mar 12, 2021

☔ The latest upstream changes (presumably #6891) made this pull request unmergeable. Please resolve the merge conflicts.

@Jarcho
Copy link
Contributor Author

Jarcho commented Mar 19, 2021

@flip1995 Anything else to point out?

@flip1995
Copy link
Member

I'll have to take another look at this PR.

@flip1995 flip1995 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 Mar 19, 2021
flip1995
flip1995 reviewed Apr 1, 2021
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.

Implementation LGTM. I have 2 questions remaining.

clippy_lints/src/matches.rs Outdated Show resolved Hide resolved
clippy_lints/src/matches.rs Outdated Show resolved Hide resolved
@Jarcho Jarcho force-pushed the redundant_pattern_matching branch 3 times, most recently from 3634c91 to ee9188f Compare April 3, 2021 12:27
@bors
Copy link
Contributor

bors commented Apr 6, 2021

☔ The latest upstream changes (presumably #6931) made this pull request unmergeable. Please resolve the merge conflicts.

@Jarcho Jarcho force-pushed the redundant_pattern_matching branch from ee9188f to 7ecaf58 Compare April 8, 2021 12:16
@bors
Copy link
Contributor

bors commented Apr 12, 2021

☔ The latest upstream changes (presumably #7047) made this pull request unmergeable. Please resolve the merge conflicts.

flip1995
flip1995 approved these changes Apr 12, 2021
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.

LGTM. Needs a rebase and a comment on the type_needs_ordered drop with the explanation you provided in the discussion above.

@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-review Status: Awaiting review from the assignee but also interested parties labels Apr 12, 2021
@Jarcho Jarcho force-pushed the redundant_pattern_matching branch from 7ecaf58 to e4d402f Compare April 15, 2021 02:06
@bors
Copy link
Contributor

bors commented Apr 16, 2021

☔ The latest upstream changes (presumably #7049) made this pull request unmergeable. Please resolve the merge conflicts.

@Jarcho
Improve redundant_pattern_matching
b658163 
Add a note when the drop order change may result in different behaviour.
@Jarcho
redundant_pattern_matching fix inverted boolean when missing Drop…
c02baba 
… trait # Please enter the commit message for your changes. Lines starting
# with '#' will be ignored, and an empty message aborts the commit.
#
# Date: Fri Apr 2 19:04:45 2021 -0400
#
# On branch redundant_pattern_matching
# Your branch is ahead of 'origin/redundant_pattern_matching' by 1 commit.
# (use "git push" to publish your local commits)
#
# Changes to be committed:
#	modified: clippy_lints/src/matches.rs
# # Please enter the commit message for your changes. Lines starting
# with '#' will be ignored, and an empty message aborts the commit.
#
# Date: Fri Apr 2 19:04:45 2021 -0400
#
# interactive rebase in progress; onto ebc6469
# Last commands done (6 commands done):
#    pick 25d211a Code cleanup and additional std types checked
#    r 0c71ce5 `redundant_pattern_matching` fix inverted boolean when missing `Drop` trait
# No commands remaining.
# You are currently editing a commit while rebasing branch 'redundant_pattern_matching' on 'ebc64690d'.
#
# Changes to be committed:
#	modified:   clippy_lints/src/matches.rs
#
@Jarcho Jarcho force-pushed the redundant_pattern_matching branch from e4d402f to c02baba Compare April 16, 2021 00:37
@flip1995
Copy link
Member

@bors r+

Thanks! Great job!

@bors
Copy link
Contributor

bors commented Apr 16, 2021

📌 Commit c02baba has been approved by flip1995

@bors
Copy link
Contributor

bors commented Apr 16, 2021

⌛ Testing commit c02baba with merge 7f2068c...

@bors
Copy link
Contributor

bors commented Apr 16, 2021

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

@bors bors merged commit 7f2068c into rust-lang:master Apr 16, 2021
@flip1995 flip1995 mentioned this pull request May 5, 2021
Closed
flip1995 added a commit to flip1995/rust-clippy that referenced this pull request May 5, 2021
@flip1995
Revert "Auto merge of rust-lang#6568 - Jarcho:redundant_pattern_match…
37b5e81 
…ing, r=flip1995"

This reverts commit 7f2068c, reversing
changes made to 1e0a3ff.
@flip1995 flip1995 mentioned this pull request May 5, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oli-obk oli-obk oli-obk left review comments

@flip1995 flip1995 flip1995 approved these changes

Assignees

@flip1995 flip1995

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.

Some(_) is not equivalent to is_some()
6 participants
@Jarcho @rust-highfive @giraffate @bors @flip1995 @oli-obk