Add a comment pointing to ICE-136223

[Shunpoco]

Fixes #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). 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). 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.

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]

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]

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

[Nadrieril]

cc @dianne

[Nadrieril]

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]

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]

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]

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]

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

[Nadrieril]

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]

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

[Shunpoco]

@rustbot review

[Nadrieril]

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

[Shunpoco]

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

I've done 😸

[Nadrieril]

alright, thank you :)

@bors r+

[bors]

📌 Commit ba12489 has been approved by Nadrieril

It is now in the queue for this repository.

[Nadrieril]

@bors rollup