Better error message for invoke

[yuyichao]

Before

julia> k(::Int) = nothing
k (generic function with 1 method)

julia> invoke(k, (Any,), 1)
ERROR: `k` has no method matching k(::Int64)
Closest candidates are:
  k(::Int64)

After

julia> k(::Int) = nothing
k (generic function with 1 method)

julia> invoke(k, (Any,), 1)
ERROR: `k` has no method matching k(::Any)
Closest candidates are:
  k(::Int64)

[dhoegh]

I have also extended some error printing in #9485, where I also made a test file for replutil.jl, you should probably also add a test of the functionality you added.

[vtjnash]

it doesn't seem like the args parameter is needed after this commit. should we just remove it always?

[yuyichao]

@vtjnash In principle yes. But is that counted as a breaking change? (hopefully it's not a very big breakage)

[vtjnash]

yes. but making breaking changes on master is fine right now (adding types is probably considered a breaking change anyways)

[yuyichao]

One issue with removing the args parameter is that the row vector vs column vector check would have to be done in the constructor.

[yuyichao]

I'm wondering if the GC stack needs to be poped before jl_throw. I assume it is since I've seen JL_GC_POP in JL_CATCH. However, it is not done in jl_no_method_error. Is it a bug or is it a special case that can be handled automatically?

[vtjnash]

jl_throw undoes all JL_GC_PUSH calls that happened inside of the JL_TRY

[yuyichao]

Ahh, I see the JL_TRY macro saves the current stack pointer (and I guess a try in julia does st similar?). Does it mean that this gc pop is not necessary?

[yuyichao]

All the suggested changes have been implemented. A few changes listed below

1. MethodError is immutable (don't see why anyone would want to mutate it...)
2. More restricted constructor (the second parameter must be a tuple type (when the thrid parameter is true) or an iterable (when the thrid parameter is false or omitted)). The old parameter should work otherwise.
3. typeof is done in the constructor, as well as the check for row/column array parameters
4. Added tests to make sure MethodError for invoke is printed correctly and also check for certain keyword in the error message to make sure the special cases for error printing works as before.

[jakebolewski]

Presumably this logic was originally not done in the constructor out of fear of a performance hit? Others will have to comment on that.

I also don't see why you need two extra fields here to replace args? Can't you just handle the tuple type case by checking if args is a type tuple? Having an extra field just for the column vector error seems like too much of a special case.

Overall, this is clearly an improvement.

[yuyichao]

Presumably this logic was originally not done in the constructor out of
fear of a performance hit? Others will have to comment on that.

That and also to preserve more information of the argument?

I also don't see why you need two extra fields here, to replace args?
Can't you just handle the tuple type case by checking if the args is a
type tuple? Having an extra field just for the column vector error seems
like too much of a special case.

There are two different issues here.

1. There's an additional field, which has nothing to do with tuple vs
   type arguments. It is used to record if any of the arguments is a row
   vector in order to produce the same error message with before.
2. There's an extra argument to the constructor, used to indicate whether
   the second argument is a tuple type or not. I've thought about
   automatically detecting it but it would not work very well since a tuple
   type is itself a tuple.... I've also thought about other magic to
   automatically detect whether the caller want to construct it with a tuple
   of types but in the end I think an additional Bool argument is not a big
   deal if we have to otherwise rely on some magical autodetection.

[yuyichao]

Alternatively I could remove the logic in the constructor and just save whatever argument passed to it and then do the conversion in the showerror. This should have the smallest performance impact and is easier to add other output based on the argument value in the future. It will make the showerror method slightly more complicated but not much more than what it is right now.

Is there a preference?

[yuyichao]

Actually I think I prefer this way (save the is_types Bool value in the structure and move all logics in showerror) since the performance should be better, the API break is smaller, and it's where the logics belongs to...