-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
remove Subtype
projections
#133258
base: master
Are you sure you want to change the base?
remove Subtype
projections
#133258
Conversation
Some changes occurred to the CTFE / Miri interpreter cc @rust-lang/miri Some changes occurred to the CTFE machinery cc @rust-lang/wg-const-eval Some changes occurred in src/tools/clippy cc @rust-lang/clippy Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt This PR changes MIR cc @oli-obk, @RalfJung, @JakobDegen, @davidtwco, @celinval, @vakaras Some changes occurred in compiler/rustc_codegen_cranelift cc @bjorn3 This PR changes Stable MIR |
@@ -114,7 +114,6 @@ impl<'tcx, M: Machine<'tcx>> FnAbiOfHelpers<'tcx> for InterpCx<'tcx, M> { | |||
} | |||
|
|||
/// Test if it is valid for a MIR assignment to assign `src`-typed place to `dest`-typed value. | |||
/// This test should be symmetric, as it is primarily about layout compatibility. | |||
pub(super) fn mir_assign_valid_types<'tcx>( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we check for subtyping here which is not symmetric, so either this comment is wrong or the impl is. I would be open to alternative change this to try subtyping in either direction 🤷 but given that this already uses src
and dest
and the behavior is preexisting, this comment is not relied on rn
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I originally made the check symmetric since I didn't want to think about which direction it has to be. 😂 So it might be worth checking that all users of this function use it correctly.
I also find the comment extremely confusing. When doing an assignment, dest
is a place and src
is a value! So what is going on here?
What I expect is a comment like this:
/// Test if it is valid for a MIR assignment to store a `src`-typed value in a `dest`-typed place.
But I am not sure if that matches the implementation, or the users...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apparently I wrote that comment. Unfortunately I can't explain what I meant...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, it should be src
-typed value and dest
-typed place 😁
.filter(|elem| !matches!(elem, ProjectionElem::OpaqueCast(_))) | ||
.collect::<Vec<_>>(), | ||
); | ||
|
||
self.super_place(place, _context, _location); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
while I don't fully understand in which cases we don't emit an OpaqueCast
even though the type of a projection contains an opaque, it apparently can happen 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea there are bugs here. The system is fragile and I haven't been able to come up with sth better, so without redoing projections from scratch, it may be best to just forbid projecting into opaques. I wanted to so that originally, but there was pushback saying some stuff should compile
@@ -682,20 +674,13 @@ impl<'a, 'tcx> Visitor<'tcx> for TypeChecker<'a, 'tcx> { | |||
} | |||
}; | |||
|
|||
let kind = match parent_ty.ty.kind() { | |||
&ty::Alias(ty::Opaque, ty::AliasTy { def_id, args, .. }) => { | |||
self.tcx.type_of(def_id).instantiate(self.tcx, args).kind() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
very much incorrect in case the type of an opaque is either not normalized with these args
or yet another opaque :3
There you said we'd need explicit types in some places, replacing these projections. (Specifically, casts and calls.) How can this PR work without adding those explicit types? |
we currently still prevent optimizations from dropping assignments causing subtyping, as the subtype projections were never enough to actually fix the unsoundness so rn the fix for the unsoundness is: make sure no optimization merges locals which are subtypes of each other |
Okay. That sounds fragile, I guess #112651 is tracking the improvement of that situation? |
This comment has been minimized.
This comment has been minimized.
@rustbot author |
cc #112651 closes #118478
see #112651 (comment) for the reasoning behind this change
r? types