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

Fix option_if_let_else #7573

Merged
merged 2 commits into from
Aug 30, 2021
Merged

Fix option_if_let_else #7573

merged 2 commits into from
Aug 30, 2021

Conversation

Jarcho
Copy link
Contributor

@Jarcho Jarcho commented Aug 16, 2021

fixes: #5822
fixes: #6737
fixes: #7567

The inference from #6137 still exists so I'm not sure if this should be moved from the nursery. Before doing that though I'd almost want to see this split into two lints. One suggesting map_or and the other suggesting map_or_else.

map_or_else tends to have longer expressions for both branches so it doesn't end up much shorter than a match expression in practice. It also seems most people find it harder to read. map_or at least has the terseness benefit of being on one line most of the time, especially when the None branch is just a literal or path expression.

changelog: break and continue statments local to the would-be closure are allowed in option_if_let_else
changelog: don't lint in const contexts in option_if_let_else
changelog: don't lint when yield expressions are used in option_if_let_else
changelog: don't lint when the captures made by the would-be closure conflict with the other branch in option_if_let_else
changelog: don't lint when a field of a local is used when the type could be pontentially moved from in option_if_let_else
changelog: in some cases, don't lint when scrutinee expression conflicts with the captures of the would-be closure in option_if_let_else

@rust-highfive
Copy link

r? @giraffate

(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 Aug 16, 2021
* `break` and `continue` statments local to the would-be closure are allowed
* don't lint in const contexts
* don't lint when yield expressions are used
* don't lint when the captures made by the would-be closure conflict with the other branch
* don't lint when a field of a local is used when the type could be pontentially moved from
* in some cases, don't lint when scrutinee expression conflicts with the captures of the would-be closure
Copy link
Contributor

@giraffate giraffate left a comment

Choose a reason for hiding this comment

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

Sorry for the late review.

changelog: don't lint when yield expressions are used in option_if_let_else

What kind of case is this? Is there a test for this?

clippy_lints/src/option_if_let_else.rs Outdated Show resolved Hide resolved
clippy_lints/src/option_if_let_else.rs Show resolved Hide resolved
@giraffate
Copy link
Contributor

Additionally, it would be helpful if you split the PR or commits for each issue next time. I'm okay with that this time.

@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-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Aug 26, 2021
@Jarcho
Copy link
Contributor Author

Jarcho commented Aug 27, 2021

yield expressions (used in async await) can't be moved into a closure. There is a test now.

@giraffate
Copy link
Contributor

@bors r+

Thanks!

@bors
Copy link
Collaborator

bors commented Aug 30, 2021

📌 Commit 3e5dcba has been approved by giraffate

@bors
Copy link
Collaborator

bors commented Aug 30, 2021

⌛ Testing commit 3e5dcba with merge fd30241...

@bors
Copy link
Collaborator

bors commented Aug 30, 2021

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

@bors bors merged commit fd30241 into rust-lang:master Aug 30, 2021
@bors bors mentioned this pull request Aug 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status)
Projects
None yet
4 participants