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

internal: Update to latest rustc_pattern_analysis #16533

Merged
merged 3 commits into from
Feb 12, 2024

Conversation

Nadrieril
Copy link
Member

@Nadrieril Nadrieril commented Feb 11, 2024

Here I go again. Two improvements this time.

  1. I've removed the need for an arena for patterns. Turns out this wasn't gaining any performance on the rustc side so may as well allocate normally.
  2. I've added a clean error path when types don't match, so rustc_pattern_analysis should never panic except internal logic errors. For now cx.bug() calls never!() but I'm not sure what never!() does. Does it display anything to the user? Otherwise a debug!() should be sufficient.

Point 2 should fix #15883 but I haven't tested it because I'm not sure how to reproduce. Could someone give me pointers as to how to write a test for the pattern code?

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 11, 2024
@HKalbasi
Copy link
Member

never! panics with debug assertions enabled and logs otherwise. It's good for the cx.bug().

About the test, I can see some panic with this test:

check_diagnostics_no_bails(
            r#"
        //- minicore: option
        fn main() {
            match Some((true, false)) {
                Some(true) | Some(false) => {}
                None => {}
            }
        }
            "#,
        );

@Nadrieril
Copy link
Member Author

Oh perfect, check_diagnostics_no_bails is what I was looking for. This does panic indeed, but only because of the call to never!(). If I remove it I get the type error that we should get. Does that mean I shold remove the never call?

@Nadrieril
Copy link
Member Author

I changed it to debug! so that I could add a regression test

@HKalbasi
Copy link
Member

I think ideally cx.bug() should never be called and Err should handle these cases and we should use never here. Alternatively we can consider calling match check only on bodies with successful type check, similar to rustc, to prevent similar panics and bugs.

But removing the cx.bug here requires updating the rustc crate, so let's merge this for now.
@bors r+

@bors
Copy link
Contributor

bors commented Feb 12, 2024

📌 Commit 43caf83 has been approved by HKalbasi

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Feb 12, 2024

⌛ Testing commit 43caf83 with merge b7972e5...

@bors
Copy link
Contributor

bors commented Feb 12, 2024

☀️ Test successful - checks-actions
Approved by: HKalbasi
Pushing b7972e5 to master...

@bors bors merged commit b7972e5 into rust-lang:master Feb 12, 2024
11 checks passed
@lnicola lnicola changed the title Update to latest rustc_pattern_analysis internal: Update to latest rustc_pattern_analysis Feb 12, 2024
@Nadrieril Nadrieril deleted the update-pat-ana branch February 12, 2024 13:55
@Nadrieril
Copy link
Member Author

Alternatively we can consider calling match check only on bodies with successful type check

That was supposed to be the case, this PR is for the edge cases where we fall to catch the type error

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"index out of bounds" with incorrect match
4 participants