Dumb question: what does this mean for the data layout of structs with union fields? Would it be possible/make sense to always recursively strip the selector bytes out of such structs and store them separately? That is, make the non-selector parts of arrays of unions and arrays of structs of unions ABI compatible with the C version of the same thing?

@c42f, so I'm actually not familiar with the C memory layout of structs with union fields, is it just the max size of the largest union type? I guess it doesn't track which element was put in the field, but relies on the user pulling out whatever they want?

is it just the max size of the largest union type?

I think it's ultimately defined by the platform ABI just as the struct layout is. But yes, I'd guess the union bits would just have the largest size and strictest alignment of types within the union.

I guess it doesn't track which element was put in the field, but relies on the user pulling out whatever they want?

Correct; in C you need to store the type tag info explicitly, or infer it somehow from other state in the system.

Bump. Probably best if @JeffBezanson, @vtjnash, or @Keno review this. No urgency really, but when you have a free moment.

Bump again.

Let's look at JuliaCon

Bump :)

This seems pretty good so far. It's good to see how small it is as I wasn't sure how much of the rest of the system knew to check these flags, and not assume that isbits told all.

The one aspect to finish this up that I think may need to be preserved is to ensure that Vector{Foo}(undef, 1)[1] throws an UndefRefException.

The one aspect to finish this up that I think may need to be preserved is to ensure that Vector{Foo}(undef, 1)[1] throws an UndefRefException.

Hmmm......do we really though? That's kind of a big crux of the change here is that Foo in a Vector{Foo}(undf, 1) now behaves more like if you did:

struct Foo
    x::Int
    y::Union{Missing, Float64}
    Foo() = new()
end
[Foo()] == Vector{Foo}(undef, 1)

So IMO, this almost feels more like a "bugfix" than change in behavior. How would we still do an UndefRefException? Special case these structs in isassigned and getindex?

Bump, this would be nice to have for 1.3

I see this was marked for triage; was anything concluded?

There was a heated discussion, I'm not sure what we concluded. @JeffBezanson, you seemed in favor.

@vtjnash, do you have any follow up here from previous discussion/questions? Anything else we need to change here?

I'll plan on merging this in 12 hours or so unless @vtjnash or anyone else has concerns.

If I define

struct WrappedUnion
    v::Union{Missing,Float64}
end

how does the storage layout now compare to Vector{Union{Missing,Float64}}?

I'd like to suggest that the storage layout for these two Vectors should be the same (cf. #32448 (comment)), and that the selector bits should be hoisted out in the same way in both cases.

That's pretty difficult to do, since it means the layout of the same struct is different depending on where/how that struct is stored. It's also hard to handle things like Vector{Union{StructWithUnions, StructWithoutUnions}}.

I still think this PR was a breaking change in its current state and needed updates to the documentation.

That's pretty difficult to do, since it means the layout of the same struct is different depending on where/how that struct is stored.

I feel like with systematic hoisting of selectors you can get consistent layout when each such struct comes in two separable but consistently laid out parts: the data and the type selector bits. I've tried to write it out in more detail over at #33120.

@vtjnash, it would be helpful if you could be more specific about what exactly you consider breaking and what documentation needs updating. As I've posted a few times here, I'm happy to push forward on things, but the lack of responses/direction has left this in its current state. Personally, I still feel this is more "bugfix" than breaking given the work and docs that went in to general isbits-Union optimizations.

I still think this PR was a breaking change in its current state and needed updates to the documentation.

I still keep running into problems that this causes internally because it was a breaking change that changes assumptions made by some users and inference/codegen. I know you wanted to push ahead faster on this then I could review, and were rejecting my requests for changes to make it non-breaking. But it seems pretty yucky now that we implement isbitsunion to mean a union of things that aren't necessarily isbits (but still document it to mean the old thing).

Personally, I still feel this is more "bugfix" than breaking given the work and docs that went in to general isbits-Union optimizations.

I generally intended the isbits optimizations to be exactly that: optimizations for unions of isbits types. The documentation was intended to reflect that. Regardless of what you feel like calling it, the fact is it changed that memory model without fixing all of the places where we assume the prior memory model, both in the documentation and in the code.