Ambiguity warnings on definition -> error on usage

[timholy]

'Nuf said: [EDIT: updated]

julia> foo(x, y) = 1
foo (generic function with 1 method)

julia> foo(x::Int, y) = 2
foo (generic function with 2 methods)

julia> foo(x, y::Int) = 3                # look Ma, no annoying warning!
foo (generic function with 3 methods)

julia> foo(3.2, 4.1)                     # You can use foo without trouble...
1

julia> foo(3, 4)                         # ...except when the correct method is ambiguous
ERROR: MethodError: foo(::Int64, ::Int64) is ambiguous. Candidates:
  foo(x::Int64, y) at REPL[2]:1
  foo(x, y::Int64) at REPL[3]:1
 in eval(::Module, ::Any) at ./boot.jl:228

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.

[JeffBezanson]

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 ambig references if a disambiguating definition is later added. That would fix #270, and who can resist closing a 3-digit issue?

[JeffBezanson]

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.

[StefanKarpinski]

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:

1. No matches: no method error.
2. One match: call the method.
3. Many matches: method ambiguity error.

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.

[JeffBezanson]

Good point --- this is all on the slow path anyway, so we could probably add that kind of logic.

[timholy]

@JeffBezanson,

It would really seal the deal if we could go back and remove the ambig references if a disambiguating definition is later added. That would fix #270, and who can resist closing a 3-digit issue?

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?

Also --- do we want to do this on every call, or only the first?

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

Also might be good to print a stack trace, since uses are harder to locate than definitions.

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 MethodError could potentially be a use-site problem. But for cases where you're inserting this into a method you're compiling, won't that mean that we end up compiling the backtrace into the function? In cases where type intersection just came to the wrong conclusion, that seems likely to cause a pretty catastrophic performance hit.

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 inference.jl?

@StefanKarpinski,

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 and this would require continuing beyond that.

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.

[JeffBezanson]

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 throw, which is quite easy to do.

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.

[StefanKarpinski]

+1 to making it an error, not just a warning.

[JeffBezanson]

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.

[timholy]

With regards to making it an error, I'm worried about:
#10174
#9588
#6048
#5460
(These are just the open issues, I didn't check closed ones.) In many/all of these cases, it is actually dispatching to the correct method. For this reason, I think we can't make it throw an error just yet.

Aside from that point, there seem to be two issues to resolve:

• Only warn once: fixed in bc6bc4d.
• Provide line info from "call site" (during compilation). See if you can stomach the solution in 23845b5. It's only a sketch, the source line info is very incomplete, but I'm not sure how to get to the line number without passing lno all the way down (or using a global).

[toivoh]

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?

[timholy]

I definitely understand wanting that. However, just to ask: would an ambiguities(MyModule, Base, ...) function be equally attractive? Something that would scan for any ambiguities in a list of modules. (I'm not promising it's possible, but I'd guess it is.) A potential advantage is that if we do end up deciding to solve #270, then it won't make much sense to do this any other way except, e.g., at module closure or upon user request.

[tkelman]

I'd be tempted to add a default flag to Pkg.test, like we have for coverage, if this ambiguity detection API only ran when asked.

[toivoh]

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.

[timholy]

"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 check_ambiguities(mod, Base) to run whenever a module is closed.

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 check_ambiguities function at all. (Or it might give some false positives.)

[timholy]

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.

[toivoh]

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.

[StefanKarpinski]

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.

These can certainly both be MethodErrors but they should definitely give different error messages, otherwise it's going to be rather confusing if no matching method and too many matching methods are printed the same.

[timholy]

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:

• Register all potential ambiguities at function-definition time
• At compile/call time, check whether any of the potentially-ambiguous method signatures intersect with the argument tuple-type. If so, throw an error.

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:

• The error-throwing is just a stub; are there some good examples I can look at to see how to get codegen to insert an error call into the compiled code?
• To be safe, I find that I have to use @noinline in the function call: for really trivial signatures like the ones in the new test/ambiguous.jl, they are inlined even before calling inlineable in inference.jl. I spent some time trying to figure out where this happens, but haven't yet been successful (pointers appreciated).
• Need tools to scan for ambiguities as an aid to developers

[tkelman]

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.

[timholy]

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.

[vtjnash]

instead of adding an extra parameter, this can just return the jl_typemap_entry_t*

[timholy]

I wondered about that. Will change.

[timholy]

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.

[timholy]

Wait, I think I figured it out; that's valid for methods in the mt->cache, but not in mt->defs.

[vtjnash]

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.

[timholy]

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.

[timholy]

This seems pretty much done, except for tools to scan for potential ambiguities (which can probably be a separate PR).

[JeffBezanson]

🎉

Don't forget to close #270 too!

[tkelman]

What are the "Skipping" messages about in this test?

[timholy]

<| seems to be exported but not defined. There's also something interesting about how it gets parsed:

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 (cluster_manager and active_repl) are use-dependent---they are defined as globals but not set until something triggers them to be defined.

julia> Base.cluster_manager
ERROR: UndefVarError: cluster_manager not defined
 in eval(::Module, ::Any) at ./boot.jl:228

[tlnagy]

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 Combinatorics.jl which throws a huge number of ambiguity warnings when imported on 0.4:

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.

[tkelman]

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.

[mbauman]

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.