ICE resolving non-existent PartialEq::Eq from match of const #65466
Assignees
Labels
A-mir
Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html
C-bug
Category: This is a bug.
glacier
ICE tracked in rust-lang/glacier.
I-ICE
Issue: The compiler panicked, giving an Internal Compilation Error (ICE) ❄️
P-high
High priority
T-compiler
Relevant to the compiler team, which will review and decide on the PR/issue.
Comments
pnkfelix
added
A-mir
pnkfelix
added a commit
to pnkfelix/rust
that referenced
this issue
Oct 25, 2019
(Or more precisely, a pair of such traits: one for `derive(PartialEq)` and one for `derive(Eq)`.) ((The addition of the second marker trait, `StructuralEq`, is largely a hack to work-around `fn (&T)` not implementing `PartialEq` and `Eq`; see also issue rust-lang#46989; otherwise I would just check if `Eq` is implemented.)) Note: this does not use trait fulfillment error-reporting machinery; it just uses the trait system to determine if the ADT was tagged or not. (Nonetheless, I have kept an `on_unimplemented` message on the new trait for structural_match check, even though it is currently not used.) Note also: this does *not* resolve the ICE from rust-lang#65466, as noted in a comment added in this commit. Further work is necessary to resolve that and other problems with the structural match checking, especially to do so without breaking stable code (adapted from test fn-ptr-is-structurally-matchable.rs): ```rust fn r_sm_to(_: &SM) {} fn main() { const CFN6: Wrap<fn(&SM)> = Wrap(r_sm_to); let input: Wrap<fn(&SM)> = Wrap(r_sm_to); match Wrap(input) { Wrap(CFN6) => {} Wrap(_) => {} }; } ``` where we would hit a problem with the strategy of unconditionally checking for `PartialEq` because the type `for <'a> fn(&'a SM)` does not currently even *implement* `PartialEq`. ---- added review feedback: * use an or-pattern * eschew `return` when tail position will do. * don't need fresh_expansion; just add `structural_match` to appropriate `allow_internal_unstable` attributes. also fixed example in doc comment so that it actually compiles.
|
Answer: under PR #67088, this produces: (which is what we want). |
|
Note to self, matching on |
bors
added a commit
to rust-lang-ci/rust
that referenced
this issue
Apr 29, 2020
…h, r=pnkfelix Const qualification for `StructuralEq` Furthers rust-lang#62411. Resolves rust-lang#62614. The goal of this PR is to implement the logic in rust-lang#67088 on the MIR instead of the HIR. It uses the `Qualif` trait to track `StructuralPartialEq`/`StructuralEq` in the final value of a `const`. Then, if we encounter a constant during HAIR lowering whose value may not be structurally matchable, we emit the `indirect_structural_match` lint. This PR contains all the tests present in rust-lang#67088 and emits the proper warnings for the corner cases. This PR does not handle rust-lang#65466, which would require that we be [more aggressive](https://github.com/rust-lang/rust/blob/42abbd8878d3b67238f3611b0587c704ba94f39c/src/librustc_mir_build/hair/pattern/const_to_pat.rs#L126-L130) when checking matched types for `PartialEq`. I think that should be done separately. Because this works on MIR and uses dataflow, this PR should accept more cases than rust-lang#67088. Notably, the qualifs in the final value of a const are encoded cross-crate, so matching on a constant whose value is defined in another crate to be `Option::<TyWithCustomEqImpl>::None` should work. Additionally, if a `const` has branching/looping, we will only emit the warning if any possible control flow path could result in a type with a custom `PartialEq` impl ending up as the final value of a `const`. I'm not sure how rust-lang#67088 handled this. AFAIK, it's not settled that these are the semantics we actually want: it's just how the `Qualif` framework happens to work. If the cross-crate part is undesirable, it would be quite easy to change the result of `mir_const_qualif().custom_eq` to `true` before encoding it in the crate metadata. This way, other crates would have to assume that all publicly exported constants may not be safe for matching. r? @pnkfelix cc @eddyb
ecstatic-morse
added a commit
to ecstatic-morse/rust
that referenced
this issue
May 17, 2020
|
Still crashing (obviously on nightly, beta and stable by now...). |
|
Fixed by #70743 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
(Spawned off of #61188 (comment) )
Sometimes the code-generation for pattern-matching a
constwill generate a invocation ofPartialEq::eq(rather than unfolding the structure of that constant inline, which is the usual compilation strategy used in rustc currently...)The difference between unfolding inline and invoking
PartialEq::eqis not meant to be observable, because of RFC 1445 (c.f. #31434): "Restrict constants in patterns"However, there are cases where rustc's static analysis of the pattern itself was not thorough enough, and it would let patterns through that do end up invoking
PartialEq::eqon constants that do not even implementPartialEqin the first place.We had been relying on the structural-match checking to catch such cases, where we would first run the structural match check, and if we found an ADT that was a non-structural-match, then we would then check if the
constin question even implementsPartialEq.But that strategy, of a delayed check for
PartialEq, does not suffice in every case. Here is an example, taken from @matthewjasper 's comment linked above:I had been tagging the above test case as being part of #61188, but at this point I think it is confusing matters, because the code/comments/commit messages say things like "this does not yet fix #61188" and it is ambgious whether that message refers to the original bug described by #61188, or the subsequent issue pointed out by @matthewjasper linked above.
So I am allocating a fresh issue to track the matter here.
The text was updated successfully, but these errors were encountered: