Skip to content

Conversation

@Zalathar
Copy link
Member

In match lowering, when choosing a test for a TestableCase::Constant, there is some ad-hoc logic for inspecting the pattern type and deciding what kind of test is suitable. There is also some very similar logic later, when partitioning cases into buckets based on the chosen test.

Instead of having that ad-hoc logic in multiple places, I think it's better to perform an up-front classification when lowering thir::PatKind::Constant to TestableCase::Constant, and then have the later steps simply match on an enum variant.

There should be no change to the resulting built MIR.

(I will note that the logic/invariants involved are a bit unclear, so there is a risk of accidental minor differences.)

@rustbot
Copy link
Collaborator

rustbot commented Dec 22, 2025

Some changes occurred in match lowering

cc @Nadrieril

@rustbot 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 Dec 22, 2025
@rustbot
Copy link
Collaborator

rustbot commented Dec 22, 2025

r? @jdonszelmann

rustbot has assigned @jdonszelmann.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot
Copy link
Collaborator

rustbot commented Dec 23, 2025

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

@Nadrieril
Copy link
Member

r? me

@rustbot rustbot assigned Nadrieril and unassigned jdonszelmann Dec 23, 2025
@Zalathar
Copy link
Member Author

Zalathar commented Dec 23, 2025

Note that there’s currently a subtle distinction between the type of a pattern node, and the type of its constant value (if it’s a const pattern).

I think it might be possible to eliminate that gap with some tweaks to how deref-patterns and pattern-type constants are lowered to THIR patterns, but I haven’t tried yet, and that’s well beyond the scope of this PR.

(I'm looking into #project-deref-patterns threads to get some context for why deref-pattern types are set up the way they currently are.)

@Nadrieril
Copy link
Member

(I'm looking into #project-deref-patterns threads to get some context for why deref-pattern types are set up the way they currently are.)

I think it's this thread: #project-deref-patterns > The string literal problem.

@Zalathar
Copy link
Member Author

I didn't mention this explicitly earlier, but my philosophy with this PR was to try to reflect the existing behaviour as much as possible. That produces results that are a bit weird/confusing, because the current behaviour is already a bit weird/confusing.

I plan to do some follow-up work to try to improve things further, so this PR represents an intermediate stepping-stone, not a final result.

Unlike the other types covered by `PatConstKind::Other`, const-float patterns
can also interact with range patterns.
@Zalathar
Copy link
Member Author

New changes:

  • Renamed PatConstKind::Eq to Other
  • Added a second patch that split out a separate PatConstKind::Float, and replaces a lot of the kind: _ matches with more specific matches

@Nadrieril
Copy link
Member

👍 finding this much clearer, ty! r=me when CI passes

@Zalathar
Copy link
Member Author

@bors r=Nadrieril

@bors
Copy link
Collaborator

bors commented Dec 24, 2025

📌 Commit 1296925 has been approved by Nadrieril

It is now in the queue for this repository.

@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 Dec 24, 2025
JonathanBrouwer added a commit to JonathanBrouwer/rust that referenced this pull request Dec 24, 2025
mir_build: Classify `TestableCase::Constant` into multiple sub-kinds

In match lowering, when choosing a test for a `TestableCase::Constant`, there is some ad-hoc logic for inspecting the pattern type and deciding what kind of test is suitable. There is also some very similar logic later, when partitioning cases into buckets based on the chosen test.

Instead of having that ad-hoc logic in multiple places, I think it's better to perform an up-front classification when lowering `thir::PatKind::Constant` to `TestableCase::Constant`, and then have the later steps simply match on an enum variant.

There should be no change to the resulting built MIR.

(I will note that the logic/invariants involved are a bit unclear, so there is a risk of accidental minor differences.)
jhpratt added a commit to jhpratt/rust that referenced this pull request Dec 24, 2025
mir_build: Classify `TestableCase::Constant` into multiple sub-kinds

In match lowering, when choosing a test for a `TestableCase::Constant`, there is some ad-hoc logic for inspecting the pattern type and deciding what kind of test is suitable. There is also some very similar logic later, when partitioning cases into buckets based on the chosen test.

Instead of having that ad-hoc logic in multiple places, I think it's better to perform an up-front classification when lowering `thir::PatKind::Constant` to `TestableCase::Constant`, and then have the later steps simply match on an enum variant.

There should be no change to the resulting built MIR.

(I will note that the logic/invariants involved are a bit unclear, so there is a risk of accidental minor differences.)
bors added a commit that referenced this pull request Dec 24, 2025
Rollup of 3 pull requests

Successful merges:

 - #150016 (stabilize `lazy_get`)
 - #150139 (Correct terminology in Clone)
 - #150238 (mir_build: Classify `TestableCase::Constant` into multiple sub-kinds)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit f9e3c61 into rust-lang:main Dec 24, 2025
11 checks passed
rust-timer added a commit that referenced this pull request Dec 24, 2025
Rollup merge of #150238 - Zalathar:pat-const-kind, r=Nadrieril

mir_build: Classify `TestableCase::Constant` into multiple sub-kinds

In match lowering, when choosing a test for a `TestableCase::Constant`, there is some ad-hoc logic for inspecting the pattern type and deciding what kind of test is suitable. There is also some very similar logic later, when partitioning cases into buckets based on the chosen test.

Instead of having that ad-hoc logic in multiple places, I think it's better to perform an up-front classification when lowering `thir::PatKind::Constant` to `TestableCase::Constant`, and then have the later steps simply match on an enum variant.

There should be no change to the resulting built MIR.

(I will note that the logic/invariants involved are a bit unclear, so there is a risk of accidental minor differences.)
@rustbot rustbot added this to the 1.94.0 milestone Dec 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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.

5 participants