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

rustc_trait_selection: adopt let else in more places #94144

Merged
merged 1 commit into from
Feb 27, 2022

Conversation

est31
Copy link
Member

@est31 est31 commented Feb 19, 2022

Continuation of #89933, #91018, #91481, #93046, #93590, #94011.

I have extended my clippy lint to also recognize tuple passing and match statements. The diff caused by fixing it is way above 1 thousand lines. Thus, I split it up into multiple pull requests to make reviewing easier. This PR handles rustc_trait_selection.

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Feb 19, 2022
@rust-highfive
Copy link
Collaborator

r? @matthewjasper

(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 Feb 19, 2022
@jackh726
Copy link
Member

I'm not exactly sure what it is, but a lot of these just...give me a weird feeling. Like, with a match, your choices are "together" - you can see that e.g. in the Some case you do one thing, and the None case you do another. But when you replace with let else, your Some case is far (depending on how the expression is formatted) from the else case. It honestly screams for like a cross control-flow boundary return, so you could do .unwrap_or_else(|| return@outer) (taking some inspiration from Kotlin). Even moving the return to the same line wouldn't be bad ... else return; (I know this was brought up in the RFC)

I guess I probably don't object to anything in particular here, but I'd like to hear your thoughts. This also is maybe something to bring up in the stabilization discussion.

@est31
Copy link
Member Author

est31 commented Feb 19, 2022

Thanks for your input. I think it's the intent to format else { return }; on one line if possible once rustfmt supports let else: rust-lang/style-team#165 . I haven't done it in this PR particular because it felt to me that the lines would be quite long already, but i did it in other PRs of this series. It is true that match puts the options "together". unwrap_or_else is not as flexible as let else is as the latter supports arbitrary refutable patterns, the former only a bunch of types it's implemented on.

@Mark-Simulacrum
Copy link
Member

Opening all of these together has spread the discussion out a little, but I think my feelings in #94146 (comment) largely echo those of @jackh726 here -- blanket application of let-else feels like it sometimes detracts more than it adds.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 19, 2022
Adopt let else in more places

Continuation of rust-lang#89933, rust-lang#91018, rust-lang#91481, rust-lang#93046, rust-lang#93590, rust-lang#94011.

I have extended my clippy lint to also recognize tuple passing and match statements. The diff caused by fixing it is way above 1 thousand lines. Thus, I split it up into multiple pull requests to make reviewing easier. This is the biggest of these PRs and handles the changes outside of rustdoc, rustc_typeck, rustc_const_eval, rustc_trait_selection, which were handled in PRs rust-lang#94139, rust-lang#94142, rust-lang#94143, rust-lang#94144.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 19, 2022
Adopt let else in more places

Continuation of rust-lang#89933, rust-lang#91018, rust-lang#91481, rust-lang#93046, rust-lang#93590, rust-lang#94011.

I have extended my clippy lint to also recognize tuple passing and match statements. The diff caused by fixing it is way above 1 thousand lines. Thus, I split it up into multiple pull requests to make reviewing easier. This is the biggest of these PRs and handles the changes outside of rustdoc, rustc_typeck, rustc_const_eval, rustc_trait_selection, which were handled in PRs rust-lang#94139, rust-lang#94142, rust-lang#94143, rust-lang#94144.
@cjgillot
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Feb 26, 2022

📌 Commit dab5c44 has been approved by cjgillot

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 26, 2022
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 26, 2022
…jgillot

rustc_trait_selection: adopt let else in more places

Continuation of rust-lang#89933, rust-lang#91018, rust-lang#91481, rust-lang#93046, rust-lang#93590, rust-lang#94011.

I have extended my clippy lint to also recognize tuple passing and match statements. The diff caused by fixing it is way above 1 thousand lines. Thus, I split it up into multiple pull requests to make reviewing easier. This PR handles rustc_trait_selection.
@bors
Copy link
Contributor

bors commented Feb 27, 2022

⌛ Testing commit dab5c44 with merge 9323028...

@bors
Copy link
Contributor

bors commented Feb 27, 2022

☀️ Test successful - checks-actions
Approved by: cjgillot
Pushing 9323028 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Feb 27, 2022
@bors bors merged commit 9323028 into rust-lang:master Feb 27, 2022
@rustbot rustbot added this to the 1.61.0 milestone Feb 27, 2022
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (9323028): comparison url.

Summary: This benchmark run did not return any relevant results. 1 results were found to be statistically significant but too small to be relevant.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants