-
Notifications
You must be signed in to change notification settings - Fork 271
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
fixes #4239, pretty printing for single arg infix op terms no longer displays "error" #4260
Conversation
…o longer displays "error"
Curiously, this bug does not occur for all symop term definitions with just one provided param. I tried with something like
and all that happened was "let" was stripped out. The function name + params remained correct. |
It's unclear to me why the ormolu container in the CI stage failed. Or, rather, whether something about my PR caused the failure, which is some kind of permissions thing?
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! I made one very minor style change. I'll merge once CI passes.
I'm not sure what's going on with the Ormolu action, maybe @aryairani knows?
We're having some trouble with our nix CI job right now, but it's not anything that needs to block this from being merged 👍🏼 |
Thanks for figuring this out. I hadn't gotten around to the alternative solutions for merging that had been proposed! |
Overview
If one were to add this code:
and then view it, one would see this output:
Observe that the function name+params is just "error" now.
The view should instead output
This closes issue #4239.
Implementation notes
In
TermPrinter.hs
, in some codepaths, the parameters of an infix function are parsed such that if there are two parameters provided, the term definition's LHS is "(symop) p1 p2" and otherwise rendered as the string "error".I added code to handle the case where only one parameter is provided in the LHS of the term definition/binding.
Interesting/controversial decisions
There were two ways I could've gone about this. One, informed by the way the pretty printer renders the definition of an infix function:
As such, I tried to pretty print this first:
However, this is a syntax error because
_
is not a valid param it seems.So instead I chose to render as a prefix symop with one param,
(>>>>) n = ...
instead.Test coverage
I added a transcript test that will ensure that the formerly-buggy code produces now
instead of the former, where "error" was in place of "(>>>>) n".