-
-
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
Ambiguity warnings on definition -> error on usage #16125
Conversation
Ha, that is a cleverly minimal fix. It would be great to have a change in this direction in the system ASAP. It would really seal the deal if we could go back and remove the |
Also --- do we want to do this on every call, or only the first? Also might be good to print a stack trace, since uses are harder to locate than definitions. |
You don't actually need to completely trust the type intersection machinery if you check for dispatch matches. Check if the actual argument types match more than one of the candidate methods when you think you might have an ambiguity. There are three cases:
I guess the hard part is knowing which methods you need to check for a match since currently you just look for the first match when doing dispatch whereas this would require continuing beyond that. |
Good point --- this is all on the slow path anyway, so we could probably add that kind of logic. |
Tempting indeed! There is one complication that arises because a method can be ambiguous with more than one other method. For example, if you define methods in this order: julia> foo(x, y) = 1
julia> foo(x::Int, y) = 2
julia> foo(x, y::Int) = 3
julia> foo(x::Number, y::Int) = 4 then 2 and 3 point to each other as an ambiguous pair, while 4 points to 2. Then if you define this method, julia> foo(x::Int, y::Int) the edge between 2 and 3 gets severed, but 2 should then point to 4. To solve this, there are two possible options: (1) pass through the whole method list and look for new ambiguities, or (2) keep track of the whole "ambiguity graph" in the first place. Is this worth the performance cost?
I haven't played with this extensively, but caching seems to already handle a version of this for us: julia> foo(x, y) = 1
foo (generic function with 1 method)
julia> foo(x::Integer, y) = 2
foo (generic function with 2 methods)
julia> foo(x, y::Integer) = 3
foo (generic function with 3 methods)
julia> foo(1, 2)
WARNING: Definition
foo(Integer, Any) at REPL[2]:1
is ambiguous with:
foo(Any, Integer) at REPL[3]:1
See the documentation for tips on eliminating ambiguities.
2
julia> foo(1,2)
2
julia> foo(0x01, 2)
WARNING: Definition
foo(Integer, Any) at REPL[2]:1
is ambiguous with:
foo(Any, Integer) at REPL[3]:1
See the documentation for tips on eliminating ambiguities.
2
julia> foo(0x01, 2)
2
julia> foo(1, 0x02)
WARNING: Definition
foo(Integer, Any) at REPL[2]:1
is ambiguous with:
foo(Any, Integer) at REPL[3]:1
See the documentation for tips on eliminating ambiguities.
2
julia> foo(1, 0x02)
2
julia> @noinline bar(x, y) = foo(x, y)
bar (generic function with 1 method)
julia> bar(1, 2)
2
Is it a use-site problem, though? I'm not fully convinced, although I guess maybe you could say it is, in the same way that a Oh, wait, maybe there is a solution: what if we have codegen print the warning, saying "while compiling foo at src/myfile.jl: 17, an ambiguity in blah blah was detected." Where would such a warning get inserted? Presumably somewhere in
I was nervous about that. Isn't it essentially predictable that this will lead to a 2-fold slowdown in method lookup? If you basically assume that, on average, we traverse half the method table. I don't have a great sense for how much method lookup adds to compile time/runtime; if it's only a small fraction, then this might be worth it. |
Hmm, this is indeed getting more complicated as we poke at it. I don't think the warning should depend on cache behavior (sometimes there is a new entry for new concrete types, sometimes not), or on when code is compiled. We also need to be sure to find at least as many ambiguities as we do now, while excluding those that are resolved by subsequent definitions. It actually seems easier to give an error (when the call actually happens) instead of a warning, since then if we discover the problem at compile time the compiler can just insert a call to The easiest way to go about this is to look for ambiguities on every slow-path method lookup. Although that's not a hot path, the search might be expensive enough to make a dent. Maybe we could do something like using the current check to set a "might be ambiguous" bit as a hint. |
+1 to making it an error, not just a warning. |
Dusting off and putting on my language-lawyer hat, if you define "matching method" as "unique most specific matching method", then this is literally a no-method error. |
With regards to making it an error, I'm worried about: Aside from that point, there seem to be two issues to resolve:
|
Since it's such a small change, would it be possible to keep the definition time warnings with a switch for those of us who still like them? |
I definitely understand wanting that. However, just to ask: would an |
I'd be tempted to add a default flag to |
I would like it to be easy to set up my Julia installation to warn me about method ambiguities as soon as I make them, without me having to explicitly ask. Then we'll see how long that will be workable, if no one else is getting the warnings then I might just be flooded with warnings. Then I guess that I would have to go with a more selective approach. But I would still prefer to get the warnings by default. |
"As soon as you make them" means "definition order matters," which it won't if we end up fixing #270. Are you saying you'd prefer to keep the order-sensitive behavior? Or just that you don't want to have to remember to ask for ambiguity detection? Combining your request for making it easy with @tkelman's comment, we could conceivably have a flag that would cause However, I'm noting that Stefan's proposal---eliminate ambiguity-detection entirely from the process of method definition, and only check on method lookup---seems to me like the only plausible way to solve #5460. In such cases, ambiguity might no longer be abstractly decidable, and you might not be able to have a |
Actually, I no longer think those issues I linked above are a deal breaker for Stefan's proposal: of the 4, 1 was stochastic and may no longer happen, 1 should clearly fall out of a "morespecific" analysis, and the remaining 2 have to do with problems our "morespecific" analysis has with varargs. But if you're looking up a specific call signature, presumably that effectively resolves the varargs, and so I'm guessing "morespecific" will work properly. |
Just don't want to have to remember to ask for ambiguity detection. I would be fine with a few spurious warnings a la #5460 in that case. |
These can certainly both be |
OK, I've reworked this. It now throws an error when you try to call an ambiguous method. The overall architecture is basically along the lines of the discussion above:
This also solves #270. However, there is no attempt to try to go back and remove potential ambiguities from the "registrations" if a more specific method is defined later; instead, at compile time the more specific one gets selected, when appropriate, because it's earlier in the method table---the question of ambiguity simply never comes up. Easy. However, there's still some work to be done:
|
Can we hold off on merging this until after we branch for release-0.5? I feel like this will have wide ranging consequences that we don't understand yet. |
I'm usually pretty cautious about these things, but here I disagree; I suspect this is fairly safe, in many ways safer than what we have now (warn, but then pick a random function to execute). It seems like the kind of behavior change we want to come with the .0 release of 0.5. An important part of testing, however, will be for people who use packages that produce a slew of ambiguity warnings when used in combination (e.g., Images and DataArrays) to give it a whirl. |
@@ -623,9 +623,10 @@ jl_typemap_entry_t *jl_typemap_assoc_by_type(union jl_typemap_t ml_or_cache, jl_ | |||
jl_typemap_lookup_by_type_(ml, types, subtype_inexact__sigseq_useenv); | |||
} | |||
|
|||
jl_lambda_info_t *jl_typemap_assoc_exact(union jl_typemap_t ml_or_cache, jl_value_t **args, size_t n, int8_t offs) | |||
jl_lambda_info_t *jl_typemap_assoc_exact(union jl_typemap_t ml_or_cache, jl_value_t **args, size_t n, int8_t offs, jl_typemap_entry_t **m) |
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.
instead of adding an extra parameter, this can just return the jl_typemap_entry_t*
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 wondered about that. Will change.
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.
For a jl_typemap_entry_t *m
, can I just set m->func.linfo
with the value of any specialized jl_lambda_info_t
I create, and thereby return both? Or would that tank other necessary information (e.g., func.method
)?
I don't really even understand what a "guard entry" is, so this is probably a pretty naive question.
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.
Wait, I think I figured it out; that's valid for methods in the mt->cache
, but not in mt->defs
.
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, "guard entries" isn't really a function of the TypeMap, instead it's a placeholder used by the cache logic using a value of NULL to signal when a there would be a match, but that the LambdaInfo for that match hasn't been created yet.
74d8d76
to
6018e5b
Compare
|
||
# Test that ambiguous fail appropriately | ||
@test_throws AmbiguousError precompile(ambig, (UInt8, Int)) | ||
# @test_throws AmbiguousError cfunction(ambig, Int, (UInt8, Int)) |
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 is the only weakness I know of: if you call cfunction
twice on an ambiguous call, you get a segfault. Haven't yet had time to track this down.
This seems pretty much done, except for tools to scan for potential ambiguities (which can probably be a separate PR). |
🎉 Don't forget to close #270 too! |
What are the "Skipping" messages about in this test? |
julia> Base.<|
ERROR: MethodError: no method matching .<(::Module, ::Base.#|)
Closest candidates are:
.<(::Any, ::AbstractArray{T,N})
.<(::AbstractArray{T,N}, ::Any)
in eval(::Module, ::Any) at ./boot.jl:228 Once I discovered that, I just assumed it was true for the rest. But now I see that the two non-operators ( julia> Base.cluster_manager
ERROR: UndefVarError: cluster_manager not defined
in eval(::Module, ::Any) at ./boot.jl:228 |
When ambiguous methods now report an error on usage rather than definition: see JuliaLang#16125
The way of handling method ambiguities changed in JuliaLang#16125. Now Julia throws an error when calling an ambiguous methods, instead of just giving a warning when it was defined. This updates the manual to reflect the change. Fixes JuliaLang#16630.
The way of handling method ambiguities changed in JuliaLang#16125. Now Julia throws an error when calling an ambiguous methods, instead of just giving a warning when it was defined. This updates the manual to reflect the change. Fixes JuliaLang#16630.
When ambiguous methods now report an error on usage rather than definition: see JuliaLang#16125
When ambiguous methods now report an error on usage rather than definition: see JuliaLang#16125
When ambiguous methods now report an error on usage rather than definition: see JuliaLang#16125
Is this going to be backported to 0.4? Since a bunch of things were excised from Base for 0.5, but are still present in 0.4, this leads all kinds of ambiguity warnings on 0.4. For example, several packages (like HypothesisTests) depend on julia> using Combinatorics
WARNING: Method definition levicivita(AbstractArray{#T<:Integer, 1}) in module Base at combinatorics.jl:611 overwritten in module Combinatorics at /Users/tamasnagy/.julia/v0.4/Combinatorics/src/permutations.jl:188.
WARNING: Method definition partitions(Integer) in module Base at combinatorics.jl:252 overwritten in module Combinatorics at /Users/tamasnagy/.julia/v0.4/Combinatorics/src/partitions.jl:26.
WARNING: Method definition partitions(Integer, Integer) in module Base at combinatorics.jl:318 overwritten in module Combinatorics at /Users/tamasnagy/.julia/v0.4/Combinatorics/src/partitions.jl:93.
WARNING: Method definition partitions(AbstractArray{T<:Any, 1}) in module Base at combinatorics.jl:380 overwritten in module Combinatorics at /Users/tamasnagy/.julia/v0.4/Combinatorics/src/partitions.jl:158.
WARNING: Method definition partitions(AbstractArray{T<:Any, 1}, Int64) in module Base at combinatorics.jl:447 overwritten in module Combinatorics at /Users/tamasnagy/.julia/v0.4/Combinatorics/src/partitions.jl:228.
WARNING: Method definition permutations(Any) in module Base at combinatorics.jl:219 overwritten in module Combinatorics at /Users/tamasnagy/.julia/v0.4/Combinatorics/src/permutations.jl:24.
WARNING: Method definition nthperm!(AbstractArray{T<:Any, 1}, Integer) in module Base at combinatorics.jl:70 overwritten in module Combinatorics at /Users/tamasnagy/.julia/v0.4/Combinatorics/src/permutations.jl:136.
WARNING: Method definition prevprod(Array{Int64, 1}, Any) in module Base at combinatorics.jl:565 overwritten in module Combinatorics at /Users/tamasnagy/.julia/v0.4/Combinatorics/src/partitions.jl:354.
WARNING: Method definition combinations(Any, Integer) in module Base at combinatorics.jl:182 overwritten in module Combinatorics at /Users/tamasnagy/.julia/v0.4/Combinatorics/src/combinations.jl:42.
WARNING: Method definition nthperm(AbstractArray{#T<:Integer, 1}) in module Base at combinatorics.jl:92 overwritten in module Combinatorics at /Users/tamasnagy/.julia/v0.4/Combinatorics/src/permutations.jl:161.
WARNING: Method definition nthperm(AbstractArray{T<:Any, 1}, Integer) in module Base at combinatorics.jl:89 overwritten in module Combinatorics at /Users/tamasnagy/.julia/v0.4/Combinatorics/src/permutations.jl:157.
WARNING: Method definition factorial(#T<:Integer, #T<:Integer) in module Base at combinatorics.jl:56 overwritten in module Combinatorics at /Users/tamasnagy/.julia/v0.4/Combinatorics/src/factorials.jl:18.
WARNING: Method definition factorial(Integer, Integer) in module Base at combinatorics.jl:66 overwritten in module Combinatorics at /Users/tamasnagy/.julia/v0.4/Combinatorics/src/factorials.jl:28.
WARNING: Method definition parity(AbstractArray{#T<:Integer, 1}) in module Base at combinatorics.jl:642 overwritten in module Combinatorics at /Users/tamasnagy/.julia/v0.4/Combinatorics/src/permutations.jl:221. This isn't the best user experience. |
No, this won't be backported. Report issues in those packages. Check first whether they've been fixed on master or if there are already open issues/PR's regarding ambiguity warnings. |
Note that these aren't ambiguity warnings — Combinatorics is actually redefining those methods in base. Combinatorics could probably add a version check to ensure it only overrides the Base stubs in versions where they're errors that are designed to be overridden. |
When ambiguous methods now report an error on usage rather than definition: see JuliaLang#16125
'Nuf said: [EDIT: updated]
Fixes #6190.
I think the plan was to eventually turn these into errors, but that requires extraordinary confidence in the type-intersection machinery and won't happen until after the type system overhaul. In the meantime, I realized this might be an easy fix.To help package developers pro-actively discover ambiguities, we might want to build some tools ([since added]) to scan method tables.