Replace some magic booleans in match-lowering with enums #126981
Conversation
|
r? @nnethercote rustbot has assigned @nnethercote. Use |
rustbot
added
S-waiting-on-review
This comment has been minimized.
This comment has been minimized.
|
r? @Nadrieril I like this a lot! r=me once the other PR is merged. |
|
☔ The latest upstream changes (presumably #127111) made this pull request unmergeable. Please resolve the merge conflicts. |
|
Some changes occurred in match lowering cc @Nadrieril |
|
Rebased after #126835 (finally) landed. |
| && schedule_drop | ||
| { | ||
| self.schedule_drop(span, region_scope, local_id, DropKind::Storage); | ||
| if let Some(region_scope) = self.region_scope_tree.var_scope(var.0.local_id) { |
There was a problem hiding this comment.
Side note: I was looking at the history of this if let (#94849), and it made me wonder if the None case can never actually occur.
I tried replacing the None case with span_bug and couldn't observe any failures in the test suites I ran. That's not entirely conclusive, but it is suggestive.
(Obviously this is all outside the scope of this PR; just noting something interesting that I noticed.)
There was a problem hiding this comment.
Hm weird, that PR does add a test that used to get a None, so something else must have changed. I can't say I understand this code enough to tell though.
There was a problem hiding this comment.
My guess is that at the time the change was made, there were fewer “early” syntactic checks for let expressions in bad places, so it was possible for one to (illegally) exist inside certain const contexts that would survive to this point and cause an unwrap panic.
The change from unwrap to if-let was requested by #94849 (comment), but it doesn't give context for why. I suspect that it would have been preferable to make the None case do a span_delayed_bug instead of silently doing nothing.
There was a problem hiding this comment.
Looks like #115677 is what moved the syntactic checks to parse-time, guaranteeing that illegal let expressions don't proceed to MIR building. That gives me a bit more confidence that the None scenario can't happen, and that we have tests that would likely hit this edge-case if it were possible.
| /// Let expressions are not permitted in this context, so it is a bug to | ||
| /// try to lower one (e.g inside lazy-boolean-or or boolean-not). | ||
| LetNotPermitted, |
There was a problem hiding this comment.
I like this! It's much easier to follow than a boolean whose value will be ignored
|
Looks good! I have only one question: what benefit do you see in an exhaustive match with a |
The new enum `DeclareLetBindings` has three variants: - `Yes`: Declare `let` bindings as normal, for `if` conditions. - `No`: Don't declare bindings, for match guards and let-else. - `LetNotPermitted`: Assert that `let` expressions should not occur.
The previous boolean used `true` to indicate that storage-live should _not_ be emitted, so all occurrences of `Yes` and `No` should be the logical opposite of the previous value.
|
Replaced the exhaustive match on |
|
Thanks! @bors r+ rollup |
bors
removed
the
S-waiting-on-review
bors
added
the
S-waiting-on-bors
Replace some magic booleans in match-lowering with enums This PR takes some boolean arguments used by the match-lowering code, and replaces them with dedicated enums that more clearly express their effect, while also making it much easier to find how each value is ultimately used.
…iaskrgr Rollup of 8 pull requests Successful merges: - rust-lang#126018 (Remove the `box_pointers` lint.) - rust-lang#126895 (Fix simd_gather documentation) - rust-lang#126981 (Replace some magic booleans in match-lowering with enums) - rust-lang#127069 (small correction to fmt::Pointer impl) - rust-lang#127157 (coverage: Avoid getting extra unexpansion info when we don't need it) - rust-lang#127160 (Add a regression test for rust-lang#123630) - rust-lang#127161 (Improve `run-make-support` library `args` API) - rust-lang#127162 (Subtree sync for rustc_codegen_cranelift) r? `@ghost` `@rustbot` modify labels: rollup
…iaskrgr Rollup of 9 pull requests Successful merges: - rust-lang#126018 (Remove the `box_pointers` lint.) - rust-lang#126895 (Fix simd_gather documentation) - rust-lang#126981 (Replace some magic booleans in match-lowering with enums) - rust-lang#127038 (Update test comment) - rust-lang#127053 (Update the LoongArch target documentation) - rust-lang#127069 (small correction to fmt::Pointer impl) - rust-lang#127157 (coverage: Avoid getting extra unexpansion info when we don't need it) - rust-lang#127160 (Add a regression test for rust-lang#123630) - rust-lang#127161 (Improve `run-make-support` library `args` API) r? `@ghost` `@rustbot` modify labels: rollup
…iaskrgr Rollup of 9 pull requests Successful merges: - rust-lang#126018 (Remove the `box_pointers` lint.) - rust-lang#126895 (Fix simd_gather documentation) - rust-lang#126981 (Replace some magic booleans in match-lowering with enums) - rust-lang#127038 (Update test comment) - rust-lang#127053 (Update the LoongArch target documentation) - rust-lang#127069 (small correction to fmt::Pointer impl) - rust-lang#127157 (coverage: Avoid getting extra unexpansion info when we don't need it) - rust-lang#127160 (Add a regression test for rust-lang#123630) - rust-lang#127161 (Improve `run-make-support` library `args` API) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of rust-lang#126981 - Zalathar:enums, r=Nadrieril Replace some magic booleans in match-lowering with enums This PR takes some boolean arguments used by the match-lowering code, and replaces them with dedicated enums that more clearly express their effect, while also making it much easier to find how each value is ultimately used.
This PR takes some boolean arguments used by the match-lowering code, and replaces them with dedicated enums that more clearly express their effect, while also making it much easier to find how each value is ultimately used.