JuliaLang / julia

The Julia Programming Language
https://julialang.org/
MIT License
45.69k stars 5.48k forks source link

"Tuple field type cannot be Union{}" error in all 1.10 and 1.11 versions #52385

Open aplavin opened 11 months ago

aplavin commented 11 months ago

I've been running some package tests on the upcoming julia version (1.10rc) and noticed this error newly introduced there, compared to 1.9:

julia> Tuple{Union{}}
# 1.9:
Tuple{Union{}}
# 1.10:
ERROR: Tuple field type cannot be Union{}

Not sure if introduced deliberately or as a side-effect of some other change, but it breaks perfectly working code. And I don't see it anywhere in Tuple docs that it shouldn't support all element types.

The above is an MWE, and below is the simplified situation where I actually encountered it:

julia> X = StructArray(a=Union{}[], b=[]);
# 1.10 throws error right here
# 1.9 lets you proceed just fine

# can work with the non-Union{} column:
julia> X.b
Any[]

# can retrieve the Union{} column as well, of course cannot access its elements
julia> X.a |> typeof
Vector{Union{}} (alias for Array{Union{}, 1})
N5N3 commented 11 months ago

This is a deliberate change, see #49111.

vtjnash commented 11 months ago

You didn't include a stacktrace, but it looks like this calls collect_structarray, which calls Core.Compiler.return_type, which is a private API in a private package with no documentation or stability guarantees. The closest actual API is Base.promote_op() which should work for this and is documented as such (though using it does not quite accurately meet the AbstractArray or collect APIs--c.f. the implementation of those in Base and how it tries to avoid them).

https://github.com/JuliaArrays/StructArrays.jl/blob/99f05561cab19fb23f478a12a8429764871ccff3/src/collect.jl#L44

help?> Core.Compiler.return_type
  │ Warning
  │
  │  The following bindings may be internal; they may change or be removed in future versions:
  │
  │    •  Core.Compiler
  │
  │    •  Core.Compiler.return_type

  No documentation found for private symbol.
aplavin commented 11 months ago

The MWE doesn't depend on return_type at all. And I don't think the structarray example does either.

See:

julia> arrs = (Union{}[], Int[])

# works on 1.9 – broken on 1.10rc
julia> Tuple{map(eltype, arrs)...}
ERROR: Tuple field type cannot be Union{}
vtjnash commented 11 months ago

Not all apply_type expressions are meaningful. They can return errors

aplavin commented 11 months ago

Creating a Tuple type with arbitrary types inside seems to make total sense, that's one of the most fundamental Julia types. I haven't seen anywhere in the docs that Tuple only supports a subset of Julia types. And this limitation does sound strange, doesn't it?

aplavin commented 10 months ago

So, should this error be removed? Both:

rdboyes commented 10 months ago

Running into this error using density() from AlgebraOfGraphics as well - a simple MWE from here: https://github.com/MakieOrg/AlgebraOfGraphics.jl/issues/472#issuecomment-1859060029

julia> draw(
           data((x=randn(100), y=randn(100))) *
           mapping(:x, :y) *
           AlgebraOfGraphics.density() *
           visual(Contour)
       )
ERROR: Tuple field type cannot be Union{}
Stacktrace:
  [1] map(f::Function, d::Dictionaries.Indices{Union{}})
    @ Dictionaries ~/.julia/packages/Dictionaries/7aBxp/src/map.jl:91
  [2] unnest(vs::Vector{@NamedTuple{}}, indices::Dictionaries.Indices{Union{}})
    @ AlgebraOfGraphics ~/.julia/packages/AlgebraOfGraphics/tbMEb/src/algebra/layer.jl:81
  [3] unnest_dictionaries(vs::Vector{@NamedTuple{}})
    @ AlgebraOfGraphics ~/.julia/packages/AlgebraOfGraphics/tbMEb/src/algebra/layer.jl:84
  [4] map(f::AlgebraOfGraphics.var"#193#194"{@NamedTuple{…}}, processedlayer::ProcessedLayer)
    @ AlgebraOfGraphics ~/.julia/packages/AlgebraOfGraphics/tbMEb/src/algebra/layer.jl:101
  [5] (::AlgebraOfGraphics.DensityAnalysis{…})(input::ProcessedLayer)
    @ AlgebraOfGraphics ~/.julia/packages/AlgebraOfGraphics/tbMEb/src/transformations/density.jl:30
  [6] call_composed
    @ Base ./operators.jl:1045 [inlined]
  [7] call_composed
    @ Base ./operators.jl:1044 [inlined]
  [8] (::ComposedFunction{AlgebraOfGraphics.Visual, AlgebraOfGraphics.DensityAnalysis{…}})(x::ProcessedLayer)
    @ Base ./operators.jl:1041
  [9] process(layer::Layer)
    @ AlgebraOfGraphics ~/.julia/packages/AlgebraOfGraphics/tbMEb/src/algebra/processing.jl:102
 [10] iterate(g::Base.Generator, s::Vararg{Any})
    @ Base ./generator.jl:47 [inlined]
 [11] collect(itr::Base.Generator{Layers, typeof(AlgebraOfGraphics.process)})
    @ Base ./array.jl:834
 [12] map
    @ ./abstractarray.jl:3310 [inlined]
 [13] ProcessedLayers(a::Layer)
    @ AlgebraOfGraphics ~/.julia/packages/AlgebraOfGraphics/tbMEb/src/algebra/layers.jl:41
 [14] compute_axes_grid(d::Layer; axis::@NamedTuple{}, palettes::@NamedTuple{})
    @ AlgebraOfGraphics ~/.julia/packages/AlgebraOfGraphics/tbMEb/src/algebra/layers.jl:114
 [15] compute_axes_grid
    @ ~/.julia/packages/AlgebraOfGraphics/tbMEb/src/algebra/layers.jl:110 [inlined]
 [16] compute_axes_grid(fig::Figure, d::Layer; axis::@NamedTuple{}, palettes::@NamedTuple{})
    @ AlgebraOfGraphics ~/.julia/packages/AlgebraOfGraphics/tbMEb/src/algebra/layers.jl:100
 [17] compute_axes_grid
    @ ~/.julia/packages/AlgebraOfGraphics/tbMEb/src/algebra/layers.jl:97 [inlined]
 [18] #241
    @ ~/.julia/packages/AlgebraOfGraphics/tbMEb/src/draw.jl:21 [inlined]
 [19] update
    @ ~/.julia/packages/AlgebraOfGraphics/tbMEb/src/draw.jl:10 [inlined]
 [20] plot!(fig::Figure, d::Layer; axis::@NamedTuple{}, palettes::@NamedTuple{})
    @ AlgebraOfGraphics ~/.julia/packages/AlgebraOfGraphics/tbMEb/src/draw.jl:21
 [21] plot!
    @ ~/.julia/packages/AlgebraOfGraphics/tbMEb/src/draw.jl:16 [inlined]
 [22] (::AlgebraOfGraphics.var"#245#246"{…})(f::Figure)
    @ AlgebraOfGraphics ~/.julia/packages/AlgebraOfGraphics/tbMEb/src/draw.jl:48
 [23] update
    @ AlgebraOfGraphics ~/.julia/packages/AlgebraOfGraphics/tbMEb/src/draw.jl:10 [inlined]
 [24] #draw#244
    @ AlgebraOfGraphics ~/.julia/packages/AlgebraOfGraphics/tbMEb/src/draw.jl:47 [inlined]
 [25] draw(d::Layer)
    @ AlgebraOfGraphics ~/.julia/packages/AlgebraOfGraphics/tbMEb/src/draw.jl:44
 [26] top-level scope
    @ REPL[7]:1
Some type information was truncated. Use `show(err)` to see complete types.
vtjnash commented 10 months ago

As with the previous person, this is due to the Dictionaries package using the disallowed private symbol Core.Compiler.return_type. Open an issue there and link https://github.com/JuliaLang/julia/issues/52385#issuecomment-1839431579?

aplavin commented 10 months ago

This issue (the one discussed here, don't know about AoG) is independent on return_type() though, but due to a breaking change in Julia 1.10. Even more, it's breaking completely sensible behavior of being able to represent Tuple{T} for T = Union{}.

There's nothing fundamentally bad with breaking changes, they should just be clearly communicated. For now, Julia promises no breaking changes in 1.x, which this clearly contradicts.

nsajko commented 10 months ago

The empty union, Union{}, is the type with no instances, thus Tuple{Union{}}, and any other tuple type with an empty union field, is also the type with no instances, so I guess we should have Tuple{Union{}} == Union{} hold. I don't see how it makes sense for Tuple{Union{}} to error.

I admit this might be difficult to fix, though.

vtjnash commented 10 months ago

Having no instances is not the same as having no subtypes. Those are 2 orthogonal properties.

We could try to return Union{} here, as that was the first attempt before making it an error, but much code behaved poorly with that, so it would be a breaking change. Making it an error--for this case which was already buggy--was not a breaking change, since packages had always been told not to call the Core.Compiler functions.

nsajko commented 10 months ago

Both Tuple and Union are part of the public API. So it seems like Tuple{Union{}} is public API, regardless of any Core.Compiler internals, no?

aplavin commented 10 months ago

was not a breaking change, since packages had always been told not to call the Core.Compiler functions.

Not sure why Core.Compiler.return_type is being brought up here over and over, it's completely unrelated to the issue.

aplavin commented 10 months ago

https://github.com/JuliaLang/julia/issues/51950 can be a "duplicate" from the internals PoV (function signatures are implemented as tuples), but for a user these are two different scenarios. f(::Union{}) = ... mentioned in https://github.com/JuliaLang/julia/issues/51950 is a method that cannot be called at all (right?), while Tuple{Union{}} can sometimes arise in generic code without any bugs or internals usage.

palday commented 10 months ago

Making it an error--for this case which was already buggy--was not a breaking change, since packages had always been told not to call the Core.Compiler functions.

@vtjnash I've opened a PR against Dictionaries.jl that only uses public API and this problem still arises, so this is definitely not an issue arising relying on compiler internals.

andyferris commented 10 months ago

Union{} is always an interesting case, and given that Tuple parameters are covariant I can see that the behavior of Tuple{Union{}} might be different than Vector{Union{}} or wherever the parameter is invariant. E.g. it might be OK to instantiate an empty Vector{Union{}} but not a Tuple{Union{}} (which would be similar to trying to instantiate a Union{}, which is plainly impossible).

Still - in terms of the implementation, having Tuple{Union{}} become Union{} seems better for users than a runtime error. Especially for generic code.

As for Core.Compiler.return_type, I'm honestly happy to use whatever works. Note that sometimes whatever logic is used in Base in methods such as map(f, ::Vector) needs to be replicated in packages for other data structures. In Julia 1.10 this ultimately uses Base.@default_eltype to infer the eltype (which itself uses Core.Compiler.return_type).

LilithHafner commented 10 months ago

This seems like legitimate Julia code that does not depend on internals, though I don't think I've ever had a use-case for Union{}[] so I don't know how reasonable this usage is.

julia> eagerzip() = error()
eagerzip (generic function with 1 method)

julia> function eagerzip(args::AbstractArray...)
           allequal(axes.(args)) || throw(DimensionMismatch())
           Base.require_one_based_indexing(args...)
           res = similar(first(args), Tuple{eltype.(args)...})
           for i in eachindex(args...)
               res[i] = getindex.(args, i)
           end
           res
       end
eagerzip (generic function with 2 methods)

julia> eagerzip([1,2,3], [5,6,7])
3-element Vector{Tuple{Int64, Int64}}:
 (1, 5)
 (2, 6)
 (3, 7)

julia> eagerzip(Int[], [])
Tuple{Int64, Any}[]

julia> eagerzip(Union{}[], [])
Tuple{Union{}, Any}[] # 1.9
ERROR: Tuple field type cannot be Union{} # 1.10
Stacktrace:
 [1] eagerzip(::Vector{Union{}}, ::Vararg{AbstractArray})
   @ Main ./REPL[201]:4
 [2] top-level scope
   @ REPL[204]:1
KristofferC commented 10 months ago

Looks effectively the same as the example in https://github.com/JuliaLang/julia/issues/52385#issuecomment-1839518854 (except much longer)?

LilithHafner commented 10 months ago

Yep! It's the same, just with a bit more motivation. The first example in the OP was, by itself, a valid regression report IMO.

vtjnash commented 10 months ago

Do you have an example of that? Arguably Union{}[] would have been the correct return there (which is what we fixed out to return in other code where it came up)

LilithHafner commented 10 months ago

No, I don't have an example of this breaking anything "in the wild" that does not depend on internals. If nobody else has such an example either, then we can call it a minor change and be done with it, but it is technically breaking.

jariji commented 10 months ago

Irrespective of semantic versioning, I don't see how it follows from any of the documented type rules that this should fail, so the error would be a special case, which isn't great from a user perspective.

vtjnash commented 10 months ago

it follows from any of the documented type rules that this should fail

It follows from the rule that the intersection of Tuple{T} and Tuple{S} is empty if the intersection of T and S is empty. That is a rule that subtyping has always had, so that is why this wasn't a major breaking change, as it only sets out to align the rest of the system with the existing rule. As for being a special case, this implementation is equivalent to adding a lower bound on the typevar for Tuple, which is not a particularly special case. Although it does also happen to implement the oft-requested feature of being able to prohibit a type var from being exactly Union{} although without yet making that feature very generally accessible.

but it is technically breaking

OT, but every change, including bugfixes, are technically breaking. But semantic versioning is mostly about not changing the result beyond the limits of what was promised. I actually wonder if there is an argument to be made that most changes from semi-working -> error or error -> working is allowable by semantic versioning therefore, since in neither case does a working program get a different return value.

aplavin commented 10 months ago

No, I don't have an example of this breaking anything "in the wild" that does not depend on internals. If nobody else has such an example either, then we can call it a minor change and be done with it

Aren't there enough examples in this thread already? I'm not sure why "internals" are even brought up here, as Core.Compiler is completely unrelated to this issue.

Julia docs say that

As per SemVer, code written for v1.0 will continue to work for all future LTS and Stable versions.

I've always interpreted that (and want to continue doing so...) that any code (not relying on internals/experimental) will continue to work. Not "only code that julia devs explicitly approve".

Moreover, as @jariji also points out, this is not just a breaking change in the abstract – it breaks totally sensible behavior, not a weird historical quirk.

Vector or AbstractVector with eltype == Union{} is a perfectly fine thing in Julia. It's also a natural thing to put such a vector into a StructArray. And this doesn't work anymore in 1.10.

Nothing in the Tuple/NamedTuple documentation suggests that they only support a subset of Julia types. Like, I can create another type with any parameter that I want:

julia> struct S{T}
       t::T
       end

julia> S{Union{}}
S{Union{}}

but not Tuple or NamedTuple for some reason.

LilithHafner commented 10 months ago

Triage thinks that this is a breaking change.

It might be acceptable to keep it, but we would need a strong example of the havoc that allowing Tuple{Union{}} would cause. The breakage here seems more bad than the original issue https://github.com/JuliaLang/julia/issues/32392.

Triage didn't see why https://github.com/JuliaLang/julia/issues/32392 is such a big deal, perhaps it would be worth expanding on what led to that report.

Possible course of action: release 1.10.1 with Tuple{Union{}} constructible

JeffBezanson commented 10 months ago

I know there are other problems with the type system that removing Tuple{Union{}} helps avoid, but it seems like a very "early" point to throw an error. Programs might very well try things like Tuple{eltype(a)} and it's not clear how to change that code to avoid the error. Can we push the problem farther downstream, closer to an "actual problem" and not just the existence of the type object?

nsajko commented 9 months ago

Issues around Tuple{Union{}} were actually discussed all the way back in 2018, in the Julia subtyping paper (Julia Subtyping: A Rational Reconstruction. Proceedings of the ACM on Programming Languages, 2018, 27, ⟨10.1145/3276483⟩. ⟨hal-01882137⟩).

We propose an alternative design. The type Tuple{Union{}} (or, more generally, any tuple type containing Union{} as one of its components) is not inhabited by any value, and dispatch-wise it behaves as Union{}. However, neither Julia 0.6.2 nor our formalization can prove it equivalent to Union{} because the judgment Tuple{Union{}}<:Union{} is not derivable: following Julia 0.6.2 semantics, the lift_union function does not lift empty unions out of tuples. Extending lift_union to lift empty unions, thus rewriting types such as Tuple{Union{}} into Union{}, is straightforward; the resulting subtype relation is not affected by the transitivity problem described above. We have modified our reference implementation along these lines. Testing over the real-world workloads does not highlight differences with the standard subtype relation, suggesting that this change does not impact the programming practice. However, this alternative design has implementation drawbacks. Assuming that a Tuple{t} type in a program where t is unknown yields a 1-element tuple type becomes incorrect, making dataflow analyses more complex. Also, uniqueness of the bottom type is lost, and testing if a type is bottom (a test that occurs surprisingly often in Julia code-base) becomes slower. These tradeoffs are being investigated by Julia developers.

and in another place in the paper:

Unprovable judgments. Julia’s subtype algorithm, and in turn our formalization, cannot prove all judgments expected to hold. For instance it cannot prove: (Tuple{T} where String <:T <:Int) <: Union{ } or Tuple{Union{ }} <: Union{ } despite all these types having no elements (the type on the left-hand side being a valid Julia type).

The latter quote also ties into #24179, which was closed for some reason.

tpapp commented 9 months ago

@JeffBezanson: FWIW, I would always prefer

Tuple{eltype(a)}

to error if eltype(a) === Union{}, because that is a seriously broken eltype and I would prefer to know as early as possible.

To take this further, I would also like all types to error if a type parameter is Union{}, eg Vector{Union{}} etc.

I get why having Union{} at the bottom is nice, for the compiler and just to have a type that is a subtype of everything, but it leaking all over the type system is not worth it IMO.

Incidentally, we should also make

SomeParametricType{Union{}} <: (SomeParametricType{T} where T<:SomeConcreteType)

false to be consistent, if the left hand side errors.

nsajko commented 9 months ago

if eltype(a) === Union{}

That can never hold, Union{} can't have instances, i.e. a isa Union{} holds for no a. EDIT: sorry, I thought you wrote typeof when you wrote eltype.

tpapp commented 9 months ago

I agree that it should not hold, but cf

julia> v = Vector{Union{}}(undef, 1)
1-element Vector{Union{}}:
 #undef

julia> eltype(v)
Union{}

I think that there are two consistent solutions:

  1. never allow Union{} as a type parameter, so Tuple{Union{}} and friends error, and thus don't include it in Foo{T} where T etc,

  2. allow Union{} as a type parameter, and then allow it everywhere, incl Tuple, and then also Foo{Union{}} is a subtype of the above where clause.

nsajko commented 9 months ago

An example: Vector{Union{}}, or AbstractArray{Union{}} in general, expresses "an array type for empty arrays". There's no sense in disallowing that construct. It even feels like it may be practically useful once in a while.

palday commented 9 months ago

An example: Vector{Union{}}, or AbstractArray{Union{}} in general, expresses "an array type for empty arrays". There's no sense in disallowing that construct. It even feels like it may be practically useful once in a decade.

And this is exactly where the trouble of disallowing Union{} arose in practice for Dictionaries.jl.

andyferris commented 9 months ago

I always thought Vector{Union{}} was a good starting point with the "eltype widening" pattern employed by BangBang.jl and to a lesser exent generators, map, etc in Base. There seems to be a long-lived desire to stop using return_type in places like that... I'm not sure if this plays a role somehow, and how it all fits together (maintaining the current quality of inference and generated code).

@tpapp I agree the extremes here are the consistent solutions, but in (1) I think the issue is about fields (or elements) and layout, not about type parameters per se (as type parameters can be used as constant values, and types in Julia, include the type Union{}, are just values that are valid for use as type parameters).

Outright banning Vector{Union{}} (or rather Memory{Union{}}) seems like a drastic move. One might question whether you should be able to construct one with more than zero elements, but given we already allow #undef elements for non-bitstype types that throw error on access (not on construction of the container) we should probably maintain consistency with that. Making an immutable struct type with a Union{} field in it uninhabitable (as it really shouldn't have a valid layout anyway) might be a fine idea, but mutable structs seem so similar in nature to the array case that they should probably behave the exact same way as arrays.

tpapp commented 9 months ago

@andyferris: you make a convincing case, I forgot about the widening pattern you mention.

What about just documenting the behavior in 1.10, ie not allowing Union{} as field type in Tuples, perhaps with a short explanation about the rationale, and leaving everything else as is?

Maybe optionally mentioning that Julia may disallow Union{} field types in immutables in the future and not consider it a breaking change.

andyferris commented 9 months ago

My suggestion for immutable structures would be to allow the actual type, but disallow its construction (even via tricks like unsafe_load, reinterpret, and similar). Basically there would be no valid layout for the compiler or runtime to work with.

For mutable "locations" maybe we allow it with the layout of Union{} taking zero bytes, yet attempting to get (or set) the value results in a runtime error. AFAICT at the moment sometimes Union{} takes zero bytes to represent (in a struct?) and sometimes is pointer-width (in an array?).

I would also vote for allowing Tuple{Union{}} as a type in Julia 1.10.1.

aplavin commented 9 months ago

What about just documenting the behavior in 1.10, ie not allowing Union{} as field type in Tuples, perhaps with a short explanation about the rationale, and leaving everything else as is?

That's exactly the change in 1.10 that breaks perfectly natural code, as evidenced by numerous examples in this thread alone.

My original case basically boils down to:

The Dictionaries situation is a bit different, but also doesn't involve anything crazy. Didn't look into other packages' issues linked above.

Disallowing these in 1.10 is not just an unambiguous breaking change – but it would make sense to ask for Tuple{Union{}} support even if it didn't work in earlier Julia versions. Simply because the behavior is so natural and fits the general picture.

vtjnash commented 9 months ago

There seems to be a long-lived desire to stop using return_type in places like that... I'm not sure if this plays a role somehow

@andyferris In this instance, it is only a push towards the (more) public API Base.promote_op instead of the definitely private Core.Compiler name. While occasionally discouraged for use, that function is documented and specified in the manual for your use

andyferris commented 9 months ago

@vtjnash thank you, that is quite helpful.

andyferris commented 9 months ago

I thought I'd just add here that I discovered our private code at @elaraai suffers from this issue, while I was upgrading from v1.9 to v1.10 today. (I managed a similar workaround to what exists inside of Base.promote_op).

aplavin commented 8 months ago

Just checked – this bug/regression reproduces on 1.10.1 as well.

LilithHafner commented 8 months ago

The examples above (esp. StructArrays.jl and Dictionaries.jl) are near accesses to internals, but do not depend on them.

We can change internals all we want and packages that depend on those internals will have to deal with the consequences, but that's not what's going on in this issue. The internals these packages depend on have not changed. The breakage happens in a usage of public API near, and just before calling internals. A bug report from a package using internals about breakage in those internals is invalid, but a bug report from such a package about a breakage in public api is still valid.

From the FAQ

We do not discourage the attempt to create packages that expose stable public interfaces while relying on non-public implementation details of Julia and buffering the differences across different Julia versions.

We should not penalize these packages for depending on internals by ignoring their bug reports.

From https://julialang.org/blog/2019/08/release-process/

Minor releases may include bug fixes, new features and "minor changes"—which is the term we're using for technically breaking changes that are sufficiently unlikely to break anyone's code and which, in fact, do not break things in the package ecosystem as determined by running PkgEval to verify that there isn't any breakage.

I stand by that statement today. A bug is a deviation of behavior from documentation, and I don't see any documentation that indicates Tuple{Union{}} cannot exist, so this change is not a bugfix. It's also not a minor change becuase it is technically breaking (see MWE in OP) and in fact does break things in the package ecosystem (StructArrays & Dictionaries, at least). So I don't think it belongs in a minor release. Hence putting back the regression label, and also the bug label because we document that we won't have semantic regressions, so a semantic regression is a bug.

More fundamentally, though, I agree with @JeffBezanson's comment that Tuple{Union{}} should be able to exist. I would oppose including this change in 2.0, even with no compatibility concerns at all.

aplavin commented 8 months ago

Maybe, my MWEs in the first post were "too minimal" :) They demonstrate the issue, but it can be unclear how such behavior arises in code that doesn't use any internals, and is not specially crafted to expose this issue.

Here's a more extended version:

using BangBang

# use a recommended mutate-or-widen approach:
xs = Union{}[]
while ...
    ...
    xs = push!!(xs, newx)
end

using StructArrays

# create a structarray with the x values and their indices
StructArray(i=eachindex(xs), x=xs)

On 1.9-, this code always works fine. On 1.10, it works as long as xs is not empty. And fails on the last line when there are no elements in xs.

This is a perfectly natural piece of code that is broken by this change in 1.10. What's even worse, is that the breakage may be missed by tests, if they don't hit the empty case. And I don't know if a reasonable workaround is possible here aside from enabling Tuple with Union{} as they were before.

LilithHafner commented 8 months ago

Investigating the source code on every frame of the stacktrace that procures in 1.10.0 and 1.10.1 I find no usage of internals.

nsajko commented 8 months ago

The "usage of internals" issue is a complete red herring here.

  1. There is no usage of internals in the expression Tuple{Union{}}
  2. Even so, @palday's PR, linked above, shows that it's possible to reimplement the functionality provided by Core.Compiler.return_type in user code using code_typed without depending on internals.
StefanKarpinski commented 8 months ago

I agree that this seems to be breaking and should be reverted. On the face of it just the fact that Tuple and Union are public and that Tuple{Union{}} used to work and now doesn't is already breaking. An argument could be made that either there's no reasonable use case for doing Tuple{Union{}} or this is a bug fix and it shouldn't ever have worked. In isolation, calling Tuple{Union{}} isn't obviously useful, so let's look at the original MWE here. Contrary to what @vtjnash keeps asserting, this example doesn't depend on return_types or any other internals in any way. The error occurs here in the expression Tuple{map(eltype, t)...} and the situation is this:

  1. You have t which is a tuple of vectors, in the example t = (Union{}[], Any[]).
    • This is clearly valid: we allow Union{} to be the element type of an array.
  2. You call map(eltype, t) to get a tuple of the element types of your arrays. Nothing wrong with any of this:
    • eltype is well-defined, non-sketchy and defined by Base
    • map is well-defined, non-sketchy and defined by Base
    • The result is (Union{}, Any)
  3. Tuple{map(eltype, t)...} is called get a tuple type from the tuple of types
    • The result used to be Tuple{Union{}, Any} which was valid
    • This now errors, instead, breaking the code

Which of these steps is invalid or sketchy? The first two are clearly ok and still work. @vtjnash are you arguing that it's not ok to call Tuple{t...} on a tuple of types to get a tuple type? Or are you arguing that everyone who does that should always have been guarding against the possibility that one of the types in t was Union{}? That seems like it would make metaprogramming with types in Julia a colossal nightmare and basically unworkable.

vtjnash commented 8 months ago

The result used to be Tuple{Union{}, Any} which was valid

It is unclear exactly what the expectations on "valid" means in this sentence. I had tried doing pretty much exactly this MWE in the past (as a part of an alternative implementation for convert), except that I quickly found that inference will unfortunately conclude that this result is not valid, and so the resulting behavior misbehaved at runtime. Having possible segfaults in distant parts of the code seemed like quite an unpleasant foot-gun, so I was motivated to add this explicit error instead where the problem is introduced. Especially since this is such an unusual edge case, as when someone writes Tuple{map(eltype, t)...}, I really don't think they expect that inference will behave as if the resulting type cannot exist. However, it does misbehave, since type-intersection cannot cope with the existence of this type (it considers it a logical contradiction, as that permits it to ignore occurrences of tuple signatures that could not be constructed/called anyways). This experience with debugging the resulting internal crashes was captured as the MWE in https://github.com/JuliaLang/julia/issues/32392, and prompted now making the error visible immediately rather than letting it turn into inference mistakes, mis-compilation, and segfaults later. As an additional benefit, it also means we could give more optimal inference results in some cases, reducing compile times and improving load times. It seemed a lot to gain (better performance and less segfaults) without much to lose (a type that seemingly should be valid, but which nevertheless can easily crash the runtime because inference does not consider it valid).

Additional design commentary: it is also notable that this error case can only happen when you specifically attempt to make a Tuple containing the counterfactual of a type that cannot have any subtypes or instances. If you have at least one object in the container, then this case is not reachable. So as long as your metaprogramming operates on the types of actual values, rather than on the eltypes, this won't be an issue for your code. This is a fairly common design pattern recommendation in Julia's style guide: that operations should use the typeof actual values rather than the eltype approximation of it. For example, map should return the typejoin of the elements, not the inferred type of the container applied to the eltype. For a another–recently fixed–example, zero now works even when the eltype is not a Number, which I think is a pretty cool application of this principle:

julia> zero(Any[1, 2])
2-element Vector{Int64}:
 0
 0
oscardssmith commented 8 months ago

Would a solution to this be making Tuple{Union} just normalize to Union{}?

mbauman commented 8 months ago

Would a solution to this be making Tuple{Union} just normalize to Union{}?

I do like that. It still feels a little strange to not get a Tuple out of Tuple{something...}, but technically Union{} <: Tuple so I think it'd resolve the StructArrays usage without trouble.

bvdmitri commented 8 months ago

Tuple{Union{}, Any} which was valid

Was it?

julia> sizeof(Union{}) # this is on Julia 1.9
ERROR: The empty type does not have a definite size since it does not have instances.

julia> sizeof(Tuple{Union{}})
8

julia> sizeof(Tuple{Union{}, Union{}})
16

julia> sizeof(Tuple{Union{}, Union{}, Union{}})
24

julia> sizeof(Tuple{Union{}, Int})
16
andyferris commented 8 months ago

Would a solution to this be making Tuple{Union} just normalize to Union{}?

I suppose that it is a solution. Code such as in StructArrays or TypedTables might use type parameters like T <: Tuple and I suppose that Union{} <: Tuple is true. (EDIT: as Matt says above)

I still wonder if it makes sense for tuples and (immutable) structs to behave differently in this case? Can immutable structs have #undef in fields? You can do that with new, right? (We seem to allow #undef in mutable slots, which seems to be where Union{} types can be "instantiated").