Skip to content

Commit

Permalink
typeintersect: fix UnionAll unaliasing bug caused by innervars. (#5…
Browse files Browse the repository at this point in the history 
…3553)

typeintersect: fix `UnionAll` unaliasing bug caused by innervars.
  • Loading branch information
@N5N3
N5N3 authored Mar 6, 2024
2 parents 6335386 + 02f27c2 commit 56f1c8a
Show file tree
Hide file tree
Showing 2 changed files with 81 additions and 16 deletions.
71 changes: 55 additions & 16 deletions src/subtype.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -881,10 +881,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;
}
Expand Down Expand Up @@ -2839,7 +2849,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) substitute `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/substitution
// is complete
Expand Down Expand Up @@ -2903,6 +2913,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.
}
}
}
Expand Down Expand Up @@ -3018,7 +3029,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_nrows(vb->innervars); i++) {
size_t len = jl_array_nrows(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
Expand All @@ -3032,11 +3044,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);
}
}
}
Expand All @@ -3056,23 +3102,16 @@ 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++;
if (envsize > 120) {
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;
Expand Down
26 changes: 26 additions & 0 deletions test/subtype.jl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2572,6 +2572,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}
Expand Down

2 comments on commit 56f1c8a

@nanosoldier
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Executing the daily package evaluation, I will reply here when finished:

@nanosoldier runtests(isdaily = true)

@nanosoldier
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The package evaluation job you requested has completed - possible new issues were detected.
The full report is available.

Please sign in to comment.