lspitzner / brittany

haskell source code formatter
GNU Affero General Public License v3.0
690 stars 72 forks source link

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

Open awalterschulze opened 6 years ago

awalterschulze commented 6 years ago

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.

lspitzner commented 6 years ago

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:

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

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

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

lspitzner commented 6 years ago

@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 6 years ago

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.

awalterschulze commented 6 years ago

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

lspitzner commented 6 years ago

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.