fix some of StaticArrays invalidating Revise

[simeonschaub]

with #39434:

julia> using Revise, SnoopCompileCore

julia> struct A end

julia> @snoopr (::Type{<:A})(::AbstractVector) = 1
18-element Vector{Any}:
  MethodInstance for JuliaInterpreter.namedtuple(::Vector{Any})
 1
  MethodInstance for JuliaInterpreter.prepare_args(::Any, ::Vector{Any}, ::Vector{Any})
 2
  MethodInstance for JuliaInterpreter.var"#determine_method_for_expr#44"(::Bool, ::typeof(JuliaInterpreter.determine_method_for_expr), ::Expr)
 3
  MethodInstance for (::JuliaInterpreter.var"#determine_method_for_expr##kw")(::NamedTuple{(:enter_generated,), Tuple{Bool}}, ::typeof(JuliaInterpreter.determine_method_for_expr), ::Expr)
 4
  MethodInstance for JuliaInterpreter.var"#enter_call_expr#45"(::Bool, ::typeof(JuliaInterpreter.enter_call_expr), ::Expr)
 5
  MethodInstance for JuliaInterpreter.enter_call_expr(::Expr)
 6
  MethodInstance for Union{}(::Vector{Any})
  "jl_method_table_insert"
  MethodInstance for Union{}(::Vector{Any})
  "invalidate_mt_cache"
  (::Type{var"#s24"} where var"#s24"<:A)(::AbstractVector{T} where T) in Main at REPL[3]:1
  "jl_method_table_insert"

with #39434 and this PR:

julia> using Revise, SnoopCompileCore

julia> struct A end

julia> @snoopr (::Type{<:A})(::AbstractVector) = 1
Any[]

[simeonschaub]

Ah, no, this doesn't work unfortunately. Perhaps we can define convert(::Type{Union{}}, ::AbstractArray) though and avoid invalidations that way.

[simeonschaub]

I have verified that this still eliminates the invalidations in question.

[timholy]

Thanks for noticing and addressing this!

[timholy]

It seems that the ideal fix here would be

julia> f(args) = Tuple{args...}
f (generic function with 1 method)

julia> @code_typed f([])
CodeInfo(
1 ─ %1 = Core._apply_iterate(Base.iterate, Core.apply_type, (Tuple,), args)::Type
└──      return %1
) => Type

and make this Tuple instead of Type. Do you understand why it isn't?

[simeonschaub]

This invalidation comes from JuliaInterpreter.namedtuple, I can see whether this can perhaps be fixed there directly. It's probably more of a general question whether the burden here should just fall on the package authors, or if we still want to work around this in Base, if it's easy enough like this.

[timholy]

That's not what I'm suggesting; I'm suggesting the fix lies in Core.Compiler, causing Tuple{container...} to be inferred as returning a Tuple even when container::Vector{Any}. Right now it's inferred as returning a Type, which seems unnecessarily broad.

[simeonschaub]

Hmm, I tried it now with these changes:

diff --git a/base/compiler/tfuncs.jl b/base/compiler/tfuncs.jl
index e8057fbe74..2942190c58 100644
--- a/base/compiler/tfuncs.jl
+++ b/base/compiler/tfuncs.jl
@@ -1134,7 +1134,9 @@ function apply_type_tfunc(@nospecialize(headtypetype), @nospecialize args...)
     else
         return Any
     end
+    istuple = isa(headtype, Type) && (headtype == Tuple)
     if !isempty(args) && isvarargtype(args[end])
+        istuple && return Type{Tuple{Vararg{Any, N}}} where N
         return isvarargtype(headtype) ? Core.TypeofVararg : Type
     end
     largs = length(args)
@@ -1177,7 +1179,6 @@ function apply_type_tfunc(@nospecialize(headtypetype), @nospecialize args...)
         end
         return allconst ? Const(ty) : Type{ty}
     end
-    istuple = isa(headtype, Type) && (headtype == Tuple)
     if !istuple && !isa(headtype, UnionAll) && !isvarargtype(headtype)
         return Union{}
     end
@@ -1237,9 +1238,9 @@ function apply_type_tfunc(@nospecialize(headtypetype), @nospecialize args...)
                     # If the names are known, keep the upper bound, but otherwise widen to Tuple.
                     # This is a widening heuristic to avoid keeping type information
                     # that's unlikely to be useful.
-                    if !(uw.parameters[1] isa Tuple || (i == 2 && tparams[1] isa Tuple))
-                        ub = Any
-                    end
+                    #if !(uw.parameters[1] isa Tuple || (i == 2 && tparams[1] isa Tuple))
+                    #    ub = Any
+                    #end
                 else
                     ub = Any
                 end

and it does seem to do the correct thing:

julia> f(x, y, z) = NamedTuple{(x...,),Tuple{y...}}(z...)
f (generic function with 1 method)

julia> @code_warntype f(Any[], Any[], Any[])
MethodInstance for f(::Vector{Any}, ::Vector{Any}, ::Vector{Any})
  from f(x, y, z) in Main at REPL[1]:1
Arguments
  #self#::Core.Const(f)
  x::Vector{Any}
  y::Vector{Any}
  z::Vector{Any}
Body::Any
1 ─ %1 = Core._apply_iterate(Base.iterate, Core.tuple, x)::Tuple
│   %2 = Core.tuple(Main.Tuple)::Core.Const((Tuple,))
│   %3 = Core._apply_iterate(Base.iterate, Core.apply_type, %2, y)::Type{Tuple{Vararg{Any, N}}} where N
│   %4 = Core.apply_type(Main.NamedTuple, %1, %3)::Type{NamedTuple{_A, _B}} where {_A, _B<:(Tuple{Vararg{Any, N}} where N)}
│   %5 = Core._apply_iterate(Base.iterate, %4, z)::Any
└──      return %5

I still see the same invalidations though, so fixing this in the apply_type tfunc does not seem to be enough.

[timholy]

Hmm, too bad. I guess I was bothered about

julia> NamedTuple{(), Union{}}
ERROR: NamedTuple field type must be a tuple type

but

julia> NamedTuple{names, Union{}} where names
NamedTuple{names, Union{}} where names

So it almost seems that nixing this type is the better solution.

But since having it be a Tuple doesn't fix the invalidation, and since we allow that UnionAll to be constructed, your original version seems fine. Is it fragile to adding the change to apply_type_tfunc? Meaning, will having it infer as <:Tuple{Vararg{Any}} nix your fix? And do you have to restrict your fix to iter::Tuple?

[simeonschaub]

Meaning, will having it infer as <:Tuple{Vararg{Any}} nix your fix?

No, this would still apply, since Union{} is still a subtype. This also explains why changing apply_type_tfunc did not fix anything.

And do you have to restrict your fix to iter::Tuple?

This would technically cause ambiguities and I am not sure how intentional ambiguities affect invalidations. If iter is not a Tuple, it will just be collected and fall back to the one for Tuple anyways.

[timholy]

This comes from a convert(Nothing, ::Any). Will that be handled by your other PR?

[simeonschaub]

I think this is a separate invalidation coming from Union{}(::Vector{Any}), because StaticArrays defines convert(::Type{<:StaticArray}, ::AbstractArray). Or did I misunderstand you? (The invalidations above are with #39434 already)