-
-
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 handling of allow_bottom_tparams
in isambiguous
#20982
Conversation
base/reflection.jl
Outdated
|
||
For parametric types, `allow_bottom_tparams` controls whether | ||
`Union{}` is considered a valid intersection of type parameters. For example: | ||
```julia |
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.
looks like this could be a doctest with minor changes
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.
Insofar as I am qualified to review (not far), this looks great!
The former version failed to detect that certain type parameters were Bottom (for example, in the case in the added docstring for `isambiguous`).
4582705
to
ec4444a
Compare
@Sacha0, when your pile of PRs merges, please do us the honor of turning that test back on. Thanks for jump-starting the effort to resolve these ambiguities. |
+1 |
@@ -941,13 +941,41 @@ function method_exists(f::ANY, t::ANY) | |||
typemax(UInt)) != 0 | |||
end | |||
|
|||
""" | |||
isambiguous(m1, m2, [allow_bottom_tparams=true]) -> Bool |
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.
unrelated aside, but I think we should gradually aim to get away from using square brackets to denote optional arguments and uniformly use the julia syntax convention for them in docstrings
With master, we have a lot of ambiguities, but a subset of these involve method signatures that are ambiguous only because of the presence of
Union{}
as a type-parameter in the intersection:This isn't the kind of intersection we should usually be worried about, so in e101000 a strategy was developed to exclude these kinds of intersections. However, I found that it doesn't correctly catch all cases, including the one above:
With this PR, I believe I've fixed the detection of
Union{}
. It has a pretty dramatic effect on our ambiguity list.Master:
This PR:
11 seems rather more manageable (the "missing" digit is not a copy/paste error). Suddenly the steps that @Sacha0 has been taking (#20970, #20971) don't seem so small anymore: those should fix 3 of the 11. Of the rest, it's just 4 functions:
cat
(4 ambiguous pairs),checkbounds_indices
(2 ambiguous pairs),colon
(1 ambiguous pair), and a constructor method forTuple
(1 ambiguous pair).I do find myself wishing we could change the keyword argument to consistently have the same name, and I find
accept_bottom_tparams
clearer thanallow_bottom_tparams
(and perhaps flipping the sense,exclude_bottom_tparams
, to be the clearest of all). But at this point, I don't think we can do that for 0.6. I did add some docstrings.Fixes #20246