ml-matches: update and improve ambiguity computation

[vtjnash]

This uses the much-enhanced ambiguity computing algorithm in ml-matches, and implements this all better. The failures here are actually all type-intersection problems (#36951), as far as I can tell.

[vtjnash]

this one seems possibly more real though, as the intersection can be either:

julia> t = typeintersect(Tuple{typeof(\),Factorization{T},Union{Array{Complex{T},2}, Array{Complex{T},1}}} where T<:Union{Float32, Float64}, Tuple{typeof(\),LQ{TA,S} where S<:AbstractMatrix{TA},StridedVecOrMat{TB}} where TB where TA)
Tuple{typeof(\),
LQ{T,S} where S<:AbstractMatrix{T},
Union{Array{Complex{T},2}, Array{Complex{T},1}}} where T<:Union{Float32, Float64}

or

julia> t = typeintersect(Tuple{typeof(\),LQ{TA,S} where S<:AbstractMatrix{TA},StridedVecOrMat{TB}} where TB where TA, Tuple{typeof(\),Factorization{T},Union{Array{Complex{T},2}, Array{Complex{T},1}}} where T<:Union{Float32, Float64})
Tuple{typeof(\),
Union{LQ{TA,S} where S<:AbstractMatrix{TA}, LQ{TA1,S} where S<:AbstractMatrix{TA1}},
Union{Array{Complex{TA},1}, Array{Complex{TA1},2}}} where TA1<:Union{Float32, Float64} where TA<:Union{Float32, Float64}

which end up matching slightly different sets of methods:

1-element Vector{Any}:
 Core.MethodMatch(Tuple{typeof(\),Union{LQ{T,S} where S<:AbstractMatrix{T}, LQ{T,S} where S<:AbstractMatrix{T}},Union{Array{Complex{T},2}, Array{Complex{T},1}}} where T<:Union{Float32, Float64}, svec(T<:Union{Float32, Float64}), \(F::LQ{T,S} where S<:AbstractMatrix{T}, B::Union{Array{Complex{T},1}, Array{Complex{T},2}}) where T<:Union{Float32, Float64} in LinearAlgebra at /data/vtjnash/julia/usr/share/julia/stdlib/v1.6/LinearAlgebra/src/lq.jl:322, true)

vs

2-element Vector{Any}:
 Core.MethodMatch(Tuple{typeof(\),LQ{T,S} where S<:AbstractMatrix{T},Union{Array{Complex{T},1}, Array{Complex{T},2}}} where T<:Union{Float32, Float64}, svec(T<:Union{Float32, Float64}), \(F::LQ{T,S} where S<:AbstractMatrix{T}, B::Union{Array{Complex{T},1}, Array{Complex{T},2}}) where T<:Union{Float32, Float64} in LinearAlgebra at /data/vtjnash/julia/usr/share/julia/stdlib/v1.6/LinearAlgebra/src/lq.jl:322, false)
 Core.MethodMatch(Tuple{typeof(\),Union{LQ{TA,S} where S<:AbstractMatrix{TA}, LQ{TA1,S} where S<:AbstractMatrix{TA1}},Union{Array{Complex{TA},1}, Array{Complex{TA1},2}}} where TA1<:Union{Float32, Float64} where TA<:Union{Float32, Float64}, svec(TA<:Union{Float32, Float64}, TB<:(Complex{TA} where TA<:Union{Float32, Float64})), \(A::LQ{TA,S} where S<:AbstractMatrix{TA}, B::StridedVecOrMat{TB}) where {TA, TB} in LinearAlgebra at /data/vtjnash/julia/usr/share/julia/stdlib/v1.6/LinearAlgebra/src/lq.jl:310, true)

[timholy]

Sorry, I should have looked at this earlier. While you've carefully carved out exemptions for ambiguity tests that run within Julia itself, what should packages do? I have a lot of packages where the tests include detect_ambiguities, and as of today we're seeing new failures. For example, https://travis-ci.org/github/JuliaDebug/JuliaInterpreter.jl/jobs/723397378.

I'm not arguing that the new algorithm is wrong (I haven't checked), but just wondering whether there's a plan for moving forward.

[vtjnash]

There’s no reason anymore to include Base and Core in your list. List just the modules you care about.

[timholy]

Is that true even if we add methods to functions owned by Base?

I'll mimic the style of tests here. I am adding a "needs news" tag.

[tkf]

I try to understand this list of methods enumerated in the expect vector. Since this PR is an improvement, they are previously false-negative and now true-positive, right? If so, is it correct to assume that I should be able to invoke a method error with some combination? However, I couldn't get the run-time ambiguity error with a few examples for this first item of expect. What am I doing wrong here?

julia> VERSION
v"1.6.0-DEV.819"

julia> convert(Union{Missing, Nothing}, missing)
missing

julia> convert(Union{Missing, Nothing}, nothing)

julia> convert(Union{Missing, Nothing, Int}, missing)
missing

julia> convert(Union{Missing, Nothing, Int}, nothing)

julia> convert(Union{Missing, Nothing, Int}, 1)
1

[vtjnash]

Not necessarily—the type system might claim that such an ambiguity exists, while no counterfactual actually does. I posted a link above that references this particular failure in the type system (#36951).

[tkf]

What is the API of detect_ambiguities? I've been assuming that it returns a non-empty vector if and only if there exist concrete argument types that would cause a method error. Could you document the exact guarantee? For example, it would be helpful to document that the users should expect false positive or negative.

If providing a guarantee is not possible or not appropriate (e.g., to allow the type system to evolve), can we demote detect_ambiguities (and detect_unbound_args?) to experimental API and explicitly mention as such.

[vtjnash]

Aside from problems with the type system, that is what this did before and is still what it does. Hence the new bug report number also. There was a bug in v1.0 where someone got a conditional backwards and accidentally additionally filtered out any ambiguity resulting from a failure of type-intersection. Hence this PR. That's now fixed.

[tkf]

that is what this did before and is still what it does

What is that? Could you explain what API detect_ambiguities provides now? Could you clarify what kind of "bound" users can assume with respect to the run-time behavior (method error)? No false positive (which is apparently not the case from your previous comment?)? No false negative (i.e., if detect_ambiguities returns an empty vector, there will be no method error; given there is no bug, of course)? Both kinds of failures are possible (i.e., it's just a hint)?

[tkf]

I noted above #36962 (comment), I think we need more documentation. So, I'm also adding needs docs

I'm also adding breaking because:

1. It breaks existing user code bases.
2. It removes a keyword argument.
3. If we retrofit a more specific API definition by introducing new documentation, it could be different from the users' mental models. It is not ideal to change the behavior of a 4-year-old API (be8679e) and claim a change in observable behavior as a bug fix or improvement when there is no documentation to ensure that the old and new behaviors are compatible in some ways.

[JeffBezanson]

This seems to cause some significant latency regressions.
Current master:

Base  ───────  20.674068 seconds
Stdlibs: ────  31.106509 seconds 60.0718%

julia> @time using Plots
[ Info: Precompiling Plots [91a5bcdd-55d7-5caf-9e0b-520d859cae80]
 94.738177 seconds (8.06 M allocations: 545.291 MiB, 0.12% gc time)

julia> @time display(plot(rand(10)))
  7.265292 seconds (10.40 M allocations: 601.608 MiB, 3.82% gc time)

julia> @time using Plots
  4.513663 seconds (7.97 M allocations: 539.723 MiB, 2.35% gc time)

with this reverted:

Base  ───────  20.185031 seconds
Stdlibs: ────  29.825570 seconds 59.6366%

julia> @time using Plots
[ Info: Precompiling Plots [91a5bcdd-55d7-5caf-9e0b-520d859cae80]
 85.283090 seconds (6.21 M allocations: 443.919 MiB, 0.12% gc time)

julia> @time display(plot(rand(10)))
  6.373428 seconds (8.95 M allocations: 514.353 MiB, 4.12% gc time)

julia> @time using Plots
  3.221717 seconds (6.12 M allocations: 438.344 MiB, 3.30% gc time)

So I don't think this change is worth it.

[vtjnash]

Yeah, I don’t think inference should be asking us to do the more expensive query (eliminate ambiguous methods) that this makes even worse. Need to first get data on what patterns this is affecting. IIUC, this is revealing problems with type-intersection that are stealing away a lot of performance by making it slightly worse in those cases.

[timholy]

I'm not entirely sure from @vtjnash's response above what the future is likely to look like. Will this be reverted? Or will this be retained but fixed to address the performance issues Jeff noticed? The reason I ask is that tons of packages are facing ambiguity-related test failures, and I'm unsure whether to migrate to the new style (the right strategy if this is being kept) or keep the old (if this is being reverted).

[timholy]

Ah, I see from #37458 (comment) that this seems likely to be reverted.

[vtjnash]

It should probably be surgically disabled again (as this code has been), rather than undoing the bug fixes entirely, but I can't get to that till next week.