Variants::Single: do not use invalid VariantIdx for uninhabited enums by RalfJung · Pull Request #133702 · rust-lang/rust

RalfJung · 2024-12-01T13:09:02Z

~~Stacked on top of #133681, only the last commit is new.~~

Currently, Variants::Single for an empty enum contains a VariantIdx of 0; looking that up in the enum variant list will ICE. That's quite confusing. So let's fix that by adding a new Variants::Empty case for types that have 0 variants.

try-job: i686-msvc

rustbot · 2024-12-01T13:09:10Z

r? @cjgillot

rustbot has assigned @cjgillot.
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 · 2024-12-01T13:09:11Z

Some changes occurred in compiler/rustc_codegen_gcc

cc @antoyo, @GuillaumeGomez

This PR changes Stable MIR

cc @oli-obk, @celinval, @ouz-a

Some changes occurred to the CTFE / Miri interpreter

cc @rust-lang/miri

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

Some changes occurred to the CTFE / Miri interpreter

cc @rust-lang/miri, @rust-lang/wg-const-eval

Some changes occurred to the CTFE machinery

cc @rust-lang/wg-const-eval

The Miri subtree was changed

cc @rust-lang/miri

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

RalfJung · 2024-12-01T13:10:52Z

 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.


@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.)

RalfJung · 2024-12-01T13:11:51Z

 #[derive(Clone, Debug, PartialEq, Eq, Hash, Serialize)]
 pub enum VariantsShape {
 /// Single enum variants, structs/tuples, unions, and all non-ADTs.
+ // FIXME: needs to become `Option` like in the internal type.


@oli-obk, @celinval, @ouz-a what is the right place to note down "this should be fixed on the next breaking stable MIR update"?

I ended up making this a separate variant, and hence also added that variant in stable-mir.

bjorn3 · 2024-12-01T13:12:22Z

Would introducing Variants::Empty instead work too? That would avoid a bunch of unwraps and allow replacing .is_uninhabited() checks with appropriate code in the Variants::Empty arm of the match.

RalfJung · 2024-12-01T13:15:39Z

I considered that, but I think that would make for a larger diff overall. Also, unless we want to enforce that all uninhabited types use Variants::Empty, we couldn't replace is_uninhabited checks.

But maybe I should actually give it a try to see how it pans out.

rustbot · 2024-12-01T13:57:44Z

rust-analyzer is developed in its own repository. If possible, consider making this change to rust-lang/rust-analyzer instead.

cc @rust-lang/rust-analyzer

RalfJung · 2024-12-01T16:48:47Z

Would introducing Variants::Empty instead work too? That would avoid a bunch of unwraps and allow replacing .is_uninhabited() checks with appropriate code in the Variants::Empty arm of the match.

Yeah that works pretty well. :)

RalfJung · 2024-12-01T16:49:54Z

- 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 =>


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

RalfJung · 2024-12-01T16:57:46Z

 return Ok(0);
 };
 match &layout.variants {
+ Variants::Empty => unreachable!(),


I don't know why this is unreachable, but the old logic would already have panicked here in at least some of same cases since the variants[index.0] below would fail for no-variant enums.

It seems to be used only in mir evaluator, which I guess would not go into unreachable code.

RalfJung · 2024-12-04T06:35:19Z

The PR this is based on landed, so this is ready for review. :)

cjgillot · 2024-12-07T17:21:07Z

+ // Types without variants use `0` as dummy variant index.
+ assert!(index.as_u32() == 0);


Isn't the point of this PR to get rid of this hack?

No. There are actually two hacks:
Some types don't have a concept of "variant", like tuples or primitives. Those are still Variants::Single and that makes sense, and they use index 0. I considered making this an Option but then the value 0 has to be duplicated in all codegen backends, so centralizing that in the layout logic seems better.
Some types are actually 0-variant types. Those are being changed in this PR.

The key invariant this PR achieves is that if we have Variants::Single, and if the type has a discriminant_range, then the index is in that range. This is useful because it means if the type is an ADT, we can use the single variant index to index the variant list without ICEing. So far, this always requires a special check for empty enums, which is error-prone.

Rollup of 8 pull requests Successful merges: - rust-lang#133702 (Variants::Single: do not use invalid VariantIdx for uninhabited enums) - rust-lang#133926 (Fix const conditions for RPITITs) - rust-lang#134161 (Overhaul token cursors) - rust-lang#134253 (Overhaul keyword handling) - rust-lang#134394 (Clarify the match ergonomics 2024 migration lint's output) - rust-lang#134420 (refactor: replace &PathBuf with &Path to enhance generality) - rust-lang#134444 (Fix `x build --stage 1 std` when using cg_cranelift as the default backend) - rust-lang#134452 (fix(LazyCell): documentation of get[_mut] was wrong) r? `@ghost` `@rustbot` modify labels: rollup

jieyouxu · 2024-12-18T13:53:54Z

I wonder if this changed cdb output in rollup:

@bors r-

jieyouxu · 2024-12-18T13:54:02Z

Trying to see:
@bors try

bors · 2024-12-18T13:55:14Z

⌛ Trying commit 397ae3c with merge 48671e3...

Variants::Single: do not use invalid VariantIdx for uninhabited enums ~~Stacked on top of rust-lang#133681, only the last commit is new.~~ Currently, `Variants::Single` for an empty enum contains a `VariantIdx` of 0; looking that up in the enum variant list will ICE. That's quite confusing. So let's fix that by adding a new `Variants::Empty` case for types that have 0 variants. try-job: i686-msvc

RalfJung · 2024-12-18T14:01:13Z

Oh gosh, do we have a windows person that knows this debug info stuff? If I can't even run the tests, how am I supposed to fix them?^^

It seems odd though since the enums mentioned there all do have inhabited variants, so nothing should change about them...

jieyouxu · 2024-12-18T14:09:20Z

Oh gosh, do we have a windows person that knows this debug info stuff? If I can't even run the tests, how am I supposed to fix them?^^

I can try to run this locally in a moment.

jieyouxu · 2024-12-18T14:50:51Z

Ok so I ran this locally, and it doesn't fail with cdb distributed as part of 10.0.26100.1742. I stared at the rollup log a bit more, and it had stuff like

tests\debuginfo\issue-13213.rs ... ignored, ignored when the debugger is cdb (Fails with exit code 0xc0000135 ("the application failed to initialize properly"))

I wonder if cdb just somehow failed to init properly... Probably requeue this PR once the try job comes back, no idea what actually went wrong... I only had a vague suspicion for this PR as this PR seemed to be the only one that actually touched debuginfo remotely 🤷

bors · 2024-12-18T16:08:06Z

☀️ Try build successful - checks-actions
Build commit: 48671e3 (48671e3deea71b4f4b32142ef5300ebb0e2117d6)

jieyouxu · 2024-12-18T16:44:45Z

@bors r=@oli-obk

bors · 2024-12-18T16:44:48Z

📌 Commit 397ae3c has been approved by oli-obk

It is now in the queue for this repository.

jdupak-ms · 2024-12-18T17:20:05Z

I know those test quite well so I can look into it tomorrow morning if you have not it figured by then. There were some breaking changes in WinDbg, so that might be a factor.

…

On Wed, 18 Dec 2024, at 15:01, Ralf Jung wrote: Oh gosh, do we have a windows person that knows this debug info stuff? If I can't even run the tests, how am I supposed to fix them?^^ It seems odd though since the enums mentioned there all do have inhabited variants, so nothing should change about them... — Reply to this email directly, view it on GitHub <#133702 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/BNUYRJZTRRALYFHIPG7KUN32GF54JAVCNFSM6AAAAABSZVHGZOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDKNJRGQYDAMZYHA>. You are receiving this because you commented.Message ID: ***@***.***>

jdupak-ms · 2024-12-18T17:22:26Z

There is a breaking change in 26100. I have fixes for that locally. (Looking just from phone so I am guessing that is the problem.)

On Wed, 18 Dec 2024, at 15:51, 许杰友 Jieyou Xu (Joe) wrote: Ok so I ran this locally, and it doesn't fail with cdb distributed as part of `10.0.26100.1742`. I stared at the rollup log at it had stuff like `tests\debuginfo\issue-13213.rs ... ignored, ignored when the debugger is cdb (Fails with exit code 0xc0000135 ("the application failed to initialize properly"))

`

…

I wonder if cdb just somehow failed to init properly... Probably requeue this PR once the try job comes back, no idea what actually went wrong... — Reply to this email directly, view it on GitHub <#133702 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/BNUYRJ4HR3IDVSHH64F6R3L2GGDWLAVCNFSM6AAAAABSZVHGZOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDKNJRGUYTMOBTG4>. You are receiving this because you commented.Message ID: ***@***.***>

jieyouxu · 2024-12-18T17:31:00Z

I know those test quite well so I can look into it tomorrow morning if you have not it figured by then. There were some breaking changes in WinDbg, so that might be a factor.

Would be great if you could double-check the test if this PR fails again, at the moment I can't repro the failure, so it might be spurious somehow in the rollup...

Rollup of 7 pull requests Successful merges: - rust-lang#133702 (Variants::Single: do not use invalid VariantIdx for uninhabited enums) - rust-lang#134427 (ci: remove duplicate task definition) - rust-lang#134432 (Fix intra doc links not generated inside footnote definitions) - rust-lang#134437 (reduce compiler `Assemble` complexity) - rust-lang#134474 (Forbid overwriting types in typeck) - rust-lang#134477 (move lint_unused_mut into sub-fn) - rust-lang#134491 (Some destructor/drop related tweaks) r? `@ghost` `@rustbot` modify labels: rollup

jdupak-ms · 2024-12-19T12:22:31Z

I know those test quite well so I can look into it tomorrow morning if you have not it figured by then. There were some breaking changes in WinDbg, so that might be a factor.
Would be great if you could double-check the test if this PR fails again, at the moment I can't repro the failure, so it might be spurious somehow in the rollup...

Everything is green for me.

Rollup merge of rust-lang#133702 - RalfJung:single-variant, r=oli-obk Variants::Single: do not use invalid VariantIdx for uninhabited enums ~~Stacked on top of rust-lang#133681, only the last commit is new.~~ Currently, `Variants::Single` for an empty enum contains a `VariantIdx` of 0; looking that up in the enum variant list will ICE. That's quite confusing. So let's fix that by adding a new `Variants::Empty` case for types that have 0 variants. try-job: i686-msvc

Variants::Single: do not use invalid VariantIdx for uninhabited enums ~~Stacked on top of rust-lang#133681, only the last commit is new.~~ Currently, `Variants::Single` for an empty enum contains a `VariantIdx` of 0; looking that up in the enum variant list will ICE. That's quite confusing. So let's fix that by adding a new `Variants::Empty` case for types that have 0 variants. try-job: i686-msvc

Propagated from rust-lang/rust#133702. Empty enums now have `Variants::Empty` as variants instead of `Variants::Single{ index: None }`.

Variants::Single: do not use invalid VariantIdx for uninhabited enums ~~Stacked on top of rust-lang#133681, only the last commit is new.~~ Currently, `Variants::Single` for an empty enum contains a `VariantIdx` of 0; looking that up in the enum variant list will ICE. That's quite confusing. So let's fix that by adding a new `Variants::Empty` case for types that have 0 variants. try-job: i686-msvc

rustbot assigned cjgillot Dec 1, 2024

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 Dec 1, 2024

RalfJung commented Dec 1, 2024
View reviewed changes

This comment has been minimized.
Sign in to view

RalfJung force-pushed the single-variant branch from 1114912 to c7b2a22Compare December 1, 2024 13:34

This comment has been minimized.
Sign in to view

RalfJung force-pushed the single-variant branch from c7b2a22 to 1b86d85Compare December 1, 2024 13:57

This comment has been minimized.
Sign in to view

RalfJung force-pushed the single-variant branch from 1b86d85 to 7a7e0dbCompare December 1, 2024 14:42

This comment has been minimized.
Sign in to view

RalfJung force-pushed the single-variant branch 2 times, most recently from a610961 to fa08c41Compare December 1, 2024 16:48

RalfJung commented Dec 1, 2024
View reviewed changes

Comment threadcompiler/rustc_transmute/src/layout/tree.rs Outdated

RalfJung force-pushed the single-variant branch from fa08c41 to e0bab7dCompare December 1, 2024 16:54

RalfJung commented Dec 1, 2024
View reviewed changes

RalfJung force-pushed the single-variant branch 2 times, most recently from b90ebfb to 786730aCompare December 1, 2024 17:28

RalfJung mentioned this pull request Dec 3, 2024
improve TagEncoding::Niche docs, sanity check, and UB checks #133681
Merged

RalfJung force-pushed the single-variant branch from c55262f to 316c70fCompare December 4, 2024 06:34

cjgillot reviewed Dec 7, 2024
View reviewed changes

bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Dec 18, 2024

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 Dec 18, 2024

jieyouxu mentioned this pull request Dec 19, 2024
Rollup of 7 pull requests #134499
Merged

bors merged commit 2a43ce0 into rust-lang:masterDec 19, 2024

rustbot added this to the 1.85.0 milestone Dec 19, 2024

RalfJung deleted the single-variant branch December 20, 2024 08:31

		// Types without variants use `0` as dummy variant index.
		assert!(index.as_u32() == 0);

Uh oh!

Uh oh!

Conversation

RalfJung commented Dec 1, 2024• edited by jieyouxu Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented Dec 1, 2024

Uh oh!

rustbot commented Dec 1, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bjorn3 commented Dec 1, 2024

Uh oh!

RalfJung commented Dec 1, 2024

Uh oh!

This comment has been minimized.

This comment has been minimized.

rustbot commented Dec 1, 2024

Uh oh!

This comment has been minimized.

This comment has been minimized.

RalfJung commented Dec 1, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RalfJung commented Dec 4, 2024

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RalfJungDec 8, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jieyouxu commented Dec 18, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jieyouxu commented Dec 18, 2024

Uh oh!

bors commented Dec 18, 2024

Uh oh!

RalfJung commented Dec 18, 2024

Uh oh!

jieyouxu commented Dec 18, 2024

Uh oh!

jieyouxu commented Dec 18, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bors commented Dec 18, 2024

Uh oh!

jieyouxu commented Dec 18, 2024

Uh oh!

bors commented Dec 18, 2024

Uh oh!

jdupak-ms commented Dec 18, 2024 via email

Uh oh!

jdupak-ms commented Dec 18, 2024 via email

Uh oh!

jieyouxu commented Dec 18, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jdupak-ms commented Dec 19, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

RalfJung commented Dec 1, 2024•
edited by jieyouxu
Loading

RalfJungDec 8, 2024•
edited
Loading

jieyouxu commented Dec 18, 2024•
edited
Loading

jieyouxu commented Dec 18, 2024•
edited
Loading

jieyouxu commented Dec 18, 2024•
edited
Loading