improve inferrabilities by telling the compiler relational invariants

[aviatesk]

Our compiler doesn't understand these relations automatically yet.

[simeonschaub]

Could you perhaps add a short comment to each of these functions explaining that this is because we don't do relational analysis yet, just because the code does look a bit odd at first glance? Also, is there any way to test this by checking the inferred return types?

[simeonschaub]

Is @assert the right tool for these kind of hints? We have been talking about eliminating them unless in debug mode, so perhaps something like

Suggested change

	@assert isa(f2, Tuple{})
	_isless(newret, f1, f2)
	_isless(newret, f1, f2::Tuple{})

would be better?

[aviatesk]

Thanks for your comment.
My original motivation to use @assert instead of typeassert was that otherwise JET will report false positive errors on the typeassertions for some complicated cases where inference is unsure about the type of one tuple but sure about that of the other, e.g.:

example false positive reports

under the definition of _isless below

    @inline function _isless(ret, I1::NTuple{N,Int}, I2::NTuple{N,Int}) where N
        newret = ifelse(ret==0, icmp(I1[N], I2[N]), ret)
        f1, f2 = Base.front(I1), Base.front(I2)
        if isa(f1, Tuple{})
            return _isless(newret, f1, f2::Tuple{}) # L133
        else
            return _isless(newret, f1, f2::Tuple{Int,Vararg{Int}}) # L135
        end
    end

julia> report_call((Int, Tuple{Vararg{Int}}, Tuple{Int,Int,Vararg{Int}})) do ret, t1, t2
           Base.IteratorsMD._isless(ret, t1, t2)
       end
═════ 2 possible errors found ═════
┌ @ none:2 Base.getproperty(Base.IteratorsMD, :_isless)(ret, t1, t2)
│┌ @ /Users/aviatesk/julia/julia/base/multidimensional.jl:133 Core.typeassert(f2, Core.apply_type(Base.IteratorsMD.Tuple))
││ invalid builtin function call: Core.typeassert(f2::Union{Tuple{Int64}, Tuple{Int64, Int64, Vararg{Int64}}}, Core.apply_type(Base.IteratorsMD.Tuple)::Type{Tuple{}})
│└────────────────────────────────────────────────────────────
│┌ @ /Users/aviatesk/julia/julia/base/multidimensional.jl:135 Base.IteratorsMD._isless(newret, f1, Core.typeassert(f2, Core.apply_type(Base.IteratorsMD.Tuple, Base.IteratorsMD.Int, Core.apply_type(Base.IteratorsMD.Vararg, Base.IteratorsMD.Int))))
││┌ @ /Users/aviatesk/julia/julia/base/multidimensional.jl:135 Core.typeassert(f2, Core.apply_type(Base.IteratorsMD.Tuple, Base.IteratorsMD.Int, Core.apply_type(Base.IteratorsMD.Vararg, Base.IteratorsMD.Int)))
│││ invalid builtin function call: Core.typeassert(f2::Tuple{}, Core.apply_type(Base.IteratorsMD.Tuple, Base.IteratorsMD.Int, Core.apply_type(Base.IteratorsMD.Vararg, Base.IteratorsMD.Int)::Core.TypeofVararg)::Type{Tuple{Int64, Vararg{Int64}}})
││└────────────────────────────────────────────────────────────

and with @assert JET can forcibly exclude those cases (but yes, this is just because JET's bug report criteria is somewhat looser on manually thrown exceptions than type failures...)

---

Anyway, I totally agree that @assert is very unnatural to be here, and there is no strong reason to make changes just for the sake of JET.
In the latest commit (bb0bff9) I focused on eliminating dynamic dispatches by adding type annotations (and also exclude some possibilities for JET to report false positives).
Hopefully in this direction we can agree with taking these changes in ?

[simeonschaub]

Yes, your version looks reasonable to me.

[simeonschaub]

LGTM.

On a related note: Did you ever think about running some of JET's analysis on base as part of CI? Making it a required check is probably difficult, since JET would always have to be updated with internal base changes right away, but perhaps it could run as a scheduled Job, similar to PkgEval and BaseBenchmarks? I am a bit worried that now that we are getting quite careful when it comes to inference and invalidations in base, it will also be really easy to accidentally introduce regressions in that area. @timholy might have some thoughts on this as well.