Skip to content

Fix MethodError while showing MethodError from invoke with UnionAll#47566

Closed
Seelengrab wants to merge 2 commits into
JuliaLang:masterfrom
Seelengrab:invoke_unionall_err
Closed

Fix MethodError while showing MethodError from invoke with UnionAll#47566
Seelengrab wants to merge 2 commits into
JuliaLang:masterfrom
Seelengrab:invoke_unionall_err

Conversation

@Seelengrab

@Seelengrab Seelengrab commented Nov 14, 2022

Copy link
Copy Markdown
Contributor

The old code had a comment about this being either a Tuple or a type, and chose to check for the type. Unwrapping the UnionAll and checking for being a tuple seemed to be the easiest way to fix this, so that's what I did.

Also adds the same comment from showerror to show_method_candidates, since that assumes the same thing.

Fixes #47559

@KristofferC

Copy link
Copy Markdown
Member

Please use descriptive titles and commit messages.

@Seelengrab Seelengrab changed the title Fix #47559 Fix MethodError while showing MethodError from invoke with UnionAll Nov 14, 2022
Comment thread test/errorshow.jl
Comment on lines +937 to +940
@testset "issue #47559" begin
err = try; invoke(Returns, Tuple{Any,Val{N}} where N, 1, Val(1)) catch ex; ex; end
@test startswith(sprint(Base.showerror, err), "MethodError: no method matching Returns(::Any, ::Val{N})")
end

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like a good start. Unfortunately, I have already worked with Aaron to write a more complete version of this PR (there are some additional subtleties to cleanup too around the use of the parameters). As a minor point of reference, for very new issues, you may want to explicitly confirm on Slack that it is not already being worked on, or at least assign yourself to it, to avoid repeating work.

This test may be helpful though, since we had not written that yet (@apaz-cli).

@Seelengrab Seelengrab Nov 15, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha - the errorshow tests ran through locally, so I thought it should work (the test faillures seem to be unrelated - ReadOnlyMemoryError in SparseArrays?). I only looked at the issue in the first place since it had to do with showerror of MethodError, so I thought it was related to the printing changes. Feel free to close this if you've already worked out a better solution than my hacky unwrap_unionall :)

Assigning myself is not really an option without write access, but I'll ping on slack next time! 😅 The test more or less follows the pattern at the start of errorshow.jl, except with the sprint inside of the @test to capture that failure correctly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

issue showing certain invoke MethodErrors with UnionAll

3 participants