make Tuple{Union{}} unconstructable #49111

vtjnash · 2023-03-22T21:25:34Z

Type intersection assumed it was equal to Union{}, so this makes it unconstructable so that holds true. This is similar to what the NamedTuple constructor does.

Secondarily, this fixes an inference bug where it would create Vararg{Union{}} and then incorrectly handle that fieldtype.

Fixes typeintersect bug with NTuple #32392
Addresses part of the concerns discussed in Invariant Tuples #24614 (comment)
Addresses part of the issues presented in subtyping regression: Vector{T} is not considered concrete #26175
May allow improving jl_type_equality_is_identity (https://github.com/JuliaLang/julia/pull/49017/files#diff-882927c6e612596e22406ae0d06adcee88a9ec05e8b61ad81b48942e2cb266e9R986)
May allow improving intersection (finish_unionall can be more aggressive at computing varval for any typevars that appears in covariant position and has lb=Union{} and ub=leaf type)

There is an upgrade helper for packages that rely on this not throwing an exception of Base.Iterators.TupleOrBottom(p...), though (judging by changes in Base), I don't expect it to be widely needed. We can do a PkgEval run though to verify.

vtjnash · 2023-03-23T02:02:30Z

@nanosoldier runbenchmarks(!"scalar", vs=":master")

vtjnash · 2023-03-23T02:02:45Z

@nanosoldier runtests()

martinholters · 2023-03-23T06:45:09Z

test/tuple.jl

+@test_throws ErrorException("Tuple field type cannot be Union{}") Tuple{Int, Vararg{Union{},1}}
+@test_throws ErrorException("Tuple field type cannot be Union{}") Tuple{Vararg{Union{},1}}
+@test Tuple{} <: Tuple{Vararg{Union{},N}} where N
+@test !(Tuple{} >: Tuple{Vararg{Union{},N}} where N)


Why is that? Is there an N for which the LHS is not >: the RHS? Isn't N==0 the only admissible choice?

yeah, subtyping cannot be allowed to fully know that though, since we don't really want the concrete type Tuple{} to be equal to a non-concrete type

Perhaps we'd better normalize Tuple{A,B,C,Vararg{Union{},N}} where N to Tuple{A,B,C} after this change?

No, that makes a different type, where N is free

What about Tuple{} >: Tuple{Vararg{Union{}}}? There's a check for <:, but should these be ==?

Tuple{Vararg{Union{}}} is invalid (it is just Tuple{} now)

I guess that raises the possibility of making Vararg{Union{}} itself also directly throw an error. I was trying to be conservative in allowing Union{} but constructing a different sort of object. There could be a case to be made that Union{} should not appear their either though (which would allow deleting several more tests as non-applicable also)

nanosoldier · 2023-03-23T07:58:08Z

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here.

maleadt · 2023-03-23T10:25:22Z

PkgEval crashed.

@nanosoldier runtests()

vtjnash · 2023-03-23T13:27:10Z

@nanosoldier runbenchmarks(["string", "join"], vs=":master")

nanosoldier · 2023-03-24T02:08:17Z

Your package evaluation job has completed - possible new issues were detected.
A full report can be found here.

martinholters · 2023-03-24T08:06:02Z

From MCMCChains:

│ ERROR: Tuple field type cannot be Union{}
│ Stacktrace:
│   [1] eltypes(t::Tuple{Vector{Union{}}})
│     @ Base.Broadcast ./broadcast.jl:739

And indeed, on master:

julia> Base.Broadcast.eltypes((Union{}[],))
Tuple{Union{}}

Should that be reworked to return (types...,) instead of Tuple{types...} to deal with the possibility of a Union{} in types?

N5N3 · 2023-03-24T08:33:56Z

Perhaps we'd better rewake #39295 and drop Broadcast.eltypes?
Even we return a (...,) instead of a Tuple{...,}, Broadcast.combine_eltypes still needs to handle possible Union{}.

martinholters · 2023-03-24T09:07:44Z

Ah, right, the result of eltypes is passed to Base._return_type. But then we know the result is Union{} if any of the arguments is Union{} (right?), so eltypes could bail out and return Union{} instead of Tuple{..., Union{}, ...} and then combine_eltypes could skip Base._return_type and return Union{} directlly. Seems like a change sufficiently small to include here.

vtjnash · 2023-03-24T18:30:55Z

Right, I just forgot to fix up a couple calls. Before:

julia> Base.promote_op(tuple, Union{}, Int)
Tuple{Union{}, Int64}

after

julia> Base.promote_op(tuple, Union{}, Int)
Union{}

@nanosoldier runtests(["PersistenceDiagrams", "MLJModels", "Observers", "MLJTestInterface", "DynamicSparseArrays", "CGcoefficient", "GeoClustering", "Tidier", "MLJ", "QEDbase", "Concorde", "DASSL", "GeneratorArrays", "TMLE", "Nullables", "GLFixedEffectModels", "UnivariateFunctions", "NodeCall", "FunctionalTables", "QuantumTomography", "MLJTestIntegration", "EnlilGrids", "Resample", "MCMCChains", "Plasmo", "AlgebraOfGraphics", "PiecewiseDeterministicMarkovProcesses", "VoronoiFVM", "MakieThemes", "ParameterEstimation", "ConcreteStructs", "ConformalPrediction", "OMEinsum", "PDFHighlights", "BinaryTraits", "HomalgProject", "POMDPStressTesting", "StochasticIntegrals", "GenerativeTopographicMapping", "FixedEffectModels", "PreprocessMD", "SimpleANOVA", "Bloqade", "ContinuousWavelets", "Tapestree", "NodalPolynomialSpaces", "RealPolynomialRoots", "Anatta", "FeatureTransforms", "StochasticAD", "WhereTraits", "EvoTrees", "SBMLToolkit", "PANDA", "CooperativeGames", "Ai4EComponentLib", "HierarchicalUtils", "FastAI", "MLJLinearModels", "GraphNeuralNetworks", "UnfoldMakie", "AlgebraicAgents"])

vtjnash · 2023-03-24T18:31:29Z

@nanosoldier runbenchmarks(["string", "join"], vs=":master")

vtjnash · 2023-03-24T18:31:45Z

@nanosoldier runbenchmarks("string" && "join", vs=":master")

nanosoldier · 2023-03-24T18:48:13Z

Your benchmark job has completed - no performance regressions were detected. A full report can be found here.

nanosoldier · 2023-03-27T07:09:31Z

Your package evaluation job has completed - possible new issues were detected.
A full report can be found here.

Type intersection assumed it was equal to Union{}, so this makes it unconstructable so that holds true. This is similar to what the NamedTuple constructor does. Secondarily, this fixes an inference bug where it would create Vararg{Union{}} and then incorrectly handle that fieldtype. - Fixes #32392 - Addresses part of the concerns discussed in #24614 (comment) - Addresses part of the issues presented in #26175 - May allow improving jl_type_equality_is_identity (https://github.com/JuliaLang/julia/pull/49017/files#diff-882927c6e612596e22406ae0d06adcee88a9ec05e8b61ad81b48942e2cb266e9R986) - May allow improving intersection (finish_unionall can be more aggressive at computing varval for any typevars that appears in covariant position and has lb=Union{} and ub=leaf type)

George9000 · 2023-04-10T18:38:13Z

@vtjnash Similar error as above in testset REPL reported on buildkite and on my local build (macOS ARM):

details

Error in testset REPL:
Error During Test at /Users/foo/applications/julia/test/testdefs.jl:21
  Got exception outside of a @test
  LoadError: Tuple field type cannot be Union{}
  Stacktrace:
    [1] complete_methods!(out::Vector{REPL.REPLCompletions.Completion}, funct::Any, args_ex::Vector{Any}, kwargs_ex::Set{Symbol}, max_method_completions::Int64, exact_nargs::Bool)
      @ REPL.REPLCompletions ~/applications/julia/usr/share/julia/stdlib/v1.10/REPL/src/REPLCompletions.jl:708
    [2] complete_methods
      @ ~/applications/julia/usr/share/julia/stdlib/v1.10/REPL/src/REPLCompletions.jl:588 [inlined]
    [3] completions(string::String, pos::Int64, context_module::Module, shift::Bool)
      @ REPL.REPLCompletions ~/applications/julia/usr/share/julia/stdlib/v1.10/REPL/src/REPLCompletions.jl:1053
    [4] completions
      @ ~/applications/julia/usr/share/julia/stdlib/v1.10/REPL/src/REPLCompletions.jl:967 [inlined]
    [5] test_complete(s::String)
      @ Main.Test72Main_REPL.REPLCompletionsTest ~/applications/julia/usr/share/julia/stdlib/v1.10/REPL/test/replcompletions.jl:154
    [6] top-level scope
      @ ~/applications/julia/usr/share/julia/stdlib/v1.10/REPL/test/replcompletions.jl:518
    [7] include(mod::Module, _path::String)
      @ Base ./Base.jl:488
    [8] include(x::String)
      @ Main.Test72Main_REPL.REPLCompletionsTest ~/applications/julia/usr/share/julia/stdlib/v1.10/REPL/test/runtests.jl:9
    [9] top-level scope
      @ ~/applications/julia/usr/share/julia/stdlib/v1.10/REPL/test/runtests.jl:10
   [10] include
      @ ./Base.jl:488 [inlined]
   [11] macro expansion
      @ ~/applications/julia/test/testdefs.jl:29 [inlined]
   [12] macro expansion
      @ ~/applications/julia/usr/share/julia/stdlib/v1.10/Test/src/Test.jl:1504 [inlined]
   [13] macro expansion
      @ ~/applications/julia/test/testdefs.jl:23 [inlined]
   [14] macro expansion
      @ ./timing.jl:502 [inlined]
   [15] runtests(name::String, path::String, isolate::Bool; seed::UInt128)
      @ Main ~/applications/julia/test/testdefs.jl:21
   [16] runtests
      @ ~/applications/julia/test/testdefs.jl:5 [inlined]
   [17] #invokelatest#2
      @ ./essentials.jl:869 [inlined]
   [18] invokelatest
      @ ./essentials.jl:864 [inlined]
   [19] #153
      @ ~/applications/julia/usr/share/julia/stdlib/v1.10/Distributed/src/remotecall.jl:425 [inlined]
   [20] run_work_thunk(thunk::Distributed.var"#153#154"{typeof(runtests), Tuple{String, String}, Base.Pairs{Symbol, UInt128, Tuple{Symbol}, @NamedTuple{seed::UInt128}}}, print_error::Bool)
      @ Distributed ~/applications/julia/usr/share/julia/stdlib/v1.10/Distributed/src/process_messages.jl:70
   [21] remotecall_fetch(::Function, ::Distributed.LocalProcess, ::String, ::Vararg{String}; kwargs::Base.Pairs{Symbol, UInt128, Tuple{Symbol}, @NamedTuple{seed::UInt128}})
      @ Distributed ~/applications/julia/usr/share/julia/stdlib/v1.10/Distributed/src/remotecall.jl:450
   [22] remotecall_fetch(::Function, ::Int64, ::String, ::Vararg{String}; kwargs::Base.Pairs{Symbol, UInt128, Tuple{Symbol}, @NamedTuple{seed::UInt128}})
      @ Distributed ~/applications/julia/usr/share/julia/stdlib/v1.10/Distributed/src/remotecall.jl:492
   [23] macro expansion
      @ ~/applications/julia/test/runtests.jl:256 [inlined]
   [24] (::var"#39#49"{Vector{Task}, var"#print_testworker_errored#45"{ReentrantLock, Int64, Int64}, var"#print_testworker_stats#43"{ReentrantLock, Int64, Int64, Int64, Int64, Int64, Int64}, Vector{Any}, Dict{String, DateTime}})()
      @ Main ./task.jl:514
  in expression starting at /Users/foo/applications/julia/usr/share/julia/stdlib/v1.10/REPL/test/replcompletions.jl:517
  in expression starting at /Users/foo/applications/julia/usr/share/julia/stdlib/v1.10/REPL/test/runtests.jl:9
ERROR: LoadError: Test run finished with errors
in expression starting at /Users/foo/applications/julia/test/runtests.jl:93
gmake[1]: *** [Makefile:25: REPL] Error 1
gmake: *** [Makefile:615: test-REPL] Error 2

…ion{}} otherwise Refs: #49111

Type intersection assumed it was equal to Union{}, so this makes it unconstructable so that holds true. This is similar to what the NamedTuple constructor does. Secondarily, this fixes an inference bug where it would create Vararg{Union{}} and then incorrectly handle that fieldtype. - Fixes JuliaLang#32392 - Addresses part of the concerns discussed in JuliaLang#24614 (comment) - Addresses part of the issues presented in JuliaLang#26175 - May allow improving jl_type_equality_is_identity (https://github.com/JuliaLang/julia/pull/49017/files#diff-882927c6e612596e22406ae0d06adcee88a9ec05e8b61ad81b48942e2cb266e9R986) - May allow improving intersection (finish_unionall can be more aggressive at computing varval for any typevars that appears in covariant position and has lb=Union{} and ub=leaf type)

…ion{}} otherwise (#49393) This was a primary motivation for #49111. Previously, we'd see some some method specializations such as `convert(::Type{T}, ::T) where T<:Float64` (apparently from inference of some tuple convert specializations), which were not necessary to have.

this was changed in JuliaLang/julia#49111 for older julia versions this cast should be a no-op

vtjnash added needs nanosoldier run This PR should have benchmarks run on it needs pkgeval Tests for all registered packages should be run with this change labels Mar 22, 2023

vtjnash requested a review from JeffBezanson March 22, 2023 21:25

vtjnash force-pushed the jn/32392 branch from bc9e9b9 to c8e197b Compare March 22, 2023 22:27

martinholters reviewed Mar 23, 2023

View reviewed changes

vtjnash removed the needs pkgeval Tests for all registered packages should be run with this change label Mar 23, 2023

vtjnash force-pushed the jn/32392 branch from c8e197b to d2a4568 Compare March 24, 2023 18:29

vtjnash removed the needs nanosoldier run This PR should have benchmarks run on it label Mar 28, 2023

vtjnash force-pushed the jn/32392 branch from d2a4568 to 0c4fbd8 Compare April 4, 2023 14:31

vtjnash added the status:merge me PR is reviewed. Merge when all tests are passing label Apr 7, 2023

vtjnash force-pushed the jn/32392 branch from 0c4fbd8 to ea6bcd5 Compare April 7, 2023 18:38

vtjnash force-pushed the jn/32392 branch from ea6bcd5 to 8950caf Compare April 9, 2023 02:21

N5N3 mentioned this pull request Apr 10, 2023

fix another case where we might return free TypeVar #48228

Merged

vtjnash merged commit 35e4a1f into master Apr 10, 2023

vtjnash deleted the jn/32392 branch April 10, 2023 14:17

giordano removed the status:merge me PR is reviewed. Merge when all tests are passing label Apr 11, 2023

vtjnash added a commit that referenced this pull request Apr 17, 2023

subtype: replace leaf-bound typevars if they would result in Tuple{Un…

a7296d5

…ion{}} otherwise Refs: #49111

vtjnash mentioned this pull request Apr 17, 2023

subtype: replace leaf-bound typevars if they would result in Tuple{Union{}} otherwise #49393

Merged

benlorenz added a commit to benlorenz/libcxxwrap-julia that referenced this pull request May 3, 2023

julia 1.10: add cast for jl_apply_tuple_type(_v)

e6c7f00

this was changed in JuliaLang/julia#49111 for older julia versions this cast should be a no-op

benlorenz mentioned this pull request May 3, 2023

julia 1.10: add cast for jl_apply_tuple_type(_v) JuliaInterop/libcxxwrap-julia#124

Merged

barche pushed a commit to JuliaInterop/libcxxwrap-julia that referenced this pull request May 3, 2023

julia 1.10: add cast for jl_apply_tuple_type(_v)

4c1d2bd

this was changed in JuliaLang/julia#49111 for older julia versions this cast should be a no-op

vtjnash mentioned this pull request Oct 31, 2023

error in method definition on v1.10 and v1.11: ERROR: Tuple field type cannot be Union{} #51950

Closed

N5N3 mentioned this pull request Dec 4, 2023

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

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

make Tuple{Union{}} unconstructable #49111

make Tuple{Union{}} unconstructable #49111

vtjnash commented Mar 22, 2023

vtjnash commented Mar 23, 2023

vtjnash commented Mar 23, 2023

martinholters Mar 23, 2023

vtjnash Mar 23, 2023 •

edited

Loading

N5N3 Mar 23, 2023 •

edited

Loading

vtjnash Mar 23, 2023

martinholters Mar 23, 2023

vtjnash Mar 24, 2023 •

edited

Loading

vtjnash Mar 24, 2023

nanosoldier commented Mar 23, 2023

maleadt commented Mar 23, 2023

vtjnash commented Mar 23, 2023

nanosoldier commented Mar 24, 2023

martinholters commented Mar 24, 2023

N5N3 commented Mar 24, 2023

martinholters commented Mar 24, 2023

vtjnash commented Mar 24, 2023

vtjnash commented Mar 24, 2023

vtjnash commented Mar 24, 2023

nanosoldier commented Mar 24, 2023

nanosoldier commented Mar 27, 2023

George9000 commented Apr 10, 2023

make Tuple{Union{}} unconstructable #49111

make Tuple{Union{}} unconstructable #49111

Conversation

vtjnash commented Mar 22, 2023

vtjnash commented Mar 23, 2023

vtjnash commented Mar 23, 2023

martinholters Mar 23, 2023

Choose a reason for hiding this comment

vtjnash Mar 23, 2023 • edited Loading

Choose a reason for hiding this comment

N5N3 Mar 23, 2023 • edited Loading

Choose a reason for hiding this comment

vtjnash Mar 23, 2023

Choose a reason for hiding this comment

martinholters Mar 23, 2023

Choose a reason for hiding this comment

vtjnash Mar 24, 2023 • edited Loading

Choose a reason for hiding this comment

vtjnash Mar 24, 2023

Choose a reason for hiding this comment

nanosoldier commented Mar 23, 2023

maleadt commented Mar 23, 2023

vtjnash commented Mar 23, 2023

nanosoldier commented Mar 24, 2023

martinholters commented Mar 24, 2023

N5N3 commented Mar 24, 2023

martinholters commented Mar 24, 2023

vtjnash commented Mar 24, 2023

vtjnash commented Mar 24, 2023

vtjnash commented Mar 24, 2023

nanosoldier commented Mar 24, 2023

nanosoldier commented Mar 27, 2023

George9000 commented Apr 10, 2023

vtjnash Mar 23, 2023 •

edited

Loading

N5N3 Mar 23, 2023 •

edited

Loading

vtjnash Mar 24, 2023 •

edited

Loading