Expand potential inner Or pattern for THIR

[ouz-a]

Code assumed there wouldn't be a deeper Or pattern inside expanded PatStack this fixes it by looking for the Or pattern inside expanded PatStack.

A more ideal solution would be recursively doing this but I haven't found a good way to do that.
fixes #97898

[rust-highfive]

r? @estebank

(rust-highfive has picked a reviewer for you, use r? to override)

[compiler-errors]

This could use a test, preferably also other negative test(s) that check that this logic is sound as well.

[estebank]

please squash your commits, r=me

[oli-obk]

Is it possible to make expand_or_pat recursively expand or patterns instead?

[ouz-a]

I couldn't make that one recursive but made a new function that is recursive.

[oli-obk]

Either the other use of expand_or_Pat is dead code now or it's still got that issue. Check whether it's dead by panicking inside of it instead of doing any logic

[ouz-a]

It can't be dead code because line 835, it's the only reason I couldn't replace the function altogether

[oli-obk]

That call may be dead after your change is what I'm thinking. Since you now expand all the nested Or patterns, line 835 may not encounter Or patterns anymore.

[ouz-a]

I did as you said, it panicked. Newly added function is only called via fn push and that is called via line 846.

[oli-obk]

I don't like that we now have one function that does the recursive work and one that doesn't. On what kind of matches did it panic? Should we call the new function at that site to reduce duplication and make it more general?

[ouz-a]

The other function just works like iterator right now, I tried to merge these two functions together but problem is, it's quite tricky because of line 838 if I remove expand_or_pat altogether and just call expand_and_extend compiler gives a warning unreachable pattern even though both of them should do the same thing at this point.

[oli-obk]

that is suspicious indeed and makes me confident that we should definitely make them work the same and figure out why they are problematic

[ouz-a]

I think I understand now why the compiler gives "unreachable pattern" err, to reach those places I need to access is_under_guard for each recursive call, but also push to the matrix has to happen after the call is_useful to trigger if v.is_empty() where the matrix has to be empty. Even if I throw all those out of the window I need to call is_useful for each v 🤷‍♂️

[estebank]

r? @oli-obk

[LeSeulArtichaut]

Here's a very similar ICE:

enum Foo {
    A,
    B,
    C,
}

fn main() {
    match Foo::A {
        | _foo @ (Foo::A | Foo::B) => {}
//      ^ note the trailing `|`
        Foo::C => {}
    };
}

The trailing | makes the pattern in the first arm to be an Or pattern with a single subpattern. I assume it should be fixed by this PR? Is it worth yet another testcase?

[ouz-a]

Here's a very similar ICE:

enum Foo {
    A,
    B,
    C,
}

fn main() {
    match Foo::A {
        | _foo @ (Foo::A | Foo::B) => {}
//      ^ note the trailing `|`
        Foo::C => {}
    };
}

The trailing | makes the pattern in the first arm to be an Or pattern with a single subpattern. I assume it should be fixed by this PR? Is it worth yet another testcase?

Yeah this PR fixes this, added it as test case, thanks!

[oli-obk]

@bors r+

[bors]

📌 Commit ddf23cb has been approved by oli-obk

It is now in the queue for this repository.