fix another case where we might return free TypeVar #48228
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Adding too many inner-vars might not be a good idea. |
vtjnash
added
the
needs pkgeval
|
Upon review, I think we should attempt to go ahead with this now. It turns out the intersection in the tests that stackoverflows, would have instead become incorrect if it was simplified: In particular, I believe |
|
@nanosoldier |
I think there have been some discussion for this pattern. |
|
Your package evaluation job has completed - possible new issues were detected. |
|
I see that did quite poorly |
vtjnash
removed
the
needs pkgeval
This comment was marked as resolved.
This comment was marked as resolved.
|
Your package evaluation job has completed - possible new issues were detected. |
|
Looks like most of the problem get fixed. |
|
Yes, looks like we are down to 2 new internal failures, and 3 fixed subtyping results (causing ambiguity tests to fail) |
|
I think we are starting to converge here towards something usable. It looks like one of N5N3's tests successfully demonstrated why the innervars update was needed (which I had previously commented out, in the absence of a test case for it). The failing package test inference looks something like: (gdb) p jl_(a)
Tuple{typeof(Phylo.API._getnode), T, N} where T<:(Phylo.AbstractTree{Phylo.OneTree, RT, NL, N, B} where B<:Phylo.AbstractBranch{RT, NL}) where N where NL where RT
(gdb) p (void*) jl_call2((void*) jl_get_global(jl_base_module, (void*)jl_symbol("show")), (void*) jl_get_global(jl_core_module, (void*)jl_symbol("stdout")), types)
Tuple{typeof(Phylo.API._getnode), Union{Union{LinkTree{RT3, NL, N, B} where {RT2<:Union{RT, RT1}, N<:Union{LinkNode{RT, NL, Data, B} where {Data, B<:Phylo.AbstractBranch{RT, NL}}, LinkNode{RT2, NL, Data, B} where {Data, B<:Phylo.AbstractBranch{RT2, NL}}, LinkNode{RT1, NL, Data, B} where {Data, B<:Phylo.AbstractBranch{RT1, NL}}}, B<:Union{LinkBranch{RT, NL}, LinkBranch{RT2, NL}, LinkBranch{RT1, NL}}, RT3<:Union{RT, RT2, RT1}}, LinkTree{var"#s63", NL, N, B} where {var"#s63"<:Union{RT, RT1}, N<:Union{LinkNode{RT, NL, Data, B} where {Data, B<:Phylo.AbstractBranch{RT, NL}}, LinkNode{var"#s63", NL, Data, B} where {Data, B<:Phylo.AbstractBranch{var"#s63", NL}}, LinkNode{RT1, NL, Data, B} where {Data, B<:Phylo.AbstractBranch{RT1, NL}}}, B<:Union{LinkBranch{RT, NL}, LinkBranch{var"#s63", NL}, LinkBranch{RT1, NL}}}} where {RT<:Phylo.Rooted, RT1<:Phylo.Rooted}, Union{LinkTree{RT2, NL, N, B} where {RT2<:Union{Phylo.Rooted, RT1, RT}, N<:Union{LinkNode{RT1, NL, Data, B} where {Data, B<:Phylo.AbstractBranch{RT1, NL}}, LinkNode{RT, NL, Data, B} where {Data, B<:Phylo.AbstractBranch{RT, NL}}, LinkNode{RT2, NL, Data, B} where {Data, B<:Phylo.AbstractBranch{RT2, NL}}}, B<:Union{LinkBranch{RT1, NL}, LinkBranch{RT, NL}, LinkBranch{RT2, NL}}}, LinkTree{var"#s63", NL, N, B} where {var"#s63"<:Union{Phylo.Rooted, RT1, RT}, N<:Union{LinkNode{RT1, NL, Data, B} where {Data, B<:Phylo.AbstractBranch{RT1, NL}}, LinkNode{RT, NL, Data, B} where {Data, B<:Phylo.AbstractBranch{RT, NL}}, LinkNode{var"#s63", NL, Data, B} where {Data, B<:Phylo.AbstractBranch{var"#s63", NL}}}, B<:Union{LinkBranch{RT1, NL}, LinkBranch{RT, NL}, LinkBranch{var"#s63", NL}}}} where {RT<:Phylo.Rooted, RT1<:Phylo.Rooted}, Union{LinkTree{var"#s65", NL, N, B} where {var"#s65"<:Union{Phylo.Rooted, RT1, RT}, N<:Union{LinkNode{RT1, NL, Data, B} where {Data, B<:Phylo.AbstractBranch{RT1, NL}}, LinkNode{RT, NL, Data, B} where {Data, B<:Phylo.AbstractBranch{RT, NL}}, LinkNode{var"#s65", NL, Data, B} where {Data, B<:Phylo.AbstractBranch{var"#s65", NL}}}, B<:Union{LinkBranch{RT1, NL}, LinkBranch{RT, NL}, LinkBranch{var"#s65", NL}}}, LinkTree{var"#s63", NL, N, B} where {var"#s63"<:Union{Phylo.Rooted, RT1, RT}, N<:Union{LinkNode{RT1, NL, Data, B} where {Data, B<:Phylo.AbstractBranch{RT1, NL}}, LinkNode{RT, NL, Data, B} where {Data, B<:Phylo.AbstractBranch{RT, NL}}, LinkNode{var"#s63", NL, Data, B} where {Data, B<:Phylo.AbstractBranch{var"#s63", NL}}}, B<:Union{LinkBranch{RT1, NL}, LinkBranch{RT, NL}, LinkBranch{var"#s63", NL}}}} where {RT<:Phylo.Rooted, RT1<:Phylo.Rooted}} where NL<:Missing, Missing}
caused by inference of
branchroute(Phylo.LinkTree{Phylo.OneRoot, String, Phylo.LinkNode{Phylo.OneRoot, String, Base.Dict{String, Any}, Phylo.LinkBranch{Phylo.OneRoot, String, Base.Dict{String, Any}, Float64}}, Phylo.LinkBranch{Phylo.OneRoot, String, Base.Dict{String, Any}, Float64}, Base.Dict{String, Any}}, String, String)
from branchroute(T, N, N) where {RT, N, T<:(Phylo.AbstractTree{Phylo.OneTree, RT, NL, N, B} where B<:Phylo.AbstractBranch{RT, NL} where N<:Phylo.AbstractNode{RT, NL} where NL)}where we eventually get stuck (recurse infinitely) on something like the intersection of these values: (Aside: it also makes me feel very uncomfortable that inference is producing that mess in the first place also. This might end up being worked around by #49167, or something like that PR.) |
N5N3
reviewed
Apr 5, 2023
src/subtype.c
Outdated
| // 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 | ||
| // of the type walk that now may have incomplete bounds: finish those now too | ||
| jl_varbinding_t *btemp = e->vars; |
There was a problem hiding this comment.
Perhaps we still need to make sure there no duplicated innervar in the env (just like the jl_varbinding_t *wrap = NULL; trick above)?
And the TODO seems get resolved?
There was a problem hiding this comment.
And perhaps we'd better not wrap res if we update the innervars?
There was a problem hiding this comment.
From my original analysis, I didn't expect that would be possible. There are only 2 sides to the type tree, so either wrapping to the left or wrapping to the right should be needed, but not more than that. With an attempt of asserting for this case, I don't think it is hit either, but the code rearrangement probably makes sense anyways to be more straightforward. I believe a variable would have to switch sides multiple times by combinations to get tricky here, and it doesn't seem like that happens in existing code.
|
Further call stack analysis, which can be simply by plugging this into |
vtjnash
force-pushed
the
jn/47476more3
branch
2 times, most recently
from
68f1c50
to
67e2297
Compare
This comment was marked as resolved.
This comment was marked as resolved.
|
Your package evaluation job has completed - possible new issues were detected. |
|
@N5N3 Phylo is a deterministic crash caused by omit_bad_union. In particular, the bound of one of the vars is being set to NULL here: But I don't think the code expects these to ever be NULL, so a later call to diff --git a/src/subtype.c b/src/subtype.c
index 7373db4e01..38b07db8d6 100644
--- a/src/subtype.c
+++ b/src/subtype.c
@@ -2834,6 +2834,7 @@ static jl_value_t *finish_unionall(jl_value_t *res JL_MAYBE_UNROOTED, jl_varbind
if (vb->ub == (jl_value_t*)btemp->var) {
btemp->ub = omit_bad_union(btemp->ub, vb->var);
if (btemp->ub == NULL) {
+ btemp->ub = jl_bottom_type;
JL_GC_POP();
return jl_bottom_type;
}
@@ -2998,7 +2999,7 @@ static jl_value_t *intersect_unionall_(jl_value_t *t, jl_unionall_t *u, jl_stenv
vb->ub = omit_bad_union(vb->ub, u->var);
JL_GC_POP();
if (vb->ub == NULL)
- res = jl_bottom_type;
+ res = vb->ub = jl_bottom_type;
}
}
if (res != jl_bottom_type)The types under comparison are deceptively simple looking Though their intersection is far from simple, with that patch (I suspect these are not real): I see the constraints that |
|
I thought setting julia> typeintersect(Tuple{Type{T}, T, Ref{T}} where T,
Tuple{Type{S}, Ref{S}, S} where S)
Tuple{Type{Union{}}, Union{}, Union{}}But I think there's no need to change the test as #49111 would "fix" it. Edit1: The bad result before Union{Tuple{T, NL} where {NL, N<:Phylo.AbstractNode{RT, NL}, T<:(AbstractTree{OneTree, RT, NL, N, B} where B<:Phylo.AbstractBranch{RT, NL})}, Tuple{T, Union{}} where {N<:Phylo.AbstractNode{RT, Union{}}, T<:(AbstractTree{OneTree, RT, Union{}, N, B} where B<:Phylo.AbstractBranch{RT, Union{}})}} where RTand |
We had an environment here that looked like while computing the upper bound for J{S} where S:
where S=T
where T
where I{T}
where J{S} where S
Then we started handling those, and filling in the values:
First replacing S with T, which creates a `res` of `J{T}`
where T
where I{T}
where J{S} where S
Then we handled T, which is also going to set `wrap=0`, so our result
for `J{T}` will not be made into `J{T} where T`.
where I{T} (wrap 0)
where J{S} where S
Here we then had finished handling all the dependencies for J{S} where
S, which resulted in an upper bound assignment of J{T}
where I{T}
where J{T}
Next, we handle I{T}, though it is now unused, so while we will make `I{T}
where T` (via innervars) here for it, this goes unuesd.
And finally, we had our resulting clause:
where J{T}
But it is missing the `where T`, since `I` (from lhs) was discarded.
Thus we need to add that back now, when handling some innervars, if we
see our term got duplicated to a higher part of the bounds before
reaching this handling for its placement movement.
Should also fix some stackoverflow in PkgEval
Had an issue where (S<:T<:S with innervar N) would add the (where N) to
each S instead of the whole expression, resulting to creation of
(Val{. where N}) instead of (Val{.} where N) later.
I think this might be NFC, since I don't think this can simultaneously affect the left and right envs, but it looks better this way.
Correct test for some other improvement also now on master.
github-merge-queue bot
pushed a commit
that referenced
this pull request
Jul 16, 2023
On master, we only record direct `innervars` (`T` -> `S<:Val{T}`).
And chained `innervars` might be ignored (`T` -> `S<:Val{V<:T}`.
Before #48228, those chained
`innervars` would have been wrapped into an `UnionAll`, thus we just
need to check outer vars' lb/ub.
Test added.
~Note: this only fix
#50456 (comment),
the other MWE still get stackoverflow.~
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Still one error caused here (this intersection is not satisfiable except by
Union{})Original PR text:
We had an environment here that looked like while computing the upper bound for J{S} where S:
where S=T
where T
where I{T}
where J{S} where S
Then we started handling those, and filling in the values:
First replacing S with T, which creates a
resofJ{T}where T
where I{T}
where J{S} where S
Then we handled T, which is also going to set
wrap=0, so our result forJ{T}will not be made intoJ{T} where T.where I{T} (wrap 0)
where J{S} where S
Here we then had finished handling all the dependencies for J{S} where S, which resulted in an upper bound assignment of J{T}
where I{T}
where J{T}
Next, we handle I{T}, though it is now unused, so while we will make
I{T} where T(via innervars) here for it, this goes unuesd.And finally, we had our resulting clause:
where J{T}
But it is missing the
where T, sinceI(from lhs) was discarded.Thus we need to add that back now, when handling some innervars, if we see our term got duplicated to a higher part of the bounds before reaching this handling for its placement movement.