Fix some vararg-related subtyping issues #31698

Keno · 2019-04-12T01:28:05Z

This is a backport from (a local version of) #31681. Turns out the same subtyping issues I addressed there exist on master, but we just tend to not see those types a whole lot.

JeffBezanson · 2019-04-12T20:05:03Z

Rebased on master to pick up the gc root fix.

Keno · 2019-04-12T21:21:18Z

I have more cases to fix that partially correct what's here, so let's do them all at once.

Keno · 2019-04-12T22:16:07Z

Current set of tests, I'm trying to fix, beyond what's in this PR:

let A = Tuple{Vararg{Val{N}, N} where N},
    B = Tuple{Vararg{Val{N}, N}} where N,
    C = Tuple{Val{2}, Val{2}}

    @test A == B
    @test issub(B, A)
    @test issub(C, A)
end
@test (Tuple{T, Vararg{T, 2}} where T<:Real) == (Tuple{Vararg{T, 3}} where T<: Real)

Keno · 2019-04-16T23:57:39Z

Note to self: The following is still wrong on this branch:

(Tuple{Vararg{T, 10}} where T<:Real) <: Base.Any16

Keno · 2019-04-18T00:35:30Z

Looks like I need to put the vararg subtyping check back for things like Type{Vararg} == Type{Vararg} which does get asked about when Vararg is used explicitly in the code.

JeffBezanson · 2019-04-18T00:39:50Z

We should try to avoid that, since forming those types should not be allowed.

Keno · 2019-04-18T00:41:59Z

It comes up in situations like the following:

push!(Any[], Vararg)

for which the compiled signature is Tuple{typeof(push!), Vector{Any}, Type{Vararg}}. I guess we could disallow Vararg from Typeof.

Keno · 2019-04-18T22:24:18Z

@JeffBezanson This is ready for re-review.

Keno · 2019-04-20T22:44:53Z

Linking #30995 here for the Type{Vararg} issue. I guess the conclusion is that that isn't legal, so we should find out who forms it and fix it.

vtjnash · 2019-04-24T15:48:31Z

src/subtype.c

-            if (npx != npy || vx || vy) {
-                if (!vy) {
+            if (npx != npy || vx != JL_VARARG_NONE || vy) {
+                if ((vx == JL_VARARG_NONE || vx == JL_VARARG_UNBOUND) && !vy) {


I think UNBOUND is computed incorrectly for subtyping's use (iirc, it was introduced for gf.c's heuristic type signature limiting), since it doesn't count number of uses. Consider Tuple{Vararg{NTuple{N}, N} where N}. To be conservative, just consider jl_is_long vs jl_is_typevar in this fast-path.

What the problem with that case? If the RHS is not a vararg, then if we have a unionall here it can't possibly be a subtype.

I don't know. I just don't trust this code to do any counting of TypeVar uses.

test/subtype.jl

src/subtype.c

JeffBezanson · 2019-04-24T22:42:51Z

I think this fixes #31805? Would be good to add that test case.

vtjnash · 2019-04-29T18:55:47Z

src/subtype.c

+            jl_vararg_kind_t vx = JL_VARARG_NONE;
+            jl_value_t *vxt = NULL;
+            int vy = 0;
+            int vnpx = npx;


vnpx seems like an odd (too long) mash of letters, since it appears to actually be the number of required (non-var) parameters (nfx for "n fields/fixed of x")

vtjnash · 2019-04-29T19:01:16Z

src/subtype.c

-            if (npx != npy || vx || vy) {
-                if (!vy) {
+            if (npx != npy || vx != JL_VARARG_NONE || vy) {
+                if ((vx == JL_VARARG_NONE || vx == JL_VARARG_UNBOUND) && !vy) {


I don't know. I just don't trust this code to do any counting of TypeVar uses.

JeffBezanson · 2019-05-16T18:31:23Z

base/compiler/typelimits.jl

-        return t # t isn't something new
+    else
+        ut = unwrap_unionall(t)
+        if isa(ut, DataType) && ut.name !== unwrap_unionall(Vararg).name && isa(c, Type) && c !== Union{} && c <: t


Can use Base._va_typename here.

(Core.Compiler.) _va_typename, but yes

Keno · 2019-05-16T21:15:49Z

PkgEval reveals the following incorrect intersection on this branch:

julia> typeintersect(Tuple{Array{Tuple{Vararg{Int64,N}},N},Tuple{Vararg{Array{Int64,1},N}}} where N, Tuple{Array{Tuple{Int64},1}, Tuple})
Union{}

(Tuple{Array{Tuple{Int64},1},Tuple{Array{Int64,1}}} is in the intersection).

…ararg Upon construction, we normalize `Vararg{T, N} where {T,N}` to `Vararg{T where T, N} where N`, thus there can be at most one UnionAll wrapper around a Vararg. In subtyping we were already assuming that there can be at most two such wrappers, so simply adjust that and add an appropriate assertion to make sure we catch any cases where this ever goes wrong.

Co-Authored-By: Keno <[email protected]>

In subtyping proper, variables introduced on the left (i.e. forall variables) don't have any equality constraints, because we have no syntax for creating them. However, intersection does sometimes create such environments, so we need to handle it in subtyping.

Keno · 2019-05-18T16:12:43Z

PkgEval is clean now. Will merge.

Keno · 2019-07-20T20:48:29Z

Let's backport this to fix #32607

* Fix jl_obvious_subtype with INT vararg constraint * Fix a vararg-related non-transitivity in subtyping * Fix another vararg subtype issue * Take advantage of their being at most one UnionAll wrapped around a Vararg Upon construction, we normalize `Vararg{T, N} where {T,N}` to `Vararg{T where T, N} where N`, thus there can be at most one UnionAll wrapper around a Vararg. In subtyping we were already assuming that there can be at most two such wrappers, so simply adjust that and add an appropriate assertion to make sure we catch any cases where this ever goes wrong. * Rewrite subtype_tuple to fix extra cases * Put back the case for naked varargs * Update test/subtype.jl Co-Authored-By: Keno <[email protected]> * Add test for #31805 * Fix style review comments * Rename variable * In person review changes * Fix bug * Handle integer bounds on left arguments in the environment In subtyping proper, variables introduced on the left (i.e. forall variables) don't have any equality constraints, because we have no syntax for creating them. However, intersection does sometimes create such environments, so we need to handle it in subtyping. (Cherry-picked from 4a38e79)

ararslan · 2019-07-25T16:02:14Z

Running PkgEval with the backports branch suggests that this didn't fix the issue

Keno requested a review from JeffBezanson April 12, 2019 01:28

JeffBezanson force-pushed the kf/varargsubtype branch from d14696e to df87943 Compare April 12, 2019 20:04

JeffBezanson approved these changes Apr 12, 2019

View reviewed changes

JeffBezanson added the types and dispatch Types, subtyping and method dispatch label Apr 12, 2019

Keno force-pushed the kf/varargsubtype branch 2 times, most recently from c67d1b5 to acec9b5 Compare April 16, 2019 23:52

Keno force-pushed the kf/varargsubtype branch from acec9b5 to 5a8bdbf Compare April 17, 2019 20:21

Keno force-pushed the kf/varargsubtype branch from 5a8bdbf to 4be19a7 Compare April 18, 2019 20:21

vtjnash mentioned this pull request Apr 23, 2019

subtyping: apply performance optimization in more places #31807

Merged

vtjnash reviewed Apr 24, 2019

View reviewed changes

test/subtype.jl Outdated Show resolved Hide resolved

vtjnash reviewed Apr 24, 2019

View reviewed changes

src/subtype.c Outdated Show resolved Hide resolved

src/subtype.c Outdated Show resolved Hide resolved

Keno force-pushed the kf/varargsubtype branch from 7cf98bb to cf256fa Compare April 25, 2019 21:21

vtjnash reviewed Apr 29, 2019

View reviewed changes

Keno mentioned this pull request May 7, 2019

fix an assertion failure from jl_obvious_subtype #31956

Closed

Keno force-pushed the kf/varargsubtype branch from cf256fa to 1acc148 Compare May 16, 2019 17:06

JeffBezanson reviewed May 16, 2019

View reviewed changes

Keno added 2 commits May 17, 2019 18:36

Fix jl_obvious_subtype with INT vararg constraint

8c03f4b

Fix a vararg-related non-transitivity in subtyping

fe7c2c2

Keno and others added 11 commits May 17, 2019 18:36

Fix another vararg subtype issue

be96d71

Rewrite subtype_tuple to fix extra cases

dae6697

Put back the case for naked varargs

ced439c

Update test/subtype.jl

9f4e95a

Co-Authored-By: Keno <[email protected]>

Add test for #31805

552d056

Fix style review comments

5570423

Rename variable

f25c35e

In person review changes

161aacf

Fix bug

e056490

Keno force-pushed the kf/varargsubtype branch from 611b947 to a3db5f6 Compare May 17, 2019 22:36

Keno merged commit 4a38e79 into master May 18, 2019

martinholters deleted the kf/varargsubtype branch May 19, 2019 20:38

JeffBezanson mentioned this pull request May 28, 2019

subtyping may forget to consider tuple elements #31805

Closed

Keno mentioned this pull request Jul 20, 2019

Unreachable reached, Julia 1.2.0-rc2.0 #32607

Closed

Keno added the backport 1.2 label Jul 20, 2019

KristofferC mentioned this pull request Jul 20, 2019

Backports for 1.2.0 release #32592

Merged

14 tasks

JeffBezanson removed the backport 1.2 label Jul 30, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix some vararg-related subtyping issues #31698

Fix some vararg-related subtyping issues #31698

Keno commented Apr 12, 2019

JeffBezanson commented Apr 12, 2019

Keno commented Apr 12, 2019

Keno commented Apr 12, 2019 •

edited

Loading

Keno commented Apr 16, 2019

Keno commented Apr 18, 2019

JeffBezanson commented Apr 18, 2019

Keno commented Apr 18, 2019

Keno commented Apr 18, 2019

Keno commented Apr 20, 2019

vtjnash Apr 24, 2019 •

edited

Loading

Keno Apr 25, 2019

vtjnash Apr 29, 2019

JeffBezanson commented Apr 24, 2019

vtjnash Apr 29, 2019

vtjnash Apr 29, 2019

JeffBezanson May 16, 2019

Keno May 16, 2019

Keno commented May 16, 2019 •

edited

Loading

Keno commented May 18, 2019

Keno commented Jul 20, 2019

ararslan commented Jul 25, 2019

Fix some vararg-related subtyping issues #31698

Fix some vararg-related subtyping issues #31698

Conversation

Keno commented Apr 12, 2019

JeffBezanson commented Apr 12, 2019

Keno commented Apr 12, 2019

Keno commented Apr 12, 2019 • edited Loading

Keno commented Apr 16, 2019

Keno commented Apr 18, 2019

JeffBezanson commented Apr 18, 2019

Keno commented Apr 18, 2019

Keno commented Apr 18, 2019

Keno commented Apr 20, 2019

vtjnash Apr 24, 2019 • edited Loading

Choose a reason for hiding this comment

Keno Apr 25, 2019

Choose a reason for hiding this comment

vtjnash Apr 29, 2019

Choose a reason for hiding this comment

JeffBezanson commented Apr 24, 2019

vtjnash Apr 29, 2019

Choose a reason for hiding this comment

vtjnash Apr 29, 2019

Choose a reason for hiding this comment

JeffBezanson May 16, 2019

Choose a reason for hiding this comment

Keno May 16, 2019

Choose a reason for hiding this comment

Keno commented May 16, 2019 • edited Loading

Keno commented May 18, 2019

Keno commented Jul 20, 2019

ararslan commented Jul 25, 2019

Keno commented Apr 12, 2019 •

edited

Loading

vtjnash Apr 24, 2019 •

edited

Loading

Keno commented May 16, 2019 •

edited

Loading