lower pattern bindings in the order they're written and base drop order on primary bindings' order #143764

dianne · 2025-07-11T01:53:20Z

To fix #142163, this PR does two things:

Makes match arms base their drop order on the first sub-branch instead of the last sub-branch. Together with the second change, this makes bindings' drop order correspond to the relative order of when each binding first appears (i.e. the order of the "primary" bindings).
Lowers pattern bindings in the order they're written (still treating the right-hand side of a @ as coming before the binding on the left). In each sub-branch of a match arm, this is the order that would be obtained if the or-alternatives chosen in that sub-branch were inlined into the arm's pattern. This both affects drop order (making bindings in or-patterns not be dropped first) and fixes the issue in this test from match lowering: Lower bindings in a predictable order #121716.

My approach to the second point is admittedly a bit trickier than may be necessary. To avoid passing around a counter when building FlatPats, I've instead added just enough information to recover the original structure of the pattern's bindings from a MatchTreeSubBranch's path through the Candidate tree. Some alternatives:

We could use a counter, then sort bindings by their ordinals when making MatchTreeSubBranches.
I'd like to experiment with always merging sub-candidates and removing test_remaining_match_pairs_after_or; that would require lowering bindings and guards in a different way. That makes it a bigger change too, though, so I figure it might be simplest to start here.
For a very big change, we could track which or-alternatives succeed at runtime to base drop order on the binding order in the particular alternatives matched.

This is a breaking change. It will need a crater run, language team sign-off, and likely updates to the Reference.

This will conflict with #143376 and probably also #143028, so they shouldn't be merged at the same time.

r? @matthewjasper or @Nadrieril

This avoids scheduling drops and immediately unscheduling them. Drops for guard bindings/temporaries are still scheduled and unscheduled as before.

rustbot · 2025-07-11T01:53:27Z

Some changes occurred in match lowering

cc @Nadrieril

compiler-errors · 2025-07-11T02:57:20Z

to kick the crater run off

@bors2 try

rust-bors · 2025-07-11T02:57:25Z

⌛ Trying commit 0cda110 with merge fafe42c…

To cancel the try build, run the command @bors2 try cancel.

lower pattern bindings in the order they're written and base drop order on primary bindings' order To fix #142163, this PR does two things: - Makes match arms base their drop order on the first sub-branch instead of the last sub-branch. Together with the second change, this makes bindings' drop order correspond to the relative order of when each binding first appears (i.e. the order of the "primary" bindings). - Lowers pattern bindings in the order they're written (still treating the right-hand side of a ``@`` as coming before the binding on the left). In each sub-branch of a match arm, this is the order that would be obtained if the or-alternatives chosen in that sub-branch were inlined into the arm's pattern. This both affects drop order (making bindings in or-patterns not be dropped first) and fixes the issue in [this test](https://github.com/rust-lang/rust/blob/2a023bf80a6fbd6a06d5460a34eb247b986286ed/tests/ui/pattern/bindings-after-at/bind-by-copy-or-pat.rs) from #121716. My approach to the second point is admittedly a bit trickier than may be necessary. To avoid passing around a counter when building `FlatPat`s, I've instead added just enough information to recover the original structure of the pattern's bindings from a `MatchTreeSubBranch`'s path through the `Candidate` tree. Some alternatives: - We could use a counter, then sort bindings by their ordinals when making `MatchTreeSubBranch`es. - I'd like to experiment with always merging sub-candidates and removing `test_remaining_match_pairs_after_or`; that would require lowering bindings and guards in a different way. That makes it a bigger change too, though, so I figure it might be simplest to start here. - For a very big change, we could track which or-alternatives succeed at runtime to base drop order on the binding order in the particular alternatives matched. This is a breaking change. It will need a crater run, language team sign-off, and likely updates to the Reference. This will conflict with #143376 and probably also #143028, so they shouldn't be merged at the same time. r? `@matthewjasper` or `@Nadrieril`

rust-bors · 2025-07-11T05:16:36Z

☀️ Try build successful (CI)
Build commit: fafe42c (fafe42c59556a4233f03dd0d900b575a9afeece8, parent: 2a023bf80a6fbd6a06d5460a34eb247b986286ed)

compiler-errors · 2025-07-11T14:45:16Z

@craterbot check

craterbot · 2025-07-11T14:45:46Z

👌 Experiment pr-143764 created and queued.
🤖 Automatically detected try build fafe42c
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

craterbot · 2025-07-12T05:42:13Z

🚧 Experiment pr-143764 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

craterbot · 2025-07-13T08:21:34Z

🎉 Experiment pr-143764 is completed!
📊 5 regressed and 3 fixed (663519 total)
📰 Open the summary report.

⚠️ If you notice any spurious failure please add them to the denylist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

dianne · 2025-07-13T10:03:10Z

The regressions look unrelated as far as I can tell: 3 crashes in lld and two errors building non-rust dependencies (jq and fltk).

The "fixed" crates also look unrelated: previously they'd encountered a file permissions error in a build script, a failure to build a non-rust dependency (jq), and a failure to resolve a dependency (arcstr).

I think that means none of the tested crates encountered the change in drop-checking. Hopefully nothing relies on the current drop order for runtime correctness (I'd be surprised/impressed if so!)

Nadrieril · 2025-07-23T09:25:13Z

compiler/rustc_mir_build/src/builder/matches/mod.rs

+    match parents {
+        None => bug!("can't inline or-pattern bindings: already inlined all bindings"),
+        Some(mut parents) => {


nit: you're starting the function with an unwrap(), I think it would be a bit clearer to not take an Option and unwrap remainder before the call below

Nadrieril · 2025-07-23T09:45:22Z

compiler/rustc_mir_build/src/builder/matches/mod.rs

+fn push_sub_branch_bindings<'c, 'tcx>(
+    flattened: &mut Vec<Binding<'tcx>>,
+    parents: Option<&'c [PatternExtraData<'tcx>]>,
+    leaf_bindings: &[SubpatternBindings<'tcx>],
+) -> Option<&'c [PatternExtraData<'tcx>]> {


Might this be simplified by having it take a it: &mut impl Iterator<Item=&[SubpatternBindings<'tcx>] and passing it parents.iter().map(|parent| parent.bindings.as_slice()).filter(|bindings| !bindings.is_empty()).chain([leaf_bindings])? Then you'd just call it.next() instead of the little loop.

Oh, true! That's much nicer. I've also put the filter after the chain so that we don't have to treat leaf candidates from unsimplified bindingless or-patterns specially. There's tradeoffs, but I think it makes the reasoning simpler overall and the asserts stronger.

No push yet since the stronger asserts uncovered an existing issue that may need fixing before this PR can be merged: we don't do any error tainting for inconsistent or-pattern bindings like x | _, so patterns like that can get to MIR building. It's not a correctness issue since we emit an error, but I'd really like to be able to assume or-patterns are well-formed here. The ill-formed or-patterns are now hitting the assert in push_sub_branch_bindings, which means it's working! But it also means ICEs until we have proper error tainting.

Hmmm, we should definitely have good error tainting. I wanna note that I'd like to eventually allow mismatched bindings for use in guard patterns: Ok(x) | Err(e) if let Some(x) = e.recover() type things; don't know if that changes anything here but thought I'd mention.

That's on my to-do list! As I understand it, it will indeed need some substantial changes, which may involve replacing much of what this PR does. My intent here is to land the breaking change to the language with as small of a compiler change as possible, so that I don't have to worry about preserving the current drop order in future changes to match lowering (e.g. to implement guard patterns).

Nadrieril

I'd like a second pair of eyes on the first commit because I'm not expert there. Big fan of the last commit, that's a very elegant solution! Let's get some t-lang eyes on this.

traviscross · 2025-07-23T15:59:20Z

We talked about this in the lang call today. Thanks to @dianne for pushing this forward. Based on the absence of breakage, and that we decided the behavior we're breaking is undesired, let's do it.

@rfcbot fcp merge

rfcbot · 2025-07-23T15:59:22Z

Team member @traviscross has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns.
See this document for info about what commands tagged team members can give me.

nikomatsakis · 2025-07-23T16:01:39Z

@rfcbot reviewed

I've not read the PR contents, but I am affirming that the desired behavior is this from the OP:

Bindings' drop order correspond to the relative order of when each binding first appears (i.e. the order of the "primary" bindings).

traviscross · 2025-07-23T16:02:36Z

Let's look into ensuring this is documented in the Reference.

cc @ehuss

tmandry · 2025-07-23T16:02:43Z

@rfcbot reviewed

rfcbot · 2025-07-23T16:02:51Z

🔔 This is now entering its final comment period, as per the review above. 🔔

matthewjasper · 2025-07-23T17:33:54Z

I'd like a second pair of eyes on the first commit because I'm not expert there. Big fan of the last commit, that's a very elegant solution! Let's get some t-lang eyes on this.

The first two commits look good to me (and the third except for the things that you've already commented on).

dianne added 3 commits July 10, 2025 17:18

don't schedule unnecessary drops when lowering or-patterns

4f1c4e2

This avoids scheduling drops and immediately unscheduling them. Drops for guard bindings/temporaries are still scheduled and unscheduled as before.

base drop order on the first sub-branch

fee2dfd

lower bindings in the order they're written

0cda110

rustbot assigned matthewjasper Jul 11, 2025

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jul 11, 2025

craterbot added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 11, 2025

craterbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels Jul 13, 2025

Nadrieril reviewed Jul 23, 2025

View reviewed changes

Nadrieril added the I-lang-nominated Nominated for discussion during a lang team meeting. label Jul 23, 2025

traviscross added P-lang-drag-1 Lang team prioritization drag level 1. https://rust-lang.zulipchat.com/#narrow/channel/410516-t-lang T-lang Relevant to the language team and removed T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jul 23, 2025

rfcbot added the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label Jul 23, 2025

rfcbot added the disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. label Jul 23, 2025

traviscross added the S-waiting-on-documentation Status: Waiting on approved PRs to documentation before merging label Jul 23, 2025

rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Jul 23, 2025

lower pattern bindings in the order they're written and base drop order on primary bindings' order #143764

Are you sure you want to change the base?

lower pattern bindings in the order they're written and base drop order on primary bindings' order #143764

Uh oh!

Conversation

dianne commented Jul 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented Jul 11, 2025

Uh oh!

compiler-errors commented Jul 11, 2025

Uh oh!

rust-bors bot commented Jul 11, 2025

Uh oh!

rust-bors bot commented Jul 11, 2025

Uh oh!

compiler-errors commented Jul 11, 2025

Uh oh!

craterbot commented Jul 11, 2025

Uh oh!

craterbot commented Jul 12, 2025

Uh oh!

craterbot commented Jul 13, 2025

Uh oh!

dianne commented Jul 13, 2025

Uh oh!

Nadrieril Jul 23, 2025

Choose a reason for hiding this comment

Uh oh!

Nadrieril Jul 23, 2025

Choose a reason for hiding this comment

Uh oh!

dianne Jul 23, 2025

Choose a reason for hiding this comment

Uh oh!

Nadrieril Jul 23, 2025

Choose a reason for hiding this comment

Uh oh!

dianne Jul 23, 2025

Choose a reason for hiding this comment

Uh oh!

Nadrieril left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

traviscross commented Jul 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rfcbot commented Jul 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nikomatsakis commented Jul 23, 2025

Uh oh!

traviscross commented Jul 23, 2025

Uh oh!

tmandry commented Jul 23, 2025

Uh oh!

rfcbot commented Jul 23, 2025

Uh oh!

matthewjasper commented Jul 23, 2025

Uh oh!

Uh oh!

dianne commented Jul 11, 2025 •

edited

Loading

Nadrieril left a comment •

edited

Loading

traviscross commented Jul 23, 2025 •

edited

Loading

rfcbot commented Jul 23, 2025 •

edited

Loading