Allow packages to provide custom hints for Exceptions

[timholy]

Package authors may be able to predict likely user errors, and it can be nice to create an informative hint. For MethodErrors, we've done this repeatedly for Base; this PR makes it possible for packages to get in on the fun.

The procedure is described in the docstring for Base.methoderror_hintsregister_error_hint.

[johnnychen94]

IIUC, from a package maintainer's view, adding this feature into codebase means he'll need to maintain two branches: one for julia 1.0 and one for julia >= 1.5. So it's likely that only a few people will use it until the next LTS version release.

[kimikage]

he'll need to maintain two branches

If you don't intend to backport the hints to older versions of the package, I don't think you need to create branches.

if VERSION >= v"1.5"
    push!(Base.methoderror_hints, recommend_oneunit)
end

However, I don't really like the interface using an array. Is that unavoidable for performance?

[johnnychen94]

I don't really like the interface using an array.

I think a shared registry is unavoidable because we want downstream packages to reuse hints defined by upstream packages. For example, let Images.jl reuse the hints defined by Colors.jl.

Introducing a hints_level (like LogLevel) might be helpful to clear the order, but that's probably not what we want to consider in this PR.

[kimikage]

What exactly does "reuse" mean? I don't think downstream hints should depend on the upstream hints. 😕

[johnnychen94]

By saing "reuse" I mean we'd better not redefine hints like this in Colors/ColorVectorSpace, just propagating all these hints along the dependency chain looks good to me.

For element-wise addition, use broadcasting with dot syntax: array .+ scalar

# julia nightly build on https://github.com/JuliaLang/julia/pull/34642
julia> x = rand(4, 4);

julia> x + 2
ERROR: MethodError: no method matching +(::Array{Float64,2}, ::Int64)
For element-wise addition, use broadcasting with dot syntax: array .+ scalar
Closest candidates are:
  +(::Any, ::Any, ::Any, ::Any...) at operators.jl:529
  +(::Complex{Bool}, ::Real) at complex.jl:301
  +(::Missing, ::Number) at missing.jl:115
  ...
Stacktrace:
 [1] top-level scope at REPL[2]:1

[kimikage]

I don't understand the reason why @timholy used the array and for loops instead of the multiple dispatch. I don't think we need the performance to show error (i.e. not warning) messages.

[StefanKarpinski]

@kimikage, How frequently do you expect your programs to generate errors?

[kimikage]

How frequently do you expect your programs to generate errors?

It depends on what we assume. I'm not good at English, so it will take at least 5 seconds to understand an error message. So the frequency is less than 0.2 Hz. 😄

Improving the performance of programs which won't work only makes sense in the cases of using CI systems and so on, because the programs won't work.

Edit:
On the contrary, do you mean that using multiple dispatch for rare error messages is a waste of memory?

[StefanKarpinski]

My point is that worrying about performance of generating error messages is unnecessary.

[timholy]

I don't think it makes sense to do this by multiple dispatch. Catching the error is handled by the runtime, so the exact dispatch of which method threw the error is not going to be compiled into some specialized MethodInstance. As a consequence, it's going to have to use runtime dispatch. Since all such handlers would have to be methods of the same function, and because there's probably little clear specialization by subtyping, the method table will essentially be a linear list. Consequently jl_lookup_generic is just going to iterate over every item on the list anyway, just like this implementation. Moreover, this implementation avoids the risk of ambiguities (and it sucks if your error-handler throws an error), and can indeed even be more optimal if necessary because it can short-circuit full subtyping.

[timholy]

For reference, my test handler took 50ns to run. Imagine there are 1000 such handlers (which I don't think is out of the realm of possibility); if each takes 50ns, then the total run time is 50us. That's still way below the threshold for caring. We could have 100ns/handler and accommodate up to 10^6 such handlers before getting to 0.1s, which I'd say is the point at which I'd start caring about performance.

[timholy]

@johnnychen94, just to be clear, when one handler issues a message, it doesn't block others from doing so too. One can imagine cases where that would be undesirable, but on balance I think it's the best choice if we're going to keep this simple. (Otherwise we'd need to develop a whole protocol for resolution, and that starts to smell overengineered.)

[johnnychen94]

just to be clear, when one handler issues a message, it doesn't block others from doing so too.

True, my expression wasn't very accurate. I meant to say "sort the order of displaying from nearest(downstream package) to farthest(e.g., Base)" :P

[kimikage]

I simply care that push!ing to a global array is not a common extension interface on Julia. 😄

[timholy]

Take the one/oneunit example; rather than omitting one(::HasNoOne), we could define it to throw a specific error, just like we could define a x::Array + y::Number method that throws an error. But there are circumstances where this causes problems: it might blocks a less-specific method that might, actually, be appropriate (as would happen for getindex), or it might cause ambiguities, or... So in general we've moved away from this kind of scheme.

This is a circumstance where we're definitely not building real code, so normal rules don't really apply. You'll note the handlers already built into Base work essentially by trying each one.

[timholy]

True, my expression wasn't very accurate. I meant to say "sort the order of displaying from nearest(downstream package) to farthest(e.g., Base)" :P

You could achieve that with pushfirst!. Packages have at least that level of control over ordering.

[johnnychen94]

You could achieve that with pushfirst!

Would this be affected by precompilation and/or package loading order?

[timholy]

Loading order, yes. Not by precompilation order (you should push! these from your package's __init__ function, perhaps I should mention this in the docstring).

[kimikage]

This is just cosmetic, but I think it's better to quote the example with ```julia...```. 😄

[timholy]

Really great suggestions, @tkf! In addition, I wrapped the handler call in a try/catch, and I added support for hints to a few other exception types.

[timholy]

I'll hold off a bit longer (there has been a lot of very useful feedback that has improved this considerably!), but from my standpoint this is good to merge. The only part I'm unhappy with is #35114, but that's an issue for another day.

[mbauman]

Holy cow, I missed this and it looks awesome. This completely supersedes #24299 (and I like just using an array of functions much much better than leaning so heavily on multiple dispatch)... excepting there are a few more ideas about the sorts of things to be registered there.

[mbauman]

should we recommend folks use @info?

[tkf]

It's printing to a buffer io so I don't think we can replace it with @info.

(Or you are thinking to temporarily create a "buffer" logger to collect messages within a dynamic scope? It might work but I feel it's too fancy to do during error printing.)

[JeffBezanson]

I'm requesting triage here since this adds new exports. I admit, I greatly dislike adding things to Base. The functionality is good, I'm mostly thinking about where it goes. It's too bad how much UI-ish stuff is in Base; in fact I think that probably extends to the whole showerror function as well. All of that stuff seems like it belongs in the REPL or another package like InteractiveUtils.

Global state is also always a problem. I think this could have issues with building system images; if you load packages that add hints and then save a system image, the handlers will be saved and then possibly re-added on startup. Emptying the dictionary in Base.__init__ might be enough to fix this.

[tkf]

Emptying the dictionary in Base.__init__ might be enough to fix this.

I guess an alternative idea may be to register a function via atexit to clean it up and avoid serializing the callbacks into the sysimage in the first place.

[timholy]

I'm requesting triage here since this adds new exports.

Sounds fine. Let me know if you want changes or just go ahead and revert if necessary.

[c42f]

It's too bad how much UI-ish stuff is in Base; in fact I think that probably extends to the whole showerror function as well. All of that stuff seems like it belongs in the REPL or another package like InteractiveUtils.

For what it's worth I'd love if these functions were moved out of Base. I've been modifying Base.showerror recently to add some more heuristics (#35243), and it seemed quite wrong to be adding what are basically REPL-related UI hacks to Base.

[JeffBezanson]

From triage: move the exports to the Experimental submodule.

[timholy]

I will move it. Do you see a good solution to the global state problem? I am happy to work towards an alternative.

[mbauman]

In our short discussion on triage, Jeff (and others) talked about one reason to simply move it is because we don't have an obvious alternative but it sure is helpful.