Make Vararg not a DataType #38136

Keno · 2020-10-22T03:46:11Z

Currently Vararg is a DataType, but is special cased in a
bunch of places to give it special behavior (e.g. in subtyping
and of course in tuple construction). However, unlike all other
DataTypes, it cannot appear as a type parameter, which caused trouble in
PR #38071. Having it be a DataType is a bit of a pun of convenience
in the first place - it's a lot more similar to a tvar (which can
be considered an implementation detail of UnionAll in the same way
Vararg is an implementation detail of Tuple), which has its own
non-type object. This PR does the same to Vararg, and moves it
from being an abstract DataType with special cased behavior to
its own custom type (called Core.VarargMarker). The user facing
behavior should be mostly unchanged, since boot.jl now has:

const Vararg{T, N} = VarargMarker{T, N}

i.e. we have a handly UnionAll wrapper that looks just like it
used to. The biggest difference is probably that VarargMarker
does not have .parameters, so code that tries to reach into
that explicitly will need to be adjusted. We could provide
a compatibility getproperty method to adapt that, but I'd
prefer to see how many packages need updating first, before
going that route.

This is essentially complete, but a few cleanup items remain.

Closes #30995 #34690 #26625 #37316

martinholters · 2020-10-22T07:08:53Z

👍 Completely agree, Vararg is sufficiently different from a data type that it should probably not be a DataType. The code changes needed to deal with the new definition in my eyes also tend to improve clarity, which is good sign. It also makes it more obvious where a Vararg is to be expected and that it usually deserves some extra thought how to deal with it.

Because it took me moment to wrap my head around it, some remarks that might help others grok what's happening:
With this PR Core.VarargMarker{Int,2} isa Core.VarargMarker, while for a normal unionall/data type, one would expect a <: instead of the isa. And indeed, previously Vararg{Int,2} <: Vararg. (This errors on this PR, but maybe should be restored?) But note that e.g. also Union{Int,Float64} isa Union, so it's not completely new and exotic. Furthermore Vararg{T,N} == Core.VarargMarker{T,N} does not imply Vararg == Core.VarargMarker. The former is a UnionAll, the latter a DataType.

JeffBezanson · 2020-10-22T22:04:59Z

src/dump.c

+        write_uint8(s->s, TAG_VARARG);
+        jl_serialize_value(s, ((jl_vararg_marker_t*)v)->T);
+        jl_serialize_value(s, ((jl_vararg_marker_t*)v)->N);
+    }


Did you encounter any particular reason this is needed? I would think the default case would work for these.

No, I thought I did, but I think the default is fine. I'll try removing it and see if things work out.

JeffBezanson · 2020-10-22T22:14:09Z

src/subtype.c

+            // N.B.: This case is only used for raw varargs that are not part
+            // of a tuple (those that are have special handling in subtype_tuple).
+            // Vararg isn't really a proper type, but it does sometimes show up
+            // as e.g. Type{Vararg}, so we'd like to handle that correctly.


Type{Vararg} is not allowed. Ideally we would give an error here too, but if that's too breaking we can instead just change this comment to say it's provided for compatibility with code that uses <: to compare Vararg objects.

This comment is old, it just moved from below. I can try giving an error and see what needs to be updated.

Oh, as Martin noticed above it looks like passing Vararg to <: is already an error in this PR :) We can try to go with that, and change this to an assertion.

JeffBezanson · 2020-10-22T23:31:12Z

Hmm, at first I thought it would be more compatible to keep Vararg::UnionAll true, but especially if we're going to change the fields of Vararg maybe it doesn't really matter. Getting rid of that constraint allows us to have one object Vararg instead of both Vararg and VarargMarker, plus Vararg{Int} isa Vararg is more directly analogous to Union{Int, Float64} isa Union. So maybe that is the right way to go.

Keno · 2020-10-22T23:39:30Z

Ok, I can try that.

Keno · 2020-10-24T03:59:38Z

I've updated this along these lines, disallowing Vararg inside a UnionAll, which I think is the right way to go, but we still end up with Vararg and VarargMarker, which need to be two separate things, since otherwise, you can't refer to the type of a Vararg (and people use a raw Vararg without any parameters as in Tuple{Vararg} and want that to mean Tuple{Vararg{Any}}, so Vararg can't be that type).

yurivish · 2020-10-24T04:17:16Z

Would this change close #34690?

julia> methods(Core.Vararg) # This does not
ERROR: TypeError: in Type, in parameter, expected Type, got Vararg
Stacktrace:
 [1] signature_type(::Any, ::Any) at ./reflection.jl:767
 [2] _methods at ./reflection.jl:830 [inlined]
 [3] methods(::Any, ::Any) at ./reflection.jl:875
 [4] methods(::Any) at ./reflection.jl:890
 [5] top-level scope at REPL[24]:1

Keno · 2020-10-24T04:19:31Z

julia> methods(Core.Vararg)
# 0 methods:

;)

Keno · 2020-10-24T16:42:30Z

@nanosoldier runtests(ALL, vs = ":master")

JeffBezanson · 2020-10-24T22:13:40Z

base/boot.jl

+
+UnionAll(v::TypeVar, @nospecialize(t)) = ccall(:jl_type_unionall, Any, (Any, Any), v, t)
+
+const Vararg = ccall(:jl_toplevel_eval_in, Any, (Any, Any), Core, _expr(:new, VarargMarker))


In this case, maybe call it TypeofVararg to go with TypeofBottom? This could also use jl_new_struct instead of eval.

nanosoldier · 2020-10-25T02:56:25Z

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

Keno · 2020-10-26T01:19:28Z

Alright, fully disallowing Vararg in UnionAll turned out to be a little aggressive. Apparently people like to write Tuple{Vararg{<:Real}} even though the <: doesn't do anything here (we already used to normalize on tuple construction to move it inside the vararg, so it doesn't even implicate the diagonal rule). I've added back that normalization into the UnionAll constructor, though it'll complain if depwarns are enabled, so we can remove it whenever people stop using this pattern or in 2.0 at the latest (it's actually only a few packages, so it's not too bad, but enough that I want to add the compat).

Keno · 2020-10-26T03:42:18Z

Alright, @JeffBezanson this works now, but there's some decisions to make for subtyping. Essentially, we need some sentinel value to indicate that a typevar must remain unbound. Previously, we'd just set it to the inner typevar, but we a) don't have that anymore and b) I don't think it necessarily makes much sense. In the commit I just pushed I'm using the vararg itself as the typevar, but of course the assertion that I added that we're not putting a vararg into subtyping fires there, so basically we need to decide what to use as the sentinel value here.

Keno · 2020-10-26T03:53:18Z

(I've pushed something I think is reasonable, but this stuff is pretty messy, since it isn't really designed to reason about int ranges)

Keno · 2020-10-26T20:09:32Z

@nanosoldier runtests(ALL, vs = ":master")

nanosoldier · 2020-10-27T01:36:07Z

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

Keno · 2020-10-27T01:42:38Z

Alright, looks like we're down to just the StaticArrays open-coding unwrapva issue, which I have a PR for at JuliaArrays/StaticArrays.jl#843.

Keno · 2020-10-27T04:32:05Z

MbedTLS segfault is unrelated: JuliaLang/MbedTLS.jl#227

JeffBezanson · 2020-10-27T17:42:23Z

base/compiler/inferencestate.jl

@@ -192,6 +192,8 @@ function sptypes_from_meth_instance(linfo::MethodInstance)
                    ty = UnionAll(tv, Type{tv})
                end
            end
+        elseif isa(v, Core.VarargMarker)
+            ty = Int


Fixes #37316?

It doesn't look like this was enough:

julia> f(::NTuple{N}) where {N} = N f (generic function with 1 method) julia> code_warntype(f, Tuple{Tuple}) Variables #self#::Core.Const(f) #unused#::Tuple{Vararg{T, N}} where T where N Body::Any 1 ─ return $(Expr(:static_parameter, 1))

Or is this check flawed in some way?

src/subtype.c

vtjnash

Looking very good so far

vtjnash · 2020-10-27T18:06:31Z

base/compiler/abstractinterpretation.jl

@@ -1045,10 +1045,11 @@ function sp_type_rewrap(@nospecialize(T), linfo::MethodInstance, isreturn::Bool)
        spsig = linfo.def.sig
        if isa(spsig, UnionAll)
            if !isempty(linfo.sparam_vals)
-                env = pointer_from_objref(linfo.sparam_vals) + sizeof(Ptr{Cvoid})
-                T = ccall(:jl_instantiate_type_in_env, Any, (Any, Any, Ptr{Any}), T, spsig, env)
+                sparam_vals = Any[isa(v, Core.VarargMarker) ? TypeVar(:N, Union{}, Any) :


Why would an sparam ever have been a Vararg object?

That's new in this PR to mark an unbound intvalued parameter (We used to use the TypeVar of the unbound Vararg as the sentinel, but we don't have that anymore). Any other non-Type, non-Int sentinel value would do also. I suppose we could change the C interface from spvals to sptypes as we've done elsewhere. Since Const is now a builtin, that should actually work ok.

vtjnash · 2020-10-27T18:09:34Z

base/compiler/tfuncs.jl

+        tt = tuple_tfunc(new_fields)
+        if isa(tt, PartialStruct)
+            if !(widenconst(tt) <: ti)
+                return PartialStruct(ti, tt.fields)


ti is not the typeof this object. That is always know to be exactly tt.typ (currently guaranteed by construction)

So the concern here is that we used to typeassert_type_instance the Vararg, which while I don't think is entirely correct, did give some precision that we're now losing here. I didn't see a great way to get that back without essentially duplicating intersection over the inference lattice, so I though at least this way we'd recover some precision.

vtjnash · 2020-10-27T18:10:30Z

base/compiler/tfuncs.jl

+                return PartialStruct(ti, tt.fields)
+            end
+        elseif !(tt <: ti)
+            return ti


vtjnash · 2020-10-27T18:13:09Z

base/compiler/typelattice.jl

@@ -220,6 +220,7 @@ widenconst(c::PartialTypeVar) = TypeVar
 widenconst(t::PartialStruct) = t.typ
 widenconst(t::Type) = t
 widenconst(t::TypeVar) = t
+widenconst(t::Core.VarargMarker) = t


Why would the typeof a variable be VarargMarker, which isn’t a type?

vtjnash · 2020-10-27T18:20:15Z

base/promotion.jl

-        if isa(N, Int)
-            return length(p)::Int + N - 1, true
+        if isdefined(last, :N) && isa(last.N, Int)
+            return length(p)::Int + last.N - 1, true


Might need a type assert (someone seems to have put effort into them previously, so I assume it mattered for invalidation)

Type assert on last.N?

src/builtins.c

src/jltypes.c

JeffBezanson · 2020-10-27T19:46:26Z

base/compiler/typelattice.jl

@@ -220,6 +220,7 @@ widenconst(c::PartialTypeVar) = TypeVar
 widenconst(t::PartialStruct) = t.typ
 widenconst(t::Type) = t
 widenconst(t::TypeVar) = t
+widenconst(t::Core.VarargMarker) = t


Shouldn't widenconst not be called on one of these?

Ideally yes, but currently inference is pretty liberal about calling both this and rewrap_unionall on non-Types and I just wanted to preserve the status quo for now. I'd argue a similar argument could be made for TypeVar just above.

base/essentials.jl

JeffBezanson · 2020-10-27T20:39:03Z

test/compiler/inference.jl

@@ -627,7 +627,7 @@ function maybe_vararg_tuple_1()
 end
 @test Type{Tuple{Vararg{Int}}} <: Base.return_types(maybe_vararg_tuple_1, ())[1]
 function maybe_vararg_tuple_2()
-    x = Type[Vararg{Int}][1]
+    x = [Vararg{Int}][1]


Suggested change

x = [Vararg{Int}][1]

x = Any[Vararg{Int}][1]

This is already tested by the test just above. I figured the most natural translation would be to just have this be typed as typeof(Vararg)..

This code has been dead since #38136. Clean it up some.

timholy · 2021-01-14T09:37:33Z

In the packages I've looked at, where this seems to be causing the most trouble is places like

map(T -> T <: Real, sig.parameters)

Is the absence of support for having Vararg on the left side of <: deliberate? If so, no worries, but if that's an oversight it might make sense to address it.

Keno · 2021-01-14T09:49:05Z

Is the absence of support for having Vararg on the left side of <: deliberate?

Yes, because the code you posted is probably buggy. You want map(T -> Base.unwrapva(T) <: Real, sig.parameters).

bkamins · 2021-03-05T12:15:00Z

Does this change qualify to be included in NEWS.md section "deprecated" section?

Currently `Vararg` is a DataType, but is special cased in a bunch of places to give it special behavior (e.g. in subtyping and of course in tuple construction). However, unlike all other DataTypes, it cannot appear as a type parameter, which caused trouble in PR JuliaLang#38071. Having it be a DataType is a bit of a pun of convenience in the first place - it's a lot more similar to a tvar (which can be considered an implementation detail of UnionAll in the same way Vararg is an implementation detail of Tuple), which has its own non-type object. This PR does the same to Vararg, and moves it from being an abstract DataType with special cased behavior to its own custom type (called `Core.TypeofVararg`). There are a few small behavior differences, but they are mostly internal. In particular, we no longer have `Vararg <: Type` and Vararg objects no longer have the .parameters field. Also, things like `Vararg{T} where T` are technically illegal now since Vararg is not a type. However, since a lot of people are using that pattern, I've brought it back with a deprecation (which is of course off by default). The only things that's disallowed is `Vararg{N, N} where N`, but I haven't seen anybody use that.

This code has been dead since JuliaLang#38136. Clean it up some.

Keno requested a review from JeffBezanson October 22, 2020 03:46

JeffBezanson reviewed Oct 22, 2020

View reviewed changes

JeffBezanson added the minor change Marginal behavior change acceptable for a minor release label Oct 22, 2020

Keno force-pushed the kf/varargnodatatype branch from f88f3fd to 1add963 Compare October 24, 2020 03:57

Keno force-pushed the kf/varargnodatatype branch from 3480882 to 6876f2c Compare October 24, 2020 05:24

JeffBezanson reviewed Oct 24, 2020

View reviewed changes

Keno mentioned this pull request Oct 27, 2020

Adjust to upcoming Base Vararg change JuliaArrays/StaticArrays.jl#843

Merged

JeffBezanson reviewed Oct 27, 2020

View reviewed changes

src/subtype.c Outdated Show resolved Hide resolved

vtjnash reviewed Oct 27, 2020

View reviewed changes

JeffBezanson reviewed Oct 27, 2020

View reviewed changes

base/essentials.jl Outdated Show resolved Hide resolved

JeffBezanson reviewed Oct 27, 2020

View reviewed changes

base/essentials.jl Outdated Show resolved Hide resolved

JeffBezanson reviewed Oct 27, 2020

View reviewed changes

aviatesk mentioned this pull request Dec 15, 2020

re-automate builtins generations JuliaDebug/JuliaInterpreter.jl#450

Closed

simeonschaub mentioned this pull request Dec 17, 2020

fix on Julia nightly julia-vscode/SymbolServer.jl#194

Merged

mcabbott mentioned this pull request Dec 19, 2020

Add more test groups, and TEMPORARILY disable some tests on Julia nightly (the tests that are causing CI to time out) mcabbott/Tullio.jl#72

Closed

simeonschaub mentioned this pull request Dec 27, 2020

add missing newline to UnionAll Vararg warning #39010

Merged

simeonschaub mentioned this pull request Jan 3, 2021

Internal error: encountered unexpected error in runtime: TypeError... #39082

Closed

maleadt mentioned this pull request Jan 4, 2021

Unreachable after Varargs change #39088

Closed

timholy mentioned this pull request Jan 4, 2021

No method matching reprcontext timholy/SnoopCompile.jl#193

Closed

fingolfin mentioned this pull request Jan 8, 2021

ERROR: TypeError: in <:, expected Type, got Vararg{Pair{String, String}} JuliaDebug/Cthulhu.jl#114

Closed

timholy mentioned this pull request Jan 8, 2021

Revise fails on nightly? timholy/Revise.jl#593

Closed

Keno added a commit that referenced this pull request Jan 10, 2021

subtype: rm dead code

2924cb0

This code has been dead since #38136. Clean it up some.

mkitti mentioned this pull request Jan 12, 2021

Convert AbstractArrays with strides to NumPy arrays JuliaPy/PyCall.jl#876

Closed

This was referenced Jan 13, 2021

potential for better type inference with Vararg #37316

Closed

TypeError in methods(Core.Vararg) #34690

Closed

dkarrasch mentioned this pull request Jan 14, 2021

Adjust Tuple{Vararg{LinearMap}} notation JuliaLinearAlgebra/LinearMaps.jl#132

Merged

This was referenced Jan 26, 2021

[release-1.6] Fix typeassert_tfunc for vararg PartialStructs #39402

Closed

[release-1.6] Illegal instruction (simple test case) #38971

Closed

simeonschaub mentioned this pull request Feb 8, 2021

fix AD demos on Julia nightly JuliaDiff/ChainRulesCore.jl#299

Merged

bkamins mentioned this pull request Mar 5, 2021

Re-generate precompile statemenst before 1.0 release JuliaData/DataFrames.jl#2642

Closed

vtjnash mentioned this pull request Mar 11, 2021

Internal error with 1.6rc1 caused by incorrect code #39862

Closed

darsnack mentioned this pull request Apr 2, 2021

Fix anonymous functions in Julia 1.6 JuliaIO/BSON.jl#94

Merged

ElOceanografo pushed a commit to ElOceanografo/julia that referenced this pull request May 4, 2021

subtype: rm dead code

3fe18fb

This code has been dead since JuliaLang#38136. Clean it up some.

oxinabox mentioned this pull request Oct 16, 2021

Dep/failure on v1.7 JuliaDiff/ChainRulesCore.jl#492

Closed

nickrobinson251 mentioned this pull request Nov 29, 2021

signature fails on method with Vararg invenia/ExprTools.jl#28

Open

fingolfin mentioned this pull request Feb 11, 2022

Don't use naked Vararg oscar-system/Oscar.jl#1072

Closed

felixcremer mentioned this pull request Sep 29, 2022

WARNING: Wrapping Vararg directly in UnionAll is deprecated (wrap the tuple instead). JuliaDataCubes/YAXArrays.jl#178

Open

timholy mentioned this pull request Apr 28, 2023

Spurious deprecation warning from Vararg !<: Type #49553

Closed


		UnionAll(v::TypeVar, @nospecialize(t)) = ccall(:jl_type_unionall, Any, (Any, Any), v, t)

		const Vararg = ccall(:jl_toplevel_eval_in, Any, (Any, Any), Core, _expr(:new, VarargMarker))

Make Vararg not a DataType #38136

Make Vararg not a DataType #38136

Conversation

Keno commented Oct 22, 2020 • edited Loading

martinholters commented Oct 22, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JeffBezanson commented Oct 22, 2020

Keno commented Oct 22, 2020

Keno commented Oct 24, 2020

yurivish commented Oct 24, 2020 • edited Loading

Keno commented Oct 24, 2020

Keno commented Oct 24, 2020

Choose a reason for hiding this comment

nanosoldier commented Oct 25, 2020

Keno commented Oct 26, 2020

Keno commented Oct 26, 2020

Keno commented Oct 26, 2020

Keno commented Oct 26, 2020

nanosoldier commented Oct 27, 2020

Keno commented Oct 27, 2020

Keno commented Oct 27, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vtjnash left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timholy commented Jan 14, 2021

Keno commented Jan 14, 2021

bkamins commented Mar 5, 2021

Keno commented Oct 22, 2020 •

edited

Loading

yurivish commented Oct 24, 2020 •

edited

Loading