Fix binders that need to be wrapped in parens #8
Conversation
src/Tidy/Codegen.purs
Outdated
| addParens bnds' = bnds' <#> \b -> case b of | ||
| BinderTyped _ _ _ -> binderParens b | ||
| BinderOp _ _ -> binderParens b | ||
| BinderInt (Just _) _ _ -> binderParens b | ||
| BinderNumber (Just _) _ _ -> binderParens b | ||
| BinderConstructor _ vars | not $ Array.null vars -> binderParens b | ||
| _ -> b |
There was a problem hiding this comment.
I think that this should be precBinder2, which should be fixed to use the same guard on BinderConstructor.
There was a problem hiding this comment.
I decided not to reuse precBinder2 here or update it to use the same guard because it can be used in a caseExpr:
case _ of
NoParensHere a ->
-- vs
\(ParensHere a) ->There was a problem hiding this comment.
OK, then we should move this to another prec function, and reuse it where appropriate. In the parser, this corresponds to all usages of binderAtom (https://github.com/purescript/purescript/blob/53b4117045f5517023f5d22f9fa506e651e4ff3e/lib/purescript-cst/src/Language/PureScript/CST/Parser.y#L575-L586). Seems like there's quite a few places where we should be using this precedence level.
src/Tidy/Codegen.purs
Outdated
| (toGuarded tokRightArrow rhs) | ||
| where | ||
| addParens b = case b of |
There was a problem hiding this comment.
Ah, good point.
src/Tidy/Codegen.purs
Outdated
| @@ -896,7 +909,7 @@ binderVar = BinderVar <<< toName | |||
|
|
|||
| -- | Constructs a named binding pattern (`@`). | |||
| binderNamed :: forall e name. ToName name Ident => name -> Binder e -> Binder e | |||
| binderNamed n = BinderNamed (toName n) tokAt | |||
| binderNamed n = BinderNamed (toName n) tokAt <<< binderParens | |||
There was a problem hiding this comment.
This should be precBinder2 rather than binderParens, I think.
There was a problem hiding this comment.
Ah.. I thought usage of @ in a binder always required a parens. On the other hand, I don't think precBinder2 is quite right because a constructor doesn't need a parens. I tried this out on Try PureScript and didn't get an error or warning:
foo :: forall a. a -> String
foo = case _ of
a@b -> ""
foo2 :: forall a. a -> String
foo2 = \a@b -> ""
data Foo = Foo
foo3 :: Foo -> String
foo3 = \a@Foo -> ""
This refactoring makes the code more combinator-like, making it easier to test all combinations
`precBinder2` was only being used by the `binderCtor` before this PR
| @@ -181,7 +181,7 @@ precBinder2 a = case a of | |||
| BinderOp _ _ -> binderParens a | |||
| BinderInt (Just _) _ _ -> binderParens a | |||
| BinderNumber (Just _) _ _ -> binderParens a | |||
| BinderConstructor _ _ -> binderParens a | |||
| BinderConstructor _ args | not $ Array.null args -> binderParens a | |||
There was a problem hiding this comment.
I found that precBinder2 is only used for the binderCtor. So, rather than defining a new precBinder3, I just updated precBinder2 here.
|
Thanks! |
Fixes #7. I also fixed a similar issue with
exprCaseandbinderTyped