typeintersect: improve `merge_env` accuracy and avoid possible false `Union{}` #48167

N5N3 · 2023-01-07T14:36:54Z

The main motivation for this PR:

8652f26
On master, we always merge the whole env during intersect_all, which is not ideal as the output bounds would be wider than input if 2 Union decision touches different vars.
This PR tries to only merge the vars we meet in the current decision. And add the missing occurs_inv/occurs_cov merge (by max.)
68e51c6
This PR also tries to avoid some false Union{} caused by circular upbound checks. With merge_env we might form more Union upbound than before.
So I think it makes sense to ignore the invalid part in a Union.

The rest commits are added mainly to avoid stackoverflow. (As we might trigger more re-intersection with change 2.)
Test added.

N5N3 · 2023-01-07T17:11:15Z

@nanosoldier runtests()

nanosoldier · 2023-01-08T14:13:20Z

Your package evaluation job has completed - possible new issues were detected. A full report can be found here.

N5N3 · 2023-01-08T16:33:09Z

@nanosoldier runtests(["LicenseGrabber", "Clustering", "AlgebraicRewriting", "ExtensibleEffects", "RDMREopt", "Cropbox", "DataTypesBasic", "ProbNumDiffEq", "SpmImageTycoonInstaller", "KalmanFilterTools", "SurrogatesMOE", "Rocket", "TypeClasses"])

nanosoldier · 2023-01-08T17:57:28Z

Your package evaluation job has completed - possible new issues were detected. A full report can be found here.

src/subtype.c

test/subtype.jl

src/subtype.c

vtjnash · 2023-01-09T15:55:22Z

I made comments on each of the individual commits (or none, if that particular commit SGTM). This seems like a good improvement.

src/subtype.c

If we always merge the whole env, then the output bounds would be widen than input if different Union decision touch different vars. Also add missing `occurs_inv/cov`'s merge (by max).

This is not ideal. As some times circular bound seems meaningful. But at least better than `Union{}` ?

1. use `obviously_in_union` to catch more unneeded duplication in `Union`. 2. under-estimate merged `lb` in more case. (Should not affect subtype path.)

Fix JuliaLang#47874 case2

I gave up fixing the deep stack overflow. Making the `env` soundness seems much easier. close JuliaLang#47874. At present, we only catch cases with pattern like A1 = Union{T,Int} where {T} A2 = Union{T2,Int} where {T,T2<:Union{T,Int}} A3 = Union{Int,A2}

This PR reverts the optimization from 748149e (part of #48167), while keeping the fix for merging occurs_inv/occurs_cov, as that optimzation makes no sense especially when typevar occurs both inside and outside the inner intersection. Close #55206

This PR reverts the optimization from 748149e (part of #48167), while keeping the fix for merging occurs_inv/occurs_cov, as that optimzation makes no sense especially when typevar occurs both inside and outside the inner intersection. Close #55206 (cherry picked from commit fb6b790)

…Lang#55299) This PR reverts the optimization from 748149e (part of JuliaLang#48167), while keeping the fix for merging occurs_inv/occurs_cov, as that optimzation makes no sense especially when typevar occurs both inside and outside the inner intersection. Close JuliaLang#55206

N5N3 added types and dispatch Types, subtyping and method dispatch needs pkgeval Tests for all registered packages should be run with this change don't squash Don't squash merge labels Jan 7, 2023

N5N3 requested a review from vtjnash January 7, 2023 14:37

N5N3 force-pushed the inter-fix2 branch from ea3ea0e to 6e0c036 Compare January 8, 2023 07:35

N5N3 force-pushed the inter-fix2 branch from b1c5772 to e0739d3 Compare January 9, 2023 08:16