Improve apply_type_nothrow precision #38071

Keno · 2020-10-17T00:36:21Z

No description provided.

martinholters

Could use more tests of all the different code paths / conditions.

base/compiler/tfuncs.jl

JeffBezanson · 2020-10-20T20:19:31Z

base/compiler/tfuncs.jl

@@ -1067,6 +1069,20 @@ function _fieldtype_tfunc(@nospecialize(s), exact::Bool, @nospecialize(name))
 end
 add_tfunc(fieldtype, 2, 3, fieldtype_tfunc, 0)

+function valid_type_param(T)


Needs a more specific name to clarify the difference with valid_tparam. Maybe all_instances_valid_tparams?

valid_tparam_type?

JeffBezanson · 2020-10-20T20:19:42Z

base/compiler/tfuncs.jl

@@ -1067,6 +1069,20 @@ function _fieldtype_tfunc(@nospecialize(s), exact::Bool, @nospecialize(name))
 end
 add_tfunc(fieldtype, 2, 3, fieldtype_tfunc, 0)

+function valid_type_param(T)
+    isvarargtype(T) && return false


I don't think this case is possible here.

This case is still wrong (it also was before, so it's not necessarily on this PR to fix it):

julia> Core.Compiler.apply_type_nothrow(Any[Core.Const(Ref),Core.Const(Vararg)],Type{<:Ref}) true

I don't think this case is possible here.

Maybe, but we may want to use this function from other places, so I thought it better to cover all the cases.

JeffBezanson · 2020-10-20T20:30:28Z

base/compiler/tfuncs.jl

@@ -1086,22 +1102,38 @@ function apply_type_nothrow(argtypes::Array{Any, 1}, @nospecialize(rt))
    for i = 2:length(argtypes)
        isa(u, UnionAll) || return false
        ai = widenconditional(argtypes[i])
-        if ai ⊑ TypeVar
+        if ai ⊑ TypeVar || ai === DataType


Probably not true since Vararg isa DataType.

@JeffBezanson and I talked about fixing this by making Vararg not a DataType, so we'll wait with this PR until @JeffBezanson gets to that change.

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.

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.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.

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.

Keno requested review from vtjnash, JeffBezanson and martinholters October 17, 2020 19:33

martinholters requested changes Oct 19, 2020

View reviewed changes

base/compiler/tfuncs.jl Outdated Show resolved Hide resolved

base/compiler/tfuncs.jl Outdated Show resolved Hide resolved

base/compiler/tfuncs.jl Outdated Show resolved Hide resolved

Keno force-pushed the kf/apply_type_nothrow branch from 8be3593 to 484ee1b Compare October 19, 2020 23:41

JeffBezanson reviewed Oct 20, 2020

View reviewed changes

Keno mentioned this pull request Oct 22, 2020

Make Vararg not a DataType #38136

Merged

Keno force-pushed the kf/apply_type_nothrow branch from 484ee1b to fd6df3c Compare December 15, 2020 21:17

Improve apply_type_nothrow precision

1731ef9

Keno force-pushed the kf/apply_type_nothrow branch from fd6df3c to 1731ef9 Compare December 15, 2020 21:32

Keno requested a review from JeffBezanson December 15, 2020 21:55

Keno merged commit 0eb4723 into master Dec 16, 2020

Keno deleted the kf/apply_type_nothrow branch December 16, 2020 02:04

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

Improve apply_type_nothrow precision (JuliaLang#38071)

f142e7f

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve apply_type_nothrow precision #38071

Improve apply_type_nothrow precision #38071

Keno commented Oct 17, 2020

martinholters left a comment

JeffBezanson Oct 20, 2020

martinholters Oct 21, 2020

JeffBezanson Oct 20, 2020

JeffBezanson Oct 20, 2020

Keno Oct 20, 2020

JeffBezanson Oct 20, 2020

Keno Oct 20, 2020

Keno Oct 21, 2020

Improve apply_type_nothrow precision #38071

Improve apply_type_nothrow precision #38071

Conversation

Keno commented Oct 17, 2020

martinholters 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