Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix typeintersect bug reported in #36443 #46446

Merged
merged 9 commits into from
Sep 2, 2022
Merged

Fix typeintersect bug reported in #36443 #46446

merged 9 commits into from
Sep 2, 2022

Conversation

N5N3
Copy link
Member

@N5N3 N5N3 commented Aug 23, 2022 •
edited
Loading

  1. On master, we use a offset field to recode the valid length difference between 2 Varargs.
    It is set outside intersect_varargs, so when we check the Vararg's eltype, the intersection result would be influenced when we have e.g. Vararg{Val{N},N}.
    The first commit move the offset setting into intersect_varargs to fix the problem.
  2. On master, set_var_to_const will ignore the offset if the typevar has been settled, thus gives many false Union{}.
    The second commit fix it.
  3. At present, when we intersect 2 Varargs length, we always return the length on the right. Which cause many order dependent error. In fact we should always return the shorter Vararg's length, as the extra elements has been consumed within intersect_tuple.
  4. At present, when we intersect 2 unbounded Varargs length, we always set them equal, which is not correct if offset != 0. The 4th commit fix it (Not all, as some Vararg has Null length, which causes Unreachable after Varargs change #39088)

close #36443, close #37257. Test added.

@N5N3 N5N3 added the types and dispatch Types, subtyping and method dispatch label Aug 23, 2022
@N5N3 N5N3 force-pushed the Fix-36443 branch 17 times, most recently from e23f9e5 to 9e1e4ed Compare August 24, 2022 08:08
@N5N3

This comment was marked as outdated.

Sign in to view
@N5N3 N5N3 force-pushed the Fix-36443 branch 2 times, most recently from 9d30a62 to 851662d Compare August 24, 2022 15:42
@Keno Keno requested a review from JeffBezanson August 24, 2022 19:51
vtjnash
vtjnash reviewed Aug 31, 2022
View reviewed changes
src/subtype.c Outdated
@@ -2877,7 +2877,7 @@ static jl_value_t *intersect_invariant(jl_value_t *x, jl_value_t *y, jl_stenv_t
jl_value_t *ii = intersect(x, y, e, 2);
e->invdepth--;
e->Rinvdepth--;
if (jl_is_typevar(x) && jl_is_typevar(y) && (jl_is_typevar(ii) || !jl_is_type(ii)))
if ((jl_is_typevar(x) || jl_is_typevar(y)) && (jl_is_typevar(ii) || !jl_is_type(ii)))
Copy link
Member

Choose a reason for hiding this comment

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

can you comment on this line change? I don't see directly see how it was related to the commit message

Copy link
Member Author

@N5N3 N5N3 Aug 31, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

IIRC, this was added as the subtype check below would ignore the offset within var_gt.
Some tests added in this PR need this change to pass. (I gave up to change var_gt.)

But yes, maybe we should not widen this branch that much. I'll try to tighten it and split this change into a seperate commit.

vtjnash
vtjnash reviewed Aug 31, 2022
View reviewed changes
src/subtype.c Show resolved Hide resolved
vtjnash
vtjnash reviewed Aug 31, 2022
View reviewed changes
src/subtype.c Outdated
Comment on lines 2170 to 2173
v = jl_box_long(iv + offset);
bb->lb = bb->ub = v;
// Here we always return the shorter `Vararg`'s length.
if (offset > 0) return jl_box_long(iv);
Copy link
Member

@vtjnash vtjnash Aug 31, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Suggested change
v = jl_box_long(iv + offset);
bb->lb = bb->ub = v;
// Here we always return the shorter `Vararg`'s length.
if (offset > 0) return jl_box_long(iv);
bb->lb = bb->ub = jl_box_long(iv + offset);
// Here we always return the shorter `Vararg`'s length.
if (offset < 0)
v = bb->lb;

Copy link
Member Author

Choose a reason for hiding this comment

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

looks like gc test dislike this change. Revert for now.

vtjnash
vtjnash reviewed Aug 31, 2022
View reviewed changes
src/subtype.c Outdated Show resolved Hide resolved
vtjnash
vtjnash reviewed Aug 31, 2022
View reviewed changes
src/subtype.c Show resolved Hide resolved
vtjnash
vtjnash reviewed Aug 31, 2022
View reviewed changes
src/subtype.c Outdated Show resolved Hide resolved
vtjnash
vtjnash reviewed Aug 31, 2022
View reviewed changes
test/subtype.jl Outdated Show resolved Hide resolved
vtjnash
vtjnash reviewed Aug 31, 2022
View reviewed changes
test/subtype.jl Outdated Show resolved Hide resolved
@vtjnash
Copy link
Member

vtjnash commented Aug 31, 2022

Great work! I had a few minor nits that I commented about, but otherwise looks good. Let's see how it does:

@nanosoldier runtests(ALL, vs=":master")

@nanosoldier
Copy link
Collaborator

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

@vtjnash
Copy link
Member

vtjnash commented Aug 31, 2022 •
edited
Loading

Good to see there are no pkgeval failures that appear related. Should be good to merge once you make the clang-sagc-subtype test happy

N5N3 and others added 9 commits September 1, 2022 08:30
@N5N3
Avoid setting offset when we intersect Vararg's eltype.
01a4a30 
`var->offset` is used to recode the length difference of 2 `Vararg`s.
But `Vararg`'s length might also be used in the type field, e.g. `Tuple{Vararg{Val{N}, N}} where {N}`, where
we should ignore `offset` when we intersect `Val{N}`.

This commit move the offset setting/erasing into `intersect_varargs`.
@N5N3 @vtjnash
Skip subtype check if intersect_invariant calls set_vat_to_const.
e6d2624 
Co-Authored-By: Jameson Nash <vtjnash+github@gmail.com>
@N5N3
Add tuple length offset when we intersect 2 Vararg's length.
f29a013
@N5N3 @vtjnash
Always return the shorter Vararg length.
38829a0 
The type `var` might be switched during intersection. Thus previous result looks order dependent.

When we intersect 2 `Vararg`s' length, we should always return the shorter one.
As we has consumed the extra elements in `intersect_tuple`.

Also fix JuliaLang#37257

Co-Authored-By: Jameson Nash <vtjnash+github@gmail.com>
@N5N3
Avoid set var's bounds if offset != 0
b7f47d5 
if `offset != 0`, then these 2 var should have different value, thus it's invalid to set bounds.
@N5N3
subtype: add tests for Vararg expansion.
a6a5f00 
Ref JuliaLang#36443
Ref JuliaLang#37257
@N5N3
Make bound_var_below return NULL if the input typevar is not valid.
1268582 
If offset > 0, the correct result is `var - offset` if expressible.

So an unbounded typevar should not be returned in this case as it might be a diagonal var.

Since the result could be improved if  `N` get fixed, set `intvalued` to 2 as a re-intersection hint.
@N5N3 @vtjnash
Avoid set var's lb if intersect return a Vararg with free length. (Nu…
2abedf6 
…ll or a local type var)

But `check_unsat_bound` should not be skipped.

Co-Authored-By: Jameson Nash <vtjnash+github@gmail.com>
@N5N3
subtyping: add tests for various Vararg bounds bugs
4654cfa 
Refs JuliaLang#39088
Refs JuliaLang#39098
@N5N3 N5N3 merged commit 852e313 into JuliaLang:master Sep 2, 2022
@N5N3 N5N3 deleted the Fix-36443 branch September 2, 2022 01:01
This was referenced Sep 2, 2022
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vtjnash vtjnash vtjnash left review comments

@JeffBezanson JeffBezanson Awaiting requested review from JeffBezanson

Assignees
No one assigned
Labels
types and dispatch Types, subtyping and method dispatch
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Wrong Vararg expansion in type intersection Unreachable reached (1.5.0-beta1 & 1.4.0)
3 participants
@N5N3 @vtjnash @nanosoldier