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

safe transmute: support Single enums #126358

Merged
merged 1 commit into from
Jun 13, 2024
Merged

Conversation

jswrenn
Copy link
Member

@jswrenn jswrenn commented Jun 12, 2024

Previously, the implementation of Tree::from_enum incorrectly treated enums with Variants::Single and Variants::Multiple identically. This is incorrect for Variants::Single enums, which delegate their layout to that of a variant with a particular index (or no variant at all if the enum is empty).

This flaw manifested first as an ICE. Tree::from_enum attempted to compute the tag of variants other than the one at Variants::Single's index, and fell afoul of a sanity-checking assertion in compiler/rustc_const_eval/src/interpret/discriminant.rs. This assertion is non-load-bearing, and can be removed; the routine its in is well-behaved even without it.

With the assertion removed, the proximate issue becomes apparent: calling Tree::from_variant on a variant that does not exist is ill-defined. A sanity check the given variant has FieldShapes::Arbitrary fails, and the analysis is (correctly) aborted with Err::NotYetSupported.

This commit corrects this chain of failures by ensuring that Tree::from_variant is not called on variants that are, as far as layout is concerned, nonexistent. Specifically, the implementation of Tree::from_enum is now partitioned into three cases:

  1. enums that are uninhabited
  2. enums for which all but one variant is uninhabited
  3. enums with multiple inhabited variants

Tree::from_variant is now only invoked in the third case. In the first case, Tree::uninhabited() is produced. In the second case, the layout is delegated to Variants::Single's index.

Fixes #125811

@rustbot
Copy link
Collaborator

rustbot commented Jun 12, 2024

r? @TaKO8Ki

rustbot has assigned @TaKO8Ki.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@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 Jun 12, 2024
@rustbot
Copy link
Collaborator

rustbot commented Jun 12, 2024

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

@@ -241,10 241,7 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> {
variant_index: VariantIdx,
) -> InterpResult<'tcx, Option<(ScalarInt, usize)>> {
match self.layout_of(ty)?.variants {
abi::Variants::Single { index } => {
assert_eq!(index, variant_index);
Copy link
Member Author

@jswrenn jswrenn Jun 12, 2024

Choose a reason for hiding this comment

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

Removing this assertion was recommended by @RalfJung in #125811 (comment), and I agree with his assessment that it isn't a load-bearing assertion. An index mismatch here doesn't suggest a bug in computing the tag.

That said, removing this assertion is irrelevant to fixing the bug — the salient changes are all in compiler/rustc_transmute/src/layout/tree.rs. Those changes fix the ICE even with this assertion intact.

Copy link
Member

Choose a reason for hiding this comment

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

I would suggest to replace the assertion by a comment, like

// The tag of a `Single` enum is like the tag of the niched variant: there's no tag
// as the discriminant is encoded entirely implicitly.
// If `write_discriminant` ever hits this case, we do a "validation read" to ensure
// the the right discriminant is encoded implicitly, so any attempt to write the
// wrong discriminant for a `Single` enum will reliably result in UB.

@rust-log-analyzer

This comment has been minimized.

@compiler-errors
Copy link
Member

@rustbot author

can take a look once this is fixed

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 12, 2024
@compiler-errors
Copy link
Member

compiler-errors commented Jun 12, 2024

Also, can you explain somewhere what this PR actually does? This PR has like no description -- typically a fix should justify what it's fixing unless the code speaks for itself, and in this case it doesn't really.

Previously, the implementation of `Tree::from_enum` incorrectly
treated enums with `Variants::Single` and `Variants::Multiple`
identically. This is incorrect for `Variants::Single` enums,
which delegate their layout to that of a variant with a particular
index (or no variant at all if the enum is empty).

This flaw manifested first as an ICE. `Tree::from_enum` attempted
to compute the tag of variants other than the one at
`Variants::Single`'s `index`, and fell afoul of a sanity-checking
assertion in `compiler/rustc_const_eval/src/interpret/discriminant.rs`.
This assertion is non-load-bearing, and can be removed; the routine
its in is well-behaved even without it.

With the assertion removed, the proximate issue becomes apparent:
calling `Tree::from_variant` on a variant that does not exist is
ill-defined. A sanity check the given variant has
`FieldShapes::Arbitrary` fails, and the analysis is (correctly)
aborted with `Err::NotYetSupported`.

This commit corrects this chain of failures by ensuring that
`Tree::from_variant` is not called on variants that are, as far as
layout is concerned, nonexistent. Specifically, the implementation
of `Tree::from_enum` is now partitioned into three cases:

  1. enums that are uninhabited
  2. enums for which all but one variant is uninhabited
  3. enums with multiple inhabited variants

`Tree::from_variant` is now only invoked in the third case. In the
first case, `Tree::uninhabited()` is produced. In the second case,
the layout is delegated to `Variants::Single`'s index.

Fixes rust-lang#125811
Copy link
Member

@compiler-errors compiler-errors left a comment

Choose a reason for hiding this comment

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

Thanks!

@compiler-errors
Copy link
Member

@bors r

@bors
Copy link
Contributor

bors commented Jun 13, 2024

📌 Commit fb662f2 has been approved by compiler-errors

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 13, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 13, 2024
…kingjubilee

Rollup of 10 pull requests

Successful merges:

 - rust-lang#125674 (Rewrite `symlinked-extern`, `symlinked-rlib` and `symlinked-libraries` `run-make` tests in `rmake.rs` format)
 - rust-lang#125688 (Walk into alias-eq nested goals even if normalization fails)
 - rust-lang#126142 (Harmonize using root or leaf obligation in trait error reporting)
 - rust-lang#126303 (Urls to docs in rust_hir)
 - rust-lang#126328 (Add Option::is_none_or)
 - rust-lang#126337 (Add test for walking order dependent opaque type behaviour)
 - rust-lang#126353 (Move `MatchAgainstFreshVars` to old solver)
 - rust-lang#126356 (docs(rustc): Improve discoverable of Cargo docs)
 - rust-lang#126358 (safe transmute: support `Single` enums)
 - rust-lang#126362 (Make `try_from_target_usize` method public)

r? `@ghost`
`@rustbot` modify labels: rollup
@RalfJung
Copy link
Member

PR already got rolled up so the comment should probably be a follow-up.

// the (unlikely?) event that an uninhabited enum is
// non-zero-sized, this assert will trigger an ICE, and this
// code should be modified such that a `layout.size` amount
// of uninhabited bytes is returned instead.
Copy link
Member

Choose a reason for hiding this comment

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

Have you tried an enum like enum E { Variant(!, i32) }? That may give rise to a non-zero-sized uninhabited enum.

@bors bors merged commit d33ec8e into rust-lang:master Jun 13, 2024
6 checks passed
@rustbot rustbot added this to the 1.81.0 milestone Jun 13, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jun 13, 2024
Rollup merge of rust-lang#126358 - jswrenn:fix-125811, r=compiler-errors

safe transmute: support `Single` enums

Previously, the implementation of `Tree::from_enum` incorrectly treated enums with `Variants::Single` and `Variants::Multiple` identically. This is incorrect for `Variants::Single` enums, which delegate their layout to that of a variant with a particular index (or no variant at all if the enum is empty).

This flaw manifested first as an ICE. `Tree::from_enum` attempted to compute the tag of variants other than the one at `Variants::Single`'s `index`, and fell afoul of a sanity-checking assertion in `compiler/rustc_const_eval/src/interpret/discriminant.rs`. This assertion is non-load-bearing, and can be removed; the routine its in is well-behaved even without it.

With the assertion removed, the proximate issue becomes apparent: calling `Tree::from_variant` on a variant that does not exist is ill-defined. A sanity check the given variant has `FieldShapes::Arbitrary` fails, and the analysis is (correctly) aborted with `Err::NotYetSupported`.

This commit corrects this chain of failures by ensuring that `Tree::from_variant` is not called on variants that are, as far as layout is concerned, nonexistent. Specifically, the implementation of `Tree::from_enum` is now partitioned into three cases:

  1. enums that are uninhabited
  2. enums for which all but one variant is uninhabited
  3. enums with multiple inhabited variants

`Tree::from_variant` is now only invoked in the third case. In the first case, `Tree::uninhabited()` is produced. In the second case, the layout is delegated to `Variants::Single`'s index.

Fixes rust-lang#125811
jswrenn added a commit to jswrenn/rust that referenced this pull request Jun 14, 2024
Previously, `Tree::from_enum`'s implementation branched into three disjoint
cases:

 1. enums that uninhabited
 2. enums for which all but one variant is uninhabited
 3. enums with multiple inhabited variants

This branching (incorrectly) did not differentiate between variantful and
variantless uninhabited enums. In both cases, we assumed (and asserted) that
uninhabited enums are zero-sized types. This assumption is false for enums like:

   enum Uninhabited { A(!, u128) }

...which, currently, has the same size as `u128`. This faulty assumption
manifested as the ICE reported in rust-lang#126460.

In this PR, we revise the first case of `Tree::from_enum` to consider only the
narrower category of "enums that are variantless". These enums, whose layouts
are described with `Variants::Single { index }`, are special in that they do not
have a variant at `index`.

The second case is revised to consider the broader category of "enums that defer
their layout to one of their variants"; i.e., enums whose layouts are described
with `Variants::Single { index }` but that do have a variant at `index`.

This PR also adds a comment requested by @RalfJung in his review of rust-lang#126358 to
`compiler/rustc_const_eval/src/interpret/discriminant.rs`.

Fixes rust-lang#126460
jswrenn added a commit to jswrenn/rust that referenced this pull request Jun 14, 2024
Previously, `Tree::from_enum`'s implementation branched into three disjoint
cases:

 1. enums that uninhabited
 2. enums for which all but one variant is uninhabited
 3. enums with multiple inhabited variants

This branching (incorrectly) did not differentiate between variantful and
variantless uninhabited enums. In both cases, we assumed (and asserted) that
uninhabited enums are zero-sized types. This assumption is false for enums like:

    enum Uninhabited { A(!, u128) }

...which, currently, has the same size as `u128`. This faulty assumption
manifested as the ICE reported in rust-lang#126460.

In this PR, we revise the first case of `Tree::from_enum` to consider only the
narrower category of "enums that are variantless". These enums, whose layouts
are described with `Variants::Single { index }`, are special in that they do not
have a variant at `index`.

The second case is revised to consider the broader category of "enums that defer
their layout to one of their variants"; i.e., enums whose layouts are described
with `Variants::Single { index }` but that do have a variant at `index`.

This PR also adds a comment requested by @RalfJung in his review of rust-lang#126358 to
`compiler/rustc_const_eval/src/interpret/discriminant.rs`.

Fixes rust-lang#126460
jswrenn added a commit to jswrenn/rust that referenced this pull request Jun 14, 2024
Previously, `Tree::from_enum`'s implementation branched into three disjoint
cases:

 1. enums that uninhabited
 2. enums for which all but one variant is uninhabited
 3. enums with multiple inhabited variants

This branching (incorrectly) did not differentiate between variantful and
variantless uninhabited enums. In both cases, we assumed (and asserted) that
uninhabited enums are zero-sized types. This assumption is false for enums like:

    enum Uninhabited { A(!, u128) }

...which, currently, has the same size as `u128`. This faulty assumption
manifested as the ICE reported in rust-lang#126460.

In this PR, we revise the first case of `Tree::from_enum` to consider only the
narrow category of "enums that are uninhabited ZSTs". These enums, whose layouts
are described with `Variants::Single { index }`, are special in their layouts
otherwise resemble the `!` type and cannot be descended into like typical enums.
This first case captures uninhabited enums like:

    enum Uninhabited { A(!, !), B(!) }

The second case is revised to consider the broader category of "enums that defer
their layout to one of their variants"; i.e., enums whose layouts are described
with `Variants::Single { index }` and that do have a variant at `index`. This
second case captures uninhabited enums that are not ZSTs, like:

    enum Uninhabited { A(!, u128) }

This PR also adds a comment requested by @RalfJung in his review of rust-lang#126358 to
`compiler/rustc_const_eval/src/interpret/discriminant.rs`.

Fixes rust-lang#126460
jswrenn added a commit to jswrenn/rust that referenced this pull request Jun 14, 2024
Previously, `Tree::from_enum`'s implementation branched into three disjoint
cases:

 1. enums that uninhabited
 2. enums for which all but one variant is uninhabited
 3. enums with multiple inhabited variants

This branching (incorrectly) did not differentiate between variantful and
variantless uninhabited enums. In both cases, we assumed (and asserted) that
uninhabited enums are zero-sized types. This assumption is false for enums like:

    enum Uninhabited { A(!, u128) }

...which, currently, has the same size as `u128`. This faulty assumption
manifested as the ICE reported in rust-lang#126460.

In this PR, we revise the first case of `Tree::from_enum` to consider only the
narrow category of "enums that are uninhabited ZSTs". These enums, whose layouts
are described with `Variants::Single { index }`, are special in their layouts
otherwise resemble the `!` type and cannot be descended into like typical enums.
This first case captures uninhabited enums like:

    enum Uninhabited { A(!, !), B(!) }

The second case is revised to consider the broader category of "enums that defer
their layout to one of their variants"; i.e., enums whose layouts are described
with `Variants::Single { index }` and that do have a variant at `index`. This
second case captures uninhabited enums that are not ZSTs, like:

    enum Uninhabited { A(!, u128) }

This PR also adds a comment requested by @RalfJung in his review of rust-lang#126358 to
`compiler/rustc_const_eval/src/interpret/discriminant.rs`.

Fixes rust-lang#126460
jswrenn added a commit to jswrenn/rust that referenced this pull request Jun 14, 2024
Previously, `Tree::from_enum`'s implementation branched into three disjoint
cases:

 1. enums that uninhabited
 2. enums for which all but one variant is uninhabited
 3. enums with multiple inhabited variants

This branching (incorrectly) did not differentiate between variantful and
variantless uninhabited enums. In both cases, we assumed (and asserted) that
uninhabited enums are zero-sized types. This assumption is false for enums like:

    enum Uninhabited { A(!, u128) }

...which, currently, has the same size as `u128`. This faulty assumption
manifested as the ICE reported in rust-lang#126460.

In this PR, we revise the first case of `Tree::from_enum` to consider only the
narrow category of "enums that are uninhabited ZSTs". These enums, whose layouts
are described with `Variants::Single { index }`, are special in their layouts
otherwise resemble the `!` type and cannot be descended into like typical enums.
This first case captures uninhabited enums like:

    enum Uninhabited { A(!, !), B(!) }

The second case is revised to consider the broader category of "enums that defer
their layout to one of their variants"; i.e., enums whose layouts are described
with `Variants::Single { index }` and that do have a variant at `index`. This
second case captures uninhabited enums that are not ZSTs, like:

    enum Uninhabited { A(!, u128) }

This PR also adds a comment requested by @RalfJung in his review of rust-lang#126358 to
`compiler/rustc_const_eval/src/interpret/discriminant.rs`.

Fixes rust-lang#126460
jswrenn added a commit to jswrenn/rust that referenced this pull request Jun 14, 2024
Previously, `Tree::from_enum`'s implementation branched into three disjoint
cases:

 1. enums that uninhabited
 2. enums for which all but one variant is uninhabited
 3. enums with multiple inhabited variants

This branching (incorrectly) did not differentiate between variantful and
variantless uninhabited enums. In both cases, we assumed (and asserted) that
uninhabited enums are zero-sized types. This assumption is false for enums like:

    enum Uninhabited { A(!, u128) }

...which, currently, has the same size as `u128`. This faulty assumption
manifested as the ICE reported in rust-lang#126460.

In this PR, we revise the first case of `Tree::from_enum` to consider only the
narrow category of "enums that are uninhabited ZSTs". These enums, whose layouts
are described with `Variants::Single { index }`, are special in their layouts
otherwise resemble the `!` type and cannot be descended into like typical enums.
This first case captures uninhabited enums like:

    enum Uninhabited { A(!, !), B(!) }

The second case is revised to consider the broader category of "enums that defer
their layout to one of their variants"; i.e., enums whose layouts are described
with `Variants::Single { index }` and that do have a variant at `index`. This
second case captures uninhabited enums that are not ZSTs, like:

    enum Uninhabited { A(!, u128) }

Finally, the third case is revised to cover the broader category of "enums with
multiple variants", which captures uninhabited enums like:

    enum Uninhabited { A(u8, !), B(!, u32) }

This PR also adds a comment requested by @RalfJung in his review of rust-lang#126358 to
`compiler/rustc_const_eval/src/interpret/discriminant.rs`.

Fixes rust-lang#126460
jswrenn added a commit to jswrenn/rust that referenced this pull request Jun 14, 2024
Previously, `Tree::from_enum`'s implementation branched into three disjoint
cases:

 1. enums that uninhabited
 2. enums for which all but one variant is uninhabited
 3. enums with multiple inhabited variants

This branching (incorrectly) did not differentiate between variantful and
variantless uninhabited enums. In both cases, we assumed (and asserted) that
uninhabited enums are zero-sized types. This assumption is false for enums like:

    enum Uninhabited { A(!, u128) }

...which, currently, has the same size as `u128`. This faulty assumption
manifested as the ICE reported in rust-lang#126460.

In this PR, we revise the first case of `Tree::from_enum` to consider only the
narrow category of "enums that are uninhabited ZSTs". These enums, whose layouts
are described with `Variants::Single { index }`, are special in their layouts
otherwise resemble the `!` type and cannot be descended into like typical enums.
This first case captures uninhabited enums like:

    enum Uninhabited { A(!, !), B(!) }

The second case is revised to consider the broader category of "enums that defer
their layout to one of their variants"; i.e., enums whose layouts are described
with `Variants::Single { index }` and that do have a variant at `index`. This
second case captures uninhabited enums that are not ZSTs, like:

    enum Uninhabited { A(!, u128) }

Finally, the third case is revised to cover the broader category of "enums with
multiple variants", which captures uninhabited enums like:

    enum Uninhabited { A(u8, !), B(!, u32) }

This PR also adds a comment requested by RalfJung in his review of rust-lang#126358 to
`compiler/rustc_const_eval/src/interpret/discriminant.rs`.

Fixes rust-lang#126460
jswrenn added a commit to jswrenn/rust that referenced this pull request Jun 14, 2024
Previously, `Tree::from_enum`'s implementation branched into three disjoint
cases:

 1. enums that uninhabited
 2. enums for which all but one variant is uninhabited
 3. enums with multiple inhabited variants

This branching (incorrectly) did not differentiate between variantful and
variantless uninhabited enums. In both cases, we assumed (and asserted) that
uninhabited enums are zero-sized types. This assumption is false for enums like:

    enum Uninhabited { A(!, u128) }

...which, currently, has the same size as `u128`. This faulty assumption
manifested as the ICE reported in rust-lang#126460.

In this PR, we revise the first case of `Tree::from_enum` to consider only the
narrow category of "enums that are uninhabited ZSTs". These enums, whose layouts
are described with `Variants::Single { index }`, are special in their layouts
otherwise resemble the `!` type and cannot be descended into like typical enums.
This first case captures uninhabited enums like:

    enum Uninhabited { A(!, !), B(!) }

The second case is revised to consider the broader category of "enums that defer
their layout to one of their variants"; i.e., enums whose layouts are described
with `Variants::Single { index }` and that do have a variant at `index`. This
second case captures uninhabited enums that are not ZSTs, like:

    enum Uninhabited { A(!, u128) }

...which represent their variants with `Variants::Single`.

Finally, the third case is revised to cover the broader category of "enums with
multiple variants", which captures uninhabited, non-ZST enums like:

    enum Uninhabited { A(u8, !), B(!, u32) }

...which represent their variants with `Variants::Multiple`.

This PR also adds a comment requested by RalfJung in his review of rust-lang#126358 to
`compiler/rustc_const_eval/src/interpret/discriminant.rs`.

Fixes rust-lang#126460
jswrenn added a commit to jswrenn/rust that referenced this pull request Jun 14, 2024
Previously, `Tree::from_enum`'s implementation branched into three disjoint
cases:

 1. enums that uninhabited
 2. enums for which all but one variant is uninhabited
 3. enums with multiple inhabited variants

This branching (incorrectly) did not differentiate between variantful and
variantless uninhabited enums. In both cases, we assumed (and asserted) that
uninhabited enums are zero-sized types. This assumption is false for enums like:

    enum Uninhabited { A(!, u128) }

...which, currently, has the same size as `u128`. This faulty assumption
manifested as the ICE reported in rust-lang#126460.

In this PR, we revise the first case of `Tree::from_enum` to consider only the
narrow category of "enums that are uninhabited ZSTs". These enums, whose layouts
are described with `Variants::Single { index }`, are special in their layouts
otherwise resemble the `!` type and cannot be descended into like typical enums.
This first case captures uninhabited enums like:

    enum Uninhabited { A(!, !), B(!) }

The second case is revised to consider the broader category of "enums that defer
their layout to one of their variants"; i.e., enums whose layouts are described
with `Variants::Single { index }` and that do have a variant at `index`. This
second case captures uninhabited enums that are not ZSTs, like:

    enum Uninhabited { A(!, u128) }

...which represent their variants with `Variants::Single`.

Finally, the third case is revised to cover the broader category of "enums with
multiple variants", which captures uninhabited, non-ZST enums like:

    enum Uninhabited { A(u8, !), B(!, u32) }

...which represent their variants with `Variants::Multiple`.

This PR also adds a comment requested by RalfJung in his review of rust-lang#126358 to
`compiler/rustc_const_eval/src/interpret/discriminant.rs`.

Fixes rust-lang#126460
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Jun 18, 2024
safe transmute: support non-ZST, variantful, uninhabited enums

Previously, `Tree::from_enum`'s implementation branched into three disjoint cases:

 1. enums that uninhabited
 2. enums for which all but one variant is uninhabited
 3. enums with multiple variants

This branching (incorrectly) did not differentiate between variantful and variantless uninhabited enums. In both cases, we assumed (and asserted) that uninhabited enums are zero-sized types. This assumption is false for enums like:

    enum Uninhabited { A(!, u128) }

...which, currently, has the same size as `u128`. This faulty assumption manifested as the ICE reported in rust-lang#126460.

In this PR, we revise the first case of `Tree::from_enum` to consider only the narrow category of "enums that are uninhabited ZSTs". These enums, whose layouts are described with `Variants::Single { index }`, are special in their layouts otherwise resemble the `!` type and cannot be descended into like typical enums. This first case captures uninhabited enums like:

    enum Uninhabited { A(!, !), B(!) }

The second case is revised to consider the broader category of "enums that defer their layout to one of their variants"; i.e., enums whose layouts are described with `Variants::Single { index }` and that do have a variant at `index`. This second case captures uninhabited enums that are not ZSTs, like:

    enum Uninhabited { A(!, u128) }

...which represent their variants with `Variants::Single`.

Finally, the third case is revised to cover the broader category of "enums with multiple variants", which captures uninhabited enums like:

    enum Uninhabited { A(u8, !), B(!, u32) }

...which represent their variants with `Variants::Multiple`.

This PR also adds a comment requested by `@RalfJung` in his review of rust-lang#126358 to `compiler/rustc_const_eval/src/interpret/discriminant.rs`.

Fixes rust-lang#126460

r? `@compiler-errors`
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Jun 18, 2024
safe transmute: support non-ZST, variantful, uninhabited enums

Previously, `Tree::from_enum`'s implementation branched into three disjoint cases:

 1. enums that uninhabited
 2. enums for which all but one variant is uninhabited
 3. enums with multiple variants

This branching (incorrectly) did not differentiate between variantful and variantless uninhabited enums. In both cases, we assumed (and asserted) that uninhabited enums are zero-sized types. This assumption is false for enums like:

    enum Uninhabited { A(!, u128) }

...which, currently, has the same size as `u128`. This faulty assumption manifested as the ICE reported in rust-lang#126460.

In this PR, we revise the first case of `Tree::from_enum` to consider only the narrow category of "enums that are uninhabited ZSTs". These enums, whose layouts are described with `Variants::Single { index }`, are special in their layouts otherwise resemble the `!` type and cannot be descended into like typical enums. This first case captures uninhabited enums like:

    enum Uninhabited { A(!, !), B(!) }

The second case is revised to consider the broader category of "enums that defer their layout to one of their variants"; i.e., enums whose layouts are described with `Variants::Single { index }` and that do have a variant at `index`. This second case captures uninhabited enums that are not ZSTs, like:

    enum Uninhabited { A(!, u128) }

...which represent their variants with `Variants::Single`.

Finally, the third case is revised to cover the broader category of "enums with multiple variants", which captures uninhabited enums like:

    enum Uninhabited { A(u8, !), B(!, u32) }

...which represent their variants with `Variants::Multiple`.

This PR also adds a comment requested by ``@RalfJung`` in his review of rust-lang#126358 to `compiler/rustc_const_eval/src/interpret/discriminant.rs`.

Fixes rust-lang#126460

r? ``@compiler-errors``
jieyouxu added a commit to jieyouxu/rust that referenced this pull request Jun 19, 2024
safe transmute: support non-ZST, variantful, uninhabited enums

Previously, `Tree::from_enum`'s implementation branched into three disjoint cases:

 1. enums that uninhabited
 2. enums for which all but one variant is uninhabited
 3. enums with multiple variants

This branching (incorrectly) did not differentiate between variantful and variantless uninhabited enums. In both cases, we assumed (and asserted) that uninhabited enums are zero-sized types. This assumption is false for enums like:

    enum Uninhabited { A(!, u128) }

...which, currently, has the same size as `u128`. This faulty assumption manifested as the ICE reported in rust-lang#126460.

In this PR, we revise the first case of `Tree::from_enum` to consider only the narrow category of "enums that are uninhabited ZSTs". These enums, whose layouts are described with `Variants::Single { index }`, are special in their layouts otherwise resemble the `!` type and cannot be descended into like typical enums. This first case captures uninhabited enums like:

    enum Uninhabited { A(!, !), B(!) }

The second case is revised to consider the broader category of "enums that defer their layout to one of their variants"; i.e., enums whose layouts are described with `Variants::Single { index }` and that do have a variant at `index`. This second case captures uninhabited enums that are not ZSTs, like:

    enum Uninhabited { A(!, u128) }

...which represent their variants with `Variants::Single`.

Finally, the third case is revised to cover the broader category of "enums with multiple variants", which captures uninhabited enums like:

    enum Uninhabited { A(u8, !), B(!, u32) }

...which represent their variants with `Variants::Multiple`.

This PR also adds a comment requested by ```@RalfJung``` in his review of rust-lang#126358 to `compiler/rustc_const_eval/src/interpret/discriminant.rs`.

Fixes rust-lang#126460

r? ```@compiler-errors```
jieyouxu added a commit to jieyouxu/rust that referenced this pull request Jun 19, 2024
safe transmute: support non-ZST, variantful, uninhabited enums

Previously, `Tree::from_enum`'s implementation branched into three disjoint cases:

 1. enums that uninhabited
 2. enums for which all but one variant is uninhabited
 3. enums with multiple variants

This branching (incorrectly) did not differentiate between variantful and variantless uninhabited enums. In both cases, we assumed (and asserted) that uninhabited enums are zero-sized types. This assumption is false for enums like:

    enum Uninhabited { A(!, u128) }

...which, currently, has the same size as `u128`. This faulty assumption manifested as the ICE reported in rust-lang#126460.

In this PR, we revise the first case of `Tree::from_enum` to consider only the narrow category of "enums that are uninhabited ZSTs". These enums, whose layouts are described with `Variants::Single { index }`, are special in their layouts otherwise resemble the `!` type and cannot be descended into like typical enums. This first case captures uninhabited enums like:

    enum Uninhabited { A(!, !), B(!) }

The second case is revised to consider the broader category of "enums that defer their layout to one of their variants"; i.e., enums whose layouts are described with `Variants::Single { index }` and that do have a variant at `index`. This second case captures uninhabited enums that are not ZSTs, like:

    enum Uninhabited { A(!, u128) }

...which represent their variants with `Variants::Single`.

Finally, the third case is revised to cover the broader category of "enums with multiple variants", which captures uninhabited enums like:

    enum Uninhabited { A(u8, !), B(!, u32) }

...which represent their variants with `Variants::Multiple`.

This PR also adds a comment requested by ````@RalfJung```` in his review of rust-lang#126358 to `compiler/rustc_const_eval/src/interpret/discriminant.rs`.

Fixes rust-lang#126460

r? ````@compiler-errors````
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jun 19, 2024
Rollup merge of rust-lang#126493 - jswrenn:fix-126460, r=compiler-errors

safe transmute: support non-ZST, variantful, uninhabited enums

Previously, `Tree::from_enum`'s implementation branched into three disjoint cases:

 1. enums that uninhabited
 2. enums for which all but one variant is uninhabited
 3. enums with multiple variants

This branching (incorrectly) did not differentiate between variantful and variantless uninhabited enums. In both cases, we assumed (and asserted) that uninhabited enums are zero-sized types. This assumption is false for enums like:

    enum Uninhabited { A(!, u128) }

...which, currently, has the same size as `u128`. This faulty assumption manifested as the ICE reported in rust-lang#126460.

In this PR, we revise the first case of `Tree::from_enum` to consider only the narrow category of "enums that are uninhabited ZSTs". These enums, whose layouts are described with `Variants::Single { index }`, are special in their layouts otherwise resemble the `!` type and cannot be descended into like typical enums. This first case captures uninhabited enums like:

    enum Uninhabited { A(!, !), B(!) }

The second case is revised to consider the broader category of "enums that defer their layout to one of their variants"; i.e., enums whose layouts are described with `Variants::Single { index }` and that do have a variant at `index`. This second case captures uninhabited enums that are not ZSTs, like:

    enum Uninhabited { A(!, u128) }

...which represent their variants with `Variants::Single`.

Finally, the third case is revised to cover the broader category of "enums with multiple variants", which captures uninhabited enums like:

    enum Uninhabited { A(u8, !), B(!, u32) }

...which represent their variants with `Variants::Multiple`.

This PR also adds a comment requested by ````@RalfJung```` in his review of rust-lang#126358 to `compiler/rustc_const_eval/src/interpret/discriminant.rs`.

Fixes rust-lang#126460

r? ````@compiler-errors````
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ICE: compiler/rustc_const_eval/src/interpret/discriminant.rs assertion failed 0 != 1
7 participants