From 0b384835bde6c9e0f63ec63159ce4e72771b92f2 Mon Sep 17 00:00:00 2001
From: Shuhei Kadowaki <40514306+aviatesk@users.noreply.github.com>
Date: Tue, 13 Sep 2022 11:35:03 +0900
Subject: [PATCH] inference: improve `isa`-constraint propagation for
 `iskindtype` objects (#46712)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Currently our inference isn't able to propagate `isa`-based type
constraint for cases like `isa(Type{<:...}, DataType)` since
`typeintersect` may return `Type` object itself when taking `Type`
object and `iskindtype`-object.

This case happens in the following kind of situation (motivated by the
discussion at <https://github.com/JuliaLang/julia/pull/46553#issuecomment-1233024271>):
```julia
julia> function isa_kindtype(T::Type{<:AbstractVector})
           if isa(T, DataType)
               # `T` here should be inferred as `DataType` rather than `Type{<:AbstractVector}`
               return T.name.name # should be inferred as ::Symbol
           end
           return nothing
       end
isa_kindtype (generic function with 1 method)

julia> only(code_typed(isa_kindtype; optimize=false))
CodeInfo(
1 ─ %1 = (T isa Main.DataType)::Bool
└──      goto #3 if not %1
2 ─ %3 = Base.getproperty(T, :name)::Any
│   %4 = Base.getproperty(%3, :name)::Any
└──      return %4
3 ─      return Main.nothing
) => Any
```

This commit improves the situation by adding a special casing for
abstract interpretation, rather than changing the behavior of
`typeintersect`.
---
 base/compiler/abstractinterpretation.jl | 26 ++++++++++++++++++-------
 test/compiler/inference.jl              |  9 +++++++++
 2 files changed, 28 insertions(+), 7 deletions(-)

diff --git a/base/compiler/abstractinterpretation.jl b/base/compiler/abstractinterpretation.jl
index 3c26f2bdef4e5..6dc7a433c6d64 100644
--- a/base/compiler/abstractinterpretation.jl
+++ b/base/compiler/abstractinterpretation.jl
@@ -1538,6 +1538,10 @@ function abstract_call_builtin(interp::AbstractInterpreter, f::Builtin, (; fargs
                     tty_lb = tty_ub # TODO: this would be wrong if !isexact_tty, but instanceof_tfunc doesn't preserve this info
                     if !has_free_typevars(tty_lb) && !has_free_typevars(tty_ub)
                         ifty = typeintersect(aty, tty_ub)
+                        if iskindtype(tty_ub) && ifty !== Bottom
+                            # `typeintersect` may be unable narrow down `Type`-type
+                            ifty = tty_ub
+                        end
                         valid_as_lattice(ifty) || (ifty = Union{})
                         elty = typesubtract(aty, tty_lb, InferenceParams(interp).MAX_UNION_SPLITTING)
                         return Conditional(a, ifty, elty)
@@ -2678,14 +2682,22 @@ end
 function conditional_change(state::VarTable, @nospecialize(typ), slot::Int)
     vtype = state[slot]
     oldtyp = vtype.typ
-    # approximate test for `typ ∩ oldtyp` being better than `oldtyp`
-    # since we probably formed these types with `typesubstract`, the comparison is likely simple
-    if ignorelimited(typ) ⊑ ignorelimited(oldtyp)
-        # typ is better unlimited, but we may still need to compute the tmeet with the limit "causes" since we ignored those in the comparison
-        oldtyp isa LimitedAccuracy && (typ = tmerge(typ, LimitedAccuracy(Bottom, oldtyp.causes)))
-        return StateUpdate(SlotNumber(slot), VarState(typ, vtype.undef), state, true)
+    if iskindtype(typ)
+        # this code path corresponds to the special handling for `isa(x, iskindtype)` check
+        # implemented within `abstract_call_builtin`
+    elseif ignorelimited(typ) ⊑ ignorelimited(oldtyp)
+        # approximate test for `typ ∩ oldtyp` being better than `oldtyp`
+        # since we probably formed these types with `typesubstract`,
+        # the comparison is likely simple
+    else
+        return nothing
     end
-    return nothing
+    if oldtyp isa LimitedAccuracy
+        # typ is better unlimited, but we may still need to compute the tmeet with the limit
+        # "causes" since we ignored those in the comparison
+        typ = tmerge(typ, LimitedAccuracy(Bottom, oldtyp.causes))
+    end
+    return StateUpdate(SlotNumber(slot), VarState(typ, vtype.undef), state, true)
 end
 
 function bool_rt_to_conditional(@nospecialize(rt), slottypes::Vector{Any}, state::VarTable, slot_id::Int)
diff --git a/test/compiler/inference.jl b/test/compiler/inference.jl
index 65d9989a6428c..e0b07422f3821 100644
--- a/test/compiler/inference.jl
+++ b/test/compiler/inference.jl
@@ -4202,3 +4202,12 @@ end
 end
 call_pure_annotated_loop(x) = Val{pure_annotated_loop(x, 1)}()
 @test only(Base.return_types(call_pure_annotated_loop, Tuple{Int})) === Val{1}
+
+function isa_kindtype(T::Type{<:AbstractVector})
+    if isa(T, DataType)
+        # `T` here should be inferred as `DataType` rather than `Type{<:AbstractVector}`
+        return T.name.name # should be inferred as ::Symbol
+    end
+    return nothing
+end
+@test only(Base.return_types(isa_kindtype)) === Union{Nothing,Symbol}