Avoid broken kind type handling when compiling isa (#27736)

* small workaround checks against incorrect subtyping for kind types for isa_tfunc

* add test for #27078

* fix pretty crazy bug where Type{T}s were inferred as Ts

* work around broken subtyping in emit_isa codegen optimization

* a test that was checking for exact/optimal inference result is now broken

Justification for allowing this test to remain broken for now:
- benchmarking the expression (including downstream toy calculations on the output, e.g. broadcast sin) using BenchmarkTools reveals no actual performance difference
- inference returning an optimal result before was probably reliant on the broken subtyping behavior; correctness >>> performance
- inference is still returning a fairly tightly bounded, correct Union type

Author: jrevels

base/compiler/abstractinterpretation.jl

@@ -638,19 +638,20 @@ function abstract_call(@nospecialize(f), fargs::Union{Tuple{},Vector{Any}}, argt
             if !isa(body, Type) && !isa(body, TypeVar)
                 return Any
             end
-            has_free_typevars(body) || return body
-            if isa(argtypes[2], Const)
-                tv = argtypes[2].val
-            elseif isa(argtypes[2], PartialTypeVar)
-                ptv = argtypes[2]
-                tv = ptv.tv
-                canconst = false
-            else
-                return Any
+            if has_free_typevars(body)
+                if isa(argtypes[2], Const)
+                    tv = argtypes[2].val
+                elseif isa(argtypes[2], PartialTypeVar)
+                    ptv = argtypes[2]
+                    tv = ptv.tv
+                    canconst = false
+                else
+                    return Any
+                end
+                !isa(tv, TypeVar) && return Any
+                body = UnionAll(tv, body)
             end
-            !isa(tv, TypeVar) && return Any
-            theunion = UnionAll(tv, body)
-            ret = canconst ? AbstractEvalConstant(theunion) : Type{theunion}
+            ret = canconst ? AbstractEvalConstant(body) : Type{body}
             return ret
         end
         return Any

base/compiler/tfuncs.jl

@@ -53,12 +53,15 @@ add_tfunc(throw, 1, 1, (@nospecialize(x)) -> Bottom, 0)
 # returns (type, isexact)
 # if isexact is false, the actual runtime type may (will) be a subtype of t
 function instanceof_tfunc(@nospecialize(t))
-    if t === Bottom || t === typeof(Bottom)
-        return Bottom, true
-    elseif isa(t, Const)
+    if isa(t, Const)
         if isa(t.val, Type)
             return t.val, true
         end
+        return Bottom, true
+    end
+    t = widenconst(t)
+    if t === Bottom || t === typeof(Bottom) || typeintersect(t, Type) === Bottom
+        return Bottom, true
     elseif isType(t)
         tp = t.parameters[1]
         return tp, !has_free_typevars(tp)
@@ -385,21 +388,29 @@ add_tfunc(typeassert, 2, 2,
               return typeintersect(v, t)
           end, 4)
 add_tfunc(isa, 2, 2,
-          function (@nospecialize(v), @nospecialize(t))
-              t, isexact = instanceof_tfunc(t)
+          function (@nospecialize(v), @nospecialize(tt))
+              t, isexact = instanceof_tfunc(tt)
+              if t === Bottom
+                  # check if t could be equivalent to typeof(Bottom), since that's valid in `isa`, but the set of `v` is empty
+                  # if `t` cannot have instances, it's also invalid on the RHS of isa
+                  if typeintersect(widenconst(tt), Type) === Union{}
+                      return Union{}
+                  end
+                  return Const(false)
+              end
               if !has_free_typevars(t)
-                  if t === Bottom
-                      return Const(false)
-                  elseif v ⊑ t
-                      if isexact
+                  if v ⊑ t
+                      if isexact && isnotbrokensubtype(v, t)
                           return Const(true)
                       end
                   elseif isa(v, Const) || isa(v, Conditional) || isdispatchelem(v)
                       # this tests for knowledge of a leaftype appearing on the LHS
                       # (ensuring the isa is precise)
                       return Const(false)
-                  elseif isexact && typeintersect(v, t) === Bottom
-                      if !iskindtype(v) #= subtyping currently intentionally answers this query incorrectly for kinds =#
+                  elseif typeintersect(v, t) === Bottom
+                      # similar to `isnotbrokensubtype` check above, `typeintersect(v, t)`
+                      # can't be trusted for kind types so we do an extra check here
+                      if !iskindtype(v)
                           return Const(false)
                       end
                   end

base/compiler/typeutils.jl

@@ -31,6 +31,12 @@ function issingletontype(@nospecialize t)
     return false
 end
 
+# Subtyping currently intentionally answers certain queries incorrectly for kind types. For
+# some of these queries, this check can be used to somewhat protect against making incorrect
+# decisions based on incorrect subtyping. Note that this check, itself, is broken for
+# certain combinations of `a` and `b` where one/both isa/are `Union`/`UnionAll` type(s)s.
+isnotbrokensubtype(a, b) = (!iskindtype(b) || !isType(a) || issingletontype(a.parameters[1]))
+
 argtypes_to_type(argtypes::Array{Any,1}) = Tuple{anymap(widenconst, argtypes)...}
 
 function isknownlength(t::DataType)
@@ -52,7 +58,7 @@ end
 # return an upper-bound on type `a` with type `b` removed
 # such that `return <: a` && `Union{return, b} == Union{a, b}`
 function typesubtract(@nospecialize(a), @nospecialize(b))
-    if a <: b
+    if a <: b && isnotbrokensubtype(a, b)
         return Bottom
     end
     if isa(a, Union)

src/cgutils.cpp

@@ -1060,13 +1060,18 @@ static void emit_type_error(jl_codectx_t &ctx, const jl_cgval_t &x, Value *type,
 
 static std::pair<Value*, bool> emit_isa(jl_codectx_t &ctx, const jl_cgval_t &x, jl_value_t *type, const std::string *msg)
 {
+    // TODO: The subtype check below suffers from incorrectness issues due to broken
+    // subtyping for kind types (see https://github.com/JuliaLang/julia/issues/27078). For
+    // actual `isa` calls, this optimization should already have been performed upstream
+    // anyway, but having this optimization in codegen might still be beneficial for
+    // `typeassert`s if we can make it correct.
     Optional<bool> known_isa;
     jl_value_t *intersected_type = type;
     if (x.constant)
         known_isa = jl_isa(x.constant, type);
-    else if (jl_subtype(x.typ, type))
+    else if (jl_is_not_broken_subtype(x.typ, type) && jl_subtype(x.typ, type)) {
         known_isa = true;
-    else {
+    } else {
         intersected_type = jl_type_intersection(x.typ, type);
         if (intersected_type == (jl_value_t*)jl_bottom_type)
             known_isa = false;

src/julia.h

@@ -1067,6 +1067,7 @@ JL_DLLEXPORT int jl_subtype_env_size(jl_value_t *t);
 JL_DLLEXPORT int jl_subtype_env(jl_value_t *x, jl_value_t *y, jl_value_t **env, int envsz);
 JL_DLLEXPORT int jl_isa(jl_value_t *a, jl_value_t *t);
 JL_DLLEXPORT int jl_types_equal(jl_value_t *a, jl_value_t *b) JL_NOTSAFEPOINT;
+JL_DLLEXPORT int jl_is_not_broken_subtype(jl_value_t *a, jl_value_t *b);
 JL_DLLEXPORT jl_value_t *jl_type_union(jl_value_t **ts, size_t n);
 JL_DLLEXPORT jl_value_t *jl_type_intersection(jl_value_t *a, jl_value_t *b);
 JL_DLLEXPORT int jl_has_empty_intersection(jl_value_t *x, jl_value_t *y);

src/subtype.c

@@ -1165,6 +1165,13 @@ JL_DLLEXPORT int jl_types_equal(jl_value_t *a, jl_value_t *b)
     return jl_subtype(a, b) && jl_subtype(b, a);
 }
 
+JL_DLLEXPORT int jl_is_not_broken_subtype(jl_value_t *a, jl_value_t *b)
+{
+    // TODO: the final commented out check here isn't correct; it should be closer to the
+    // `issingletype` check used by `isnotbrokensubtype` in `base/compiler/typeutils.jl`
+    return !jl_is_kind(b) || !jl_is_type_type(a); // || jl_is_datatype_singleton((jl_datatype_t*)jl_tparam0(a));
+}
+
 int jl_tuple_isa(jl_value_t **child, size_t cl, jl_datatype_t *pdt)
 {
     if (jl_is_tuple_type(pdt) && !jl_is_va_tuple(pdt)) {

test/compiler/compiler.jl

@@ -1179,7 +1179,7 @@ let isa_tfunc = Core.Compiler.T_FFUNC_VAL[
     @test isa_tfunc(Array{Real}, Type{AbstractArray{Int}}) === Const(false)
     @test isa_tfunc(Array{Real, 2}, Const(AbstractArray{Real, 2})) === Const(true)
     @test isa_tfunc(Array{Real, 2}, Const(AbstractArray{Int, 2})) === Const(false)
-    @test isa_tfunc(DataType, Int) === Bool # could be improved
+    @test isa_tfunc(DataType, Int) === Union{}
     @test isa_tfunc(DataType, Const(Type{Int})) === Bool
     @test isa_tfunc(DataType, Const(Type{Array})) === Bool
     @test isa_tfunc(UnionAll, Const(Type{Int})) === Bool # could be improved
@@ -1189,7 +1189,7 @@ let isa_tfunc = Core.Compiler.T_FFUNC_VAL[
     @test isa_tfunc(typeof(Union{}), Const(Int)) === Const(false) # any result is ok
     @test isa_tfunc(typeof(Union{}), Const(Union{})) === Const(false)
     @test isa_tfunc(typeof(Union{}), typeof(Union{})) === Const(false)
-    @test isa_tfunc(typeof(Union{}), Union{}) === Const(false) # any result is ok
+    @test isa_tfunc(typeof(Union{}), Union{}) === Union{} # any result is ok
     @test isa_tfunc(typeof(Union{}), Type{typeof(Union{})}) === Const(true)
     @test isa_tfunc(typeof(Union{}), Const(typeof(Union{}))) === Const(true)
     let c = Conditional(Core.SlotNumber(0), Const(Union{}), Const(Union{}))
@@ -1204,7 +1204,7 @@ let isa_tfunc = Core.Compiler.T_FFUNC_VAL[
     @test isa_tfunc(Val{1}, Type{Val{T}} where T) === Bool
     @test isa_tfunc(Val{1}, DataType) === Bool
     @test isa_tfunc(Any, Const(Any)) === Const(true)
-    @test isa_tfunc(Any, Union{}) === Const(false) # any result is ok
+    @test isa_tfunc(Any, Union{}) === Union{} # any result is ok
     @test isa_tfunc(Any, Type{Union{}}) === Const(false)
     @test isa_tfunc(Union{Int64, Float64}, Type{Real}) === Const(true)
     @test isa_tfunc(Union{Int64, Float64}, Type{Integer}) === Bool
@@ -1245,7 +1245,7 @@ let subtype_tfunc = Core.Compiler.T_FFUNC_VAL[
     @test subtype_tfunc(Type{Union{}}, Union{Type{Int64}, Type{Float64}}) === Const(true)
     @test subtype_tfunc(Type{Union{}}, Union{Type{T}, Type{Float64}} where T) === Const(true)
     let c = Conditional(Core.SlotNumber(0), Const(Union{}), Const(Union{}))
-        @test subtype_tfunc(c, Const(Bool)) === Bool # any result is ok
+        @test subtype_tfunc(c, Const(Bool)) === Const(true) # any result is ok
     end
     @test subtype_tfunc(Type{Val{1}}, Type{Val{T}} where T) === Bool
     @test subtype_tfunc(Type{Val{1}}, DataType) === Bool
@@ -1717,3 +1717,8 @@ Base.iterate(i::Iterator27434, ::Val{2}) = i.z, Val(3)
 Base.iterate(::Iterator27434, ::Any) = nothing
 @test @inferred splat27434(Iterator27434(1, 2, 3)) == (1, 2, 3)
 @test Core.Compiler.return_type(splat27434, Tuple{typeof(Iterators.repeated(1))}) == Union{}
+
+# issue #27078
+f27078(T::Type{S}) where {S} = isa(T, UnionAll) ? f27078(T.body) : T
+T27078 = Vector{Vector{T}} where T
+@test f27078(T27078) === T27078.body

test/sets.jl

@@ -557,7 +557,8 @@ end
     x = @inferred replace(x -> x > 1, [1, 2], missing)
     @test isequal(x, [1, missing]) && x isa Vector{Union{Int, Missing}}
 
-    x = @inferred replace([1, missing], missing=>2)
+    @test_broken @inferred replace([1, missing], missing=>2)
+    x = replace([1, missing], missing=>2)
     @test x == [1, 2] && x isa Vector{Int}
     x = @inferred replace([1, missing], missing=>2, count=1)
     @test x == [1, 2] && x isa Vector{Union{Int, Missing}}