fix invalidations in sort! from Static.jl

[ranocha]

This should hopefully fix quite some invalidations coming from Static.jl.

Here is the code:

               _
   _       _ _(_)_     |  Documentation: https://docs.julialang.org
  (_)     | (_) (_)    |
   _ _   _| |_  __ _   |  Type "?" for help, "]?" for Pkg help.
  | | | | | | |/ _` |  |
  | | |_| | | | (_| |  |  Version 1.8.0 (2022-08-17)
 _/ |\__'_|_|_|\__'_|  |  Official https://julialang.org/ release
|__/                   |

(@v1.8) pkg> activate --temp

(jl_PDrBpd) pkg> add Static
    Updating registry at `~/.julia/registries/General.toml`
   Resolving package versions...
    Updating `/tmp/jl_PDrBpd/Project.toml`
  [aedffcd0] + Static v0.7.6
    Updating `/tmp/jl_PDrBpd/Manifest.toml`
  [615f187c] + IfElse v0.1.1
  [aedffcd0] + Static v0.7.6

julia> using SnoopCompileCore

julia> invalidations = @snoopr using Static

julia> using SnoopCompile

julia> trees = invalidation_trees(invalidations)
7-element Vector{SnoopCompile.MethodInvalidations}:
...
 inserting !(::False) in Static at ~/.julia/packages/Static/sVI3g/src/Static.jl:427 invalidated:
...
                 160: signature Tuple{typeof(!), Any} triggered MethodInstance for sort!(::Vector{String}, ::Int64, ::Int64, ::Base.Sort.InsertionSortAlg, ::Base.Order.Ordering) (127 children)

Since !lt(...) is used here in an if stament - and if statements only accept Bools -
it appears to be sane to assert that lt returns a Bool here.

See also #46481, #46490

[Tokazama]

Another instance where it might be easier in the long run to annotate the function definition itself?

lt(::Left, x::T, y::T)::Bool where {T<:Floats} = slt_int(y, x)
lt(::Right, x::T, y::T)::Bool where {T<:Floats} = slt_int(x, y)

[ranocha]

I'm not sure whether it works and fixes the invalidations but since you seem to be sure I did what you suggested in bbf1bde

[Tokazama]

You can ensure that these fix invalidations in master by building from you branch of Julia and looking at the invalidations again. I didn't see invalidations when I added a Bool assertion on these, but depending on compiler work this may not be reflective of what happens for 1.8.

[ranocha]

There do not seem to be invalidations anymore on recent master with this PR on top (after moving the type assertions to the definitions of lt as suggested by @Tokazama). I'm building Julia again from the release-1.8 branch with this PR on top of it to see how it looks there.

[ranocha]

The version proposed by @Tokazama does not fix invalidations from a clean build of Julia from the release-1.8 branch with this PR on top of it (on my system). However, the version I implemented at first did fix the invalidations. Thus, I reverted the change proposed by @Tokazama.

[timholy]

@Tokazama, inference stops checking whether all possible callees return Bool if the number of methods exceeds 3. Thus in many cases, just adding method annotations does nothing to fix invalidations.

[Tokazama]

@Tokazama, inference stops checking whether all possible callees return Bool if the number of methods exceeds 3. Thus in many cases, just adding method annotations does nothing to fix invalidations.

That's really interesting because one of the ideas being thrown around to resolve invalidations more proactively by Stefan was to have some way of declaring all method variants inherit a type assertion (e.g., function isequal::Bool end would then require all methods return a Bool or error even if they don't have the type assert).

[timholy]

#42372, #36463

[timholy]

Since this calls isless, this seems perfectly safe.