-
-
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
Method ambiguity detection: false positive? #6048
Comments
This actually seems like the perfect example of why an ambiguity warning is needed, since the dispatch is not greedy and thus has to make an arbitrary choice of which method to dispatch. |
Both method 2 and 3 fit the call |
Yes, actually, it makes sense. |
Wait, no. 1, 2 and 3 all fit @JeffBezanson what gives? |
@JeffBezanson can confirm, but IIUC ... parameter lists are considered a worse fit than a function with the correct number of parameters, so 2 clearly wins over 1, but 2 and 3 both have two parameters and they both match foo(Bar,Bar) equally well. |
But here, 1 is winning. Another strange one: |(b::Bar, u::Union(Function, Int)) = 4
# Warning: New definition
# |(Bar,Union(Int64,Function)) at ...
# is ambiguous with:
# |(Any,Function) at deprecated.jl:19.
# To fix, define
# |(Bar,Function)
# before the new definition. |
That second example makes sense… you're clashing with the old deprecated pipe operator (now But I agree that it seems like |
On re-considering, case 1 has only one argument, a tuple of And also cases 2 and 3 have an |
@mbauman: Isn't |(b::Bar, u::Union(Function, Int)) = 4 equivalent to |(b::Bar, u::Function) = 4
|(b::Bar, u::Int) = 4 ? The Union may be less specific, the the pair So, basically, 1, which can't disambiguate 2 and 3 for arity reasons, disambiguates them anyway because it is still more specific, from a type standpoint. Does not compute :-/ If 1 prevails, deterministically, it should be considered to disambiguate 2 and 3, and the warning should not be displayed. |
Case 1 should fail to disambiguate both for arity and type reasons, the type of b is a tuple (of unknown length) of Note that the tuple length is not defined until the actual call, and the majik that has ... compile a tuple of the actual call args only for the case 1 before choosing which to dispatch. Both The But all that is at call time, its not information available at definition time when the warning happens. Thats dynamic dispatch though ;) |
I don't understand why the existence of a variadic function can't be taken into account at compile time... What prevents it from being deduced from the source code? Aside, apologies, for the confusion in the code examples, I went from |
Well, everything is possible but this would require some special handling, as I said case 1 has an arity of 1 and a type of tuple. So type deduction won't work, it needs special casing that the last parameter is variadic, and check that some expansion of the variadic could be a better match for the ambiguous case and so hide it. Its up to the experts, but to me it seems like a lot of work in the compiler to suppress a warning that actually tells you what to do to prevent it, define Defining that also means you have explicitly thought about it and made it do something sensible, not merely forgotten that the variadic will be called in the |
So that's the (current) of affairs, I thought that you were trying to explain a fundamental limitation, and, of course, I couldn't make sense of it. Performance/exhaustiveness compromises are sometimes necessary, no problem with that. In my case, I'm relying on I've added I'm leaving this open, for now, for future reference. |
This looks a lot like #5536 |
I do think this warning is unnecessary. Definitions 2 and 3 are ambiguous with each other, but it doesn't matter because neither of them would be called for Types are more important than argument count for specificity. A definition |
Indeed, many thanks :-) |
Semantically, foo(Bar,Bar)` is covered by the first definition, and the above example behaves as expected.
Is the warning really warranted?
The text was updated successfully, but these errors were encountered: