Stream: t-compiler

Topic: ICE access field of uninhabited enum #69191


pnkfelix (Mar 05 2020 at 21:35, on Zulip):

Hi @oli and @eddyb ; I imagine you're both busy, so I'll try to keep this quick

pnkfelix (Mar 05 2020 at 21:37, on Zulip):

the panic message actually makes it easy to understand what's gone wrong, at a fine grain:

pnkfelix (Mar 05 2020 at 21:37, on Zulip):

We're trying to access field 0 of the following "union layout"

LayoutDetails {
        variants: Single {
            index: 0,
        },
        fields: Union(
            0,
        ),
        abi: Uninhabited,
...
}
pnkfelix (Mar 05 2020 at 21:38, on Zulip):

but its actually an uninhabited type, as indicated by the ABI

pnkfelix (Mar 05 2020 at 21:38, on Zulip):

Now, the actual type to led to this in my case looks like: enum UninhabitedUnivariant { Variant(Void), }

pnkfelix (Mar 05 2020 at 21:40, on Zulip):

so here's my question: In an ideal world, would the above: 1. have fields: FieldPlacement::Union(1), or 2. we would have a separate FieldPlacement variant for uninhabited types, or 3. don't change the data at all; change the panicking code to first check the LayoutDetails.abi to catch the uninhabited case?

pnkfelix (Mar 05 2020 at 21:43, on Zulip):

another way of putting my question: Is our long term intention to actually attach a layout here that indicates that there is a field (and it is just uninhabited)? Or are we better off always explicitly saying, in the LayoutDetails representation itself, that "such a type has no realized fields" (which is what FieldPlacement::Uninhabited proposed in option (2) above would be used for).

pnkfelix (Mar 05 2020 at 21:43, on Zulip):

/me suddenly remembers how late it is in the EU

pnkfelix (Mar 05 2020 at 21:57, on Zulip):

for the short term I will take the most narrow solution, from option (3), and have the code here do something like throw_ub!(Unreachable);

pnkfelix (Mar 05 2020 at 22:05, on Zulip):

oh, we cannot do (2), at least not in all cases, due to #49298

pnkfelix (Mar 05 2020 at 22:05, on Zulip):

and since (2) cannot apply in all cases, it probably is a fragile solution to #69191

pnkfelix (Mar 05 2020 at 22:30, on Zulip):

wow, even my "most narrow solution" caused existing test issue-64506.rs to regress (obviously wasn't as narrow as it could have been)

pnkfelix (Mar 05 2020 at 22:31, on Zulip):

in fact, maybe #69191 is another instance of #64506 ?

pnkfelix (Mar 05 2020 at 22:33, on Zulip):

interesting, it seems like a deliberate decision was made to treat structs different from enums in PR #64987

pnkfelix (Mar 05 2020 at 22:37, on Zulip):

so then: @oli, what was the reason to treat structs different from enums here? Expediency of implementation? Or because they should never arise in the manner that structs do (since we don't allow partially initialized enums) ?

oli (Mar 05 2020 at 22:57, on Zulip):

I think 1) is the right solution. I don't remember if I tried it. It's kinda weird because you need to pick a variant to continue with. So if you had multiple uninhabited variants, I'm not sure if we wouldn't just end up running into other assertions

oli (Mar 05 2020 at 22:58, on Zulip):

If you don't pick a variant and continue into the enum code, everything will die badly iirc.

oli (Mar 05 2020 at 22:59, on Zulip):

But again, I don't remember what exactly I tried. I was attempting to go with minimal changes to the code though

pnkfelix (Mar 06 2020 at 00:16, on Zulip):

this may be an instance of where we treat univariant enums specially

pnkfelix (Mar 06 2020 at 00:35, on Zulip):

pnkfelix said:

this may be an instance of where we treat univariant enums specially

okay, yes, I can see how to easily generalize PR #64987 to treat univariant enums just like structs.

pnkfelix (Mar 06 2020 at 01:06, on Zulip):

... and that didn't work. Back to my original plan of doing something like (3), but even more narrowly than my original vision.

eddyb (Mar 06 2020 at 10:11, on Zulip):

just found this notification. I would've gotten to it much sooner if it wasn't on Zulip (because that can't notify me on my phone)

eddyb (Mar 06 2020 at 10:12, on Zulip):

Discord PMs are probably the only thing guaranteed to reach me

eddyb (Mar 06 2020 at 10:12, on Zulip):

(quickly, I mean)

eddyb (Mar 06 2020 at 10:12, on Zulip):

@pnkfelix I'm really sorry for all the time wasted, but no matter how busy I am, layout bugs are a priority

eddyb (Mar 06 2020 at 10:13, on Zulip):

I just wish I saw all this sooner

eddyb (Mar 06 2020 at 10:16, on Zulip):

ftr the explanation and proposed fix are in https://github.com/rust-lang/rust/issues/69763#issuecomment-595688090, @oli is trying it out as we speak

oli (Mar 06 2020 at 10:16, on Zulip):

just finished the tests, it works

eddyb (Mar 06 2020 at 10:17, on Zulip):

@pnkfelix so what do you want to do with your PR? should we still merge it? what do we do about the rollup (ideally we would avoid "critical fix" and rollups interacting...)

eddyb (Mar 06 2020 at 10:19, on Zulip):

hmm the rollup failed so I can at least take it out of the rollup

eddyb (Mar 06 2020 at 10:19, on Zulip):

why is the tree closed?!

lqd (Mar 06 2020 at 10:19, on Zulip):

the tree is closed don't worry about it

lqd (Mar 06 2020 at 10:19, on Zulip):

some msys2 problems

lqd (Mar 06 2020 at 10:20, on Zulip):

https://github.com/msys2/MSYS2-packages/issues/1884

eddyb (Mar 06 2020 at 10:20, on Zulip):

okay I was scared it was some release thing

eddyb (Mar 06 2020 at 10:40, on Zulip):

also I can't tell what the timeframe is, when's the deadline for landing this?

oli (Mar 06 2020 at 10:42, on Zulip):

https://forge.rust-lang.org/ (March 12th I think?)

oli (Mar 06 2020 at 10:42, on Zulip):

So thursday next week

eddyb (Mar 06 2020 at 10:42, on Zulip):

@pnkfelix sorry again for all the friction, I've r+'d https://github.com/rust-lang/rust/pull/69768, hope that's okay with you?

eddyb (Mar 06 2020 at 10:43, on Zulip):

@oli okay that makes sense, so this has to land in the next day or two

oli (Mar 06 2020 at 10:43, on Zulip):

yea, because we still need to backport it to beta

oli (Mar 06 2020 at 10:44, on Zulip):

this will basically be insta-stable

eddyb (Mar 06 2020 at 10:47, on Zulip):

@oli btw have you tried testing with two uninhabited variants? that should also be broken on master and fixed by the PR. or is that unreachable from const-prop?

oli (Mar 06 2020 at 10:47, on Zulip):

I've tried some things with nested fields, but not multiple variants, gonna do so now

eddyb (Mar 06 2020 at 10:47, on Zulip):

the 0th variant has been broken forever, basically

eddyb (Mar 06 2020 at 10:49, on Zulip):

@pnkfelix btw, I explained to @oli on IRC that you couldn't have fixed this outside for_variant, or at least not as generally, without increasing the size of some types, because there's no way to have Variants::Multiple without at least one byte for the tag, so uninhabited enum variants are currently an "on-demand" hack in for_variant

eddyb (Mar 06 2020 at 10:51, on Zulip):

but we can't paper over this forever, which is why I think we should do the thing at the end of https://github.com/rust-lang/rust/issues/69763#issuecomment-595699745

oli (Mar 06 2020 at 10:51, on Zulip):

I considered creating Variants::None just before deciding to let the code fall through the normal path and not touching too much XD

eddyb (Mar 06 2020 at 10:52, on Zulip):

Variants::None isn't good either, since you want to be able to precompute variants

oli (Mar 06 2020 at 10:53, on Zulip):

well, for enum Foo {} Variants::Single is kinda lying

eddyb (Mar 06 2020 at 10:53, on Zulip):

this is why I prefer Variants::Multiple with () or ! discriminants

oli (Mar 06 2020 at 10:54, on Zulip):

do we even need Variants::Single at that point?

eddyb (Mar 06 2020 at 10:54, on Zulip):

so that you can always access the discriminant, but because it'd be a ZST we can just pretend it's at offset 0 etc.

eddyb (Mar 06 2020 at 10:55, on Zulip):

@oli funny you should ask that. I think we can then remove the variant index from Variants::Single which would let us replace it with Option

eddyb (Mar 06 2020 at 10:55, on Zulip):

I guess we don't need Option when we can use variants: Vec<Layout>... oh

oli (Mar 06 2020 at 10:56, on Zulip):

refactorings wooo

eddyb (Mar 06 2020 at 10:56, on Zulip):

@oli ah, no, we do need to keep the VariantIdx in Variants::Single because the individual variants still use it

eddyb (Mar 06 2020 at 10:56, on Zulip):

so less refactoring possible :P

oli (Mar 06 2020 at 10:58, on Zulip):

eddyb said:

oli btw have you tried testing with two uninhabited variants? that should also be broken on master and fixed by the PR. or is that unreachable from const-prop?

multiple variants work after my PR, but ICE before it: https://play.rust-lang.org/?version=beta&mode=debug&edition=2018&gist=3586057c35fc8f9f1fc86b6315c427c0

eddyb (Mar 06 2020 at 10:59, on Zulip):

feel free to add another test and r=me it again

eddyb (Mar 06 2020 at 11:00, on Zulip):

looks like it does indeed only affect the first variant: https://play.rust-lang.org/?version=beta&mode=debug&edition=2018&gist=5976ea4f169aa9f05aa8188d29150a90

pnkfelix (Mar 06 2020 at 11:19, on Zulip):

@eddyb landing my PR as well might still be good in terms of beta backport?

pnkfelix (Mar 06 2020 at 11:19, on Zulip):

I

pnkfelix (Mar 06 2020 at 11:20, on Zulip):

I won’t be at design meeting today but we should resolve this Question there , for expediency

eddyb (Mar 06 2020 at 11:20, on Zulip):

well, you have two PRs, feel free to pick either. I wonder if your PR handles the two-variant testcase @oli just added to his PR

eddyb (Mar 06 2020 at 11:21, on Zulip):

if it does I don't care too much which gets merged first

pnkfelix (Mar 06 2020 at 11:22, on Zulip):

I should have thought to add a test for that. Let’s see....

eddyb (Mar 06 2020 at 11:24, on Zulip):

(the bug affects all uninhabited enums, but only their first variant, an univariant uninhabited enum just happens to only have that variant so it's easier to trigger it)

pnkfelix (Mar 06 2020 at 11:27, on Zulip):

by the way, you chose funny names for your multivariant enum cases

pnkfelix (Mar 06 2020 at 11:27, on Zulip):

"Univariant2" :laughing:

oli (Mar 06 2020 at 11:28, on Zulip):

heh

oli (Mar 06 2020 at 11:28, on Zulip):

I'm lazy

pnkfelix (Mar 06 2020 at 11:30, on Zulip):

mine seems to handle the multivariant case as well, from local testing. I'll copy your test into my PR for completeness, and then try to make sure that all of this is on the agenda for today's T-compiler design meeting.

pnkfelix (Mar 06 2020 at 11:31, on Zulip):

(that is, we should decide quickly which one of these two PR's to beta-backport. I think mine is "obviously" less risky for backport. But I don't have a huge preference and thus don't mind being absent from the discussion.)

oli (Mar 06 2020 at 11:32, on Zulip):

I agree that for insta stabilization felix's PR is better

eddyb (Mar 06 2020 at 11:35, on Zulip):

the only scary thing is that there could be other code that might get broken by the for_variant bug, but most of it should be older than what actually caused the regression

pnkfelix (Mar 06 2020 at 11:36, on Zulip):

oli said:

I'm lazy

and I only now just noticed "Warriont" :rofl:

pnkfelix (Mar 06 2020 at 11:38, on Zulip):

eddyb said:

pnkfelix I'm really sorry for all the time wasted, but no matter how busy I am, layout bugs are a priority

none of this is your fault. I'm the one who dropped the ball on this (by self-assigning two weeks ago and then not looking at it, despite it being a beta regression).

oli (Mar 06 2020 at 11:38, on Zulip):

"Warriont"

I'm not lazy where it's important

eddyb (Mar 06 2020 at 11:42, on Zulip):

pnkfelix said:

eddyb said:

pnkfelix I'm really sorry for all the time wasted, but no matter how busy I am, layout bugs are a priority

none of this is your fault. I'm the one who dropped the ball on this (by self-assigning two weeks ago and then not looking at it, despite it being a beta regression).

oh wow, I thought this was more recent, I guess I didn't even get to look at the issue

eddyb (Mar 06 2020 at 11:42, on Zulip):

like I thought it was discovered in the past couple days and that's why I hadn't seen it (yet) in my notifications

oli (Mar 06 2020 at 11:46, on Zulip):

no, it's been open for weeks, but I haven't actually done any rustc work in 3 weeks and kept putting it off

eddyb (Mar 06 2020 at 12:08, on Zulip):

weren't you on vacation :P? I've been checking GH notifications for weeks

oli (Mar 06 2020 at 12:09, on Zulip):

well that, too

pnkfelix (Mar 06 2020 at 12:38, on Zulip):

I'm going to change the topic back to the issue's one starting from where I said "the only conflict ...", since this went far afield from the design meeting discussion.

oli (Mar 06 2020 at 12:39, on Zulip):

oops

pnkfelix (Mar 06 2020 at 12:39, on Zulip):

see, problem solved. Zulip does some things right. :)

Last update: May 29 2020 at 16:35UTC