Skip to content

remove special-casing of boxes from match exhaustiveness/usefulness analysis #143414

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jul 5, 2025
Merged

remove special-casing of boxes from match exhaustiveness/usefulness analysis #143414

merged 1 commit into from
Jul 5, 2025

Conversation

dianne
Copy link
Contributor

@dianne dianne commented Jul 4, 2025 •
edited
Loading

As a first step in replacing box_patterns with deref_patterns, this treats box patterns as deref patterns in the THIR and exhaustiveness analysis. This allows a bunch of special-casing to be removed. The emitted MIR is unchanged.

Incidentally, this fixes a bug caused by box patterns being treated like structs rather than pointers, where enabling exhaustive_patterns (#51085) could give rise to spurious unreachable_patterns lints on arms required for exhaustiveness. Following the lint's advice to remove the match arm would result in an error. I'm not sure what the current state of exhaustive_patterns is with regard to reference/box opsem, or whether there's any intention to have unreachable_patterns be more granular than the whole arm, but regardless this should hopefully make them easier to handle consistently.

Tracking issue for deref patterns: #87121

r? @Nadrieril

@dianne
treat box patterns as deref patterns in THIR and usefulness analysis
98659a3 
This removes special-casing of boxes from `rustc_pattern_analysis`, as a
first step in replacing `box_patterns` with `deref_patterns`.
Incidentally, it fixes a bug caused by box patterns being represented as
structs rather than pointers, where `exhaustive_patterns` could generate
spurious `unreachable_patterns` lints on arms required for
exhaustiveness; following the lint's advice would result in an error.
@rustbot
Copy link
Collaborator

rustbot commented Jul 4, 2025

Nadrieril is currently at their maximum review capacity.
They may take a while to respond.

@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 Jul 4, 2025
@rustbot
Copy link
Collaborator

rustbot commented Jul 4, 2025

Some changes occurred in match checking

cc @Nadrieril

Some changes occurred in exhaustiveness checking

cc @Nadrieril

@Nadrieril
Copy link
Member

I'm very happy we can finally remove this :D

@bors r+

@bors
Copy link
Collaborator

bors commented Jul 4, 2025

📌 Commit 98659a3 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 Jul 4, 2025
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jul 4, 2025
@matthiaskrgr
Rollup merge of rust-lang#143414 - dianne:box-usefulness-cleanup, r=N…
958a699 
…adrieril

remove special-casing of boxes from match exhaustiveness/usefulness analysis

As a first step in replacing `box_patterns` with `deref_patterns`, this treats box patterns as deref patterns in the THIR and exhaustiveness analysis. This allows a bunch of special-casing to be removed. The emitted MIR is unchanged.

Incidentally, this fixes a bug caused by box patterns being treated like structs rather than pointers, where enabling `exhaustive_patterns` (rust-lang#51085) could give rise to spurious `unreachable_patterns` lints on arms required for exhaustiveness. Following the lint's advice to remove the match arm would result in an error. I'm not sure what the current state of `exhaustive_patterns` is with regard to reference/box opsem, or whether there's any intention to have `unreachable_patterns` be more granular than the whole arm, but regardless this should hopefully make them easier to handle consistently.

Tracking issue for deref patterns: rust-lang#87121

r? `@Nadrieril`
@matthiaskrgr matthiaskrgr mentioned this pull request Jul 4, 2025
Closed
bors added a commit that referenced this pull request Jul 4, 2025
@bors
Auto merge of #143454 - matthiaskrgr:rollup-sno3sdt, r=matthiaskrgr
6c31589 
Rollup of 9 pull requests

Successful merges:

 - #141532 (std: sys: net: uefi: tcp4: Implement write)
 - #143085 (Port `#[non_exhaustive]` to the new attribute parsing infrastructure)
 - #143298 (`tests/ui`: A New Order [23/N])
 - #143372 (Remove names_imported_by_glob_use query.)
 - #143386 (Assign dependency bump PRs to me)
 - #143406 (Remove some unnecessary `unsafe` in VecCache)
 - #143408 (mbe: Gracefully handle macro rules that end after `=>`)
 - #143414 (remove special-casing of boxes from match exhaustiveness/usefulness analysis)
 - #143444 (clean up GVN TypeId test)

r? `@ghost`
`@rustbot` modify labels: rollup
@dianne
Copy link
Contributor Author

dianne commented Jul 4, 2025 •
edited
Loading

between planning this and actually implementing it, I completely forgot to force it to check that box patterns aren't used in the same column as Box constructors; it'll probably ICE in that case. I think that check is still gated by deref_patterns since I didn't want to affect perf for matches without deref patterns or to make that first PR more complex by adding logic to avoid the perf hit. I'll try fixing that in a separate PR. I'm not too worried about that ICE causing trouble in the mean time (surely no one would actually write Box { .. }.. right? especially not in the same column as box pat).

edit: I also wholly forgot that rust-analyzer has all of this box pattern special casing too (though it sort of looks like there's no logic yet to lower box patterns from their HIR to their match_check IR?). in any case, replacing the special-casing there can probably be a separate PR to the r-a repo.

@matthiaskrgr matthiaskrgr mentioned this pull request Jul 4, 2025
Merged
bors added a commit that referenced this pull request Jul 4, 2025
@bors
Auto merge of #143459 - matthiaskrgr:rollup-gsv6uzl, r=matthiaskrgr
d98a5da 
Rollup of 8 pull requests

Successful merges:

 - #141532 (std: sys: net: uefi: tcp4: Implement write)
 - #143085 (Port `#[non_exhaustive]` to the new attribute parsing infrastructure)
 - #143372 (Remove names_imported_by_glob_use query.)
 - #143386 (Assign dependency bump PRs to me)
 - #143406 (Remove some unnecessary `unsafe` in VecCache)
 - #143408 (mbe: Gracefully handle macro rules that end after `=>`)
 - #143414 (remove special-casing of boxes from match exhaustiveness/usefulness analysis)
 - #143444 (clean up GVN TypeId test)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit e0dd7ec into rust-lang:master Jul 5, 2025
11 checks passed
@rustbot rustbot added this to the 1.90.0 milestone Jul 5, 2025
rust-timer added a commit that referenced this pull request Jul 5, 2025
@rust-timer
Unrolled build for #143414
93acf53 
Rollup merge of #143414 - dianne:box-usefulness-cleanup, r=Nadrieril

remove special-casing of boxes from match exhaustiveness/usefulness analysis

As a first step in replacing `box_patterns` with `deref_patterns`, this treats box patterns as deref patterns in the THIR and exhaustiveness analysis. This allows a bunch of special-casing to be removed. The emitted MIR is unchanged.

Incidentally, this fixes a bug caused by box patterns being treated like structs rather than pointers, where enabling `exhaustive_patterns` (#51085) could give rise to spurious `unreachable_patterns` lints on arms required for exhaustiveness. Following the lint's advice to remove the match arm would result in an error. I'm not sure what the current state of `exhaustive_patterns` is with regard to reference/box opsem, or whether there's any intention to have `unreachable_patterns` be more granular than the whole arm, but regardless this should hopefully make them easier to handle consistently.

Tracking issue for deref patterns: #87121

r? `@Nadrieril`
This was referenced Jul 5, 2025
Merged
Merged
@dianne dianne deleted the box-usefulness-cleanup branch July 6, 2025 00:53
bors added a commit that referenced this pull request Jul 9, 2025
@bors
Auto merge of #143472 - dianne:deref-pat-column-check, r=Nadrieril
6b3ae3f 
`rustc_pattern_analysis`: always check that deref patterns don't match on the same place as normal constructors

In #140106, deref pattern validation was tied to the `deref_patterns` feature to temporarily avoid affecting perf. However:
- As of #143414, box patterns are represented as deref patterns in `rustc_pattern_analysis`. Since they can be used by enabling `box_patterns` instead of `deref_patterns`, it was possible for them to skip validation, resulting in an ICE. This fixes that and adds a regression test.
- External tooling (e.g. rust-analyzer) will also need to validate matches containing deref patterns, which was not possible. This fixes that by making `compute_match_usefulness` validate deref patterns by default.

In order to avoid doing an extra pass for anything with patterns, the second commit makes `RustcPatCtxt` keep track of whether it encounters a deref pattern, so that it only does the check if so. This is purely for performance. If the perf impact of the first commit is negligible and the complexity cost introduced by the second commit is significant, it may be worth dropping the latter.

r? `@Nadrieril`
github-actions bot pushed a commit to devnexen/miri that referenced this pull request Jul 10, 2025
@bors
Auto merge of #143472 - dianne:deref-pat-column-check, r=Nadrieril
55e7236 
`rustc_pattern_analysis`: always check that deref patterns don't match on the same place as normal constructors

In rust-lang/rust#140106, deref pattern validation was tied to the `deref_patterns` feature to temporarily avoid affecting perf. However:
- As of rust-lang/rust#143414, box patterns are represented as deref patterns in `rustc_pattern_analysis`. Since they can be used by enabling `box_patterns` instead of `deref_patterns`, it was possible for them to skip validation, resulting in an ICE. This fixes that and adds a regression test.
- External tooling (e.g. rust-analyzer) will also need to validate matches containing deref patterns, which was not possible. This fixes that by making `compute_match_usefulness` validate deref patterns by default.

In order to avoid doing an extra pass for anything with patterns, the second commit makes `RustcPatCtxt` keep track of whether it encounters a deref pattern, so that it only does the check if so. This is purely for performance. If the perf impact of the first commit is negligible and the complexity cost introduced by the second commit is significant, it may be worth dropping the latter.

r? `@Nadrieril`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@Nadrieril Nadrieril

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
Milestone
1.90.0
Development

Successfully merging this pull request may close these issues.
4 participants
@dianne @rustbot @Nadrieril @bors