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

Fix MethodError while showing MethodError from invoke with UnionAll #47566

Closed
wants to merge 2 commits into from

Conversation

Seelengrab
Copy link
Contributor

@Seelengrab Seelengrab commented Nov 14, 2022

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
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 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
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).

Copy link
Contributor Author

@Seelengrab Seelengrab Nov 15, 2022

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