fix some cases of diagonal subtyping when a var also appears invariantly

[JeffBezanson]

This is a part of #31833 that seems to be pretty non-breaking.

fix #26108, fix #26716

[JeffBezanson]

@nanosoldier runtests(ALL, vs = ":master")

[nanosoldier]

Your test job has completed - possible new issues were detected. A full report can be found here. cc @maleadt

[vtjnash]

GeometryTypes package is an interesting case:

MethodError: *(::StaticArrays.SArray{Tuple{2,2},Int64,2,4}, ::HyperRectangle{2,Int64}) is ambiguous. Candidates:
*(m::StaticArrays.SArray{Tuple{N1,N1},T1,2,L} where L, h::HyperRectangle{N2,T2}) where {N1, N2, T1, T2} in GeometryTypes at /home/pkgeval/.julia/packages/GeometryTypes/06goY/src/hyperrectangles.jl:83
*(m::StaticArrays.SArray{Tuple{N,N},T1,2,L} where L, h::HyperRectangle{N,T2}) where {N, T1, T2} in GeometryTypes at /home/pkgeval/.julia/packages/GeometryTypes/06goY/src/hyperrectangles.jl:121

So we have A = Tuple{V{Tuple{N,N}}, V{N}} where N vs. B = Tuple{V{Tuple{N1,N1}}, V{N2}} where {N1, N2}, where before this change A <: B, while it seems that afterwards it's presumably not considering N1 to be in invariant position anymore.

[JeffBezanson]

Yeah, that's one of the trickiest cases. It's hard to say what we even want Ref{Tuple{T,T}} where T to mean. I tried making that T concrete but maybe it can't be.

[vtjnash]

Right. It's not obvious whether that only means the values for T are required to be the same (because the Tuple itself is invariant), or whether for soundness it must mean that T is also concrete. I'm leaning towards the former interpretation, but maybe it's better if it is the latter (as done here).

[vtjnash]

Note: I think that case is described in #26175 (comment), where we see it's currently undecidable given Tuple{T, T} where T whether Tuple{R, R} (for any value of R except Union{}) is a proper subtype of it, and is therefore also partly the motivation for #24614 (and this PR).

[JeffBezanson]

Ok, I may have found a version of this that fixes #31824 too, without needing to change any convert methods and without changing that case from GeometryTypes 🎉 .

[JeffBezanson]

@nanosoldier runtests(ALL, vs = ":master")

[maleadt]

I did a bunch of Nanosoldier development changing the report repo layout, so I had to restart your job:
@nanosoldier runtests(ALL, vs = ":master")

[nanosoldier]

Your test job has completed - possible new issues were detected. A full report can be found here. cc @maleadt

[JeffBezanson]

Hmm, the correct commit is listed at the top of the report, but in the versioninfo output in the logs it looks like it actually ran with the wrong commit. And indeed GeometryTypes passes locally but failed with the same error in the pkgeval run.

[maleadt]

Hmm, the correct commit is listed at the top of the report, but in the versioninfo output in the logs it looks like it actually ran with the wrong commit.

The commit is always going to be different, because we build the merge commit (which doesn't really exist) instead of the head of this pull request. It's possible that some caching is happening though, and that the previous merge commit was used. I'll have a look.

EDIT: yes, that's what happening:

julia> repo = NewPkgEval.get_repo("JuliaLang/julia")
LibGit2.GitRepo("/home/maleadt/.julia/packages/NewPkgEval/JClmG/deps/downloads/JuliaLang/julia")

julia> NewPkgEval.get_repo_commit(repo, "pull/34272/merge")
("refs/remotes/origin/pull/34272/merge", Git Commit:
Commit Author: Name: Jeff Bezanson, Email: jeff.bezanson@gmail.com, Time: 2020-01-06 01:02:36-05:00
Committer: Name: GitHub, Email: noreply@github.com, Time: 2020-01-06 01:02:36-05:00
SHA: dfcba8768f7c22fc8bee10bd7562f4e1eb27766e
Message:
Merge 49623bd294a505b2cafc4e1eedd4a51e2bda3efc into 18783434e9c0e48152b99d5e3717deb8711f492b
)

[maleadt]

Trying a more fine-grained run first, as suggested by the report (the system is now busy doing releast-1.4 builds):

@nanosoldier runtests(["ClusteringGA", "CurrenciesBase", "FameSVD", "GeometryTypes", "Graph500", "HTTP", "IncrementalInference", "JuliaDB", "KissMCMC", "LCMCore", "Lerche", "LoopThrottle", "MbedTLS", "Mocking", "OhMyREPL", "RigidBodyDynamics", "Sched", "StanMamba"], vs = ":master")

[nanosoldier]

Your test job has completed - possible new issues were detected. A full report can be found here. cc @maleadt

[JeffBezanson]

Ok, IncrementalInference and Sched failures seem to be intermittent, and MbedTLS also crashes on other branches, so this looks OK so far.

[JeffBezanson]

@nanosoldier runtests(ALL, vs = ":master")

[nanosoldier]

Your test job has completed - possible new issues were detected. A full report can be found here. cc @maleadt

[JeffBezanson]

Starting to look good! QuantumAlgebra has an unreachable instruction to investigate; that's about it.

[JeffBezanson]

Fixed! Also double-checked that all the other failing packages pass locally.
This is now ready to merge 🎉 🎉

[martinholters]

I'm confused...

julia> Type{Tuple{T, Any} where T} <: Type{Tuple{Any,Any}} <: Type{Tuple{T, T}} where T
true

julia> Type{Tuple{T, Any} where T} <:                         Type{Tuple{T, T}} where T
false

In fact, we have

julia> Type{Tuple{T, Any} where T} == Type{Tuple{Any,Any}}
true

which looks right to me, as (Tuple{T, Any} where T) == Tuple{Any,Any}, noting the covariance of T here. But Type{Tuple{Any,Any}} <: Type{Tuple{T, T}} where T also looks fine: LHS and RHS become equal for one specific choice of T on the RHS, namely Any. And T does not fall under the diagonal rule. (It would if the where were inside the Type{}.) So that would leave me with the conclusion that Type{Tuple{T, Any} where T} <: Type{Tuple{T, T}} where T should hold. Yet here, we test for the opposite. Wrong test or wrong reasoning? (Note that as it is now, this violates transitivity, so there must be a bug somewhere.)

[martinholters]

The same violation of transitivity can be observed for this simpler case, BTW:

julia> Ref{Tuple{T} where T} <: Ref{Tuple{Any}} <: Ref{Tuple{T}} where T
true

julia> Ref{Tuple{T} where T} <:                    Ref{Tuple{T}} where T
false

I'd guess we have an issue for this somewhere, but couldn't find one at a quick glance.