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.

Sign up for GitHub

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

Jump to bottom

Normalize opaque alias type #116378

Closed
wants to merge 2 commits into from
Closed

Normalize opaque alias type #116378

wants to merge 2 commits into from

Conversation

ouz-a
Copy link
Contributor

@ouz-a ouz-a commented Oct 3, 2023 •
edited

In cases like #116332, we might end up with Alias(Opaque), which causes match to ICE, this fixes that by normalizing opaque value we get.

Fixes #116332
Fixes #116265
Fixes #116383

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 3, 2023
@ouz-a ouz-a force-pushed the validate_alias2 branch 2 times, most recently from 446a2bb to 7d17d04 Compare October 3, 2023 10:49
@lqd
Copy link
Member

lqd commented Oct 3, 2023

Does this also fix #116333 and #116265?

@ouz-a
Copy link
Contributor Author

ouz-a commented Oct 3, 2023

Does this also fix #116333 and #116265?

I don't have minimal example for the first one, and I have working solution for the second one as well.

@ouz-a
Copy link
Contributor Author

ouz-a commented Oct 3, 2023

added fix for #116265 in second commit

@ouz-a ouz-a mentioned this pull request Oct 3, 2023
Closed
lcnr
lcnr reviewed Oct 3, 2023
View reviewed changes
Copy link
Contributor

@lcnr lcnr left a comment •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there are two fixes here, for the subtyping issues, we can run the Subtyper pass in run_runtime_lowering_passes after &reveal_all::RevealAdd, this should normalize all opaques.

For the opaque types issue I would expect a normalize_erasing_regions call to happen earlier, so that parent_ty is never an unnormalized opaque if we're in Reveal::All

@ouz-a
Copy link
Contributor Author

ouz-a commented Oct 3, 2023 •
edited

I think there are two fixes here, for the subtyping issues, we can run the Subtyper pass in run_runtime_lowering_passes after &reveal_all::RevealAdd, this should normalize all opaques.

For the opaque types issue I would expect a normalize_erasing_regions call to happen earlier, so that parent_ty is never an unnormalized opaque if we're in Reveal::All

I'm in no favor of these solutions but there is something weird going on with reveal_all, even if visit_place does nothing it causes ICE on issue #116332, I made visit_place empty function to see if it really is causing an issue on it's own and to my surprise even empty visit_place is triggering ICE.

But removing visit_place also causes OpaqueCast errors to trigger so...
Edit: interestingly calling normalize_erasing_regions for second time fixes problems but fails some ui tests.

@oli-obk
Copy link
Contributor

oli-obk commented Oct 3, 2023

Part of the issue is that a field projection can result in an opaque type again, and after stripping the opaque type projection (which would turn it into the hidden type), we end up failing follow up projections.

Your fixes resolve this at the sites that cause ICEs. I've been trying to figure out if we can instead make projecions reveal immediately and thus not require users of the Place::ty method to be aware of this footgun.

I don't remember where I stopped on Friday while investigating this, but after the long German weekend (tomorrow) I'll post my findings here.

A quick fix that I'd prefer would be to stop stripping OpaqueTy projections and allow them again in the mir visitor.

@matthiaskrgr
Copy link
Member

I wonder if its best to revert #115025 for now (before the nightly release in a couple hours) since we are already getting "my crate failed to build" reports from nightly users :/

@ouz-a
Copy link
Contributor Author

ouz-a commented Oct 4, 2023

Going to open a new PR for these fixes, we are already departed from original goal of this PR.

@lqd
Copy link
Member

lqd commented Oct 4, 2023

I wonder if its best to revert #115025 for now

did you mean also revert #115759 and not just #115025?

@matthiaskrgr
Copy link
Member

Well, could also revert both 😅

@ouz-a ouz-a mentioned this pull request Oct 4, 2023
Merged
@ouz-a
Copy link
Contributor Author

ouz-a commented Oct 4, 2023

Closing this in favor of #116415

@ouz-a ouz-a closed this Oct 4, 2023
@ouz-a ouz-a deleted the validate_alias2 branch October 4, 2023 10:47
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Oct 5, 2023
@matthiaskrgr
Rollup merge of rust-lang#116415 - ouz-a:move_subtyper, r=oli-obk
9fe16a4 
Move subtyper below reveal_all and change reveal_all

In previous attempt rust-lang#116378 we tried to handle `Opaque` in few different places, but this isn't necessary, after moving subtyper below reveal_all and calling `super_place` on reveal_all, issues cease to exist.

r? `@oli-obk`

Fixes rust-lang#116332
Fixes rust-lang#116265
Fixes rust-lang#116383
Fixes rust-lang#116333
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Oct 5, 2023
@matthiaskrgr
Rollup merge of rust-lang#116415 - ouz-a:move_subtyper, r=oli-obk
08cc742 
Move subtyper below reveal_all and change reveal_all

In previous attempt rust-lang#116378 we tried to handle `Opaque` in few different places, but this isn't necessary, after moving subtyper below reveal_all and calling `super_place` on reveal_all, issues cease to exist.

r? ``@oli-obk``

Fixes rust-lang#116332
Fixes rust-lang#116265
Fixes rust-lang#116383
Fixes rust-lang#116333
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Oct 5, 2023
@rust-timer
Unrolled build for rust-lang#116415
fa06acc 
Rollup merge of rust-lang#116415 - ouz-a:move_subtyper, r=oli-obk

Move subtyper below reveal_all and change reveal_all

In previous attempt rust-lang#116378 we tried to handle `Opaque` in few different places, but this isn't necessary, after moving subtyper below reveal_all and calling `super_place` on reveal_all, issues cease to exist.

r? ``@oli-obk``

Fixes rust-lang#116332
Fixes rust-lang#116265
Fixes rust-lang#116383
Fixes rust-lang#116333
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matthiaskrgr matthiaskrgr matthiaskrgr left review comments

@compiler-errors compiler-errors compiler-errors left review comments

@lcnr lcnr lcnr left review comments

Assignees

@oli-obk oli-obk

Labels
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.
Projects
None yet
Milestone
No milestone
7 participants
@ouz-a @lqd @oli-obk @matthiaskrgr @compiler-errors @lcnr @rustbot