pyret-lang
pyret-lang copied to clipboard
brownplt
make type check errors use the to-string() method directly
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?
I had commented (https://github.com/brownplt/pyret-lang/commit/b5dfaa321d443c40e409c1ccdd9ae9b3cc1a9dbd#r41383423) about this and committed (https://github.com/brownplt/pyret-lang/commit/92ec0c056e31b34adea64b86aa3172fea86f9f3e) 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?"...
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...
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.
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.