error formatting semicolon do notation which contains a let and in #181

New Issue

2018-09-14T12:29:51+02:00

awalterschulze commented

2018-09-14 12:29:51 +02:00

(Migrated from github.com)

Please excuse my haskell, but I am learning.
I just starting using brittany, thank you so much for this tool, I really appreciate it :)

I get an error on the following code:

module Data.Katydid.Relapse.Incorrect (

) where

import Data.Foldable (foldlM)

import Data.Katydid.Parser.Parser
import Data.Katydid.Relapse.Smart
import Data.Katydid.Relapse.Zip
import Data.Katydid.Relapse.IfExprs
import Data.Katydid.Relapse.Derive

zipderiv :: Tree t => Grammar -> [Pattern] -> t -> Either String [Pattern]
zipderiv g ps tree =
    if all unescapable ps 
    then return ps 
    else 
        let ifs = calls g ps
            d = zipderiv g
            nulls = map nullable
    in do {
        childps <- evalIfExprs ifs (getLabel tree);
        (zchildps, zipper) <- return $ zippy childps;
        childres <- foldlM d zchildps (getChildren tree);
        let unzipns = unzipby zipper (nulls childres)
        in return $ returns g (ps, unzipns)
    }

$ brittany Incorrect.hs
ERROR: brittany pretty printer returned syntactically invalid result.

This code has no problem

    in do
        childps <- evalIfExprs ifs (getLabel tree)
        (zchildps, zipper) <- return $ zippy childps
        childres <- foldlM d zchildps (getChildren tree)
        let unzipns = unzipby zipper (nulls childres)
        return $ returns g (ps, unzipns)

And neither does this

        unzipns <- return $ unzipby zipper (nulls childres);
        return $ returns g (ps, unzipns)

which makes me suspect the problem is in reformatting the do with curlies, which contains a let and in
This is low priority, since I can fix the code myself, but I thought I'd report it anyway.

Please excuse my haskell, but I am learning. I just starting using brittany, thank you so much for this tool, I really appreciate it :) I get an error on the following code: ```haskell module Data.Katydid.Relapse.Incorrect ( ) where import Data.Foldable (foldlM) import Data.Katydid.Parser.Parser import Data.Katydid.Relapse.Smart import Data.Katydid.Relapse.Zip import Data.Katydid.Relapse.IfExprs import Data.Katydid.Relapse.Derive zipderiv :: Tree t => Grammar -> [Pattern] -> t -> Either String [Pattern] zipderiv g ps tree = if all unescapable ps then return ps else let ifs = calls g ps d = zipderiv g nulls = map nullable in do { childps <- evalIfExprs ifs (getLabel tree); (zchildps, zipper) <- return $ zippy childps; childres <- foldlM d zchildps (getChildren tree); let unzipns = unzipby zipper (nulls childres) in return $ returns g (ps, unzipns) } ``` ``` $ brittany Incorrect.hs ERROR: brittany pretty printer returned syntactically invalid result. ``` This code has no problem ```haskell in do childps <- evalIfExprs ifs (getLabel tree) (zchildps, zipper) <- return $ zippy childps childres <- foldlM d zchildps (getChildren tree) let unzipns = unzipby zipper (nulls childres) return $ returns g (ps, unzipns) ``` And neither does this ```haskell unzipns <- return $ unzipby zipper (nulls childres); return $ returns g (ps, unzipns) ``` which makes me suspect the problem is in reformatting the `do` with curlies, which contains a `let` and `in` This is low priority, since I can fix the code myself, but I thought I'd report it anyway.

lspitzner commented

2018-09-14 17:59:37 +02:00

(Migrated from github.com)

thanks for reporting.

minimal, currently broken testcase for this is

func = do
  let bind = sufficiently loooooooooooooooooooooooooooooooooooooong expression
    in bind

which i suppose we want to be layouted exactly like that (?)

Two approaches:

Fix the layouter for let..in to not assume that the indentation of let and in is always allowed to be the same

Rewrite the syntax-tree to

func = do
  let bind = sufficiently loooooooooooooooooooooooooooooooooooooong expression
  bind

Both of which have annoying downsides, afaict.

On a related note, I just noticed that supporting brace style is more complex than I had expected, because

func = do {
  let bind = sufficiently loooooooooooooooooooooooooooooooooooooong expression;
  bind
}

is not valid haskell. But that is a different topic.

thanks for reporting. minimal, currently broken testcase for this is ~~~~.hs func = do let bind = sufficiently loooooooooooooooooooooooooooooooooooooong expression in bind ~~~~ which i suppose we want to be layouted exactly like that (?) Two approaches: 1) Fix the layouter for `let..in` to not assume that the indentation of `let` and `in` is always allowed to be the same 2) Rewrite the syntax-tree to ~~~~.hs func = do let bind = sufficiently loooooooooooooooooooooooooooooooooooooong expression bind ~~~~ Both of which have annoying downsides, afaict. On a related note, I just noticed that supporting brace style is more complex than I had expected, because ~~~~.hs func = do { let bind = sufficiently loooooooooooooooooooooooooooooooooooooong expression; bind } ~~~~ is _not_ valid haskell. But that is a different topic.

lspitzner commented

2018-09-14 18:02:12 +02:00

(Migrated from github.com)

@awalterschulze would you be opposed to 2), or see downsides to it? (my objection is mostly philosophical, in that it would be a first where brittany actually changed the shape of the AST. And the effect on handling of comments during the transformation will probably be tricky.)

awalterschulze commented

2018-09-14 18:31:35 +02:00

(Migrated from github.com)

I thought the minimally broken test case was with curly braces

func = do {
  let bind = sufficiently loooooooooooooooooooooooooooooooooooooong expression
    in bind
}

And the break happens, because brittany rewrites all curly brace do notations to space based do notations.

I wouldn't mind either way, as long as brittany does not give an error.
But I did find that when I was using the curly brace style, I never wanted to write the in in the first place, I needed to, since as you say, it is not possible to write it without the in
So personally I would prefer 2, but it is rewriting it to space based do notation, which allows you to not write in.

I thought the minimally broken test case was with curly braces ```haskell func = do { let bind = sufficiently loooooooooooooooooooooooooooooooooooooong expression in bind } ``` And the break happens, because brittany rewrites all curly brace do notations to space based do notations. I wouldn't mind either way, as long as brittany does not give an error. But I did find that when I was using the curly brace style, I never wanted to write the `in` in the first place, I needed to, since as you say, it is not possible to write it without the `in` So personally I would prefer 2, but it is rewriting it to space based do notation, which allows you to not write `in`.

awalterschulze commented

2018-09-14 18:36:18 +02:00

(Migrated from github.com)

On the other hand, maybe removing in is more of a thing that hlint should recommend.

On the other hand, maybe removing `in` is more of a thing that `hlint` should recommend.

lspitzner commented

2018-09-14 20:18:39 +02:00

(Migrated from github.com)

both testcases are valid. both are valid haskell, and are currently transformed into (the same) non-valid haskell by brittany.

On the other hand, maybe removing in is more of a thing that hlint should recommend.

yeah, that matches my reservation.

I'll give this a bit of time, perhaps someone can think of a better alternative. But it should definitely be fixed.

both testcases are valid. both are valid haskell, and are currently transformed into (the same) non-valid haskell by brittany. > On the other hand, maybe removing in is more of a thing that hlint should recommend. yeah, that matches my reservation. I'll give this a bit of time, perhaps someone can think of a better alternative. But it should definitely be fixed.

👍 2

Sign in to join this conversation.

No Milestone

No project

No Assignees

1 Participants

Notifications

Due Date

The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: hexagoxel/brittany#181