Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Split thir::PatKind::ExpandedConstant into two distinct kinds #136529

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Zalathar
Copy link
Contributor

@Zalathar Zalathar commented Feb 4, 2025

These two kinds of marker node are superficially similar, but are actually very different in practice, and there is no code that actually wants to treat them as the same thing.


For historical reference, PatKind::InlineConst was introduced in #116482 to fix THIR unsafeck for inline-const blocks in range pattern endpoints.

It was then modified by #132708 (and renamed to PatKind::ExpandedConstant) to also mark named constants, for a diagnostic improvement for code that accidentally tries to perform an irrefutable binding to a name that is already used by a constant. The two sub-variants were indicated by a boolean field, which also changed the significance of the def_id field.

But tellingly, there was no code that actually wanted to equivocate between the two. Every occurrence was specifically interested in inline-const, or specifically interested in named-const, or was simply trying to traverse/unpeel pattern nodes without regard for their meaning.

These two kinds of marker node are superficially similar, but are actually very
different in practice, and there is no code that actually wants to treat them
as the same thing.
@rustbot
Copy link
Collaborator

rustbot commented Feb 4, 2025

r? @fmease

rustbot has assigned @fmease.
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 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 Feb 4, 2025
@rustbot
Copy link
Collaborator

rustbot commented Feb 4, 2025

Some changes occurred in match checking

cc @Nadrieril

Some changes occurred in exhaustiveness checking

cc @Nadrieril

@Nadrieril
Copy link
Member

The commonality between them is that they both correspond to "this was a const turned into a pattern". This has semantic meaning in a way that splitting it in two variants doesn't imo. E.g. if we added a new way to have a const turn into a pattern I'd want us to keep using ExpandedConst.

Said differently, I feel like the small gain at the two use sites comes at the cost of every consumer of THIR having to wonder if they should handle these two differently.

@fmease fmease assigned Nadrieril and unassigned fmease Feb 4, 2025
@Zalathar
Copy link
Contributor Author

Zalathar commented Feb 4, 2025

The commonality between them is that they both correspond to "this was a const turned into a pattern". This has semantic meaning in a way that splitting it in two variants doesn't imo. E.g. if we added a new way to have a const turn into a pattern I'd want us to keep using ExpandedConst.

That's the theory, but in practice there are zero places that make use of that semantic meaning. Currently I don't think it's even possible for code to care about that meaning, because the two kinds of “expanded constant” are handled inconsistently (e.g. by range patterns, which retain inline-const markers but discard named-const markers).

Said differently, I feel like the small gain at the two use sites comes at the cost of every consumer of THIR having to wonder if they should handle these two differently.

The gain is that I couldn't figure out what ExpandedConstant was supposed to mean, until I realised that it was two completely unrelated things that had been squished into a single PatKind because they both happen to involve “constants” and have similarly-named fields.

And right now every consumer should handle them differently (and does handle them differently), because they mean and do totally different things.


If you really think it's right for them to be the same, I'm open to ideas for how to make them less confusing and inconsistent.

But I really don't like the current situation, where they are pretending to be the same in a way that only makes both of them harder to deal with.

@Zalathar
Copy link
Contributor Author

Zalathar commented Feb 4, 2025

E.g. another plan might be:

  • Replace the is_inline (very confusing name) and def_id fields with some kind of kind: ExpandedConstKind with different variants for inline-const blocks and named constants, each holding an appropriate ID so that there's less risk of misinterpreting the common def_id.
  • Change range-pattern handling to preserve named-constant expansion markers, even though they currently aren't needed, to uphold the idea that they have semantic meaning and aren't just special-purpose nodes for different purposes.
    • (Or at least document that we currently don't do that because it isn't needed right now, but would be sensible to do if it ever is needed.)

I didn't take that approach in this PR because I assumed that separating them is simpler, but it would still be an improvement IMO.

@Nadrieril
Copy link
Member

The fact that you were confused is a strong point.

in practice there are zero places that make use of that semantic meaning

Tbf that's expected, because this shouldn't make a semantic difference. We only remember that info for diagnostic/unsafe-checking purposes.

The plan you propose is basically how I was thinking about it. I do see that propagating NamedConstant doesn't make much sense and might create confusing diagnostics though.

For context imo mixing diagnostics-related code and semantics-related code is a big source of the technical debt we have in the compiler today. Hence my preference for not doing that, which is why I argued for merging these two variants when they were introduced.

What was confusing about ExpandedConstant exactly? Should the docs have mentioned why we keep that around? Would "this THIR node is the output of const_to_pat (modulo hacks for ranges)" have been clearer?

My ideal state would be that we do unsafeck pre-THIR so we don't have to do this weird hack around range patterns, and we keep ExpandedConstant exactly as today excapt we drop the is_inline boolean.

@Zalathar
Copy link
Contributor Author

Zalathar commented Feb 4, 2025

Thanks, I think I understand the situation a bit more now.

I'll have a think about what the docs could say to make things clearer.

@Nadrieril

This comment was marked as outdated.

@Zalathar Zalathar marked this pull request as draft February 5, 2025 07:19
@Zalathar Zalathar added S-experimental Status: Ongoing experiment that does not require reviewing and won't be merged in its current state. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 5, 2025
jhpratt added a commit to jhpratt/rust that referenced this pull request Feb 14, 2025
…-errors

mir_build: Clarify some code for lowering `hir::PatExpr` to THIR

A few loosely-related improvements to the code that lowers certain parts of HIR patterns to THIR.

I was originally deferring this until after rust-lang#136529, but that PR probably won't happen, whereas these changes should hopefully be uncontroversial.

r? Nadrieril or reroll
jhpratt added a commit to jhpratt/rust that referenced this pull request Feb 15, 2025
…-errors

mir_build: Clarify some code for lowering `hir::PatExpr` to THIR

A few loosely-related improvements to the code that lowers certain parts of HIR patterns to THIR.

I was originally deferring this until after rust-lang#136529, but that PR probably won't happen, whereas these changes should hopefully be uncontroversial.

r? Nadrieril or reroll
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Feb 15, 2025
Rollup merge of rust-lang#137028 - Zalathar:thir-pat-expr, r=compiler-errors

mir_build: Clarify some code for lowering `hir::PatExpr` to THIR

A few loosely-related improvements to the code that lowers certain parts of HIR patterns to THIR.

I was originally deferring this until after rust-lang#136529, but that PR probably won't happen, whereas these changes should hopefully be uncontroversial.

r? Nadrieril or reroll
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-experimental Status: Ongoing experiment that does not require reviewing and won't be merged in its current state. 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.

4 participants