-
-
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 problems with TypeVars in intersection and type_match_ #11194
Conversation
The second commit fixes #8652 |
...whoops, no it doesn't (or rather, it breaks other stuff). It made it through I'll discuss more in #8652. |
deb0fb2
to
2e26314
Compare
OK, this one should work (it passes locally). Interestingly, it also fixes #8045. |
Hmm, the PR (not the push) has now failed twice on the |
It is possible that there's a difference — the PR build merges the branch into master. You could also try rebasing to see if the push then fails, too. |
I just did an up-to-the-minute rebase, and it passed locally (but this was 64-bit, which also passed on Travis). |
The CI problems seem to have been fixed. Nice! |
Of my various open PRs, now that Jeff seems back (already!) I'm going to bump this particular one. It's short, I think it's one of the last barriers to merging static compilation, and Jeff seems likely to be the only suitable reviewer. |
57a47b3
to
f9d0d83
Compare
Hah, but I just discovered that a small addition also fixes #11367. |
Thanks @timholy, amazing work, as always! |
if (jl_is_typevar(b)) { | ||
for(i=0; i < penv->n; i+=2) { | ||
if (penv->data[i] == b && !jl_is_typevar(penv->data[i+1])) { | ||
if (jl_types_equal((jl_value_t*)a, penv->data[i+1])) { |
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.
This seems very odd to me, since a
is a typevar. I would think jl_types_equal
would always return false in this 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.
You're right, it probably can be deleted. I'll check it out.
Putting us all to shame --- just try to find a neuroscientist who knows as much about type systems! :) |
I'm just trying to keep up with @simonster in doing our clan proud. Besides, #11242 was type-system boot camp, in every respect except shaving my head (which is good, since there is so little to shave). |
…8652) In jl_args_morespecific(a,b), one of the important tests is to call type_match_(b,a), essentially reasoning that if b <: a then a cannot be more specific than b. The problem is that this test can fail for an independent reason, like invariance. This takes the drastic step of keeping invariance=false for the type_match_ check in jl_args_morespecific. Surprisingly, this doesn't appear to break anything.
f9d0d83
to
65cc7f3
Compare
Right as usual, Jeff. And thanks for the speedy review! |
Fix problems with TypeVars in intersection and type_match_
👏 |
Fixes #11136. Don't merge until @JeffBezanson gets enough free time to look this over.