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

Fix for reverted PR #103880 #105958

Closed
wants to merge 4 commits into from
Closed

Fix for reverted PR #103880 #105958

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b2656e7
use non-ascribed field ty in place field projection
b-naber Nov 2, 2022
b26fea5
don't include additional opaquecast
b-naber Dec 20, 2022
15fa511
add tests
b-naber Dec 20, 2022
2bd63fc
cargo.lock
b-naber Dec 20, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion compiler/rustc_mir_build/src/build/matches/util.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ impl<'pat, 'tcx> MatchPair<'pat, 'tcx> {
let may_need_cast = match place {
PlaceBuilder::Local { local, ref projection } => {
let ty = Place::ty_from(local, projection, &cx.local_decls, cx.tcx).ty;
ty != pattern.ty && ty.has_opaque_types()
ty.has_opaque_types() && !pattern.ty.has_opaque_types()
Copy link
Contributor

@lcnr lcnr Dec 21, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

i fear that this may only make the bug harder to trigger rather than actually fixing it.

You could imagine having a field like (subtype, opaque) to (supertype, opaque) which hits both the old and the new check.

What is the exact reason that we get an ICE there? Is the issue subtyping in OpaqueCast or something else?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hm I'm not sure, the comment above l.109 states Only add the OpaqueCast projection if the given place is an opaque type and the expected type from the pattern is not. I don't know how OpaqueCasts are handled later or why we need them (they're only used for opaque types, aren't they?). Would we really need an OpaqueCast only because of the subtype/suptype here?

I think the ICE is triggered because the type in the OpaqueCast is still generic in codegen and we therefore cannot get a layout for it.

Copy link
Contributor

Choose a reason for hiding this comment

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

ah (subtype, opaque) to (supertype, opaque) doesn't trigger this with your fix.

still worried about (subtype, opaque) to (supertype, concrete).

I think the ICE is triggered because the type in the OpaqueCast is still generic in codegen and we therefore cannot get a layout for it.

This doesn't make sense to me. OpaqueCast should never be generic during codegen. We should substitute the concrete generic arguments during codegen if the OpaqueCast projection still exist there. An alternative might be that we're missing a normalization for the opaque cast but that also seems weird 🤔

}
_ => true,
};
Expand Down