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

Variants::Single: do not use invalid VariantIdx for uninhabited enums #133702

Merged
merged 4 commits into from
Dec 19, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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_abi/src/callconv.rs
Original file line number Diff line number Diff line change
Expand Up @@ -206,7 +206,7 @@ impl<'a, Ty> TyAndLayout<'a, Ty> {
let (mut result, mut total) = from_fields_at(*self, Size::ZERO)?;

match &self.variants {
abi::Variants::Single { .. } => {}
abi::Variants::Single { .. } | abi::Variants::Empty => {}
abi::Variants::Multiple { variants, .. } => {
// Treat enum variants like union members.
// HACK(eddyb) pretend the `enum` field (discriminant)
Expand Down
7 changes: 4 additions & 3 deletions compiler/rustc_abi/src/layout.rs
Original file line number Diff line number Diff line change
Expand Up @@ -213,8 +213,9 @@ impl<Cx: HasDataLayout> LayoutCalculator<Cx> {
&self,
) -> LayoutData<FieldIdx, VariantIdx> {
let dl = self.cx.data_layout();
// This is also used for uninhabited enums, so we use `Variants::Empty`.
LayoutData {
variants: Variants::Single { index: VariantIdx::new(0) },
variants: Variants::Empty,
fields: FieldsShape::Primitive,
backend_repr: BackendRepr::Uninhabited,
largest_niche: None,
Expand Down Expand Up @@ -1004,8 +1005,8 @@ impl<Cx: HasDataLayout> LayoutCalculator<Cx> {
Variants::Multiple { tag, tag_encoding, tag_field, .. } => {
Variants::Multiple { tag, tag_encoding, tag_field, variants: best_layout.variants }
}
Variants::Single { .. } => {
panic!("encountered a single-variant enum during multi-variant layout")
Variants::Single { .. } | Variants::Empty => {
panic!("encountered a single-variant or empty enum during multi-variant layout")
}
};
Ok(best_layout.layout)
Expand Down
6 changes: 4 additions & 2 deletions compiler/rustc_abi/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1504,10 +1504,12 @@ impl BackendRepr {
#[derive(PartialEq, Eq, Hash, Clone, Debug)]
#[cfg_attr(feature = "nightly", derive(HashStable_Generic))]
pub enum Variants<FieldIdx: Idx, VariantIdx: Idx> {
/// A type with no valid variants. Must be uninhabited.
Empty,

/// Single enum variants, structs/tuples, unions, and all non-ADTs.
Single {
/// Always 0 for non-enums/generators.
/// For enums without a variant, this is an invalid index!
/// Always `0` for types that cannot have multiple variants.
index: VariantIdx,
},

Expand Down
2 changes: 2 additions & 0 deletions compiler/rustc_codegen_cranelift/src/discriminant.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ pub(crate) fn codegen_set_discriminant<'tcx>(
return;
}
match layout.variants {
Variants::Empty => unreachable!("we already handled uninhabited types"),
Variants::Single { index } => {
assert_eq!(index, variant_index);
}
Expand Down Expand Up @@ -85,6 +86,7 @@ pub(crate) fn codegen_get_discriminant<'tcx>(
}

let (tag_scalar, tag_field, tag_encoding) = match &layout.variants {
Variants::Empty => unreachable!("we already handled uninhabited types"),
Variants::Single { index } => {
let discr_val = layout
.ty
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -212,21 +212,17 @@ pub(super) fn build_enum_type_di_node<'ll, 'tcx>(
),
|cx, enum_type_di_node| {
match enum_type_and_layout.variants {
Variants::Single { index: variant_index } => {
if enum_adt_def.variants().is_empty() {
// Uninhabited enums have Variants::Single. We don't generate
// any members for them.
return smallvec![];
}

build_single_variant_union_fields(
cx,
enum_adt_def,
enum_type_and_layout,
enum_type_di_node,
variant_index,
)
Variants::Empty => {
// We don't generate any members for uninhabited types.
return smallvec![];
}
Variants::Single { index: variant_index } => build_single_variant_union_fields(
cx,
enum_adt_def,
enum_type_and_layout,
enum_type_di_node,
variant_index,
),
Variants::Multiple {
tag_encoding: TagEncoding::Direct,
ref variants,
Expand Down Expand Up @@ -303,6 +299,7 @@ pub(super) fn build_coroutine_di_node<'ll, 'tcx>(
)
}
Variants::Single { .. }
| Variants::Empty
| Variants::Multiple { tag_encoding: TagEncoding::Niche { .. }, .. } => {
bug!(
"Encountered coroutine with non-direct-tag layout: {:?}",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -392,7 +392,7 @@ fn compute_discriminant_value<'ll, 'tcx>(
variant_index: VariantIdx,
) -> DiscrResult {
match enum_type_and_layout.layout.variants() {
&Variants::Single { .. } => DiscrResult::NoDiscriminant,
&Variants::Single { .. } | &Variants::Empty => DiscrResult::NoDiscriminant,
&Variants::Multiple { tag_encoding: TagEncoding::Direct, .. } => DiscrResult::Value(
enum_type_and_layout.ty.discriminant_for_variant(cx.tcx, variant_index).unwrap().val,
),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -358,8 +358,8 @@ fn build_discr_member_di_node<'ll, 'tcx>(
let containing_scope = enum_or_coroutine_type_di_node;

match enum_or_coroutine_type_and_layout.layout.variants() {
// A single-variant enum has no discriminant.
&Variants::Single { .. } => None,
// A single-variant or no-variant enum has no discriminant.
&Variants::Single { .. } | &Variants::Empty => None,

&Variants::Multiple { tag_field, .. } => {
let tag_base_type = tag_base_type(cx.tcx, enum_or_coroutine_type_and_layout);
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_codegen_llvm/src/type_of.rs
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ fn uncached_llvm_type<'a, 'tcx>(
if let (&ty::Adt(def, _), &Variants::Single { index }) =
(layout.ty.kind(), &layout.variants)
{
if def.is_enum() && !def.variants().is_empty() {
if def.is_enum() {
write!(&mut name, "::{}", def.variant(index).name).unwrap();
}
}
Expand Down
4 changes: 2 additions & 2 deletions compiler/rustc_codegen_ssa/src/debuginfo/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -65,8 +65,8 @@ fn tag_base_type_opt<'tcx>(
});

match enum_type_and_layout.layout.variants() {
// A single-variant enum has no discriminant.
Variants::Single { .. } => None,
// A single-variant or no-variant enum has no discriminant.
Variants::Single { .. } | Variants::Empty => None,

Variants::Multiple { tag_encoding: TagEncoding::Niche { .. }, tag, .. } => {
// Niche tags are always normalized to unsized integers of the correct size.
Expand Down
7 changes: 4 additions & 3 deletions compiler/rustc_codegen_ssa/src/mir/place.rs
Original file line number Diff line number Diff line change
Expand Up @@ -243,6 +243,7 @@ impl<'a, 'tcx, V: CodegenObject> PlaceRef<'tcx, V> {
return bx.cx().const_poison(cast_to);
}
let (tag_scalar, tag_encoding, tag_field) = match self.layout.variants {
Variants::Empty => unreachable!("we already handled uninhabited types"),
Variants::Single { index } => {
let discr_val = self
.layout
Expand Down Expand Up @@ -365,9 +366,9 @@ impl<'a, 'tcx, V: CodegenObject> PlaceRef<'tcx, V> {
return;
}
match self.layout.variants {
Variants::Single { index } => {
assert_eq!(index, variant_index);
}
Variants::Empty => unreachable!("we already handled uninhabited types"),
Variants::Single { index } => assert_eq!(index, variant_index),

Variants::Multiple { tag_encoding: TagEncoding::Direct, tag_field, .. } => {
let ptr = self.project_field(bx, tag_field);
let to =
Expand Down
25 changes: 12 additions & 13 deletions compiler/rustc_const_eval/src/interpret/discriminant.rs
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> {
}
}

/// Read discriminant, return the runtime value as well as the variant index.
/// Read discriminant, return the variant index.
/// Can also legally be called on non-enums (e.g. through the discriminant_value intrinsic)!
///
/// Will never return an uninhabited variant.
Expand All @@ -65,21 +65,17 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> {
// We use "tag" to refer to how the discriminant is encoded in memory, which can be either
// straight-forward (`TagEncoding::Direct`) or with a niche (`TagEncoding::Niche`).
let (tag_scalar_layout, tag_encoding, tag_field) = match op.layout().variants {
Variants::Empty => {
throw_ub!(UninhabitedEnumVariantRead(None));
}
Variants::Single { index } => {
// Do some extra checks on enums.
if ty.is_enum() {
// Hilariously, `Single` is used even for 0-variant enums.
// (See https://github.com/rust-lang/rust/issues/89765).
if ty.ty_adt_def().unwrap().variants().is_empty() {
throw_ub!(UninhabitedEnumVariantRead(index))
}
if op.layout().is_uninhabited() {
// For consistency with `write_discriminant`, and to make sure that
// `project_downcast` cannot fail due to strange layouts, we declare immediate UB
// for uninhabited variants.
if op.layout().for_variant(self, index).is_uninhabited() {
throw_ub!(UninhabitedEnumVariantRead(index))
}
// for uninhabited enums.
throw_ub!(UninhabitedEnumVariantRead(Some(index)));
}
// Since the type is inhabited, there must be an index.
return interp_ok(index);
}
Variants::Multiple { tag, ref tag_encoding, tag_field, .. } => {
Expand Down Expand Up @@ -199,11 +195,13 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> {
// `uninhabited_enum_branching` MIR pass. It also ensures consistency with
// `write_discriminant`.
if op.layout().for_variant(self, index).is_uninhabited() {
throw_ub!(UninhabitedEnumVariantRead(index))
throw_ub!(UninhabitedEnumVariantRead(Some(index)))
}
interp_ok(index)
}

/// Read discriminant, return the user-visible discriminant.
/// Can also legally be called on non-enums (e.g. through the discriminant_value intrinsic)!
pub fn discriminant_for_variant(
&self,
ty: Ty<'tcx>,
Expand Down Expand Up @@ -243,6 +241,7 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> {
}

match layout.variants {
abi::Variants::Empty => unreachable!("we already handled uninhabited types"),
abi::Variants::Single { .. } => {
// The tag of a `Single` enum is like the tag of the niched
// variant: there's no tag as the discriminant is encoded
Expand Down
5 changes: 3 additions & 2 deletions compiler/rustc_const_eval/src/interpret/validity.rs
Original file line number Diff line number Diff line change
Expand Up @@ -302,7 +302,7 @@ impl<'rt, 'tcx, M: Machine<'tcx>> ValidityVisitor<'rt, 'tcx, M> {
};
}
}
Variants::Single { .. } => {}
Variants::Single { .. } | Variants::Empty => {}
}

// Now we know we are projecting to a field, so figure out which one.
Expand Down Expand Up @@ -344,6 +344,7 @@ impl<'rt, 'tcx, M: Machine<'tcx>> ValidityVisitor<'rt, 'tcx, M> {
// Inside a variant
PathElem::Field(def.variant(index).fields[FieldIdx::from_usize(field)].name)
}
Variants::Empty => panic!("there is no field in Variants::Empty types"),
Variants::Multiple { .. } => bug!("we handled variants above"),
}
}
Expand Down Expand Up @@ -1010,7 +1011,7 @@ impl<'rt, 'tcx, M: Machine<'tcx>> ValidityVisitor<'rt, 'tcx, M> {
}
// Don't forget potential other variants.
match &layout.variants {
Variants::Single { .. } => {
Variants::Single { .. } | Variants::Empty => {
// Fully handled above.
}
Variants::Multiple { variants, .. } => {
Expand Down
4 changes: 2 additions & 2 deletions compiler/rustc_const_eval/src/interpret/visitor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -218,8 +218,8 @@ pub trait ValueVisitor<'tcx, M: Machine<'tcx>>: Sized {
// recurse with the inner type
self.visit_variant(v, idx, &inner)?;
}
// For single-variant layouts, we already did anything there is to do.
Variants::Single { .. } => {}
// For single-variant layouts, we already did everything there is to do.
Variants::Single { .. } | Variants::Empty => {}
}

interp_ok(())
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,7 @@ fn check_validity_requirement_lax<'tcx>(
}

match &this.variants {
Variants::Empty => return Ok(false),

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.

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.

Copy link
Member Author

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.

Variants::Single { .. } => {
// All fields of this single variant have already been checked above, there is nothing
// else to do.
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_middle/src/mir/interpret/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -392,7 +392,7 @@ pub enum UndefinedBehaviorInfo<'tcx> {
/// A discriminant of an uninhabited enum variant is written.
UninhabitedEnumVariantWritten(VariantIdx),
/// An uninhabited enum variant is projected.
UninhabitedEnumVariantRead(VariantIdx),
UninhabitedEnumVariantRead(Option<VariantIdx>),
/// Trying to set discriminant to the niched variant, but the value does not match.
InvalidNichedEnumVariantWritten { enum_ty: Ty<'tcx> },
/// ABI-incompatible argument types.
Expand Down
15 changes: 9 additions & 6 deletions compiler/rustc_middle/src/ty/layout.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 =>
Copy link
Member Author

Choose a reason for hiding this comment

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

Now that ! and hence empty enums use Variants::Empty, this check does not seem to be needed any more. :)

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() {
Expand Down Expand Up @@ -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)
Expand All @@ -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, .. } => {
Expand Down
31 changes: 7 additions & 24 deletions compiler/rustc_mir_transform/src/jump_threading.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand Down Expand Up @@ -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.
Copy link
Member Author

Choose a reason for hiding this comment

The 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(
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_mir_transform/src/large_enums.rs
Original file line number Diff line number Diff line change
Expand Up @@ -216,7 +216,7 @@ impl EnumSizeOpt {
};
let layout = tcx.layout_of(typing_env.as_query_input(ty)).ok()?;
let variants = match &layout.variants {
Variants::Single { .. } => return None,
Variants::Single { .. } | Variants::Empty => return None,
Variants::Multiple { tag_encoding: TagEncoding::Niche { .. }, .. } => return None,

Variants::Multiple { variants, .. } if variants.len() <= 1 => return None,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,10 @@ fn variant_discriminants<'tcx>(
tcx: TyCtxt<'tcx>,
) -> FxHashSet<u128> {
match &layout.variants {
Variants::Empty => {
// Uninhabited, no valid discriminant.
FxHashSet::default()
}
Variants::Single { index } => {
let mut res = FxHashSet::default();
res.insert(
Expand Down
1 change: 1 addition & 0 deletions compiler/rustc_smir/src/rustc_smir/convert/abi.rs
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,7 @@ impl<'tcx> Stable<'tcx> for rustc_abi::Variants<rustc_abi::FieldIdx, rustc_abi::
rustc_abi::Variants::Single { index } => {
VariantsShape::Single { index: index.stable(tables) }
}
rustc_abi::Variants::Empty => VariantsShape::Empty,
rustc_abi::Variants::Multiple { tag, tag_encoding, tag_field, variants } => {
VariantsShape::Multiple {
tag: tag.stable(tables),
Expand Down
Loading
Loading