-
Notifications
You must be signed in to change notification settings - Fork 13k
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
Variants::Single: do not use invalid VariantIdx for uninhabited enums #133702
Changes from all commits
21de42b
e023590
85f0138
397ae3c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -734,21 +734,22 @@ where | |
let layout = match this.variants { | ||
Variants::Single { index } | ||
// If all variants but one are uninhabited, the variant layout is the enum layout. | ||
if index == variant_index && | ||
// Don't confuse variants of uninhabited enums with the enum itself. | ||
// For more details see https://github.com/rust-lang/rust/issues/69763. | ||
this.fields != FieldsShape::Primitive => | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Now that |
||
if index == variant_index => | ||
{ | ||
this.layout | ||
} | ||
|
||
Variants::Single { index } => { | ||
Variants::Single { .. } | Variants::Empty => { | ||
// Single-variant and no-variant enums *can* have other variants, but those are | ||
// uninhabited. Produce a layout that has the right fields for that variant, so that | ||
// the rest of the compiler can project fields etc as usual. | ||
|
||
let tcx = cx.tcx(); | ||
let typing_env = cx.typing_env(); | ||
|
||
// Deny calling for_variant more than once for non-Single enums. | ||
if let Ok(original_layout) = tcx.layout_of(typing_env.as_query_input(this.ty)) { | ||
assert_eq!(original_layout.variants, Variants::Single { index }); | ||
assert_eq!(original_layout.variants, this.variants); | ||
} | ||
|
||
let fields = match this.ty.kind() { | ||
|
@@ -902,6 +903,7 @@ where | |
), | ||
|
||
ty::Coroutine(def_id, args) => match this.variants { | ||
Variants::Empty => unreachable!(), | ||
Variants::Single { index } => TyMaybeWithLayout::Ty( | ||
args.as_coroutine() | ||
.state_tys(def_id, tcx) | ||
|
@@ -927,6 +929,7 @@ where | |
let field = &def.variant(index).fields[FieldIdx::from_usize(i)]; | ||
TyMaybeWithLayout::Ty(field.ty(tcx, args)) | ||
} | ||
Variants::Empty => panic!("there is no field in Variants::Empty types"), | ||
|
||
// Discriminant field for enums (where applicable). | ||
Variants::Multiple { tag, .. } => { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -35,7 +35,6 @@ | |
//! Likewise, applying the optimisation can create a lot of new MIR, so we bound the instruction | ||
//! cost by `MAX_COST`. | ||
|
||
use rustc_abi::{TagEncoding, Variants}; | ||
use rustc_arena::DroplessArena; | ||
use rustc_const_eval::const_eval::DummyMachine; | ||
use rustc_const_eval::interpret::{ImmTy, Immediate, InterpCx, OpTy, Projectable}; | ||
|
@@ -565,31 +564,15 @@ impl<'a, 'tcx> TOFinder<'a, 'tcx> { | |
StatementKind::SetDiscriminant { box place, variant_index } => { | ||
let Some(discr_target) = self.map.find_discr(place.as_ref()) else { return }; | ||
let enum_ty = place.ty(self.body, self.tcx).ty; | ||
// `SetDiscriminant` may be a no-op if the assigned variant is the untagged variant | ||
// of a niche encoding. If we cannot ensure that we write to the discriminant, do | ||
// nothing. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @cjgillot the old logic here seems unnecessarily cautious. Miri already for a while now considers it UB to "write" an untagged discriminant if the data does not currently encode that variant. So IMO we can also rely on this for optimizations, this slightly strengthening jump threading. (I thought we already rely on this for optimizations somewhere, but I am not sure.) |
||
let Ok(enum_layout) = self.ecx.layout_of(enum_ty) else { | ||
// `SetDiscriminant` guarantees that the discriminant is now `variant_index`. | ||
// Even if the discriminant write does nothing due to niches, it is UB to set the | ||
// discriminant when the data does not encode the desired discriminant. | ||
let Some(discr) = | ||
self.ecx.discriminant_for_variant(enum_ty, *variant_index).discard_err() | ||
else { | ||
return; | ||
}; | ||
let writes_discriminant = match enum_layout.variants { | ||
Variants::Single { index } => { | ||
assert_eq!(index, *variant_index); | ||
true | ||
} | ||
Variants::Multiple { tag_encoding: TagEncoding::Direct, .. } => true, | ||
Variants::Multiple { | ||
tag_encoding: TagEncoding::Niche { untagged_variant, .. }, | ||
.. | ||
} => *variant_index != untagged_variant, | ||
}; | ||
if writes_discriminant { | ||
let Some(discr) = | ||
self.ecx.discriminant_for_variant(enum_ty, *variant_index).discard_err() | ||
else { | ||
return; | ||
}; | ||
self.process_immediate(bb, discr_target, discr, state); | ||
} | ||
self.process_immediate(bb, discr_target, discr, state); | ||
} | ||
// If we expect `lhs ?= true`, we have an opportunity if we assume `lhs == true`. | ||
StatementKind::Intrinsic(box NonDivergingIntrinsic::Assume( | ||
|
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.
This line is either unreachable or it obviously changes behavior of the code. Not yet sure which.
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.
It should be unreachable because of
BackendRepr::Uninhabited
and return on line 114.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.
The new logic makes more sense -- empty-variant types are never valid. We just didn't bother specifically checking for this before.
But yeah it may well be unreachable. It doesn't really matter.