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

exhaustiveness checker mishandles const reference to ADT in match pattern #53708

Closed
hcs64 opened this issue Aug 25, 2018 · 15 comments
Closed

exhaustiveness checker mishandles const reference to ADT in match pattern #53708

hcs64 opened this issue Aug 25, 2018 · 15 comments
Assignees
Labels
A-const-eval Area: Constant evaluation (MIR interpretation) A-exhaustiveness-checking Relating to exhaustiveness / usefulness checking of patterns A-MIR Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html C-bug Category: This is a bug. E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. P-medium Medium priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@hcs64
Copy link

hcs64 commented Aug 25, 2018

Update from @pnkfelix:

Our behavior on this test case (playpen) has changed, but it is still not correct.

#[derive(PartialEq, Eq)]
struct S;

fn main() {
    const C: &S = &S;
    match C {
        C => {}
    }
}

emits error:

error[E0004]: non-exhaustive patterns: `&S` not covered
 --> src/main.rs:6:11
  |
6 |     match C {
  |           ^ pattern `&S` not covered
  |
  = help: ensure that all possible cases are being handled, possibly by adding wildcards or more match arms

If you work-around the exhaustiveness checker bug by e.g. adding an unreachable arm, then the compiler accepts the code (and as far as I can tell, assigns it the correct runtime semantics).

Original bug report follows:


struct S;

fn main() {
    const C: &S = &S;
    match C {
        C => {}
    }
}

gives:
error: internal compiler error: librustc_mir/hair/pattern/_match.rs:432: bad constructor ConstantValue(Const { ty: &S, val: Scalar(Ptr(Pointer { alloc_id: AllocId(1), offset: Size { raw: 0 } })) }) for adt S

The relevant stack is:

  14: rustc_mir::hair::pattern::_match::Constructor::variant_index_for_adt
  15: rustc_mir::hair::pattern::_match::constructor_sub_pattern_tys
  16: rustc_mir::hair::pattern::_match::is_useful_specialized
  17: <core::iter::Map<I, F> as core::iter::iterator::Iterator>::try_fold
  18: rustc_mir::hair::pattern::_match::is_useful
  19: rustc_mir::hair::pattern::_match::is_useful_specialized
  20: <core::iter::Map<I, F> as core::iter::iterator::Iterator>::try_fold
  21: rustc_mir::hair::pattern::_match::is_useful
  22: rustc_mir::hair::pattern::check_match::check_arms
  23: rustc_mir::hair::pattern::_match::MatchCheckCtxt::create_and_enter
  24: <rustc_mir::hair::pattern::check_match::MatchVisitor<'a, 'tcx> as rustc::hir::intravisit::Visitor<'tcx>>::visit_expr
  25: <rustc_mir::hair::pattern::check_match::MatchVisitor<'a, 'tcx> as rustc::hir::intravisit::Visitor<'tcx>>::visit_expr
  26: <rustc_mir::hair::pattern::check_match::MatchVisitor<'a, 'tcx> as rustc::hir::intravisit::Visitor<'tcx>>::visit_body
  27: rustc::session::Session::track_errors
  28: rustc_mir::hair::pattern::check_match::check_match
  29: rustc::ty::query::<impl rustc::ty::query::config::QueryAccessors<'tcx> for rustc::ty::query::queries::check_match<'tcx>>::compute
  30: rustc::dep_graph::graph::DepGraph::with_task_impl
  31: <rustc::ty::query::plumbing::JobOwner<'a, 'tcx, Q>>::start
  32: rustc::ty::query::plumbing::<impl rustc::ty::context::TyCtxt<'a, 'gcx, 'tcx>>::force_query_with_job
  33: rustc::ty::query::plumbing::<impl rustc::ty::context::TyCtxt<'a, 'gcx, 'tcx>>::get_query
  34: rustc::hir::intravisit::Visitor::visit_nested_body
  35: rustc::hir::Crate::visit_all_item_likes
  36: rustc_mir::hair::pattern::check_match::check_crate

It looks like this will take some effort to fix, I patched constructor_sub_pattern_tys to return vec![], but then rustc erroneously reports
error[E0004]: non-exhaustive patterns: '&S' not covered
and with an enum:

enum E {A}

fn main() {
    const C: &E = &E::A;
    match C {
        C => {}
        _ => {}
    }
}

it hits:
error: internal compiler error: librustc\traits\codegen\mod.rs:169: Encountered errors '[FulfillmentError(Obligation(predicate=Binder(TraitPredicate(<E as std::cmp::PartialEq>)),depth=1),Unimplemented)]' resolving bounds after type-checking

@memoryruins memoryruins added the I-ICE Issue: The compiler panicked, giving an Internal Compilation Error (ICE) ❄️ label Aug 25, 2018
@oli-obk
Copy link
Contributor

oli-obk commented Aug 26, 2018

cc @varkor you know this code very well. any pointers?

@varkor
Copy link
Member

varkor commented Aug 26, 2018

Okay, this is actually more interesting than first appears from the example: the matching code dealing with constants behind references is completely broken.

fn main() {
    const C: &bool = &true;
    let x = match C {
        C => "C",
        &true => "true",
        &false => "false",
    };
    println!("{}", x);
}

If you miss off the &true or &false branches, it won't compile, because it doesn't think the match is exhaustive, but if you run it, of course "C" is hit every time.
I think this will only fail when it is exhaustive, rather than thinking it's exhaustive when it's not, so I don't think it's a soundness bug, but something's going very wrong. I'll look into it.

@varkor varkor self-assigned this Aug 26, 2018
@varkor
Copy link
Member

varkor commented Aug 27, 2018

I think the problem here is this:

(&ty::Ref(_, rty, _), &PatternKind::Constant { ref value }) => {
Pattern {
ty: pat.ty,
span: pat.span,
kind: box PatternKind::Deref {
subpattern: Pattern {
ty: rty,
span: pat.span,
kind: box PatternKind::Constant { value: value.clone() },
}
}
}
}

When we encounter a constant, we try to turn it into a pattern, but the handling for references is wrong. We convert it into a deref pattern, but the underlying constant value it refers to still has type &T (and corresponding pointer value), rather than T.
I.e. the box PatternKind::Constant { value: value.clone() } line is incorrect, and should be referring to a dereferenced value instead.

@oli-obk: is there a way to dereference a ty::Const in this way?

@artemshein
Copy link

Just got it in my app. Is there a workaround or matching with constants is just broken for now?

@varkor
Copy link
Member

varkor commented Sep 27, 2018

@artemshein: I would try to avoid using constants in patterns right now; sorry – I haven't got around to fixing this yet.

@oli-obk oli-obk self-assigned this Dec 14, 2018
bors added a commit that referenced this issue Jan 15, 2019
Get rid of the fake stack frame for reading from constants

r? @RalfJung

fixes the ice in #53708 but still keeps around the wrong "non-exhaustive match" error

cc @varkor
Centril added a commit to Centril/rust that referenced this issue Jan 24, 2019
Get rid of the fake stack frame for reading from constants

r? @RalfJung

fixes the ice in rust-lang#53708 but still keeps around the wrong "non-exhaustive match" error

cc @varkor
Centril added a commit to Centril/rust that referenced this issue Jan 24, 2019
Get rid of the fake stack frame for reading from constants

r? @RalfJung

fixes the ice in rust-lang#53708 but still keeps around the wrong "non-exhaustive match" error

cc @varkor
@estebank
Copy link
Contributor

estebank commented Apr 16, 2019

So, there's something that can be done, which is calling report_fulfillment_errors which will in this case emit

error[E0277]: can't compare `S` with `S`
   |
   = help: the trait `std::cmp::PartialEq` is not implemented for `S`
   = note: required because of the requirements on the impl of `std::cmp::PartialEq` for `&S`

I do not know if this is correct though.

@oli-obk
Copy link
Contributor

oli-obk commented Apr 17, 2019

We don't want to report an error on this code. We should be accepting it as correct

@estebank estebank added A-MIR Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html C-bug Category: This is a bug. I-nominated T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Apr 19, 2019
@estebank
Copy link
Contributor

estebank commented Apr 20, 2019

@oli-obk I believe we should be accepting it as correct, but emitting the E0277 instead of an ICE can be a stop gap in the meantime to allow my current abort_if_errors removal effort to plow forward.


Edit: Scratch that, changing the code to be const C: S = S; causes the following error, which seems correct to me, right?

error: to use a constant of type `S` in a pattern, `S` must be annotated with `#[derive(PartialEq, Eq)]`
 --> ../../src/test/ui/consts/match_ice.rs:8:9
  |
8 |         C => {}
  |         ^

If that is the case, then the same error needs to be emitted for &S.

@estebank
Copy link
Contributor

I think I figured how to correctly fix the ICE (by emitting the error in my previous comment) but the exhaustiveness check is still broken.

@estebank estebank added A-const-eval Area: Constant evaluation (MIR interpretation) and removed I-ICE Issue: The compiler panicked, giving an Internal Compilation Error (ICE) ❄️ I-nominated labels Apr 20, 2019
@oli-obk
Copy link
Contributor

oli-obk commented Apr 20, 2019

won't the ICE reoccur if you add the suggested derives?

@estebank
Copy link
Contributor

In the linked commit I added a test for the struct with the derives and otherwise unchanged and we continue to get the uncovered patterns error, but otherwise things look fine.

estebank added a commit to estebank/rust that referenced this issue Apr 22, 2019
bors added a commit that referenced this issue Apr 23, 2019
Don't stop evaluating due to errors before borrow checking

r? @oli-obk

Fix #60005. Follow up to #59903. Blocked on #53708, fixing the ICE in `src/test/ui/consts/match_ice.rs`.
@pnkfelix pnkfelix changed the title ICE: const reference to ADT in match pattern rustc erroneously rejects const reference to ADT in match pattern Jun 6, 2019
@pnkfelix pnkfelix changed the title rustc erroneously rejects const reference to ADT in match pattern exhaustiveness checker mishandles const reference to ADT in match pattern Jun 6, 2019
@pnkfelix
Copy link
Member

pnkfelix commented Jun 6, 2019

triage: P-medium.

@Nadrieril
Copy link
Member

This appears to be fixed on nightly, probably due to #70743

@oli-obk oli-obk closed this as completed Oct 21, 2020
@varkor varkor added the E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. label Oct 21, 2020
@varkor
Copy link
Member

varkor commented Oct 21, 2020

(If there's already a test for this case, feel free to close again: I just want to be sure.)

Nadrieril added a commit to Nadrieril/rust that referenced this issue Oct 21, 2020
This issue was accidentally fixed recently, probably by rust-lang#70743
@Nadrieril
Copy link
Member

There wasn't. I just added one in #78072

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-const-eval Area: Constant evaluation (MIR interpretation) A-exhaustiveness-checking Relating to exhaustiveness / usefulness checking of patterns A-MIR Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html C-bug Category: This is a bug. E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. P-medium Medium priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

9 participants