Show closest matching methods on MethodError

[jhasse]

Implements my idea described in #7686 and #7512.

Screenshot what it looks like:

[ivarne]

Looks good to me. Bonus for doing the whitespace removal in a separate commit.

[mbauman]

Looks great. A few questions:

• What do you think about limiting the printout to the top ~3 methods? If I'm reading it correctly, some methods with an ::IO first argument would display hundreds of matches (e.g. show).
• How does this behave when you mistype a keyword argument? Perhaps this functionality should be turned off when the original call had kwargs? (at least until they're better supported by methods).

[simonster]

More concisely: isa(i[1], i[2]). Also might be clearer to use for (arg, sigtype) in zip... instead of referring to i[1] and i[2] here.

[simonster]

I agree with @mbauman that the output here needs to be limited. I would propose showing only methods with the maximum value of right_matches, and not showing any methods if more than a few have the maximum value. In the future, it might be nice to use a non-binary metric for distance between types, e.g. the distance to the lowest common ancestor in the type hierarchy, if that is not too expensive.

[ivarne]

As this code will only be executed when the REPL is about to present an error to the user, I don't think there are much that could possibly be done to make it too slow. Network connections to slow servers would probably be acceptable from a performance point of view, although there are other reasons why we should avoid that.

[jhasse]

Thanks for the suggestions :)

New version of my patch truncates the list after three candidates and displays ... so the user knows there's more. In return it now also displays the list for functions with two arguments.

@simonster Thanks! I got rid of i[1] and i[2] like your suggested. Unfortunately isa doesn't work for Types.

[jhasse]

@mbauman Because I'm new to Julia I don't know much about keyword arguments. As far as I can tell mistyping one will give ERROR: unrecognized keyword argument "k" and no MethodError, so there should be no problem.

[jhasse]

Any news about getting this merged?

[StefanKarpinski]

This seems like a good change. @JeffBezanson, any objections?

[StefanKarpinski]

Ok, works for me and is quite nice. Thank you and sorry for the delayed merge!

[dhoegh]

Could this be backported? @JuliaBackports

[ivarne]

We'll see how this works on master for a few days, and unless we get some negative feedback, I think this might be a nice thing to include in 0.3.4.

[nalimilan]

Sounds pretty cool!

[simonster]

This seems very slow for functions with many methods. On my system, it takes nearly 10 seconds to print an error message for convert:

$ time julia -e 'try convert(Matrix{Float64}, [1.]) end'

real    0m0.240s
user    0m0.178s
sys     0m0.492s

$ time julia -e 'convert(Matrix{Float64}, [1.])'
ERROR: `convert` has no method matching convert(::Type{Array{Float64,2}}, ::Array{Float64,1})
Closest candidates are:
  convert(::Type{T}, ::T)
  convert(::Type{Ptr{Void}}, ::Array{T,N})
  convert(::Type{Ptr{T}}, ::Array{T,N})
  ...

 in process_options at ./client.jl:222
 in _start at ./client.jl:382
 in _start_3B_4047 at /usr/local/julia/usr/bin/../lib/julia/sys.so


real    0m9.958s
user    0m9.793s
sys     0m0.607s

[JeffBezanson]

In many ways this feature is really slick, but I have to be honest that its output often seems a bit odd. E.g. above, "perhaps you'd like to convert this to a pointer instead?" doesn't really make sense. The idea of a "close" method is subjective.

[tkelman]

The performance improvement from 052f54e relies on 0.4-only syntax, right? Can anyone evaluate the performance impact of backporting this to release-0.3? We may want to postpone this one to just after 0.3.4 if we can't get a solid feeling about it in the next week.

[ivarne]

No, there is no 0.4 syntax in the fix. The constructor for Zip2 has been there for a long time, and the only thing we do is specify that we want to skip the specialization in zip and use the constructor for Zip2{Any,Any} instead.

[JeffBezanson]

This is strictly a new feature, and so not something we would normally backport.

[StefanKarpinski]

While that's true, this is a purely interactive feature and one that only occurs upon errors, so it would not be crazy to consider backporting it.

[JeffBezanson]

I'm against backporting it. We don't want to start randomly picking new functionality to backport. Nor can one argue that this feature is so important as to be worth the risk, and worth an exception from our pretty clear policy. Sure, it's not likely to cause a problem, but the point of a backporting policy is to avoid worrying about that. And to avoid a debate about whether every new feature deserves backporting.