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

fixes #4239, pretty printing for single arg infix op terms no longer displays "error" #4260

Merged
merged 3 commits into from
Aug 17, 2023

Conversation

kylegoetz
Copy link
Contributor

Overview

If one were to add this code:

(>>>>) : Nat -> Nat -> ()
(>>>>) n = cases
  _ -> bug ""

and then view it, one would see this output:

(>>>>) : Nat -> Nat -> ()
 error = cases _ -> bug ""

Observe that the function name+params is just "error" now.

The view should instead output

(>>>>) : Nat -> Nat -> ()
(>>>>) n = cases _ -> bug ""

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:

a >>>> b = ...

As such, I tried to pretty print this first:

n >>> _ = ...

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

(>>>>) n = cases _ -> bug ""

instead of the former, where "error" was in place of "(>>>>) n".

@kylegoetz
Copy link
Contributor Author

Curiously, this bug does not occur for all symop term definitions with just one provided param. I tried with something like

(>>>>) n = let
  a = 5
  b = 2
  ()

and all that happened was "let" was stripped out. The function name + params remained correct.

@kylegoetz
Copy link
Contributor Author

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?

remote: Permission to unisonweb/unison.git denied to github-actions[bot].
[423](https://github.com/unisonweb/unison/actions/runs/5827085920/job/15802381729?pr=4256#step:5:424)
fatal: unable to access 'https://github.com/unisonweb/unison/': The requested URL returned error: 403
[424](https://github.com/unisonweb/unison/actions/runs/5827085920/job/15802381729?pr=4256#step:5:425)
Error: Invalid status code: 128
[425](https://github.com/unisonweb/unison/actions/runs/5827085920/job/15802381729?pr=4256#step:5:426)
    at ChildProcess.<anonymous> (/home/runner/work/_actions/stefanzweifel/git-auto-commit-action/v4/index.js:17:19)
[426](https://github.com/unisonweb/unison/actions/runs/5827085920/job/15802381729?pr=4256#step:5:427)
    at ChildProcess.emit (node:events:527:28)
[427](https://github.com/unisonweb/unison/actions/runs/5827085920/job/15802381729?pr=4256#step:5:428)
    at maybeClose (node:internal/child_process:1092:16)
[428](https://github.com/unisonweb/unison/actions/runs/5827085920/job/15802381729?pr=4256#step:5:429)
    at Process.ChildProcess._handle.onexit (node:internal/child_process:302:5) {
[429](https://github.com/unisonweb/unison/actions/runs/5827085920/job/15802381729?pr=4256#step:5:430)
  code: 128
[430](https://github.com/unisonweb/unison/actions/runs/5827085920/job/15802381729?pr=4256#step:5:431)
}
[431](https://github.com/unisonweb/unison/actions/runs/5827085920/job/15802381729?pr=4256#step:5:432)
Error: Invalid status code: 128
[432](https://github.com/unisonweb/unison/actions/runs/5827085920/job/15802381729?pr=4256#step:5:433)
    at ChildProcess.<anonymous> (/home/runner/work/_actions/stefanzweifel/git-auto-commit-action/v4/index.js:17:19)
[433](https://github.com/unisonweb/unison/actions/runs/5827085920/job/15802381729?pr=4256#step:5:434)
    at ChildProcess.emit (node:events:527:28)
[434](https://github.com/unisonweb/unison/actions/runs/5827085920/job/15802381729?pr=4256#step:5:435)
    at maybeClose (node:internal/child_process:1092:16)
[435](https://github.com/unisonweb/unison/actions/runs/5827085920/job/15802381729?pr=4256#step:5:436)
    at Process.ChildProcess._handle.onexit (node:internal/child_process:302:5)

Copy link
Member

@pchiusano pchiusano left a 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?

@ChrisPenner ChrisPenner merged commit e5c7ff7 into unisonweb:trunk Aug 17, 2023
5 of 7 checks passed
@ChrisPenner
Copy link
Contributor

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 👍🏼

@kylegoetz
Copy link
Contributor Author

kylegoetz commented Aug 17, 2023

Thanks for figuring this out. I hadn't gotten around to the alternative solutions for merging that had been proposed!

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.

3 participants