match lowering: Hide `Candidate` from outside the lowering algorithm #127159

Nadrieril · 2024-06-30T09:53:34Z

The internals of Candidate are tricky and a source of confusion. This PR makes it so we don't expose Candidates outside the lowering algorithm. Now:

false edges are handled in lower_match_tree;
lower_match_tree takes a list of patterns as input;
lower_match_tree returns a flat datastructure that contains only the necessary information.

r? @matthewjasper

try-job: x86_64-rust-for-linux

Zalathar · 2024-07-01T10:27:38Z

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

-            // the failure block.
-            previous_candidate.as_mut().unwrap().next_candidate_start_block = Some(otherwise_block)
-        } else {
+        if !refutable {


Side note: refutable: bool seems like another good candidate for a dedicated enum in the future.

Possibly yes. I have hopes to remove the need for it, we'll see

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

Zalathar · 2024-07-10T01:13:21Z

So if I understand this correctly:

There is a flat list of MatchTreeBranch at the top level, and each one corresponds 1:1 with an arm of the match expression.
- Should we assert that this is true just before zipping the arms/branches back together?
Each of those branches contains a flat list of 1 or more MatchTreeSubBranch.
- Should we assert that the list of sub-branches is not empty?
- A sub-branch represents a “way” for that arm to decide that it should jump to checking the guard (or jump to the arm body if the arm has no guard).
- Arms without or-patterns should have exactly 1 sub-branch.
- For arms with or-patterns, the exact number of sub-branches is an implementation detail, depending on opportunities for simplification (which typically require there to be no guard).
- The guard needs to be lowered separately for each sub-branch (because each sub-branch might bind different values).
- If the guard fails, we jump to that sub-branch's otherwise_block, which will have been wired up to jump to the next sub-branch (if one exists), or otherwise to the next top-level arm branch.

Nadrieril · 2024-07-10T07:57:50Z

Yep, all correct. I don't feel the need to assert the number of arms, it's just a collect().into_iter().map().collect() in lower_match_tree.

Zalathar · 2024-07-22T03:24:51Z

I was playing around with this, and one thing I found was that it's fairly easy to change MatchTreeBranch::from_candidate and MatchTreeSubBranch::from_sub_candidate so that they take &Candidate by reference instead of Candidate by value.

Obviously both ways are possible (since the candidates aren't used afterwards), but the by-reference version feels a bit nicer to me, because there's no need to worry about being able to move things at the right time.

Similarly, it's possible to have lower_match_tree take a &BuiltMatchTree by reference instead of BuiltMatchTree by value, with only minimal tweaks to the relevant code.

(Not a blocking concern; I'm happy to propose this change as a follow-up PR.)

Nadrieril · 2024-07-22T06:26:18Z

I was playing around with this, and one thing I found was that it's fairly easy to change MatchTreeBranch::from_candidate and MatchTreeSubBranch::from_sub_candidate so that they take &Candidate by reference instead of Candidate by value.

Obviously both ways are possible (since the candidates aren't used afterwards), but the by-reference version feels a bit nicer to me, because there's no need to worry about being able to move things at the right time.

I made them take Candidate exactly because it feels important that we don't use the Candidates afterward. These functions are meant to be used exactly in one spot, why would there be a worry about being able to move things?

Similarly, it's possible to have lower_match_tree take a &BuiltMatchTree by reference instead of BuiltMatchTree by value, with only minimal tweaks to the relevant code.

I'm guessing you mean bind_pattern could take &MatchTreeBranch? Sure, that one feels less important. But again I'm curious what your usecase is, given that this is meant to be called exactly once per branch directly after lower_match_tree.

Zalathar · 2024-07-22T07:47:35Z

Ah, I didn't have a specific use case (beyond trying to get rid of the slightly ugly length check and try_into in bind_pattern), so if you did it this way on purpose then I won't worry about it.

Various notes on match lowering This is an assortment of comments for things that I found unclear or confusing when I was learning how match lowering works. This PR only adds/modifies comments, so there are no functional changes. I have tried to avoid touching code that would conflict with rust-lang#127159. r? `@Nadrieril`

Rollup merge of rust-lang#128085 - Zalathar:notes, r=Nadrieril Various notes on match lowering This is an assortment of comments for things that I found unclear or confusing when I was learning how match lowering works. This PR only adds/modifies comments, so there are no functional changes. I have tried to avoid touching code that would conflict with rust-lang#127159. r? `@Nadrieril`

bors · 2024-07-31T14:19:33Z

📌 Commit 08bcc01 has been approved by matthewjasper

It is now in the queue for this repository.

…wjasper match lowering: Hide `Candidate` from outside the lowering algorithm The internals of `Candidate` are tricky and a source of confusion. This PR makes it so we don't expose `Candidate`s outside the lowering algorithm. Now: - false edges are handled in `lower_match_tree`; - `lower_match_tree` takes a list of patterns as input; - `lower_match_tree` returns a flat datastructure that contains only the necessary information. r? `@matthewjasper`

…iaskrgr Rollup of 7 pull requests Successful merges: - rust-lang#123813 (Add `REDUNDANT_IMPORTS` lint for new redundant import detection) - rust-lang#127159 (match lowering: Hide `Candidate` from outside the lowering algorithm) - rust-lang#128162 (Cleanup sys module to match house style) - rust-lang#128296 (Update target-spec metadata for loongarch64 targets) - rust-lang#128417 (Add `f16` and `f128` math functions) - rust-lang#128431 (Add myself as VxWorks target maintainer for reference) - rust-lang#128437 (improve bootstrap to allow selecting llvm tools individually) r? `@ghost` `@rustbot` modify labels: rollup

…wjasper match lowering: Hide `Candidate` from outside the lowering algorithm The internals of `Candidate` are tricky and a source of confusion. This PR makes it so we don't expose `Candidate`s outside the lowering algorithm. Now: - false edges are handled in `lower_match_tree`; - `lower_match_tree` takes a list of patterns as input; - `lower_match_tree` returns a flat datastructure that contains only the necessary information. r? ``@matthewjasper``

Rollup of 7 pull requests Successful merges: - rust-lang#117468 (Stabilize Wasm relaxed SIMD) - rust-lang#123813 (Add `REDUNDANT_IMPORTS` lint for new redundant import detection) - rust-lang#127060 (Migrate `symbol-visibility` `run-make` test to rmake) - rust-lang#127159 (match lowering: Hide `Candidate` from outside the lowering algorithm) - rust-lang#128296 (Update target-spec metadata for loongarch64 targets) - rust-lang#128416 (android: Remove libstd hacks for unsupported Android APIs) - rust-lang#128431 (Add myself as VxWorks target maintainer for reference) r? `@ghost` `@rustbot` modify labels: rollup

tgross35 · 2024-07-31T21:27:40Z

I think this may have somehow failed the rollup at #128454 (comment). I am not entirely sure how but this seems like the only PR in the rollup that affects codegen.

I'll start a try job to confirm. The RFL job failed but I assume that was just the first CI job to complete and others would have failed too. Also going to ask on Zulip.

@bors r-

tgross35 · 2024-07-31T21:28:25Z

@bors try

bors · 2024-07-31T21:29:35Z

⌛ Trying commit 08bcc01 with merge c52c83f...

match lowering: Hide `Candidate` from outside the lowering algorithm The internals of `Candidate` are tricky and a source of confusion. This PR makes it so we don't expose `Candidate`s outside the lowering algorithm. Now: - false edges are handled in `lower_match_tree`; - `lower_match_tree` takes a list of patterns as input; - `lower_match_tree` returns a flat datastructure that contains only the necessary information. r? `@matthewjasper` try-job: x86_64-rust-for-linux

tgross35 · 2024-07-31T21:30:06Z

I am totally wrong, the wasm PR updated stdarch. Sorry about that, please re-r+ after the try job completes.

bors · 2024-07-31T21:49:28Z

☀️ Try build successful - checks-actions
Build commit: c52c83f (c52c83f6ff32a78324decbca0ba3839309e99e52)

tgross35 · 2024-07-31T21:53:12Z

@bors r=matthewjasper

bors · 2024-07-31T21:53:15Z

📌 Commit 08bcc01 has been approved by matthewjasper

It is now in the queue for this repository.

…iaskrgr Rollup of 7 pull requests Successful merges: - rust-lang#123813 (Add `REDUNDANT_IMPORTS` lint for new redundant import detection) - rust-lang#126697 ([RFC] mbe: consider the `_` in 2024 an expression) - rust-lang#127159 (match lowering: Hide `Candidate` from outside the lowering algorithm) - rust-lang#128244 (Peel off explicit (or implicit) deref before suggesting clone on move error in borrowck, remove some hacks) - rust-lang#128431 (Add myself as VxWorks target maintainer for reference) - rust-lang#128438 (Add special-case for [T, 0] in dropck_outlives) - rust-lang#128457 (Fix docs for OnceLock::get_mut_or_init) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#127159 - Nadrieril:hide-candidate, r=matthewjasper match lowering: Hide `Candidate` from outside the lowering algorithm The internals of `Candidate` are tricky and a source of confusion. This PR makes it so we don't expose `Candidate`s outside the lowering algorithm. Now: - false edges are handled in `lower_match_tree`; - `lower_match_tree` takes a list of patterns as input; - `lower_match_tree` returns a flat datastructure that contains only the necessary information. r? ```@matthewjasper```

rustbot assigned matthewjasper Jun 30, 2024

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 Jun 30, 2024

This comment has been minimized.

Sign in to view

This comment was marked as resolved.

Sign in to view

Nadrieril mentioned this pull request Jul 1, 2024

coverage: Treat each match arm as a "branch" for branch coverage #124154

Draft

Nadrieril force-pushed the hide-candidate branch from 6054a5b to f44350d Compare July 1, 2024 07:06

Zalathar reviewed Jul 1, 2024

View reviewed changes

Zalathar reviewed Jul 7, 2024

View reviewed changes

compiler/rustc_mir_build/src/build/matches/mod.rs Outdated Show resolved Hide resolved

Zalathar mentioned this pull request Jul 10, 2024

Support mcdc analysis for pattern matching #124278

Open

This comment was marked as resolved.

Sign in to view

Nadrieril force-pushed the hide-candidate branch from f44350d to d4c09a6 Compare July 19, 2024 17:50

This comment has been minimized.

Sign in to view

Nadrieril force-pushed the hide-candidate branch from d4c09a6 to 8172e31 Compare July 19, 2024 18:35

This comment was marked as resolved.

Sign in to view

Nadrieril force-pushed the hide-candidate branch from 8172e31 to 62c2c6e Compare July 20, 2024 15:28

Zalathar mentioned this pull request Jul 23, 2024

Various notes on match lowering #128085

Merged

This comment was marked as outdated.

Sign in to view

Nadrieril added 3 commits July 29, 2024 09:50

Small simplification

9e05fb6

Set up false edges in lower_match_tree

e2fd9aa

Abstract out the candidate manipulation not in the main algorithm

cbdacec

Nadrieril force-pushed the hide-candidate branch from 62c2c6e to cd91954 Compare July 29, 2024 07:58

Visiting bindings is straightforward now

c747166

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 Jul 31, 2024

matthiaskrgr mentioned this pull request Jul 31, 2024

Rollup of 7 pull requests #128439

Closed

tgross35 mentioned this pull request Jul 31, 2024

Rollup of 7 pull requests #128454

Closed

matthiaskrgr mentioned this pull request Jul 31, 2024

Rollup of 7 pull requests #128461

Merged

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jul 31, 2024

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 31, 2024

bors merged commit 19f6ff0 into rust-lang:master Aug 1, 2024
7 checks passed

rustbot added this to the 1.82.0 milestone Aug 1, 2024

Nadrieril deleted the hide-candidate branch August 1, 2024 07:12

lqd mentioned this pull request Oct 20, 2024

Regression: geoarrow crate does not compile in release mode on 1.82 #131960

Open

matthiaskrgr mentioned this pull request Nov 15, 2024

ICE: compiler/rustc_mir_build/src/build/matches/mod.rs None #133063

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

match lowering: Hide `Candidate` from outside the lowering algorithm #127159

match lowering: Hide `Candidate` from outside the lowering algorithm #127159

Nadrieril commented Jun 30, 2024 •

edited by tgross35

Loading

This comment has been minimized.

This comment was marked as resolved.

Zalathar Jul 1, 2024

Nadrieril Jul 1, 2024

Zalathar commented Jul 10, 2024

Nadrieril commented Jul 10, 2024 •

edited

Loading

This comment was marked as resolved.

This comment has been minimized.

This comment was marked as resolved.

Zalathar commented Jul 22, 2024 •

edited

Loading

Nadrieril commented Jul 22, 2024

Zalathar commented Jul 22, 2024

This comment was marked as outdated.

bors commented Jul 31, 2024

tgross35 commented Jul 31, 2024

tgross35 commented Jul 31, 2024

bors commented Jul 31, 2024

tgross35 commented Jul 31, 2024

bors commented Jul 31, 2024

tgross35 commented Jul 31, 2024

bors commented Jul 31, 2024

match lowering: Hide Candidate from outside the lowering algorithm #127159

match lowering: Hide Candidate from outside the lowering algorithm #127159

Conversation

Nadrieril commented Jun 30, 2024 • edited by tgross35 Loading

This comment has been minimized.

This comment was marked as resolved.

Zalathar Jul 1, 2024

Choose a reason for hiding this comment

Nadrieril Jul 1, 2024

Choose a reason for hiding this comment

Zalathar commented Jul 10, 2024

Nadrieril commented Jul 10, 2024 • edited Loading

This comment was marked as resolved.

This comment has been minimized.

This comment was marked as resolved.

Zalathar commented Jul 22, 2024 • edited Loading

Nadrieril commented Jul 22, 2024

Zalathar commented Jul 22, 2024

This comment was marked as outdated.

bors commented Jul 31, 2024

tgross35 commented Jul 31, 2024

tgross35 commented Jul 31, 2024

bors commented Jul 31, 2024

tgross35 commented Jul 31, 2024

bors commented Jul 31, 2024

tgross35 commented Jul 31, 2024

bors commented Jul 31, 2024

match lowering: Hide `Candidate` from outside the lowering algorithm #127159

match lowering: Hide `Candidate` from outside the lowering algorithm #127159

Nadrieril commented Jun 30, 2024 •

edited by tgross35

Loading

Nadrieril commented Jul 10, 2024 •

edited

Loading

Zalathar commented Jul 22, 2024 •

edited

Loading