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 printing braces in effects #385

Merged
merged 1 commit into from Mar 14, 2019

Conversation

Projects
None yet
2 participants
@mrdziuban
Copy link
Contributor

mrdziuban commented Mar 14, 2019

@aryairani this should fix #375. Given the code in multiple-effects.u, the output is:

-- Before
ability Console
ability State s
Console.state : s -> Effect (State s) a -> a
multiHandler : s -> [w] -> Nat -> Effect State s, Console a -> ()

-- After
ability Console
ability State s
Console.state : s -> Effect (State s) a -> a
multiHandler : s -> [w] -> Nat -> Effect {State s, Console} a -> ()

I just had one question — I haven't fully grokked when it's necessary to pass 0 or 3 to go, but I tried changing it to 3 in this case and got this for multiHandler:

multiHandler : s -> [w] -> Nat -> Effect {(State s), Console} a -> ()

Is having the parentheses around State s desirable since it's printed with parentheses in the Console.state type?

@pchiusano pchiusano merged commit 41a14be into unisonweb:master Mar 14, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@pchiusano

This comment has been minimized.

Copy link
Member

pchiusano commented Mar 14, 2019

That number is the “ambient precedence” - generally you only want to put parens if the immediately surrounding expression has higher precedence. The general idea is this ambient precedence follows the precedence of the parser, so the prettyprinter prints things with a minimum of parens needed to produce the same AST.

In this case, the parens inside the ability list are redundant so I think it’s right to pass 0 as the ambient precedence.

Hope that is helpful!

@mrdziuban mrdziuban deleted the mrdziuban:effect-printer branch Mar 14, 2019

@mrdziuban

This comment has been minimized.

Copy link
Contributor Author

mrdziuban commented Mar 14, 2019

Thanks @pchiusano, that does help!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.