Follow-up #46693 - Also exclude Type{Tuple{Union{...}, ...}} (#46723)

* Follow-up #46693 - Also exclude `Type{Tuple{Union{...}, ...}}` The check in #46693 was attempting to guarantee that the runtime value was a `DataType`, but was missing at least the case where a tuple of unions could have been re-distributed. Fix that up and refactor while we're at it. * Apply suggestions from code review Co-authored-by: Jameson Nash <[email protected]> Co-authored-by: Jameson Nash <[email protected]>
JuliaLang · Sep 13, 2022 · 969193b · 969193b · vtjnash · Sep 16, 2022
1 parent 8a59be6
commit 969193b
Show file tree

Hide file tree

Showing 3 changed files with 29 additions and 5 deletions.
diff --git a/base/compiler/tfuncs.jl b/base/compiler/tfuncs.jl
@@ -824,9 +824,8 @@ function getfield_nothrow(@nospecialize(s00), @nospecialize(name), boundscheck::
     if isa(s, Union)
         return getfield_nothrow(rewrap_unionall(s.a, s00), name, boundscheck) &&
                getfield_nothrow(rewrap_unionall(s.b, s00), name, boundscheck)
-    elseif isType(s)
-        sv = s.parameters[1]
-        s = s0 = typeof(sv)
+    elseif isType(s) && isTypeDataType(s.parameters[1])
+        s = s0 = DataType
     end
     if isa(s, DataType)
         # Can't say anything about abstract types
@@ -941,10 +940,11 @@ function _getfield_tfunc(@nospecialize(s00), @nospecialize(name), setfield::Bool
             s = typeof(sv)
         else
             sv = s.parameters[1]
-            if isa(sv, DataType) && isa(name, Const) && (!isType(sv) && sv !== Core.TypeofBottom)
+            if isTypeDataType(sv) && isa(name, Const)
                 nv = _getfield_fieldindex(DataType, name)
                 if nv == DATATYPE_NAME_FIELDINDEX
-                    # N.B. This doesn't work in general, because
+                    # N.B. This only works for fields that do not depend on type
+                    # parameters (which we do not know here).
                     return Const(sv.name)
                 end
                 s = DataType

diff --git a/base/compiler/typeutils.jl b/base/compiler/typeutils.jl
@@ -23,6 +23,29 @@ function hasuniquerep(@nospecialize t)
     return false
 end
 
+"""
+    isTypeDataType(@nospecialize t)
+
+For a type `t` test whether ∀S s.t. `isa(S, rewrap_unionall(Type{t}, ...))`,
+we have `isa(S, DataType)`. In particular, if a statement is typed as `Type{t}`
+(potentially wrapped in some UnionAll), then we are guaranteed that this statement
+will be a DataType at runtime (and not e.g. a Union or UnionAll typeequal to it).
+"""
+function isTypeDataType(@nospecialize t)
+    isa(t, DataType) || return false
+    isType(t) && return false
+    # Could be Union{} at runtime
+    t === Core.TypeofBottom && return false
+    if t.name === Tuple.name
+        # If we have a Union parameter, could have been redistributed at runtime,
+        # e.g. `Tuple{Union{Int, Float64}, Int}` is a DataType, but
+        # `Union{Tuple{Int, Int}, Tuple{Float64, Int}}` is typeequal to it and
+        # is not.
+        return _all(isTypeDataType, t.parameters)
+    end
+    return true
+end
+
 function has_nontrivial_const_info(lattice::PartialsLattice, @nospecialize t)
     isa(t, PartialStruct) && return true
     isa(t, PartialOpaque) && return true

diff --git a/test/compiler/inline.jl b/test/compiler/inline.jl
@@ -1511,4 +1511,5 @@ end
 
 # Test getfield modeling of Type{Ref{_A}} where _A
 @test Core.Compiler.getfield_tfunc(Type, Core.Compiler.Const(:parameters)) !== Union{}
+@test !isa(Core.Compiler.getfield_tfunc(Type{Tuple{Union{Int, Float64}, Int}}, Core.Compiler.Const(:name)), Core.Compiler.Const)
 @test fully_eliminated(Base.ismutable, Tuple{Base.RefValue})