Isolate the diagnostic code that expects thir::Pat to be printable
#128304
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
In several cases this avoids the need to clone the underlying pattern, and then print the clone later.
|
r? @fee1-dead rustbot has assigned @fee1-dead. Use |
rustbot
added
S-waiting-on-review
|
Some changes occurred in exhaustiveness checking cc @Nadrieril Some changes occurred in match checking cc @Nadrieril |
|
Noting mostly for my own reference, another mention of obstacles to switching over to |
|
It looks like |
This hides the fact that we print `WitnessPat` by converting it to `thir::Pat` and then printing that.
This gives a clearer view of the (diagnostic) code that expects to be able to print THIR patterns, and makes it possible to experiment with requiring some kind of context (for ID lookup) when printing patterns.
Zalathar
force-pushed
the
thir-pat-display
branch
from
3ba3ec2 to
ae0ec73
Compare
|
Looks good, thank you! r? Nadrieril @bors r+ |
bors
added
S-waiting-on-bors
matthiaskrgr
added a commit
to matthiaskrgr/rust
that referenced
this pull request
Jul 29, 2024
…eril Isolate the diagnostic code that expects `thir::Pat` to be printable Currently, `thir::Pat` implements `fmt::Display` (and `IntoDiagArg`) directly, for use by a few diagnostics. That makes it tricky to experiment with alternate representations for THIR patterns, because the patterns currently need to be printable on their own. That immediately rules out possibilities like storing subpatterns as a `PatId` index into a central list (instead of the current directly-owned `Box<Pat>`). This PR therefore takes an incremental step away from that obstacle, by removing `thir::Pat` from diagnostic structs in `rustc_pattern_analysis`, and hiding the pattern-printing process behind a single public `Pat::to_string` method. Doing so makes it easier to identify and update the code that wants to print patterns, and gives a place to pass in additional context in the future if necessary. --- I'm currently not sure whether switching over to `PatId` is actually desirable or not, but I think this change makes sense on its own merits, by reducing the coupling between `thir::Pat` and the pattern-analysis error types.
This was referenced Jul 29, 2024
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Jul 29, 2024
…iaskrgr Rollup of 6 pull requests Successful merges: - rust-lang#128182 (handle no_std targets on std builds) - rust-lang#128277 (miri: fix offset_from behavior on wildcard pointers) - rust-lang#128304 (Isolate the diagnostic code that expects `thir::Pat` to be printable) - rust-lang#128307 (Clean and enable `rustdoc::unescaped_backticks` for `core/alloc/std/test/proc_macro`) - rust-lang#128322 (CI: move RFL job forward to v6.11-rc1) - rust-lang#128333 (Miri subtree update) r? `@ghost` `@rustbot` modify labels: rollup
rust-timer
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Jul 29, 2024
Rollup merge of rust-lang#128304 - Zalathar:thir-pat-display, r=Nadrieril Isolate the diagnostic code that expects `thir::Pat` to be printable Currently, `thir::Pat` implements `fmt::Display` (and `IntoDiagArg`) directly, for use by a few diagnostics. That makes it tricky to experiment with alternate representations for THIR patterns, because the patterns currently need to be printable on their own. That immediately rules out possibilities like storing subpatterns as a `PatId` index into a central list (instead of the current directly-owned `Box<Pat>`). This PR therefore takes an incremental step away from that obstacle, by removing `thir::Pat` from diagnostic structs in `rustc_pattern_analysis`, and hiding the pattern-printing process behind a single public `Pat::to_string` method. Doing so makes it easier to identify and update the code that wants to print patterns, and gives a place to pass in additional context in the future if necessary. --- I'm currently not sure whether switching over to `PatId` is actually desirable or not, but I think this change makes sense on its own merits, by reducing the coupling between `thir::Pat` and the pattern-analysis error types.
Zalathar
added a commit
to Zalathar/rust
that referenced
this pull request
Jul 31, 2024
This reverts commit ae0ec73. The original change in rust-lang#128304 was intended to be a step towards being able to print `thir::Pat` even after switching to `PatId`. But because the only patterns that need to be printed are the synthetic ones created by pattern analysis (for diagnostic purposes only), it makes more sense to completely separate the printable patterns from the real THIR patterns.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently,
thir::Patimplementsfmt::Display(andIntoDiagArg) directly, for use by a few diagnostics.That makes it tricky to experiment with alternate representations for THIR patterns, because the patterns currently need to be printable on their own. That immediately rules out possibilities like storing subpatterns as a
PatIdindex into a central list (instead of the current directly-ownedBox<Pat>).This PR therefore takes an incremental step away from that obstacle, by removing
thir::Patfrom diagnostic structs inrustc_pattern_analysis, and hiding the pattern-printing process behind a single publicPat::to_stringmethod. Doing so makes it easier to identify and update the code that wants to print patterns, and gives a place to pass in additional context in the future if necessary.I'm currently not sure whether switching over to
PatIdis actually desirable or not, but I think this change makes sense on its own merits, by reducing the coupling betweenthir::Patand the pattern-analysis error types.