Add a comment pointing to ICE-136223 #136397

Shunpoco · 2025-02-01T20:08:19Z

Steps how the ICE happen

This explanation is based on the test case &Some(Some(x)) = &Some(&mut Some(0)).
The case should fail with E0596 error, but it catches the debug assertion instead.

For the first &: In check_pat_ref(), the value max_ref_mutbl becomes MutblCap::Not (here). Once max_ref_mutbl becomes Not, it will never be back to MutblCap::Mut.
For &mut: In peel_off_references(), because Some(x) doesn't have & nor &mut, &mut in &mut Some(0) is not consumed then default_binding_mode (def_br) becomes ByRef::Yes(Mutability::Mut) (around here). This will be inherited to the next step. So this pattern has the mismatch between def_br=Yes(Mut) and max_ref_mutbl=Not now.
For the value 0: Because of the step 2, the default_binding_mode is Yes(Mut), but max_ref_mutbl is Not from the step 1. It causes the assertion error here.

What this PR fixes

Step 1 has happened from this commit by deleting no_ref_mut_behind_and from the if block. In my understanding, after RFC3627 is released, step 1 should happen not only 2024 edition but also other editions to track MutblCap value. But for now, it should not happen for non-2024 edition. So I put it back.

NOTE: I think there is another solution - We should return an E0596 error in calc_default_binding_mode() instead of the debug assertion. Since the assertion is caused by the mismatch between def_br = Yes(Mut) and max_ref_mutbl = Not, but in my understanding this violation is the same as E0596. check_pat_ident() does returns E0596 by a similar reason here.

rustbot · 2025-02-01T20:08:27Z

r? @petrochenkov

rustbot has assigned @petrochenkov.
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

Shunpoco · 2025-02-01T20:34:23Z

r? @Nadrieril
I think you're the best person to review it since you reviewed #127008 and #135337 ? 🙏

Nadrieril · 2025-02-04T09:09:55Z

cc @dianne

Nadrieril · 2025-02-04T09:14:38Z

While this fix would work, this is fixing the wrong thing imo: we should be able to compute this cap regardless of features. I'm guessing removing the bug! call would suffice, or make it conditional ont he features for which it makes sense

dianne · 2025-02-04T09:38:21Z

oh no, I thought I already fixed this. it completely slipped my mind that it might need a backport. or maybe I messed up with the fix. I'm sorry! I'll try reproducing this myself, but #135434 should already make the debug assert depend on currently enabled feature gates (see a56f9ad and #135434 (comment)). it's merged, so if you pull from master, you should see this:

        #[cfg(debug_assertions)]
        if def_br == ByRef::Yes(Mutability::Mut)
            && max_ref_mutbl != MutblCap::Mut
            && self.downgrade_mut_inside_shared()
        {
            span_bug!(pat.span, "Pattern mutability cap violated!");
        }

Edit: I can't reproduce it anymore, so I think it is indeed already fixed.

Shunpoco · 2025-02-04T09:55:50Z

Thanks both!
Since #135434 already solved this regression, I want to merge only the test code to make sure we will not have the regression. Is that make sense? (I will revert my change on pat.rs)

dianne · 2025-02-04T10:02:27Z

I agree there should be a regression test. Technically, the test is present in-tree here already:

rust/tests/ui/pattern/rfc-3627-match-ergonomics-2024/experimental/borrowck-errors.rs

Line 35 in 8a8b464

if let &Some(Some(x)) = &Some(&mut Some(0)) {

but documenting it as a regression test in some way could be good.

Shunpoco · 2025-02-04T10:36:14Z

@dianne
Thanks. So I will add a comment on the test case on rfc-3627-match-ergonomics-2024/experimental/borrowck-errors.rs.

Nadrieril · 2025-02-04T11:12:08Z

Ahh ok that makes more sense, I was indeed sure we had this test in our test suite. I didn't consider the possible race condition.

Shunpoco · 2025-02-05T00:27:41Z

I reverted my change/test, and added a comment to borrowck-errors.rs.

Shunpoco · 2025-02-05T00:27:56Z

@rustbot review

Nadrieril · 2025-02-07T00:13:38Z

This will clash with #136577. Once that is merged, could you rebase and squash this PR?

Signed-off-by: Shunpoco <tkngsnsk313320@gmail.com>

Shunpoco · 2025-02-07T23:09:38Z

This will clash with #136577. Once that is merged, could you rebase and squash this PR?

I've done 😸

Nadrieril · 2025-02-08T00:59:42Z

alright, thank you :)

@bors r+

bors · 2025-02-08T00:59:45Z

📌 Commit ba12489 has been approved by Nadrieril

It is now in the queue for this repository.

Nadrieril · 2025-02-08T00:59:50Z

@bors rollup

…mutability-cap-violated, r=Nadrieril Add a comment pointing to ICE-136223 Fixes rust-lang#136223 ## Steps how the ICE happen This explanation is based on the test case `&Some(Some(x)) = &Some(&mut Some(0))`. The case should fail with E0596 error, but it catches the debug assertion instead. 1. For the first `&`: In check_pat_ref(), the value max_ref_mutbl becomes MutblCap::Not ([here](https://github.com/rust-lang/rust/blob/fdd1a3b02687817cea41f6bacae3d5fbed2b2cd0/compiler/rustc_hir_typeck/src/pat.rs#L2394-L2396)). Once max_ref_mutbl becomes Not, it will never be back to MutblCap::Mut. 2. For `&mut`: In peel_off_references(), because Some(x) doesn't have `&` nor `&mut`, `&mut` in `&mut Some(0)` is not consumed then default_binding_mode (def_br) becomes `ByRef::Yes(Mutability::Mut)` (around [here](https://github.com/rust-lang/rust/blob/fdd1a3b02687817cea41f6bacae3d5fbed2b2cd0/compiler/rustc_hir_typeck/src/pat.rs#L519-L536)). This will be inherited to the next step. So this pattern has the mismatch between `def_br=Yes(Mut)` and `max_ref_mutbl=Not` now. 3. For the value `0`: Because of the step 2, the default_binding_mode is `Yes(Mut)`, but max_ref_mutbl is `Not` from the step 1. It causes the assertion error [here](https://github.com/rust-lang/rust/blob/fdd1a3b02687817cea41f6bacae3d5fbed2b2cd0/compiler/rustc_hir_typeck/src/pat.rs#L427-L430). ## What this PR fixes Step 1 has happened from [this commit](rust-lang@e2f3ce9) by deleting `no_ref_mut_behind_and` from the if block. In my understanding, after RFC3627 is released, step 1 should happen not only 2024 edition but also other editions to track MutblCap value. But for now, it should not happen for non-2024 edition. So I put it back. NOTE: I think there is another solution - We should return an E0596 error in calc_default_binding_mode() instead of the debug assertion. Since the assertion is caused by the mismatch between `def_br = Yes(Mut)` and `max_ref_mutbl = Not`, but in my understanding this violation is the same as E0596. check_pat_ident() does returns E0596 by a similar reason [here](https://github.com/rust-lang/rust/blob/fdd1a3b02687817cea41f6bacae3d5fbed2b2cd0/compiler/rustc_hir_typeck/src/pat.rs#L837-L856).

…kingjubilee Rollup of 13 pull requests Successful merges: - rust-lang#134999 (Add cygwin target.) - rust-lang#135439 (Make `-O` mean `OptLevel::Aggressive`) - rust-lang#136397 (Add a comment pointing to ICE-136223) - rust-lang#136681 (resolve `llvm-config` path properly on cross builds) - rust-lang#136686 (Clean up `HashMap` and `HashSet` docs.) - rust-lang#136694 (Update minifier version to `0.3.4`) - rust-lang#136706 (compiler: mostly-finish `rustc_abi` updates) - rust-lang#136710 (Document `Sum::sum` returns additive identities for `[]`) - rust-lang#136724 (Make `AsyncFnOnce`, `AsyncFnMut`, `AsyncFn` non-`#[fundamental]`) - rust-lang#136727 (Have a break from review rotation) - rust-lang#136730 (transmutability: fix ICE when passing wrong ADT to ASSUME) - rust-lang#136736 (Small resolve refactor) - rust-lang#136746 (Emit an error if `-Zdwarf-version=1` is requested) r? `@ghost` `@rustbot` modify labels: rollup

…mutability-cap-violated, r=Nadrieril Add a comment pointing to ICE-136223 Fixes rust-lang#136223 ## Steps how the ICE happen This explanation is based on the test case `&Some(Some(x)) = &Some(&mut Some(0))`. The case should fail with E0596 error, but it catches the debug assertion instead. 1. For the first `&`: In check_pat_ref(), the value max_ref_mutbl becomes MutblCap::Not ([here](https://github.com/rust-lang/rust/blob/fdd1a3b02687817cea41f6bacae3d5fbed2b2cd0/compiler/rustc_hir_typeck/src/pat.rs#L2394-L2396)). Once max_ref_mutbl becomes Not, it will never be back to MutblCap::Mut. 2. For `&mut`: In peel_off_references(), because Some(x) doesn't have `&` nor `&mut`, `&mut` in `&mut Some(0)` is not consumed then default_binding_mode (def_br) becomes `ByRef::Yes(Mutability::Mut)` (around [here](https://github.com/rust-lang/rust/blob/fdd1a3b02687817cea41f6bacae3d5fbed2b2cd0/compiler/rustc_hir_typeck/src/pat.rs#L519-L536)). This will be inherited to the next step. So this pattern has the mismatch between `def_br=Yes(Mut)` and `max_ref_mutbl=Not` now. 3. For the value `0`: Because of the step 2, the default_binding_mode is `Yes(Mut)`, but max_ref_mutbl is `Not` from the step 1. It causes the assertion error [here](https://github.com/rust-lang/rust/blob/fdd1a3b02687817cea41f6bacae3d5fbed2b2cd0/compiler/rustc_hir_typeck/src/pat.rs#L427-L430). ## What this PR fixes Step 1 has happened from [this commit](rust-lang@e2f3ce9) by deleting `no_ref_mut_behind_and` from the if block. In my understanding, after RFC3627 is released, step 1 should happen not only 2024 edition but also other editions to track MutblCap value. But for now, it should not happen for non-2024 edition. So I put it back. NOTE: I think there is another solution - We should return an E0596 error in calc_default_binding_mode() instead of the debug assertion. Since the assertion is caused by the mismatch between `def_br = Yes(Mut)` and `max_ref_mutbl = Not`, but in my understanding this violation is the same as E0596. check_pat_ident() does returns E0596 by a similar reason [here](https://github.com/rust-lang/rust/blob/fdd1a3b02687817cea41f6bacae3d5fbed2b2cd0/compiler/rustc_hir_typeck/src/pat.rs#L837-L856).

…kingjubilee Rollup of 10 pull requests Successful merges: - rust-lang#136397 (Add a comment pointing to ICE-136223) - rust-lang#136681 (resolve `llvm-config` path properly on cross builds) - rust-lang#136686 (Clean up `HashMap` and `HashSet` docs.) - rust-lang#136694 (Update minifier version to `0.3.4`) - rust-lang#136706 (compiler: mostly-finish `rustc_abi` updates) - rust-lang#136710 (Document `Sum::sum` returns additive identities for `[]`) - rust-lang#136724 (Make `AsyncFnOnce`, `AsyncFnMut`, `AsyncFn` non-`#[fundamental]`) - rust-lang#136727 (Have a break from review rotation) - rust-lang#136730 (transmutability: fix ICE when passing wrong ADT to ASSUME) - rust-lang#136736 (Small resolve refactor) r? `@ghost` `@rustbot` modify labels: rollup

…kingjubilee Rollup of 8 pull requests Successful merges: - rust-lang#136397 (Add a comment pointing to ICE-136223) - rust-lang#136686 (Clean up `HashMap` and `HashSet` docs.) - rust-lang#136706 (compiler: mostly-finish `rustc_abi` updates) - rust-lang#136710 (Document `Sum::sum` returns additive identities for `[]`) - rust-lang#136724 (Make `AsyncFnOnce`, `AsyncFnMut`, `AsyncFn` non-`#[fundamental]`) - rust-lang#136727 (Have a break from review rotation) - rust-lang#136730 (transmutability: fix ICE when passing wrong ADT to ASSUME) - rust-lang#136736 (Small resolve refactor) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#136397 - Shunpoco:issue-136223-ICE-pattern-mutability-cap-violated, r=Nadrieril Add a comment pointing to ICE-136223 Fixes rust-lang#136223 ## Steps how the ICE happen This explanation is based on the test case `&Some(Some(x)) = &Some(&mut Some(0))`. The case should fail with E0596 error, but it catches the debug assertion instead. 1. For the first `&`: In check_pat_ref(), the value max_ref_mutbl becomes MutblCap::Not ([here](https://github.com/rust-lang/rust/blob/fdd1a3b02687817cea41f6bacae3d5fbed2b2cd0/compiler/rustc_hir_typeck/src/pat.rs#L2394-L2396)). Once max_ref_mutbl becomes Not, it will never be back to MutblCap::Mut. 2. For `&mut`: In peel_off_references(), because Some(x) doesn't have `&` nor `&mut`, `&mut` in `&mut Some(0)` is not consumed then default_binding_mode (def_br) becomes `ByRef::Yes(Mutability::Mut)` (around [here](https://github.com/rust-lang/rust/blob/fdd1a3b02687817cea41f6bacae3d5fbed2b2cd0/compiler/rustc_hir_typeck/src/pat.rs#L519-L536)). This will be inherited to the next step. So this pattern has the mismatch between `def_br=Yes(Mut)` and `max_ref_mutbl=Not` now. 3. For the value `0`: Because of the step 2, the default_binding_mode is `Yes(Mut)`, but max_ref_mutbl is `Not` from the step 1. It causes the assertion error [here](https://github.com/rust-lang/rust/blob/fdd1a3b02687817cea41f6bacae3d5fbed2b2cd0/compiler/rustc_hir_typeck/src/pat.rs#L427-L430). ## What this PR fixes Step 1 has happened from [this commit](rust-lang@e2f3ce9) by deleting `no_ref_mut_behind_and` from the if block. In my understanding, after RFC3627 is released, step 1 should happen not only 2024 edition but also other editions to track MutblCap value. But for now, it should not happen for non-2024 edition. So I put it back. NOTE: I think there is another solution - We should return an E0596 error in calc_default_binding_mode() instead of the debug assertion. Since the assertion is caused by the mismatch between `def_br = Yes(Mut)` and `max_ref_mutbl = Not`, but in my understanding this violation is the same as E0596. check_pat_ident() does returns E0596 by a similar reason [here](https://github.com/rust-lang/rust/blob/fdd1a3b02687817cea41f6bacae3d5fbed2b2cd0/compiler/rustc_hir_typeck/src/pat.rs#L837-L856).

rustbot assigned petrochenkov Feb 1, 2025

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 1, 2025

rustbot assigned Nadrieril and unassigned petrochenkov Feb 1, 2025

Shunpoco marked this pull request as ready for review February 1, 2025 21:02

dianne mentioned this pull request Feb 4, 2025

ICE: Pattern mutability cap violated! #136223

Closed

Nadrieril added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 4, 2025

Shunpoco force-pushed the issue-136223-ICE-pattern-mutability-cap-violated branch from a9a2dec to d65b564 Compare February 4, 2025 23:37

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 5, 2025

Add comment for regression rust-lang#136223 on borrowck-errors.rs

ba12489

Signed-off-by: Shunpoco <tkngsnsk313320@gmail.com>

Shunpoco force-pushed the issue-136223-ICE-pattern-mutability-cap-violated branch from d65b564 to ba12489 Compare February 7, 2025 22:04

bors removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 8, 2025

bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Feb 8, 2025

Nadrieril changed the title ~~Fix ICE-136223: &mut inside & should not panic by Pattern mutability cap violated! assertion~~ Add a comment pointing to ICE-136223 Feb 8, 2025

workingjubilee mentioned this pull request Feb 8, 2025

Rollup of 13 pull requests #136752

Closed

workingjubilee mentioned this pull request Feb 9, 2025

Rollup of 10 pull requests #136757

Closed

workingjubilee mentioned this pull request Feb 9, 2025

Rollup of 8 pull requests #136762

Merged

bors merged commit ef22c14 into rust-lang:master Feb 9, 2025
6 checks passed

rustbot added this to the 1.86.0 milestone Feb 9, 2025

Shunpoco deleted the issue-136223-ICE-pattern-mutability-cap-violated branch February 9, 2025 13:07

Add a comment pointing to ICE-136223 #136397

Add a comment pointing to ICE-136223 #136397

Uh oh!

Conversation

Shunpoco commented Feb 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Steps how the ICE happen

What this PR fixes

Uh oh!

rustbot commented Feb 1, 2025

Uh oh!

Shunpoco commented Feb 1, 2025

Uh oh!

Nadrieril commented Feb 4, 2025

Uh oh!

Nadrieril commented Feb 4, 2025

Uh oh!

dianne commented Feb 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Shunpoco commented Feb 4, 2025

Uh oh!

dianne commented Feb 4, 2025

Uh oh!

Shunpoco commented Feb 4, 2025

Uh oh!

Nadrieril commented Feb 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Shunpoco commented Feb 5, 2025

Uh oh!

Shunpoco commented Feb 5, 2025

Uh oh!

Nadrieril commented Feb 7, 2025

Uh oh!

Shunpoco commented Feb 7, 2025

Uh oh!

Nadrieril commented Feb 8, 2025

Uh oh!

bors commented Feb 8, 2025

Uh oh!

Nadrieril commented Feb 8, 2025

Uh oh!

Uh oh!

Uh oh!

Shunpoco commented Feb 1, 2025 •

edited

Loading

dianne commented Feb 4, 2025 •

edited

Loading

Nadrieril commented Feb 4, 2025 •

edited

Loading