Adopt let else in more places #94146
Conversation
rustbot
added
the
T-compiler
|
r? @cjgillot (rust-highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
| @@ -385,12 +385,9 @@ impl<Tag: Provenance, Extra> Allocation<Tag, Extra> { | |||
| ) -> AllocResult { | |||
| assert!(self.mutability == Mutability::Mut); | |||
|
|
|||
| let val = match val { | |||
| ScalarMaybeUninit::Scalar(scalar) => scalar, | |||
| ScalarMaybeUninit::Uninit => { | |||
There was a problem hiding this comment.
I don't think this change is a good one: the let else based version drops the information that we are handling the ScalarMaybeUninit::Uninit case in else, so it is unclear why one would even call mark_init. I think I find the old code more readable.
There was a problem hiding this comment.
Yeah it requires knowledge that the only two enum variants are Scalar and Uninit. let else hides all non matching patterns into a single else clause. This also has maintainability issues as you won't get an non exhaustive match error if you add a variant that might be relevant for the code. I have added variants in the past and the non exhaustive match error was very helpful because it pointed me to all those places, some you might never have thought of, that needed change. Generally this second issue exists with if let as well though.
Anyways, I'll change it back.
There was a problem hiding this comment.
Not related to this commit, but I wonder if this type would be more readable as a newtype for Option: struct ScalarMaybeUninit(Option<...>).
There was a problem hiding this comment.
I think having a variant named Uninit is quite useful and evocative. Code that matches on this needs to be very careful to correctly implement the subtle semantics of uninit values, so I'd rather not mix that up with an innocent-looking None.
This comment has been minimized.
This comment has been minimized.
| Async::Yes { closure_id, .. } => closure_id, | ||
| Async::No => return self.lower_fn_body_block(span, decl, body), | ||
| let Async::Yes { closure_id, .. } = asyncness else { | ||
| return self.lower_fn_body_block(span, decl, body); | ||
| }; |
There was a problem hiding this comment.
I think for bool-like enums the previous expression was clearer -- and probably more generally, most exhaustive matches probably shouldn't be converted.
(Maybe with the exception of Option, where the Some branch is exhaustively covered by the pattern in the let).
(Edit: I see the other comment -- it seems like this is just a more general form of that comment, in some sense. Ideally the takeaway might be applied across all the PRs, not just in individual places, to ease review -- particularly as this is touching a lot of code).
There was a problem hiding this comment.
Thanks for the feedback, part of this is trying to find out where the feature shines and where it doesn't, also to inform development of the lint. I think it makes totally sense to forego warning if there is some large pattern with an enum that is not known to people (like Async). Outside of Result and Option I can't think of any enums that would be so widely known.
There was a problem hiding this comment.
Alright I have removed any let else introductions in this PR and the others, where there was some non obvious enum. I hope I haven't missed any. Note that I did leave the let else introduction for Err(NoSolution) and Err(ErrorReported) in the rustdoc, typeck and trait_selection PRs tho, as I think that in that instance, there is little value in knowing that the error case is NoSolution. But idk, feel free to disagree. My plan is to make the lint not warn about such cases anyways. 90% of the changes are about pure Option and Results, where the "else" clause pattern is _. The remaining places have a complicated enum pattern in the "some" clause, but a _ in the else clause.
There was a problem hiding this comment.
I think it's reasonable to use let else for enums where it's reasonably obvious that there are only two cases. If you have a variant Async::Yes and the only other variant is Async::No, you don't have to spell out the latter to guess (correctly) that only those two variants exist.
(By contrast, in the ScalarMaybeUninit case above, it's a little less obvious. Though honestly I wonder if that case would be clearer as an Option newtype.)
There was a problem hiding this comment.
I think we lose some clarity when moving to let/else over an exhaustive match, in pretty much all cases. For example, Async::No is probably the only other variant, but even so, when reading the above code I'm sort of left to wonder "is that the other variant" and re-parse it out such that the yes case is here and the no case is there. Plus, with a slightly complicated pattern on the Yes variant, it's possible for some of the Yes's to also be put into the No variant (e.g., if it was something like Async::Yes(Some(foo), ..), and so I end up spending more time parsing the pattern when the match would've been immediately clear.
There was a problem hiding this comment.
FTR I've also removed Err(ErrorReported) from the typeck PR due to reviewer feedback.
cjgillot
added
S-waiting-on-author
|
r? @cjgillot |
|
@bors r+ rollup |
|
📌 Commit 2ef8af6 has been approved by |
bors
added
S-waiting-on-bors
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.
…askrgr Rollup of 14 pull requests Successful merges: - rust-lang#93580 (Stabilize pin_static_ref.) - rust-lang#93639 (Release notes for 1.59) - rust-lang#93686 (core: Implement ASCII trim functions on byte slices) - rust-lang#94002 (rustdoc: Avoid duplicating macros in sidebar) - rust-lang#94019 (removing architecture requirements for RustyHermit) - rust-lang#94023 (adapt static-nobundle test to use llvm-nm) - rust-lang#94091 (Fix rustdoc const computed value) - rust-lang#94093 (Fix pretty printing of enums without variants) - rust-lang#94097 (Add module-level docs for `rustc_middle::query`) - rust-lang#94112 (Optimize char_try_from_u32) - rust-lang#94113 (document rustc_middle::mir::Field) - rust-lang#94122 (Fix miniz_oxide types showing up in std docs) - rust-lang#94142 (rustc_typeck: adopt let else in more places) - rust-lang#94146 (Adopt let else in more places) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
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 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 #94139, #94142, #94143, #94144.