use PatKind::Error when an ADT const value has violation

[bvanjoi]

Fixes #115599

Since the to_pat behavior has been changed in the #111913 update, the kind of inlined_const_ast_pat has transformed from PatKind::Leaf { pattern: Pat { kind: Wild, ..} } to PatKind::Constant. This caused a scenario where there are no matched candidates, leading to a testing of the candidates. This process ultimately attempts to test the string const, triggering the bug! invocation finally.

r? @oli-obk

[compiler-errors]

This is confusing -- I expect this to be a hard error. Also, why is this being considered an indirect_structural_match, there's no indirection here, right?

[oli-obk]

I've been trying to use godbolt on my phone for the last 5 mins and failing, so I can't check what the behaviour used to be. But I also expect it to have gone from an error to an ICE. If that is so, we should figure out how to get the error back

[compiler-errors]

@oli is correct, this was an error on 1.69:

error: to use a constant of type `String` in a pattern, `String` must be annotated with `#[derive(PartialEq, Eq)]`
 --> <source>:7:12
  |
7 |     if let CONST_STRING = empty_str {}
  |            ^^^^^^^^^^^^

And then it became a different error in 1.70 -- we should find out why:

error: to use a constant of type `Vec<u8>` in a pattern, `Vec<u8>` must be annotated with `#[derive(PartialEq, Eq)]`
 --> <source>:7:12
  |
7 |     if let CONST_STRING = empty_str {}
  |            ^^^^^^^^^^^^
  |
  = note: the traits must be derived, manual `impl`s are not sufficient
  = note: see https://doc.rust-lang.org/stable/std/marker/trait.StructuralEq.html for details

And then it became an ICE in 1.72

[oli-obk]

we should find out why:

Did String's PartialEq impl get removed and replaced by a derive?

[compiler-errors]

@oli-obk: ohhh, you're right it did #109980

[rustbot]

Some changes might have occurred in exhaustiveness checking

cc @Nadrieril

[bvanjoi]

why is this being considered an indirect_structural_match, there's no indirection here, right?

According to my understanding, PartialEq for Vec is implemented as shown here: https://doc.rust-lang.org/src/alloc/vec/partial_eq.rs.html#23. This implies that comparison between constants of type Vec is indirect. Maybe I have some misunderstandings regarding this...

If that is so, we should figure out how to get the error back

I've implemented a special handling for mir::Const::Val: if it's of type ty::Adt and contains an unmarked type, it will immediately throw an error.

[bvanjoi]

ci is green.

@rustbot ready

[oli-obk]

I've implemented a special handling for mir::Const::Val

why is this just for mir::Const::Val?

[bvanjoi]

mir::ConstantKind::ty has already been handled by self.recur, for example:

#[derive(PartialEq, Eq)]
pub enum Foo {
    FooA(()),
    FooB(Vec<()>),
}

impl Foo {
    const A1: Foo = Foo::FooA(());
}

fn main() {
    let foo = Foo::FooA(());
    match foo { 
        Foo::A1 => {},
        _ => {},
    }
}

Although Vec<()> violates structural match, only the Foo::FooA(()) branch is retained after recur. Therefore, I think it shouldn't throw an error.

[oli-obk]

That makes sense, thanks! Please add that as an explanation for why we only care about Val here.

[oli-obk]

@bors r+ rollup

[bors]

📌 Commit 3cc0b01 has been approved by oli-obk

It is now in the queue for this repository.

[bors]

☔ The latest upstream changes (presumably #115937) made this pull request unmergeable. Please resolve the merge conflicts.

[Nadrieril]

This is starting to feel like we need PatKind::Error. I might try that again

[oli-obk]

This is starting to feel like we need PatKind::Error. I might try that again

If we have to handle error Constants and types anyway, is it worth a PatKind::Error? I guess we don't know without an impl 😅

[Nadrieril]

I had another case where returning an error through PatKind::Constant caused a panic because the code relied on the constant evaluating to a number

[bvanjoi]

That can probably be avoided with ... as then the String type does not show up anymore.

I attempted it and it indeed entered into the perform_test logic resulting in a panic.

would doing that everywhere solve your issue, too?

I don't quite understand this advice. Did you mean to say:

if let Some(non_sm_ty) = structural {
      let ty = Ty::new_error(tcx, e);
      PatKind::Constant { value: mir::Const::Ty(ty::Const::new_error(tcx, e, ty)) }
}

[oli-obk]

I don't quite understand this advice. Did you mean to say:

basically my hope is that if every site in this module uses ty::Const::new_error with let ty = Ty::new_error(tcx, e); instead of the type from the function argument or from the constant, we'd also avoid your issue, but I assume that's what you meant with

I attempted it and it indeed entered into the perform_test logic resulting in a panic.

giving an entirely new panic, not the bug! that you are fixing.

If so, let's just land your PR as it is and we'll figure out more refactorings later

[bvanjoi]

giving an entirely new panic, not the bug! that you are fixing.

It's not a new panic, merely the bug! that I am aiming to fix.

[bvanjoi]

basically my hope is that if every site in this module uses ty::Const::new_error with let ty = Ty::new_error(tcx, e); instead of the type from the function argument or from the constant, we'd also avoid your issue,

I will try it, @rustbot author

[Nadrieril]

Alright, I introduce PatKind::Error in #116715. If #116715 gets merged first you can use it instead of PatKind::Wild. If this gets merged first I'll take care of it.

[Nadrieril]

#116715 got merged! Could you try PatKind::Error(e) instead of PatKind::Wild now?

[bvanjoi]

I've utilized PathKind::Error, which aids in removing the irrefutable if let pattern warning.

@rustbot ready

[oli-obk]

@bors r+ rollup

Thanks!

[bors]

📌 Commit 223674a has been approved by oli-obk

It is now in the queue for this repository.