-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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.
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
fix #31899, type intersection involving Int in upper bound #31960
Conversation
@@ -1779,7 +1781,10 @@ static jl_value_t *finish_unionall(jl_value_t *res JL_MAYBE_UNROOTED, jl_varbind | |||
jl_tvar_t *newvar = vb->var; | |||
JL_GC_PUSH2(&res, &newvar); | |||
// try to reduce var to a single value | |||
if (obviously_egal(vb->lb, vb->ub)) { | |||
if (jl_is_long(vb->ub) && jl_is_typevar(vb->lb)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
jl_is_typevar(vb->lb)
IIUC, doesn't this also need checks that vb->ub <: jl_any_type
&& vb->lb >: Union{}
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that would be more precise, I just wanted to be conservative here and also not complicate the code too much.
src/subtype.c
Outdated
@@ -961,6 +961,8 @@ static int subtype(jl_value_t *x, jl_value_t *y, jl_stenv_t *e, int param) | |||
if (yy) record_var_occurrence(yy, e, param); | |||
if (yr) { | |||
if (xx) record_var_occurrence(xx, e, param); | |||
if (xx->lb == jl_bottom_type) | |||
return 1; // allow Bottom <: non-type | |||
return subtype(xx->lb, yy->ub, e, 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would have thought subtype
should already handle lhs == Union{}
, instead of needing to add a fast-path here to catch some cases of it just for the egal case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it should be ok to change that. I thought there might be some reason we don't have that general fast path, but I'm not sure what it would be.
No description provided.