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

Fall back when relating two opaques by substs in MIR typeck #100092

Merged
merged 2 commits into from
Aug 30, 2022

Conversation

compiler-errors
Copy link
Member

@compiler-errors compiler-errors commented Aug 3, 2022

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.

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Aug 3, 2022
@rust-highfive
Copy link
Collaborator

r? @jackh726

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 3, 2022
@compiler-errors
Copy link
Member Author

r? @oli-obk

@oli-obk
Copy link
Contributor

oli-obk commented Aug 24, 2022

We already have some logic somewhere to detect infinitely recursive opaque types.

Why is this case not handled by opaque_type_cycle_error

@compiler-errors
Copy link
Member Author

@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).

@oli-obk
Copy link
Contributor

oli-obk commented Aug 25, 2022

Considering opaque_type_cycle_error operates completely on TypeckResults, we could just remove it from wf checking and make it work completely inside the typeck query. That way it would run before mir borrowck's typeck ICEs (which usually doesn't get wfcheck run before)

@compiler-errors
Copy link
Member Author

@oli-obk:
I'm not sure I understand how to do this without having query cycles in the case of mutually recursive opaque types from different items.
Also I don't know how this would work with mutually recursive TAITs either, since we don't necessarily have all the information in a single typeck query to answer whether the TAIT is recursive in case all of the defining usages aren't in one body.

@oli-obk
Copy link
Contributor

oli-obk commented Aug 29, 2022

I'm not sure I understand how to do this without having query cycles in the case of mutually recursive opaque types from different items.

oof. yea good point.

@oli-obk
Copy link
Contributor

oli-obk commented Aug 29, 2022

@bors r+

weird unfortunate edge case, but I like the solution. We should investigate whether we should remove all of the (&ty::Opaque(a_def_id, _), &ty::Opaque(b_def_id, _)) if a_def_id == b_def_id match arms and handle this inside register_opaque_type instead.

@bors
Copy link
Contributor

bors commented Aug 29, 2022

📌 Commit 534426d 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. 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. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Aug 29, 2022
@oli-obk
Copy link
Contributor

oli-obk commented Aug 29, 2022

@bors r+

@bors
Copy link
Contributor

bors commented Aug 29, 2022

📌 Commit 534426d 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 29, 2022
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 30, 2022
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
@bors bors merged commit e63424d into rust-lang:master Aug 30, 2022
@rustbot rustbot added this to the 1.65.0 milestone Aug 30, 2022
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 27, 2022
…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...
Aaron1011 pushed a commit to Aaron1011/rust that referenced this pull request Jan 6, 2023
…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...
@compiler-errors compiler-errors deleted the issue-100075 branch August 11, 2023 20:03
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.

yet another silly delay_span_bug issue
6 participants