Force inference of convert(::Type{T}, ::T) where T via typeasserts

[thchr]

I was looking at invalidations in StaticArrays and this place came up.
The situation is that there exist some callers of last with OneTo{Any} which gets inferred as Any. An example is Base.to_shape(::OneTo{<:Any}) (which in turn gets called by some specializations of similar, which get invalidated by StaticArrays).

But the allowable typevars in OneTo{T} are T<:Integer, so we can constrain the result of the access to the field values to Integer rather than Any. This e.g. lets Base.to_shape(::OneTo{<:Any}) infer as Int64 instead of Any.

I figured I might as well do the same thing for first(::OneTo) while I was at it.

This seems to cut out roughly 260 invalidations from StaticArrays (~about 50% of current invalidations).

[aviatesk]

Or alternatively, we can add type constraint on the method signatures, e.g. first(r::OneTo{T}) where {T<:Integer} = oneunit(T).

And I guess we should do this automatically in inference, i.e. restrict method signatures using struct type information?

[jakobnissen]

@aviatesk A bit more ambitiously: #6383 - though it seems it would be much easier to add this optimisation in inference that it would be to make Base.OneTo{<:Any} <: Base.OneTo{<:Integer} be true.

[thchr]

Or alternatively, we can add type constraint on the method signatures, e.g. first(r::OneTo{T}) where {T<:Integer} = oneunit(T).

The issue with restricting the definition above to e.g. last(::OneTo{T}) where {T<:Integer} is that there exist callers of last(::OneTo{T}) where T; so if we restrict to T<:Integer it seems those callers would still dispatch to last(r::OrdinalRange{T}) where T and again infer a return type of Any rather than Integer.
Examples of such callers are e.g. similar(::Type{BitArray}, ::Tuple{Union{Integer, OneTo}}) via Base.to_shape which originate from displaysize(::TTY) on Windows. (I'll submit a PR to suggest a fix for that one in a few minutes though)

And I guess we should do this automatically in inference, i.e. restrict method signatures using struct type information?

This would be lovely :)

[vtjnash]

The issue with restricting the definition above to e.g. last(::OneTo{T}) where {T<:Integer} is that there exist callers of last(::OneTo{T}) where T

That is impossible:

julia> Base.OneTo{Float64}
ERROR: TypeError: in OneTo, in T, expected T<:Integer, got Type{Float64}

[thchr]

That is impossible:

I of course agree that it is impossible to have a OneTo{T} where !(T <: Integer) - but what I mean is that there seems to be callers which don't know this. I.e., there exist poorly inferred last(::OneTo{T}) "calls" which don't realize that T is constrained to Integer.

E.g., I see a method instance for similar(::Type{BitArray}, ::Tuple{Union{Integer, Base.OneTo}}): the OneTo here doesn't have any constraints on T and is equivalent to OneTo{<:Any}, right? This then leads to a method instance existing for last(::OneTo{<:Any}), even though any run time evaluations of that would have to be OneTo{<:Integer}.
Please correct me if I'm misunderstanding or misphrasing this.

[vtjnash]

Would this work instead for it:

diff --git a/base/number.jl b/base/number.jl
index 7436655bfa..77f9eb79fa 100644
--- a/base/number.jl
+++ b/base/number.jl
@@ -4,7 +4,7 @@
 
 # Numbers are convertible
 convert(::Type{T}, x::T)      where {T<:Number} = x
-convert(::Type{T}, x::Number) where {T<:Number} = T(x)
+convert(::Type{T}, x::Number) where {T<:Number} = T(x)::T
 
 """
     isinteger(x) -> Bool

The output looks correct to me for inference of that function for the general case without needing this PR's specific change

[vtjnash]

No, we can see those are different:

julia> Base.OneTo{<:Any}
Base.OneTo{<:Any}

julia> Base.OneTo{<:Integer}
Base.OneTo

[thchr]

Ah, makes sense: thanks for clarifying.

Your convert(T, ...) suggestion achieved the same thing, indeed, and seems much better: I changed to that.

Is a test for this, or?

[vtjnash]

@test Base.return_types(convert, (Type{<:Number}, Number)) == Any[ Number, Number ]
should be okay as a test

[vtjnash]

I think it may be worth doing all of them at once, WDYT?

$ git grep -n 'convert(.\+\<T(' 

base/Enums.jl:20:Base.cconvert(::Type{T}, x::Enum{T2}) where {T<:Integer,T2<:Integer} = T(x)
base/array.jl:613:convert(::Type{T}, a::AbstractArray) where {T<:Array} = a isa T ? a : T(a)
base/baseext.jl:19:convert(::Type{T}, arg)  where {T<:VecElement} = T(arg)
base/bitarray.jl:580:convert(T::Type{<:BitArray}, a::AbstractArray) = a isa T ? a : T(a)
base/char.jl:184:convert(::Type{T}, x::Number) where {T<:AbstractChar} = T(x)
base/char.jl:185:convert(::Type{T}, x::AbstractChar) where {T<:Number} = T(x)
base/char.jl:186:convert(::Type{T}, c::AbstractChar) where {T<:AbstractChar} = T(c)
base/number.jl:7:convert(::Type{T}, x::Number) where {T<:Number} = T(x)::T
base/pointer.jl:23:convert(::Type{T}, x::Ptr) where {T<:Integer} = T(UInt(x))
base/range.jl:255:convert(::Type{T}, r::AbstractRange) where {T<:AbstractRange} = r isa T ? r : T(r)
base/set.jl:551:convert(::Type{T}, s::AbstractSet) where {T<:AbstractSet} = T(s)
base/strings/basic.jl:232:convert(::Type{T}, s::AbstractString) where {T<:AbstractString} = T(s)::T
base/twiceprecision.jl:273:convert(::Type{T}, x::TwicePrecision) where {T<:Number} = T(x)
doc/src/manual/conversion-and-promotion.md:114:Note that the behavior of `convert(T, x)` appears to be nearly identical to `T(x)`.
doc/src/manual/conversion-and-promotion.md:184:convert(::Type{T}, x::Number) where {T<:Number} = T(x)
stdlib/Dates/src/periods.jl:437:        @eval Base.convert(::Type{$T}, x::$Tc) = $T(divexact(value(x), $N))
stdlib/Dates/test/periods.jl:286:    Base.convert(::Type{T}, b::Beat) where {T<:Dates.Millisecond} = T(Dates.toms(b))
stdlib/LinearAlgebra/src/bidiag.jl:203:convert(T::Type{<:Bidiagonal}, m::AbstractMatrix) = m isa T ? m : T(m)
stdlib/LinearAlgebra/src/factorization.jl:57:convert(::Type{T}, f::Factorization) where {T<:Factorization} = T(f)
stdlib/LinearAlgebra/src/factorization.jl:59:convert(::Type{T}, f::Factorization) where {T<:AbstractArray} = T(f)
stdlib/LinearAlgebra/src/givens.jl:47:convert(::Type{T}, r::AbstractRotation) where {T<:AbstractRotation} = T(r)
stdlib/LinearAlgebra/src/special.jl:72:convert(T::Type{<:LowerTriangular}, m::Union{LowerTriangular,UnitLowerTriangular}) = m isa T ? m : T(m)
stdlib/LinearAlgebra/src/special.jl:73:convert(T::Type{<:UpperTriangular}, m::Union{UpperTriangular,UnitUpperTriangular}) = m isa T ? m : T(m)
stdlib/LinearAlgebra/src/symmetric.jl:195:convert(T::Type{<:Symmetric}, m::Union{Symmetric,Hermitian}) = m isa T ? m : T(m)
stdlib/LinearAlgebra/src/symmetric.jl:196:convert(T::Type{<:Hermitian}, m::Union{Symmetric,Hermitian}) = m isa T ? m : T(m)
stdlib/LinearAlgebra/test/svd.jl:137:            @test convert(Array, usv) ≈ T(asym)
stdlib/SharedArrays/src/SharedArrays.jl:377:convert(T::Type{<:SharedArray}, a::Array) = T(a)
test/atomics.jl:292:Base.convert(T::Type{<:UndefComplex}, S) = T()
test/int.jl:103:        @test convert(Unsigned, T(3.0)) === UInt(3)
test/sorting.jl:476:            Base.convert(::Type{T_30763{$T}}, n::Integer) = T_30763{$T}($T(n))
test/testhelpers/Furlongs.jl:28:Base.convert(::Type{T}, y::Number) where {T<:Furlong{0}} = T(y)
test/testhelpers/Furlongs.jl:33:Base.convert(::Type{T}, y::Furlong) where {T<:Furlong{0}} = T(y)
test/testhelpers/Furlongs.jl:36:Base.convert(::Type{T}, y::Furlong) where {T<:Furlong} = T(y)
test/testhelpers/OffsetArrays.jl:450:Base.convert(::Type{T}, M::AbstractArray) where {T<:OffsetArray} = M isa T ? M : T(M)

[vtjnash]

(or even more broadly, it might apply to any of these methods, or even to any convert method except for Tuples)

julia> methods(convert, Tuple{Type{T},T} where T)
# 72 methods for generic function "convert" from Base:

[thchr]

(or even more broadly, it might apply to any of these methods, or even to any convert method except for Tuples)

Okay, that's a much bigger change, yeah 😮. I went ahead and did that in bf242fb, going through them one by one via

julia> ms = methods(convert, Tuple{Type{T},T} where T)
julia> for (i,m) in enumerate(ms)
           fn = String(m.file)
           fn = replace(fn, "/cache/build/default-amdci5-5/julialang/julia-master/usr/share/julia/stdlib/v1.9/"=>"/stdlib/")
           b = "/home/tchr/dev/julia" # hardcoded path
           if !startswith(fn, "/stdlib/")
               fn = "/base/"*fn
           end
           edit(b * fn, m.line)
       end

There were two entries in SparseArrays that I didn't look at since they are no longer in Base.

This change seems related to the discussion in #42372, so I figured I'd cross-link that. It's not my understanding that this change actually makes a decision on the proposal there, since this only covers convert methods and not constructors.

[thchr]

There are test-failures related to the new test which I don't fully understand. Instead of always returning Any[Integer, Integer], the test of Base.return_types(convert, (Type{Number}, Number)) returns one of the following, depending on the test-run:

Any[Integer, Integer]
Any[Union{}, Main.Furlongs.Furlong{0}, Main.Furlongs.Furlong{0}, Main.Furlongs.Furlong, Any, Any, Any, Main.Furlongs.Furlong, Main.Furlongs.Furlong, Union{}, Union{}, Union{}, Main.Quaternions.Quaternion, Number, Number]
Any[Union{}, Union{}, Union{}, Main.Furlongs.Furlong{0}, Main.Furlongs.Furlong{0}, Main.Furlongs.Furlong, Any, Any, Any, Main.Furlongs.Furlong, Main.Furlongs.Furlong, Number, Union{}, Number]
Any[Main.Quaternions.Quaternion, Number, Number]
Any[Main.Test76Main_LinearAlgebra_generic.TestGeneric.MyDual, Union{}, Union{}, Main.Quaternions.Quaternion, Union{}, Union{}, Main.Furlongs.Furlong{0}, Main.Furlongs.Furlong{0}, Main.Furlongs.Furlong, Any, Any, Any, Main.Furlongs.Furlong, Main.Furlongs.Furlong, Number, Number]
Any[Main.Test67Main_LinearAlgebra_generic.TestGeneric.MyDual, Union{}, Main.Quaternions.Quaternion, Union{}, Union{}, Main.Furlongs.Furlong{0}, Main.Furlongs.Furlong{0}, Main.Furlongs.Furlong, Any, Any, Any, Main.Furlongs.Furlong, Main.Furlongs.Furlong, Number, Union{}, Number]

It seems like different convert method definitions exist in different test-runs?
Any suggestions on how to make this test more robust or whether this is indicative of a real issue with the changes would be welcome!

[vtjnash]

Those come from some of the testhelper files. That does make this test much less reliable than desired. Let's just drop it?

[thchr]

Oki, done.

[thchr]

Gentle bump for this: anything else needed here?