Skip to content
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

Show better "closest candidates" when there is a MethodError #32470

Open
ViralBShah opened this issue Jul 1, 2019 · 16 comments
Open

Show better "closest candidates" when there is a MethodError #32470

ViralBShah opened this issue Jul 1, 2019 · 16 comments
Labels
display and printing Aesthetics and correctness of printed representations of objects. error handling Handling of exceptions by Julia or the user

Comments

@ViralBShah
Copy link
Member

ViralBShah commented Jul 1, 2019

There are a couple of points around MethodError that we could improve from a user friendliness perspective. First, the closest candidates shown could be more specific to the error. Second, perhaps we don't want to show the closest candidates by default, and have a way for the user to ask for them.

julia> 5+"a"
ERROR: MethodError: no method matching +(::Int64, ::String)
Closest candidates are:
  +(::Any, ::Any, ::Any, ::Any...) at operators.jl:529
  +(::T<:Union{Int128, Int16, Int32, Int64, Int8, UInt128, UInt16, UInt32, UInt64, UInt8}, ::T<:Union{Int128, Int16, Int32, Int64, Int8, UInt128, UInt16, UInt32, UInt64, UInt8}) where T<:Union{Int128, Int16, Int32, Int64, Int8, UInt128, UInt16, UInt32, UInt64, UInt8} at int.jl:53
  +(::Union{Int16, Int32, Int64, Int8}, ::BigInt) at gmp.jl:468
  ...
Stacktrace:
 [1] top-level scope at REPL[1]:1

These are basically things that would greatly improve the experience for new users who may not be as well versed with the system just yet.

cc @JeffBezanson @Keno

@ViralBShah ViralBShah added error handling Handling of exceptions by Julia or the user triage This should be discussed on a triage call labels Jul 1, 2019
@Keno
Copy link
Member

Keno commented Jul 1, 2019

I think we may want to have a modal in web contexts (click a button to get more info) and a similar mechanism in the repl (similar to the stack trace jumping) that prints this info. At that point, we could also use up a bit more vertical space to give users better info heuristically (right now it needs to be fairly compact since it prints on every method error, but if it's an opt in there's no such constraint).

@Keno Keno added the display and printing Aesthetics and correctness of printed representations of objects. label Jul 1, 2019
@KristofferC
Copy link
Sponsor Member

I often find the closest candidate list useful.

Sure, the heuristics might need to be tweaked but it is very nice when you get one argument wrong, or transpose a set of arguments.

@Keno
Copy link
Member

Keno commented Jul 1, 2019

Yes, I agree it's useful sometimes. I think the only question is whether we want to always print it by default or have an opt in way to get it (with then potentially more useful information). Or maybe the correct thing to do is to attempt to design those improved heuristics first and see if it actually needs more vertical space.

@KristofferC
Copy link
Sponsor Member

What's the actual problem with the closest candidate list again? I haven't seen a single complaint about it for years.

These are basically things that would greatly improve the experience for new users who may not be as well versed with the system just yet.

I don't really understand this, if anything, the closest candidate list should be more useful for new users than for experienced ones.

@Keno
Copy link
Member

Keno commented Jul 1, 2019

The complaint is that e.g.

  +(::T<:Union{Int128, Int16, Int32, Int64, Int8, UInt128, UInt16, UInt32, UInt64, UInt8}, ::T<:Union{Int128, Int16, Int32, Int64, Int8, UInt128, UInt16, UInt32, UInt64, UInt8}) where T<:Union{Int128, Int16, Int32, Int64, Int8, UInt128, UInt16, UInt32, UInt64, UInt8} at int.jl:53

is a scary thing to see for somebody who's never used the language before.

@KristofferC
Copy link
Sponsor Member

KristofferC commented Jul 1, 2019

is a scary thing to see for somebody who's never used the language before.

Maybe, but users have the property of "has never used the language" for a quite short time. Then they become regular users. In my opinion, it is better to design things for regular users and instead write tutorials for new users to make them become regular users. But sure, even for regular users the closest candidate list sometimes has a low signal to noise ratio.

Union / typealias printing could in general be improved, cf. getting a stacktrace involving StridedArray.

@mbauman
Copy link
Sponsor Member

mbauman commented Jul 1, 2019

This old idea of mine might be a sensible path forward: #24299

My complaint about the closest candidate list is that it is often wholly unhelpful. 5+"a" is a pretty good example: is +(::Any, ::Any, ::Any, ::Any...) really a close candidate? (Edit: "a" + "b" is even better; it also prints out the 3+ argument method as the closest method... but amusingly upon making it a 3-argument call it still prints out the same method) Essentially there are two classes of problems here:

  • You passed slightly wrong types/arguments to a method — here it's great!
  • You passed wholly improper arguments and are using the function "wrong" in the first place — here it's worthless.

New users are more likely to experience the latter.

@Keno
Copy link
Member

Keno commented Jul 1, 2019

For example, I'm just spitballing here without regard to compactness (or availability of information currently)

julia>
ERROR: MethodError: no method matching +(::Int64, ::String)

You may have to implement a method for this argument combination, or
you may want to consider one of the suggestions below:

[1] Argument 2 has type `String`. Is this correct?
| 
|            1+"a"
|              ~~~
|                 \ String
|
| Method matches exist for the following types in this position:
|   * Int128, Int16, Int32, Int64, Int8, UInt128, UInt16, UInt32, UInt64 or UInt8 [method at int.jl:53]
|   * Ptr [method at pointer.jl:161]
|   * Complex [method at complex.jl:308]
|   * Number [method at promotion.jl:313]
|   * Missing [method at missing.jl:95 ]
|   * LinearAlgebra.UniformScaling [ method in LinearAlgebra at uniformscaling.jl:110 ]
|   * ...
 \ ---

We'd need some way of explaining to the system whether or not it suggest the +(::Any, ::Any, ::Any, ::Any...) method. For example, we could have a dispatch, plugin mechanism like:

function Base.give_useful_method_error(::typeof(+), T::Type{<:Tuple{Any, Any}})
      filter(!isa(VarargSuggestion), all_suggestions(+, T))
end

@Keno
Copy link
Member

Keno commented Jul 1, 2019

(The idea here being that every type of suggestion is represented by a separate suggestion type, so there could be suggestions for:

  • You messed up this argument
  • You missed an argument here
  • You forgot an argument at the end

And individual libraries could hook into this system to provide more intelligent suggestions (and potentially it's own suggestion types). #24299 that Matt referenced could go a long way in that direction. I think this kind of thing could be particularly useful for libraries. E.g. imagine Zygote using this to say something like No primitive adjoint has been defined for function `foo` , as opposed to throwing some arcane MethodError at you.

@tkf
Copy link
Member

tkf commented Jul 2, 2019

For example, we could have a dispatch, plugin mechanism like:

function Base.give_useful_method_error(::typeof(+), T::Type{<:Tuple{Any, Any}})
      filter(!isa(VarargSuggestion), all_suggestions(+, T))
end

Ideally, it would be nice for this mechanism to have two separate overloadable APIs: one for function author and one for type author (like getindex has a mechanism to dispatch on the array type and index type separately). I think there are cases where type author has a better chance of providing more user-friendly message. For example:

julia> 1 + Some(1)
ERROR: MethodError: no method matching +(::Int64, ::Some{Int64})

In this case, Some type can say something like Do you forget to unwrap `1` with `something` function? but + function can't say anything specific.

@tkf
Copy link
Member

tkf commented Jul 2, 2019

Or, maybe it does not have to be two separate APIs as the function/callable type is just the first type in the method signature (I'm thinking .sig of Method). The overloadable API can just take the form of provide_suggestions(::Type{T}, f, args...) where T may be typeof(f) or typeof(args[i]). The only valid way to overload this function is to specify the type T of the first argument (and maybe the number of args).

@antoine-levitt
Copy link
Contributor

antoine-levitt commented Jul 2, 2019

I think the following are good heuristics:

  • It's more common to mess up on the type of arguments than on the number.

  • If I use an "exotic" type (ie one not defined outside base), it's likely that I did that on purpose, so showing results with that type removed is less likely to be helpful.

  • Methods that have a docstring attached to them are usually more interesting than those that don't.

One thing that could help is to have the name of the arguments in addition to their type.

But of course the worst problem by far is unions: the Union{Int128, Int16, Int32, Int64, Int8, UInt128, UInt16, UInt32, UInt64, UInt8} above is bad, but the all-time champion is StridedArray. mul!(randn(1)) is very bad for those suffering from C++ error message PTSD. Printing something sensible here would be amazing.

@vtjnash
Copy link
Sponsor Member

vtjnash commented Jul 2, 2019

one minor change might be to hide the types for the arguments that match (the _1 below):

ERROR: MethodError: no method matching mul!(_1::Array{Float64,1})
Closest candidates are:
  mul!(_1, ::Number, ::AbstractArray) at /Users/jameson/julia/usr/share/julia/stdlib/v1.3/LinearAlgebra/src/generic.jl:26
  mul!(_1, ::AbstractArray, ::Number) at /Users/jameson/julia/usr/share/julia/stdlib/v1.3/LinearAlgebra/src/generic.jl:27
  mul!(_1, ::Union{DenseArray{T<:Union{Complex{Float32}, Complex{Float64}, Float32, Float64},1}, DenseArray{T<:Union{Complex{Float32}, Complex{Float64}, Float32, Float64},2}, Base.ReinterpretArray{T<:Union{Complex{Float32}, Complex{Float64}, Float32, Float64},2,S,A} where S where A<:Union{SubArray{T,N,A,I,true} where I<:Union{Tuple{Vararg{Real,N} where N}, Tuple{AbstractUnitRange,Vararg{Any,N} where N}} where A<:DenseArray where N where T, DenseArray}, Base.ReinterpretArray{T<:Union{Complex{Float32}, Complex{Float64}, Float32, Float64},1,S,A} where S where A<:Union{SubArray{T,N,A,I,true} where I<:Union{Tuple{Vararg{Real,N} where N}, Tuple{AbstractUnitRange,Vararg{Any,N} where N}} where A<:DenseArray where N where T, DenseArray}, Base.ReshapedArray{T<:Union{Complex{Float32}, Complex{Float64}, Float32, Float64},1,A,MI} where MI<:Tuple{Vararg{Base.MultiplicativeInverses.SignedMultiplicativeInverse{Int64},N} where N} where A<:Union{Base.ReinterpretArray{T,N,S,A} where S where A<:Union{SubArray{T,N,A,I,true} where I<:Union{Tuple{Vararg{Real,N} where N}, Tuple{AbstractUnitRange,Vararg{Any,N} where N}} where A<:DenseArray where N where T, DenseArray} where N where T, SubArray{T,N,A,I,true} where I<:Union{Tuple{Vararg{Real,N} where N}, Tuple{AbstractUnitRange,Vararg{Any,N} where N}} where A<:DenseArray where N where T, DenseArray}, Base.ReshapedArray{T<:Union{Complex{Float32}, Complex{Float64}, Float32, Float64},2,A,MI} where MI<:Tuple{Vararg{Base.MultiplicativeInverses.SignedMultiplicativeInverse{Int64},N} where N} where A<:Union{Base.ReinterpretArray{T,N,S,A} where S where A<:Union{SubArray{T,N,A,I,true} where I<:Union{Tuple{Vararg{Real,N} where N}, Tuple{AbstractUnitRange,Vararg{Any,N} where N}} where A<:DenseArray where N where T, DenseArray} where N where T, SubArray{T,N,A,I,true} where I<:Union{Tuple{Vararg{Real,N} where N}, Tuple{AbstractUnitRange,Vararg{Any,N} where N}} where A<:DenseArray where N where T, DenseArray}, SubArray{T<:Union{Complex{Float32}, Complex{Float64}, Float32, Float64},1,A,I,L} where L where I<:Tuple{Vararg{Union{Int64, AbstractRange{Int64}, Base.AbstractCartesianIndex},N} where N} where A<:Union{Base.ReinterpretArray{T,N,S,A} where S where A<:Union{SubArray{T,N,A,I,true} where I<:Union{Tuple{Vararg{Real,N} where N}, Tuple{AbstractUnitRange,Vararg{Any,N} where N}} where A<:DenseArray where N where T, DenseArray} where N where T, Base.ReshapedArray{T,N,A,MI} where MI<:Tuple{Vararg{Base.MultiplicativeInverses.SignedMultiplicativeInverse{Int64},N} where N} where A<:Union{Base.ReinterpretArray{T,N,S,A} where S where A<:Union{SubArray{T,N,A,I,true} where I<:Union{Tuple{Vararg{Real,N} where N}, Tuple{AbstractUnitRange,Vararg{Any,N} where N}} where A<:DenseArray where N where T, DenseArray} where N where T, SubArray{T,N,A,I,true} where I<:Union{Tuple{Vararg{Real,N} where N}, Tuple{AbstractUnitRange,Vararg{Any,N} where N}} where A<:DenseArray where N where T, DenseArray} where N where T, DenseArray}, SubArray{T<:Union{Complex{Float32}, Complex{Float64}, Float32, Float64},2,A,I,L} where L where I<:Tuple{Vararg{Union{Int64, AbstractRange{Int64}, Base.AbstractCartesianIndex},N} where N} where A<:Union{Base.ReinterpretArray{T,N,S,A} where S where A<:Union{SubArray{T,N,A,I,true} where I<:Union{Tuple{Vararg{Real,N} where N}, Tuple{AbstractUnitRange,Vararg{Any,N} where N}} where A<:DenseArray where N where T, DenseArray} where N where T, Base.ReshapedArray{T,N,A,MI} where MI<:Tuple{Vararg{Base.MultiplicativeInverses.SignedMultiplicativeInverse{Int64},N} where N} where A<:Union{Base.ReinterpretArray{T,N,S,A} where S where A<:Union{SubArray{T,N,A,I,true} where I<:Union{Tuple{Vararg{Real,N} where N}, Tuple{AbstractUnitRange,Vararg{Any,N} where N}} where A<:DenseArray where N where T, DenseArray} where N where T, SubArray{T,N,A,I,true} where I<:Union{Tuple{Vararg{Real,N} where N}, Tuple{AbstractUnitRange,Vararg{Any,N} where N}} where A<:DenseArray where N where T, DenseArray} where N where T, DenseArray}}, ::Union{DenseArray{T<:Union{Complex{Float32}, Complex{Float64}, Float32, Float64},1}, Base.ReinterpretArray{T<:Union{Complex{Float32}, Complex{Float64}, Float32, Float64},1,S,A} where S where A<:Union{SubArray{T,N,A,I,true} where I<:Union{Tuple{Vararg{Real,N} where N}, Tuple{AbstractUnitRange,Vararg{Any,N} where N}} where A<:DenseArray where N where T, DenseArray}, Base.ReshapedArray{T<:Union{Complex{Float32}, Complex{Float64}, Float32, Float64},1,A,MI} where MI<:Tuple{Vararg{Base.MultiplicativeInverses.SignedMultiplicativeInverse{Int64},N} where N} where A<:Union{Base.ReinterpretArray{T,N,S,A} where S where A<:Union{SubArray{T,N,A,I,true} where I<:Union{Tuple{Vararg{Real,N} where N}, Tuple{AbstractUnitRange,Vararg{Any,N} where N}} where A<:DenseArray where N where T, DenseArray} where N where T, SubArray{T,N,A,I,true} where I<:Union{Tuple{Vararg{Real,N} where N}, Tuple{AbstractUnitRange,Vararg{Any,N} where N}} where A<:DenseArray where N where T, DenseArray}, SubArray{T<:Union{Complex{Float32}, Complex{Float64}, Float32, Float64},1,A,I,L} where L where I<:Tuple{Vararg{Union{Int64, AbstractRange{Int64}, Base.AbstractCartesianIndex},N} where N} where A<:Union{Base.ReinterpretArray{T,N,S,A} where S where A<:Union{SubArray{T,N,A,I,true} where I<:Union{Tuple{Vararg{Real,N} where N}, Tuple{AbstractUnitRange,Vararg{Any,N} where N}} where A<:DenseArray where N where T, DenseArray} where N where T, Base.ReshapedArray{T,N,A,MI} where MI<:Tuple{Vararg{Base.MultiplicativeInverses.SignedMultiplicativeInverse{Int64},N} where N} where A<:Union{Base.ReinterpretArray{T,N,S,A} where S where A<:Union{SubArray{T,N,A,I,true} where I<:Union{Tuple{Vararg{Real,N} where N}, Tuple{AbstractUnitRange,Vararg{Any,N} where N}} where A<:DenseArray where N where T, DenseArray} where N where T, SubArray{T,N,A,I,true} where I<:Union{Tuple{Vararg{Real,N} where N}, Tuple{AbstractUnitRange,Vararg{Any,N} where N}} where A<:DenseArray where N where T, DenseArray} where N where T, DenseArray}}) where T<:Union{Complex{Float32}, Complex{Float64}, Float32, Float64} at /Users/jameson/julia/usr/share/julia/stdlib/v1.3/LinearAlgebra/src/matmul.jl:65

Perhaps another option would be to print head(help(f))? (in this case mul!(Y, A, B) -> Y)

@StefanKarpinski
Copy link
Sponsor Member

I wonder if a good rule for choosing which methods to show would be to assume the type of each argument is wrong one at a time and show the method that is most specific given the types of the other arguments?

@JeffBezanson
Copy link
Sponsor Member

We should also apply a strict limit instead of printing .... If we can't narrow it down to less than 4-ish candidates, our guesses are probably not useful. In those cases we could just suggest typing ?f.

@StefanKarpinski StefanKarpinski removed the triage This should be discussed on a triage call label Jul 3, 2019
@StefanKarpinski
Copy link
Sponsor Member

On triage @Jameson brought up the idea that you could only print as much detail about a non-matching argument type as is required to make it not match.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
display and printing Aesthetics and correctness of printed representations of objects. error handling Handling of exceptions by Julia or the user
Projects
None yet
Development

No branches or pull requests

9 participants