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

make type check errors use the to-string() method directly #1527

Open
wants to merge 2 commits into
base: horizon
Choose a base branch
from

Conversation

jpolitz
Copy link
Member

@jpolitz jpolitz commented Sep 7, 2020

It looks like constructor printing used to be overridden for the sake
of errors. However, this is silly because we want constructor printing
for spying on types and so on within the compiler.

So make the error rendering contexts use to-string explicitly.

Down the line this should be a richer renderable type struct, but
ED.embed does constructor printing as well so stringifying with
the existing stringifier is better.

@blerner @mkolosick can you do a brief review of this to make sure I'm not missing something obvious?

It looks like constructor printing used to be overridden for the sake
of errors. However, this is silly because we want constructor printing
for spying on types and so on within the compiler.

So make the error rendering contexts use to-string explicitly.

Down the line this should be a richer renderable type struct, but
ED.embed does constructor printing as well so stringifying with
the existing stringifier is better.
@jpolitz
Copy link
Member Author

jpolitz commented Sep 7, 2020

#1525

@jpolitz jpolitz mentioned this pull request Sep 7, 2020
@blerner
Copy link
Member

blerner commented Sep 7, 2020

I had commented (b5dfaa3#r41383423) about this and committed (92ec0c0) a fix, and this PR seems to take a different approach. (In any case, #1525 is effectively currently fixed on horizon, without this PR.) I don't quite understand "However, this is silly because we want constructor printing for spying on types and so on within the compiler" though -- why? I got the sense that since this method was simply commented out, that it was a straggler of a commit that didn't mean to be committed.

I'd be happier with the inverse of this PR, I think: make to-string default behavior be pretty-printing, and add a .to-repr method, or a .debug-repr method, if we really need the constructor-level printing. The common case of rendering the error seems to be way more common than the hypothetical case of examining types, just based on the size of the PR and the fiddliness of "did I fix all of them?"...

@blerner
Copy link
Member

blerner commented Sep 7, 2020

If we do go with this PR, then you'll need to revert that commit, or else you won't get back your constructor printing, anyway...

@jpolitz
Copy link
Member Author

jpolitz commented Sep 7, 2020

The issue with overriding _output is that it then becomes super annoying to get all the source location printing, etc back. We'd end up implementing a really big method to show the same information we get from the un-overridden torepr. Seems better to have a dedicated method for rendering for users (it can upgrade to a valueskeleton or similar in the future if needed) and still give us the option of getting all the detailed source locations without rewriting a few hundred lines that need to be maintained just to get that back.

@jpolitz
Copy link
Member Author

jpolitz commented Sep 7, 2021

To follow on to this discussion – I just ran into a situation where I wanted to print a string-dict that contained types. That is really onerous to manage with a specialized non-_output method for errors. This convinces me that I think _output needs to be the constructor printing, and that _output should be overridden only to give more helpful constructor printing, not for different purposes.

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.

None yet

2 participants