Skip to content

Commit

Permalink
typeintersect: fix incorrect innervar handling under circular env (#5…
Browse files Browse the repository at this point in the history 
…4545)

The infinite loop encountered in #54516 has been traced back to a
circular bound during `finish_unionall`.
As we insert innervar more eagerly now, the direct `jl_has_typevar`
could not find all circularity.
To address this, `has_typevar_via_flatten_env` is added to perform
thorough check.
Although there is some code duplication with `reachable_var`, it could
be improved in future refactoring.

#54516 also highlighted another free var escaping regression since
v1.10. This regression is not solely
the result of incomplete checks, it is also caused by the missing final
substitution of `vb`'s bound, which has now been corrected.

At last, this PR adds an assertion of sorting complexity, which should
facilitate the detection of similar issues by PkgEval.
 
close #54516
  • Loading branch information
@N5N3
N5N3 authored May 28, 2024
1 parent 0bf392e commit 92dfdca
Show file tree
Hide file tree
Showing 2 changed files with 108 additions and 23 deletions.
124 changes: 101 additions & 23 deletions src/subtype.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2814,6 +2814,50 @@ static jl_value_t *omit_bad_union(jl_value_t *u, jl_tvar_t *t)
return res;
}

// TODO: fuse with reachable_var?
static int has_typevar_via_flatten_env(jl_value_t *x, jl_tvar_t *t, jl_ivarbinding_t *allvars, int8_t *checked) {
if (jl_is_unionall(x)) {
jl_tvar_t *var = ((jl_unionall_t *)x)->var;
if (has_typevar_via_flatten_env(var->lb, t, allvars, checked) ||
has_typevar_via_flatten_env(var->ub, t, allvars, checked))
return 1;
return has_typevar_via_flatten_env(((jl_unionall_t *)x)->body, t, allvars, checked);
}
else if (jl_is_uniontype(x)) {
return has_typevar_via_flatten_env(((jl_uniontype_t *)x)->a, t, allvars, checked) ||
has_typevar_via_flatten_env(((jl_uniontype_t *)x)->b, t, allvars, checked);
}
else if (jl_is_vararg(x)) {
jl_vararg_t *v = (jl_vararg_t *)x;
return (v->T && has_typevar_via_flatten_env(v->T, t, allvars, checked)) ||
(v->N && has_typevar_via_flatten_env(v->N, t, allvars, checked));
}
else if (jl_is_datatype(x)) {
for (size_t i = 0; i < jl_nparams(x); i++) {
if (has_typevar_via_flatten_env(jl_tparam(x, i), t, allvars, checked))
return 1;
}
return 0;
}
else if (jl_is_typevar(x)) {
if (t == (jl_tvar_t *)x)
return 1;
size_t ind = 0;
jl_ivarbinding_t *itemp = allvars;
while (itemp && *itemp->var != (jl_tvar_t *)x)
{
ind++;
itemp = itemp->next;
}
if (itemp == NULL || checked[ind])
return 0;
checked[ind] = 1;
return has_typevar_via_flatten_env(*itemp->lb, t, allvars, checked) ||
has_typevar_via_flatten_env(*itemp->ub, t, allvars, checked);
}
return 0;
}

// Caller might not have rooted `res`
static jl_value_t *finish_unionall(jl_value_t *res JL_MAYBE_UNROOTED, jl_varbinding_t *vb, jl_unionall_t *u, jl_stenv_t *e)
{
Expand Down Expand Up @@ -2911,8 +2955,11 @@ static jl_value_t *finish_unionall(jl_value_t *res JL_MAYBE_UNROOTED, jl_varbind
// remove/replace/rewrap free occurrences of this var in the environment
int wrapped = 0;
jl_ivarbinding_t *pwrap = NULL;
int vcount = icount + current_env_length(e);
int8_t *checked = (int8_t *)alloca(vcount);
for (jl_ivarbinding_t *btemp = allvars, *pbtemp = NULL; btemp != NULL; btemp = btemp->next) {
int bdepth0 = btemp->root->depth0;
int innerflag = 0;
ivar = *btemp->var;
ilb = *btemp->lb;
iub = *btemp->ub;
Expand All @@ -2934,18 +2981,9 @@ static jl_value_t *finish_unionall(jl_value_t *res JL_MAYBE_UNROOTED, jl_varbind
else if (ilb == (jl_value_t*)vb->var) {
*btemp->lb = vb->lb;
}
else if (bdepth0 == vb->depth0 && !jl_has_typevar(vb->lb, ivar) && !jl_has_typevar(vb->ub, ivar)) {
// if our variable is T, and some outer variable has constraint S = Ref{T},
// move the `where T` outside `where S` instead of putting it here. issue #21243.
if (newvar != vb->var)
*btemp->lb = jl_substitute_var(ilb, vb->var, (jl_value_t*)newvar);
if (!wrapped) pwrap = pbtemp;
wrapped = 1;
}
else {
*btemp->lb = jl_new_struct(jl_unionall_type, vb->var, ilb);
innerflag |= 1;
}
assert((jl_value_t*)ivar != *btemp->lb);
}
if (jl_has_typevar(iub, vb->var)) {
assert(btemp->root->var == ivar || bdepth0 == vb->depth0);
Expand All @@ -2971,14 +3009,35 @@ static jl_value_t *finish_unionall(jl_value_t *res JL_MAYBE_UNROOTED, jl_varbind
// Tuple{Float64, T3, T4} where {S3, T3<:Tuple{S3}, T4<:S3}
*btemp->ub = vb->ub;
}
else if (bdepth0 == vb->depth0 && !jl_has_typevar(vb->lb, ivar) && !jl_has_typevar(vb->ub, ivar)) {
if (newvar != vb->var)
*btemp->ub = jl_substitute_var(iub, vb->var, (jl_value_t*)newvar);
if (!wrapped) pwrap = pbtemp;
wrapped = 1;
else {
innerflag |= 2;
}
else
*btemp->ub = jl_new_struct(jl_unionall_type, vb->var, iub);
if (innerflag) {
memset(checked, 0, vcount);
if (bdepth0 != vb->depth0 ||
has_typevar_via_flatten_env(vb->lb, ivar, allvars, checked) ||
has_typevar_via_flatten_env(vb->ub, ivar, allvars, checked)) {
if (innerflag & 1)
*btemp->lb = jl_new_struct(jl_unionall_type, vb->var, ilb);
if (innerflag & 2)
*btemp->ub = jl_new_struct(jl_unionall_type, vb->var, iub);
}
else {
assert(btemp->root != vb);
// if our variable is T, and some outer variable has constraint S = Ref{T},
// move the `where T` outside `where S` instead of putting it here. issue #21243.
if (newvar != vb->var) {
if (innerflag & 1)
*btemp->lb = jl_substitute_var(ilb, vb->var, (jl_value_t*)newvar);
if (innerflag & 2)
*btemp->ub = jl_substitute_var(iub, vb->var, (jl_value_t*)newvar);
}
if (!wrapped)
pwrap = pbtemp;
wrapped = 1;
}
}
assert((jl_value_t*)ivar != *btemp->lb);
assert((jl_value_t*)ivar != *btemp->ub);
}
pbtemp = btemp;
Expand All @@ -2997,24 +3056,31 @@ static jl_value_t *finish_unionall(jl_value_t *res JL_MAYBE_UNROOTED, jl_varbind
pwrap->next = inew;
else
allvars = inew;
vcount++;
}

// Re-sort the innervar inside the (reversed) var list.
// `jl_has_typevar` is used as the partial-ordering predicate.
// If this is slow, we could possibly switch to a simpler graph sort, such as Tarjan's SCC.
if (icount > 0) {
jl_ivarbinding_t *pib1 = NULL;
#ifndef NDEBUG
size_t sort_count = 0;
#endif
while (1) {
jl_ivarbinding_t *ib1 = pib1 == NULL ? allvars : pib1->next;
if (ib1 == NULL) break;
if (jl_has_free_typevars(*ib1->lb) || jl_has_free_typevars(*ib1->ub)) {
assert((++sort_count) <= (vcount * (vcount + 1)) >> 1);
int lbfree = jl_has_free_typevars(*ib1->lb);
int ubfree = jl_has_free_typevars(*ib1->ub);
if (lbfree || ubfree) {
int changed = 0;
jl_ivarbinding_t *pib2 = ib1, *ib2 = ib1->next;
while (ib2 != NULL) {
int isinnervar = ib2->root->var != *ib2->var;
if (isinnervar && ib1->root->depth0 == ib2->root->depth0 &&
(jl_has_typevar(*ib1->lb, *ib2->var) ||
jl_has_typevar(*ib1->ub, *ib2->var))) {
((lbfree && jl_has_typevar(*ib1->lb, *ib2->var)) ||
(ubfree && jl_has_typevar(*ib1->ub, *ib2->var)))) {
pib2->next = ib2->next;
ib2->next = ib1;
ib2->root = ib1->root;
Expand Down Expand Up @@ -3052,6 +3118,13 @@ static jl_value_t *finish_unionall(jl_value_t *res JL_MAYBE_UNROOTED, jl_varbind
if (jl_has_typevar(iub, ivar))
*btemp2->ub = jl_substitute_var(iub, ivar, nivar);
}
if (!wrapped && !varval) {
// newvar also needs bounds substitution.
if (jl_has_typevar(vb->lb, ivar))
vb->lb = jl_substitute_var(vb->lb, ivar, nivar);
if (jl_has_typevar(vb->ub, ivar))
vb->ub = jl_substitute_var(vb->ub, ivar, nivar);
}
*btemp1->var = (jl_tvar_t *)nivar;
}
}
Expand All @@ -3067,11 +3140,13 @@ static jl_value_t *finish_unionall(jl_value_t *res JL_MAYBE_UNROOTED, jl_varbind
}
if (root != vb) icount--;
if (root->innervars != NULL) {
size_t len = jl_array_nrows(root->innervars);
jl_array_t *rinnervars = root->innervars;
JL_GC_PROMISE_ROOTED(rinnervars);
size_t len = jl_array_nrows(rinnervars);
if (icount > len)
jl_array_grow_end(root->innervars, icount - len);
jl_array_grow_end(rinnervars, icount - len);
if (icount < len)
jl_array_del_end(root->innervars, len - icount);
jl_array_del_end(rinnervars, len - icount);
}
else if (icount > 0) {
root->innervars = jl_alloc_array_1d(jl_array_any_type, icount);
Expand Down Expand Up @@ -3104,6 +3179,9 @@ static jl_value_t *finish_unionall(jl_value_t *res JL_MAYBE_UNROOTED, jl_varbind
res = jl_bottom_type;
}
else {
// re-fresh newvar if bounds changed.
if (vb->lb != newvar->lb || vb->ub != newvar->ub)
newvar = jl_new_typevar(newvar->name, vb->lb, vb->ub);
if (newvar != vb->var)
res = jl_substitute_var(res, vb->var, (jl_value_t*)newvar);
varval = (jl_value_t*)newvar;
Expand Down
7 changes: 7 additions & 0 deletions test/subtype.jl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2650,3 +2650,10 @@ let S = Tuple{Val, Val{T}} where {T}, R = Tuple{Val{Val{T}}, Val{T}} where {T},
@testintersect(Tuple{Val{A}, A} where {B, A<:Union{Val{B}, GenericMemory{B}}}, S{1}, R{1})
@testintersect(Tuple{Val{A}, A} where {B, A<:Union{Val{B}, GenericMemory{:not_atomic,Int,B}}}, S{1}, R{1})
end

#issue 54516
let S = Tuple{Val{<:T}, Union{Int,T}} where {T},
T = Tuple{Union{Int,T}, Val{<:T}} where {T}
@testintersect(S, T, !Union{})
@test !Base.has_free_typevars(typeintersect(S, T))
end

0 comments on commit 92dfdca

Please sign in to comment.