-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
Fall back when relating two opaques by substs in MIR typeck #100092
Conversation
r? @jackh726 (rust-highfive has picked a reviewer for you, use r? to override) |
r? @oli-obk |
1cc292b
to
534426d
Compare
We already have some logic somewhere to detect infinitely recursive opaque types. Why is this case not handled by |
@oli-obk: So currently (before this PR), when we equate an opaque w/ substs1 and the same opaque w/ substs2, where substs1 and substs2 can't be equated, results in an error in mir typeck. So we actually don't infer anything for the opaque, so we don't actually see the opaque being cyclical (at least not in the opaque hidden type that mir_borrowck results gives us). |
Considering |
@oli-obk: |
oof. yea good point. |
@bors r+ weird unfortunate edge case, but I like the solution. We should investigate whether we should remove all of the |
@bors r+ |
Rollup of 9 pull requests Successful merges: - rust-lang#95376 (Add `vec::Drain{,Filter}::keep_rest`) - rust-lang#100092 (Fall back when relating two opaques by substs in MIR typeck) - rust-lang#101019 (Suggest returning closure as `impl Fn`) - rust-lang#101022 (Erase late bound regions before comparing types in `suggest_dereferences`) - rust-lang#101101 (interpret: make read-pointer-as-bytes a CTFE-only error with extra information) - rust-lang#101123 (Remove `register_attr` feature) - rust-lang#101175 (Don't --bless in pre-push hook) - rust-lang#101176 (rustdoc: remove unused CSS selectors for `.table-display`) - rust-lang#101180 (Add another MaybeUninit array test with const) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
…ll-relate, r=oli-obk Remove `commit_if_ok` probe from NLL type relation It was not really necessary to add the `commit_if_ok` in rust-lang#100092 -- I added it to protect us against weird inference error messages due to recursive RPIT calls, but we are always on the error path when this happens anyways, and I can't come up with an example that makes this manifest. Fixes rust-lang#103599 r? `@oli-obk` since you reviewed rust-lang#100092, feel free to re-roll. :b: :loudspeaker: beta-nominating this since it's on beta (which forks in ~a week~ two days :fearful:) -- worst case we could revert the original PR on beta and land this on nightly, to give it some extra soak time...
…ll-relate, r=oli-obk Remove `commit_if_ok` probe from NLL type relation It was not really necessary to add the `commit_if_ok` in rust-lang#100092 -- I added it to protect us against weird inference error messages due to recursive RPIT calls, but we are always on the error path when this happens anyways, and I can't come up with an example that makes this manifest. Fixes rust-lang#103599 r? `@oli-obk` since you reviewed rust-lang#100092, feel free to re-roll. :b: :loudspeaker: beta-nominating this since it's on beta (which forks in ~a week~ two days :fearful:) -- worst case we could revert the original PR on beta and land this on nightly, to give it some extra soak time...
This is certainly one way to fix #100075. Not really confident it's the best way to do it, though.
The root cause of this issue is that during MIR type-check, we end up trying to equate an opaque against the same opaque def-id but with different substs. Because of the way that we replace RPITs during (HIR) typeck with an inference variable, we don't end up emitting a type-checking error, so the delayed MIR bug causes an ICE.
See the
src/test/ui/impl-trait/issue-100075-2.rs
test below to make that clear -- in that example, we try to equate{impl Sized} substs=[T]
and{impl Sized} substs=[Option<T>]
, which causes an ICE. This new logic will instead cause us to infer{impl Sized} substs=[Option<T>]
as the hidden type for{impl Sized} substs=[T]
, which causes a proper error to be emitted later on when we check that an opaque isn't recursive.I'm open to closing this in favor of something else. Ideally we'd fix this in typeck, but the thing we do to ensure backwards compatibility with weird RPIT cases makes that difficult. Also open to discussing this further.