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

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

Merged
merged 1 commit into from
Oct 15, 2023

Conversation

bvanjoi
Copy link
Contributor

@bvanjoi bvanjoi commented Oct 7, 2023

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

@rustbot 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 Oct 7, 2023
@compiler-errors
Copy link
Member

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
Copy link
Contributor

oli-obk commented Oct 7, 2023

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
Copy link
Member

@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
Copy link
Contributor

oli-obk commented Oct 7, 2023

we should find out why:

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

@compiler-errors
Copy link
Member

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

@oli-obk oli-obk 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 Oct 10, 2023
@rustbot
Copy link
Collaborator

rustbot commented Oct 10, 2023

Some changes might have occurred in exhaustiveness checking

cc @Nadrieril

@bvanjoi
Copy link
Contributor Author

bvanjoi commented Oct 10, 2023

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
Copy link
Contributor Author

bvanjoi commented Oct 11, 2023

ci is green.

@rustbot ready

@rustbot 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 Oct 11, 2023
@oli-obk
Copy link
Contributor

oli-obk commented Oct 11, 2023

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

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

@bvanjoi
Copy link
Contributor Author

bvanjoi commented Oct 11, 2023

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.

@bvanjoi bvanjoi changed the title remove the bug invoke during compare string const use PatKind::wild when an ADT const value has violation Oct 11, 2023
@oli-obk
Copy link
Contributor

oli-obk commented Oct 11, 2023

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

@oli-obk
Copy link
Contributor

oli-obk commented Oct 11, 2023

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Oct 11, 2023

📌 Commit 3cc0b01 has been approved by oli-obk

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 11, 2023
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Oct 11, 2023
use `PatKind::wild` when an ADT const value has violation

Fixes rust-lang#115599

Since the [to_pat](https://github.com/rust-lang/rust/pull/111913/files#diff-6d8d99538aca600d633270051580c7a9e40b35824ea2863d9dda2c85a733b5d9R126-R155) behavior has been changed in the rust-lang#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`
@bors
Copy link
Contributor

bors commented Oct 11, 2023

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

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Oct 11, 2023
@Nadrieril
Copy link
Member

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

@oli-obk
Copy link
Contributor

oli-obk commented Oct 12, 2023

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
Copy link
Member

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
Copy link
Contributor Author

bvanjoi commented Oct 12, 2023

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
Copy link
Contributor

oli-obk commented Oct 12, 2023

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
Copy link
Contributor Author

bvanjoi commented Oct 12, 2023

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
Copy link
Contributor Author

bvanjoi commented Oct 12, 2023

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
Copy link
Member

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
Copy link
Member

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

@bvanjoi bvanjoi changed the title use PatKind::wild when an ADT const value has violation use PatKind::Error when an ADT const value has violation Oct 15, 2023
@bvanjoi
Copy link
Contributor Author

bvanjoi commented Oct 15, 2023

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

@rustbot ready

@rustbot 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 Oct 15, 2023
@oli-obk
Copy link
Contributor

oli-obk commented Oct 15, 2023

@bors r+ rollup

Thanks!

@bors
Copy link
Contributor

bors commented Oct 15, 2023

📌 Commit 223674a has been approved by oli-obk

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 15, 2023
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Oct 15, 2023
use `PatKind::Error` when an ADT const value has violation

Fixes rust-lang#115599

Since the [to_pat](https://github.com/rust-lang/rust/pull/111913/files#diff-6d8d99538aca600d633270051580c7a9e40b35824ea2863d9dda2c85a733b5d9R126-R155) behavior has been changed in the rust-lang#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`
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 15, 2023
…iaskrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#115955 (Stabilize `{IpAddr, Ipv6Addr}::to_canonical`)
 - rust-lang#116033 (report `unused_import` for empty reexports even it is pub)
 - rust-lang#116172 (Broaden the consequences of recursive TLS initialization)
 - rust-lang#116341 (Implement sys::args for UEFI)
 - rust-lang#116522 (use `PatKind::Error` when an ADT const value has violation)
 - rust-lang#116732 (Make x capable of resolving symlinks)
 - rust-lang#116755 (Remove me from libcore review rotation)
 - rust-lang#116760 (Remove trivial cast in `guaranteed_eq`)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 15, 2023
…iaskrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#116172 (Broaden the consequences of recursive TLS initialization)
 - rust-lang#116341 (Implement sys::args for UEFI)
 - rust-lang#116522 (use `PatKind::Error` when an ADT const value has violation)
 - rust-lang#116732 (Make x capable of resolving symlinks)
 - rust-lang#116755 (Remove me from libcore review rotation)
 - rust-lang#116760 (Remove trivial cast in `guaranteed_eq`)
 - rust-lang#116771 (Ignore let-chains formatting)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 51be0df into rust-lang:master Oct 15, 2023
11 checks passed
@rustbot rustbot added this to the 1.75.0 milestone Oct 15, 2023
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Oct 15, 2023
Rollup merge of rust-lang#116522 - bvanjoi:fix-115599, r=oli-obk

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

Fixes rust-lang#115599

Since the [to_pat](https://github.com/rust-lang/rust/pull/111913/files#diff-6d8d99538aca600d633270051580c7a9e40b35824ea2863d9dda2c85a733b5d9R126-R155) behavior has been changed in the rust-lang#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``
bors-ferrocene bot added a commit to ferrocene/ferrocene that referenced this pull request Oct 17, 2023
54: Pull upstream master 2023 10 17 r=pietroalbini a=Veykril

* rust-lang/rust#116196
* rust-lang/rust#116824
* rust-lang/rust#116822
* rust-lang/rust#116477
* rust-lang/rust#116826
* rust-lang/rust#116820
  * rust-lang/rust#116811
  * rust-lang/rust#116808
  * rust-lang/rust#116805
  * rust-lang/rust#116800
  * rust-lang/rust#116798
  * rust-lang/rust#116754
* rust-lang/rust#114370
* rust-lang/rust#116804
  * rust-lang/rust#116802
  * rust-lang/rust#116790
  * rust-lang/rust#116786
  * rust-lang/rust#116709
  * rust-lang/rust#116430
  * rust-lang/rust#116257
  * rust-lang/rust#114157
* rust-lang/rust#116731
* rust-lang/rust#116550
* rust-lang/rust#114330
* rust-lang/rust#116724
* rust-lang/rust#116782
  * rust-lang/rust#116776
  * rust-lang/rust#115955
  * rust-lang/rust#115196
* rust-lang/rust#116775
* rust-lang/rust#114589
* rust-lang/rust#113747
* rust-lang/rust#116772
  * rust-lang/rust#116771
  * rust-lang/rust#116760
  * rust-lang/rust#116755
  * rust-lang/rust#116732
  * rust-lang/rust#116522
  * rust-lang/rust#116341
  * rust-lang/rust#116172
* rust-lang/rust#110604
* rust-lang/rust#110729
* rust-lang/rust#116527
* rust-lang/rust#116688
* rust-lang/rust#116757
  * rust-lang/rust#116753
  * rust-lang/rust#116748
  * rust-lang/rust#116741
  * rust-lang/rust#116594
* rust-lang/rust#116691
* rust-lang/rust#116643
* rust-lang/rust#116683
* rust-lang/rust#116635
* rust-lang/rust#115515
* rust-lang/rust#116742
  * rust-lang/rust#116661
  * rust-lang/rust#116576
  * rust-lang/rust#116540
* rust-lang/rust#116352
* rust-lang/rust#116737
  * rust-lang/rust#116730
  * rust-lang/rust#116723
  * rust-lang/rust#116715
  * rust-lang/rust#116603
  * rust-lang/rust#116591
  * rust-lang/rust#115439
* rust-lang/rust#116264
* rust-lang/rust#116727
  * rust-lang/rust#116704
  * rust-lang/rust#116696
  * rust-lang/rust#116695
  * rust-lang/rust#116644
  * rust-lang/rust#116630
* rust-lang/rust#116728
  * rust-lang/rust#116689
  * rust-lang/rust#116679
  * rust-lang/rust#116618
  * rust-lang/rust#116577
  * rust-lang/rust#115653
* rust-lang/rust#116702
* rust-lang/rust#116015
* rust-lang/rust#115822
* rust-lang/rust#116407
* rust-lang/rust#115719
* rust-lang/rust#115524
* rust-lang/rust#116705
* rust-lang/rust#116645
* rust-lang/rust#116233
* rust-lang/rust#115108
* rust-lang/rust#116670
* rust-lang/rust#116676
* rust-lang/rust#116666



Co-authored-by: Benoît du Garreau <bdgdlm@outlook.com>
Co-authored-by: Colin Finck <colin@reactos.org>
Co-authored-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
Co-authored-by: Joshua Liebow-Feeser <joshlf@users.noreply.github.com>
Co-authored-by: León Orell Valerian Liehr <me@fmease.dev>
Co-authored-by: Trevor Gross <tmgross@umich.edu>
Co-authored-by: Evan Merlock <evan@merlock.dev>
Co-authored-by: joboet <jonasboettiger@icloud.com>
Co-authored-by: Ralf Jung <post@ralfj.de>
Co-authored-by: DaniPopes <57450786+DaniPopes@users.noreply.github.com>
Co-authored-by: Mark Rousskov <mark.simulacrum@gmail.com>
Co-authored-by: onur-ozkan <work@onurozkan.dev>
Co-authored-by: Nicholas Nethercote <n.nethercote@gmail.com>
Co-authored-by: The 8472 <git@infinite-source.de>
Co-authored-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
Co-authored-by: reez12g <reez12g@gmail.com>
Co-authored-by: Jakub Beránek <berykubik@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ICE error when matching uninitialized constant with constant pattern in Rust
6 participants