Improve error message for which(fn, types)
#47369
Conversation
|
Marking for triage, for discussion of whether |
|
Suggestion from triage: stringify the MethodError to wrap it in an ErrorException that adds some text like "call would result in the following method error:". |
apaz-cli
force-pushed
the
ap/which
branch
2 times, most recently
from
6141895
to
9551f58
Compare
|
Wouldn't it make more sense to wrap the actual method error and then display it correctly? |
|
Test for #47559 yoinked from #47566, thanks @Seelengrab ❤️ 🙂 |
|
After CI runs, there should still be one failing test, which is part of We should also run pkgeval to try to find anything else that this breaks. Also, is this sort of change to MethodError something that we should triage? If we don't want to change how |
|
@nanosoldier |
|
Your package evaluation job has completed - possible new issues were detected. A full report can be found here. |
|
Quite some errors because master is 1.10 and this branch 1.9 (JuliaCI/Nanosoldier.jl#131). |
|
Looking at the logs this is running on a quite old Julia commit. Running on merge commit with PkgEval would be nice :P |
|
Those are a Pkg bug and not just a branch difference though, right? Eg https://s3.amazonaws.com/julialang-reports/nanosoldier/pkgeval/by_hash/e9ab711_vs_5f0da83/Casacore.primary.log |
|
Rebased. This is also a good PR to test some new PkgEval features, so sorry for the noise (I'll delete those comments afterwards): @nanosoldier |
|
@nanosoldier |
My point is that the bug is fixed on master Julia but not on this branch. |
|
@nanosoldier |
|
Your package evaluation job has completed - possible new issues were detected. A full report can be found here. |
|
PackageEval reveals that there are many packages with tests relying on parsing MethodError. |
Apply these changes if JuliaLang/julia#47369 gets merged. The error message for MethodError currently applies to both calls to functions and `invoke()`, but did not specify which the error came from. Fixing this will break some tests, hence this PR.
Apply these changes if JuliaLang/julia#47369 gets merged. The error message for MethodError currently applies to both calls to functions and `invoke()`, but did not specify which the error came from. Fixing this will break some tests, hence this PR.
Apply these changes if JuliaLang/julia#47369 gets merged. The error message for MethodError currently applies to both calls to functions and `invoke()`, but did not specify which the error came from. Fixing this will break some tests, hence this PR.
Apply these changes if JuliaLang/julia#47369 gets merged. The error message for MethodError currently applies to both calls to functions and `invoke()`, but did not specify which the error came from. Fixing this will break some tests, hence this PR.
Apply these changes if JuliaLang/julia#47369 gets merged. The error message for MethodError currently applies to both calls to functions and `invoke()`, but did not specify which the error came from. Fixing this will break some tests, hence this PR.
The text of the MethodError message is not a stable API, so use the currently stable API of checking the field value. Discussion at JuliaLang/julia#47369. Co-authored-by: Jameson Nash <[email protected]>
|
Do we want to update this and get it merged? |
|
@ViralBShah yes, pushed an update with removing some changes and adding others. Just waiting for final review (of the exact text, not the implementation), if you want to look at it? |
vtjnash
added
merge me
The `MethodError` printing has detailed text on what failed about this dispatch, which can be useful to someone trying to understand why `which` failed, given that `which` is simply a model of the dispatch done by `invoke`. Reuse that printing, but add a short custom header to it and convert to an ErrorException, per triage discussion that this would be clearest. Also add more text to `MethodError` to guide new users in interpreting the exact meaning of some of the cryptic text. Fixes JuliaLang#47322
|
CI: tests passed, ASAN timeout is unrelated. |
Error messages for
MethodErrorare much more helpful in determining why the method was not successfully dispatched than simply "No unique matching method found."Fixes #47322