From 396f5d128cc48e6b3dc7526fb304cc349c1e8e1f Mon Sep 17 00:00:00 2001
From: N5N3 <2642243996@qq.com>
Date: Thu, 7 Mar 2024 05:55:49 +0800
Subject: [PATCH] typeintersect: fix `UnionAll` unaliasing bug caused by
 innervars. (#53553)

typeintersect: fix `UnionAll` unaliasing bug caused by innervars.
(cherry picked from commit 56f1c8ae62c07cb940e0c4fc02d5dfac9ec73147)
---
 src/subtype.c   | 71 ++++++++++++++++++++++++++++++++++++++-----------
 test/subtype.jl | 26 ++++++++++++++++++
 2 files changed, 81 insertions(+), 16 deletions(-)

diff --git a/src/subtype.c b/src/subtype.c
index 3d874872d6b44..10d2f19ae3209 100644
--- a/src/subtype.c
+++ b/src/subtype.c
@@ -872,10 +872,20 @@ static jl_unionall_t *unalias_unionall(jl_unionall_t *u, jl_stenv_t *e)
     // in the environment, rename to get a fresh var.
     JL_GC_PUSH1(&u);
     while (btemp != NULL) {
-        if (btemp->var == u->var ||
-            // outer var can only refer to inner var if bounds changed
+        int aliased = btemp->var == u->var ||
+            // outer var can only refer to inner var if bounds changed (mainly for subtyping path)
             (btemp->lb != btemp->var->lb && jl_has_typevar(btemp->lb, u->var)) ||
-            (btemp->ub != btemp->var->ub && jl_has_typevar(btemp->ub, u->var))) {
+            (btemp->ub != btemp->var->ub && jl_has_typevar(btemp->ub, u->var));
+        if (!aliased && btemp->innervars != NULL) {
+            for (size_t i = 0; i < jl_array_len(btemp->innervars); i++) {
+                jl_tvar_t *ivar = (jl_tvar_t*)jl_array_ptr_ref(btemp->innervars, i);
+                if (ivar == u->var) {
+                    aliased = 1;
+                    break;
+                }
+            }
+        }
+        if (aliased) {
             u = jl_rename_unionall(u);
             break;
         }
@@ -2833,7 +2843,7 @@ static jl_value_t *finish_unionall(jl_value_t *res JL_MAYBE_UNROOTED, jl_varbind
 
     // I. Handle indirect innervars (make them behave like direct innervars).
     //   1) record if btemp->lb/ub has indirect innervars.
-    //   2) subtitute `vb->var` with `varval`/`varval`
+    //   2) substitute `vb->var` with `varval`/`newvar`
     //   note: We only store the innervar in the outmost `varbinding`,
     //       thus we must check all inner env to ensure the recording/subtitution
     //       is complete
@@ -2897,6 +2907,7 @@ static jl_value_t *finish_unionall(jl_value_t *res JL_MAYBE_UNROOTED, jl_varbind
                     }
                     envind++;
                 }
+                // FIXME: innervar that depend on `ivar` should also be updated.
             }
         }
     }
@@ -3012,7 +3023,8 @@ static jl_value_t *finish_unionall(jl_value_t *res JL_MAYBE_UNROOTED, jl_varbind
     }
 
     if (vb->innervars != NULL) {
-        for (size_t i = 0; i < jl_array_len(vb->innervars); i++) {
+        size_t len = jl_array_len(vb->innervars), count = 0;
+        for (size_t i = 0; i < len; i++) {
             jl_tvar_t *var = (jl_tvar_t*)jl_array_ptr_ref(vb->innervars, i);
             // the `btemp->prev` walk is only giving a sort of post-order guarantee (since we are
             // iterating 2 trees at once), so once we set `wrap`, there might remain other branches
@@ -3026,11 +3038,45 @@ static jl_value_t *finish_unionall(jl_value_t *res JL_MAYBE_UNROOTED, jl_varbind
             if (wrap) {
                 if (wrap->innervars == NULL)
                     wrap->innervars = jl_alloc_array_1d(jl_array_any_type, 0);
+                // FIXME: `var`'s dependence should also be pushed into `wrap->innervars`.
                 jl_array_ptr_1d_push(wrap->innervars, (jl_value_t*)var);
+                jl_array_ptr_set(vb->innervars, i, (jl_value_t*)NULL);
+            }
+        }
+        for (size_t i = 0; i < len; i++) {
+            jl_tvar_t *var = (jl_tvar_t*)jl_array_ptr_ref(vb->innervars, i);
+            if (var) {
+                if (count < i)
+                    jl_array_ptr_set(vb->innervars, count, (jl_value_t*)var);
+                count++;
+            }
+        }
+        if (count != len)
+            jl_array_del_end(vb->innervars, len - count);
+        if (res != jl_bottom_type) {
+            while (count > 1) {
+                int changed = 0;
+                // Now need to re-sort the vb->innervars using the partial-ordering predicate `jl_has_typevar`.
+                // If this is slow, we could possibly switch to a simpler graph sort than this triple loop, such as Tarjan's SCC.
+                // But for now we use a variant on selection sort for partial-orders.
+                for (size_t i = 0; i < count - 1; i++) {
+                    jl_tvar_t *vari = (jl_tvar_t*)jl_array_ptr_ref(vb->innervars, i);
+                    for (size_t j = i+1; j < count; j++) {
+                        jl_tvar_t *varj = (jl_tvar_t*)jl_array_ptr_ref(vb->innervars, j);
+                        if (jl_has_typevar(varj->lb, vari) || jl_has_typevar(varj->ub, vari)) {
+                            jl_array_ptr_set(vb->innervars, j, (jl_value_t*)vari);
+                            jl_array_ptr_set(vb->innervars, i, (jl_value_t*)varj);
+                            changed = 1;
+                            break;
+                        }
+                    }
+                    if (changed) break;
+                }
+                if (!changed) break;
             }
-            else if (res != jl_bottom_type) {
-                if (jl_has_typevar(res, var))
-                    res = jl_type_unionall((jl_tvar_t*)var, res);
+            for (size_t i = 0; i < count; i++) {
+                jl_tvar_t *var = (jl_tvar_t*)jl_array_ptr_ref(vb->innervars, i);
+                res = jl_type_unionall(var, res);
             }
         }
     }
@@ -3050,9 +3096,6 @@ static jl_value_t *finish_unionall(jl_value_t *res JL_MAYBE_UNROOTED, jl_varbind
 static jl_value_t *intersect_unionall_(jl_value_t *t, jl_unionall_t *u, jl_stenv_t *e, int8_t R, int param, jl_varbinding_t *vb)
 {
     jl_varbinding_t *btemp = e->vars;
-    // if the var for this unionall (based on identity) already appears somewhere
-    // in the environment, rename to get a fresh var.
-    // TODO: might need to look inside types in btemp->lb and btemp->ub
     int envsize = 0;
     while (btemp != NULL) {
         envsize++;
@@ -3060,13 +3103,9 @@ static jl_value_t *intersect_unionall_(jl_value_t *t, jl_unionall_t *u, jl_stenv
             vb->limited = 1;
             return t;
         }
-        if (btemp->var == u->var || btemp->lb == (jl_value_t*)u->var ||
-            btemp->ub == (jl_value_t*)u->var) {
-            u = jl_rename_unionall(u);
-            break;
-        }
         btemp = btemp->prev;
     }
+    u = unalias_unionall(u, e);
     JL_GC_PUSH1(&u);
     vb->var = u->var;
     e->vars = vb;
diff --git a/test/subtype.jl b/test/subtype.jl
index cd856b0b7a2ff..3245e0583cda8 100644
--- a/test/subtype.jl
+++ b/test/subtype.jl
@@ -2563,6 +2563,32 @@ let a = Tuple{Union{Nothing, Type{Pair{T1}} where T1}}
     @test !Base.has_free_typevars(typeintersect(a, b))
 end
 
+#issue 53366
+let Y = Tuple{Val{T}, Val{Val{T}}} where T
+    A = Val{Val{T}} where T
+    T = TypeVar(:T, UnionAll(A.var, Val{A.var}))
+    B = UnionAll(T, Val{T})
+    X = Tuple{A, B}
+    @testintersect(X, Y, !Union{})
+end
+
+#issue 53621 (requires assertions enabled)
+abstract type A53621{T, R, C, U} <: AbstractSet{Union{C, U}} end
+struct T53621{T, R<:Real, C, U} <: A53621{T, R, C, U} end
+let
+    U = TypeVar(:U)
+    C = TypeVar(:C)
+    T = TypeVar(:T)
+    R = TypeVar(:R)
+    CC = TypeVar(:CC, Union{C, U})
+    UU = TypeVar(:UU, Union{C, U})
+    S1 = UnionAll(T, UnionAll(R, Type{UnionAll(C, UnionAll(U, T53621{T, R, C, U}))}))
+    S2 = UnionAll(C, UnionAll(U, UnionAll(CC, UnionAll(UU, UnionAll(T, UnionAll(R, T53621{T, R, CC, UU}))))))
+    S = Tuple{S1, S2}
+    T = Tuple{Type{T53621{T, R}}, AbstractSet{T}} where {T, R}
+    @testintersect(S, T, !Union{})
+end
+
 #issue 53371
 struct T53371{A,B,C,D,E} end
 S53371{A} = Union{Int, <:A}