Brittany can't cope with -XTupleSections #41

New Issue

2017-08-05T14:38:19+02:00

fmthoma commented

2017-08-05 14:38:19 +02:00

(Migrated from github.com)

Config:

…
conf_forward:
  options_ghc:
    - -XLambdaCase
    - -XMultiWayIf
    - -XGADTs
    - -XPatternGuards
    - -XViewPatterns
    - -XRecursiveDo
    - -XTupleSections
    - -XExplicitForAll
    - -XImplicitParams
    - -XQuasiQuotes
    - -XTemplateHaskell
    - -XBangPatterns
…

Input:

foo = fmap (, ())

Error:

ERROR: encountered unknown syntactical constructs:
ExplicitTuple|..

Config: ```yaml … conf_forward: options_ghc: - -XLambdaCase - -XMultiWayIf - -XGADTs - -XPatternGuards - -XViewPatterns - -XRecursiveDo - -XTupleSections - -XExplicitForAll - -XImplicitParams - -XQuasiQuotes - -XTemplateHaskell - -XBangPatterns … ``` Input: ```haskell foo = fmap (, ()) ``` Error: ``` ERROR: encountered unknown syntactical constructs: ExplicitTuple|.. ```

matthew-piziak commented

2017-12-13 19:10:32 +01:00

(Migrated from github.com)

Hey @lspitzner! I'd like to take a crack at solving this one. Where should I start?

lspitzner commented

2017-12-14 00:53:20 +01:00

(Migrated from github.com)

@matthew-piziak great! There are several things to look at

https://github.com/lspitzner/brittany/blob/master/doc/implementation/bridoc-design.md
and the other docs in that folder.
the relevant syntax trees. For example the output of echo "func = (13, 14)" | brittany --dump-ast-full, especially compared to the output for a tuplesection.
The ExplicitTuple case in Expr.hs. The unknownNodeError "ExplicitTuple|.." lexpr line is the one that will need a better implementation. Perhaps add some docDebug "mylabel" nodes inside the existing code for tuples, and see how that behaves. No other file will need changing.

I am too familiar with the codebase to have a good feeling what part needs more explanation, so feel free to ask.

@matthew-piziak great! There are several things to look at 1) https://github.com/lspitzner/brittany/blob/master/doc/implementation/bridoc-design.md and the other docs in that folder. 2) the relevant syntax trees. For example the output of `echo "func = (13, 14)" | brittany --dump-ast-full`, especially compared to the output for a tuplesection. 3) The `ExplicitTuple` case in `Expr.hs`. The `unknownNodeError "ExplicitTuple|.." lexpr` line is the one that will need a better implementation. Perhaps add some `docDebug "mylabel"` nodes inside the existing code for tuples, and see how that behaves. No other file will need changing. I am too familiar with the codebase to have a good feeling what part needs more explanation, so feel free to ask.

matthew-piziak commented

2017-12-14 21:32:48 +01:00

(Migrated from github.com)

Thank you for the warm introduction, @lspitzner!

I am trying to transform the following guard:

    | Just argExprs <- args `forM` (\case (L _ (Present e)) -> Just e; _ -> Nothing)

As I read it this traversal only succeeds if all tuple arguments are Present.

Presumably the goal is to replace _ -> Nothing (fail on empty sections) with (L _ (Missing Placeholder)) -> ?. The problem is, I don't know what sort of HsExpr that would correspond to. It seems like the layout code should mostly also work for tuple sections. I feel like there is a kind of "empty" HsExpr that I should be putting in the ? place. Any advice?

Thank you for the warm introduction, @lspitzner! I am trying to transform the following guard: ``` | Just argExprs <- args `forM` (\case (L _ (Present e)) -> Just e; _ -> Nothing) ``` As I read it this traversal only succeeds if all tuple arguments are `Present`. Presumably the goal is to replace `_ -> Nothing` (fail on empty sections) with `(L _ (Missing Placeholder)) -> ?`. The problem is, I don't know what sort of `HsExpr` that would correspond to. It seems like the layout code should _mostly_ also work for tuple sections. I feel like there is a kind of "empty" `HsExpr` that I should be putting in the `?` place. Any advice?

matthew-piziak commented

2017-12-14 21:45:02 +01:00

(Migrated from github.com)

Or maybe we want layoutExpr for all the args except the Missing ones, which should be emptyDoc?

Or maybe we want `layoutExpr` for all the args except the `Missing` ones, which should be `emptyDoc`?

matthew-piziak commented

2017-12-14 21:57:34 +01:00

(Migrated from github.com)

I tried naïvely transforming this...

| Just argExprs <- args `forM` (\case (L _ (Present e)) -> Just e; _ -> Nothing) -> do
argDocs <- docSharedWrapper layoutExpr `mapM` argExprs

...to this:

| argExprs  <- fmap (\case (L _ (Present e)) -> Just e; (L _ (Missing PlaceHolder)) -> Nothing) args -> do
argDocs <- docSharedWrapper (fmap $ maybe docEmpty layoutExpr) argExprs

But the types are tricky for me:

    • Couldn't match type ‘[]’
                     with ‘MultiRWSS.MultiRWST
                             '[Config, Language.Haskell.GHC.ExactPrint.Types.Anns]
                             '[[BrittanyError], Seq String]
                             '[NodeAllocIndex]
                             Identity’
      Expected type: MultiRWSS.MultiRWST
                       '[Config, Language.Haskell.GHC.ExactPrint.Types.Anns]
                       '[[BrittanyError], Seq String]
                       '[NodeAllocIndex]
                       Identity
                       [ToBriDocM BriDocNumbered]
        Actual type: [[ToBriDocM BriDocNumbered]]
    • In a stmt of a 'do' block:
        argDocs <- docSharedWrapper
                     (fmap $ maybe docEmpty layoutExpr) argExprs

I tried naïvely transforming this... ``` | Just argExprs <- args `forM` (\case (L _ (Present e)) -> Just e; _ -> Nothing) -> do argDocs <- docSharedWrapper layoutExpr `mapM` argExprs ``` ...to this: ``` | argExprs <- fmap (\case (L _ (Present e)) -> Just e; (L _ (Missing PlaceHolder)) -> Nothing) args -> do argDocs <- docSharedWrapper (fmap $ maybe docEmpty layoutExpr) argExprs ``` But the types are tricky for me: ``` • Couldn't match type ‘[]’ with ‘MultiRWSS.MultiRWST '[Config, Language.Haskell.GHC.ExactPrint.Types.Anns] '[[BrittanyError], Seq String] '[NodeAllocIndex] Identity’ Expected type: MultiRWSS.MultiRWST '[Config, Language.Haskell.GHC.ExactPrint.Types.Anns] '[[BrittanyError], Seq String] '[NodeAllocIndex] Identity [ToBriDocM BriDocNumbered] Actual type: [[ToBriDocM BriDocNumbered]] • In a stmt of a 'do' block: argDocs <- docSharedWrapper (fmap $ maybe docEmpty layoutExpr) argExprs ```

lspitzner commented

2017-12-14 22:01:24 +01:00

(Migrated from github.com)

Right, that guard currently filters out the "all are Just" case. Your last comment is definitely the right direction (while creating a new HsExpr was not, after all this code should turn AST into Bridoc structure, so docEmpty is a much better idea). Let me test that bit myself, one sec.

Right, that guard currently filters out the "all are Just" case. Your last comment is definitely the right direction (while creating a new HsExpr was not, after all this code should turn AST into Bridoc structure, so `docEmpty` is a much better idea). Let me test that bit myself, one sec.

matthew-piziak commented

2017-12-14 22:08:22 +01:00

(Migrated from github.com)

This sort of works!

      | argExprs  <- fmap (\case (L _ (Present e)) -> Just e; (L _ (Missing PlaceHolder)) -> Nothing) args -> do
      argDocs <- return (maybe docEmpty layoutExpr <$> argExprs)

This sort of works! ``` | argExprs <- fmap (\case (L _ (Present e)) -> Just e; (L _ (Missing PlaceHolder)) -> Nothing) args -> do argDocs <- return (maybe docEmpty layoutExpr <$> argExprs) ```

matthew-piziak commented

2017-12-14 22:09:37 +01:00

(Migrated from github.com)

At least, it compiles, and transforms func = (14,) to func = (14, ). Progress! I worry that I removed docSharedWrapper, and of course special logic needs to be added to tighten up the sections (since (14,) is probably preferable to (14, )).

At least, it compiles, and transforms `func = (14,)` to `func = (14, )`. Progress! I worry that I removed `docSharedWrapper`, and of course special logic needs to be added to tighten up the sections (since `(14,)` is probably preferable to `(14, )`).

lspitzner commented

2017-12-14 22:12:51 +01:00

(Migrated from github.com)

Yeah, you actually want the wrapper, otherwise this becomes exponential. At least in theory. But complexity is always theory. Use this:

argDocs <- docSharedWrapper (maybe docEmpty layoutExpr) `mapM` argExprs

(Oh and writing stuff in the guard is not necessary anymore, because.. it is not guarding anything anymore.)

But you seem on the right track, the special logic is indeed the only remaining bit.

Yeah, you actually want the wrapper, otherwise this becomes exponential. At least in theory. But complexity is always theory. Use this: ~~~~.hs argDocs <- docSharedWrapper (maybe docEmpty layoutExpr) `mapM` argExprs ~~~~ (Oh and writing stuff in the guard is not necessary anymore, because.. it is not guarding anything anymore.) But you seem on the right track, the special logic is indeed the only remaining bit.

lspitzner commented

2017-12-14 22:14:47 +01:00

(Migrated from github.com)

or wait, is this nonsense in this case? i am not certain. one sec.

matthew-piziak commented

2017-12-14 22:20:00 +01:00

(Migrated from github.com)

I don't suppose there's an analog of docEmpty that slurps up surrounding whitespace?

I don't suppose there's an analog of `docEmpty` that slurps up surrounding whitespace?

matthew-piziak commented

2017-12-14 22:23:11 +01:00

(Migrated from github.com)

For now I'll just try to match the docEmpty elements in the logic for FirstLast.

For now I'll just try to match the `docEmpty` elements in the logic for `FirstLast`.

lspitzner commented

2017-12-14 22:23:25 +01:00

(Migrated from github.com)

nah, it is not nonsense. use the wrapper version. the difference is approximately the difference between:

printAction m = m >>= print
do
  printAction getLine
  printAction getLine
-- versus
do
  x <- pure <$> getLine -- we execute `getLine` once and place (pure line) in `x`
  printAction x
  printAction x

And we want the version that allocates a new bridoc node (i.e. executes getLine) only once.

(hmm - i should copy this explanation to the docs.)

nah, it is not nonsense. use the wrapper version. the difference is approximately the difference between: ~~~~.hs printAction m = m >>= print do printAction getLine printAction getLine -- versus do x <- pure <$> getLine -- we execute `getLine` once and place (pure line) in `x` printAction x printAction x ~~~~ And we want the version that allocates a new bridoc node (i.e. executes `getLine`) only once. (hmm - i should copy this explanation to the docs.)

lspitzner commented

2017-12-14 22:25:39 +01:00

(Migrated from github.com)

Maybe you want to carry the Maybe along longer. You can always create the docEmpty later.

Maybe you want to carry the `Maybe` along longer. You can always create the `docEmpty` later.

lspitzner commented

2017-12-14 22:31:06 +01:00

(Migrated from github.com)

have you considered different cases? how many spaces would you insert for:

(1,2,)
(,2,3)
(1,,3)

?

(given that the full tuple would currently be printed as (1, 2, 3))

have you considered different cases? how many spaces would you insert for: ~~~~.hs (1,2,) (,2,3) (1,,3) ~~~~ ? (given that the full tuple would currently be printed as `(1, 2, 3)`)

matthew-piziak commented

2017-12-14 22:48:54 +01:00

(Migrated from github.com)

Good question. Here are a few, but it's a hard decision.

-- no spaces
(,,)
(1,,)
(,2,)
(,,3)
(1,2,)
(,2,3)
(1,,3)
(1,2,3)

-- space after comma
(, , )
(1, , )
(, 2, )
(, , 3)
(1, 2, )
(, 2, 3)
(1, , 3)
(1, 2, 3)

-- space between comma and value
(,,)
(1,,)
(, 2,)
(,, 3)
(1, 2,)
(, 2, 3)
(1,, 3)
(1, 2, 3)

-- space between value-comma and value
(,,)
(1,,)
(,2,)
(1, 2,)
(, 2, 3)
(1,,3)
(1, 2, 3)

-- no spaces except for full tuples
(,,)
(1,,)
(,2,)
(,,3)
(1,2,)
(,2,3)
(1,,3)
(1, 2, 3)

Good question. Here are a few, but it's a hard decision. ``` -- no spaces (,,) (1,,) (,2,) (,,3) (1,2,) (,2,3) (1,,3) (1,2,3) -- space after comma (, , ) (1, , ) (, 2, ) (, , 3) (1, 2, ) (, 2, 3) (1, , 3) (1, 2, 3) -- space between comma and value (,,) (1,,) (, 2,) (,, 3) (1, 2,) (, 2, 3) (1,, 3) (1, 2, 3) -- space between value-comma and value (,,) (1,,) (,2,) (1, 2,) (, 2, 3) (1,,3) (1, 2, 3) -- no spaces except for full tuples (,,) (1,,) (,2,) (,,3) (1,2,) (,2,3) (1,,3) (1, 2, 3) ```

matthew-piziak commented

2017-12-14 23:04:04 +01:00

(Migrated from github.com)

I'm a fan of this one personally:

-- space between comma and value (with words)
(,,)
(one,,)
(, two,)
(,, three)
(one, two,)
(, two, three)
(one,, three)
(one, two, three)

What do you think?

I'm a fan of this one personally: ``` -- space between comma and value (with words) (,,) (one,,) (, two,) (,, three) (one, two,) (, two, three) (one,, three) (one, two, three) ``` What do you think?

lspitzner commented

2017-12-14 23:32:56 +01:00

(Migrated from github.com)

i don't have much of an opinion. I like the symmetry of space-after-comma, but for

(1, , , )
-- or
(1,,,)

i might agree with the latter.

Really, I don't want to decide this. If it is not grossly inconsistent (and your alternatives all follow some clear rule), i'm fine with anything (that retains the full-tuple behaviour). Maybe you could ask other users of -XTupleSections.

i don't have much of an opinion. I like the symmetry of space-after-comma, but for ~~~~.hs (1, , , ) -- or (1,,,) ~~~~ i might agree with the latter. Really, I don't want to decide this. If it is not grossly inconsistent (and your alternatives all follow some clear rule), i'm fine with anything (that retains the full-tuple behaviour). Maybe you could ask other users of `-XTupleSections`.

lspitzner commented

2017-12-14 23:36:51 +01:00

(Migrated from github.com)

oh, and we have not considered the multiline case yet :p

myTupleSection =
  ( verylaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaargefirstelement
  ,
  , verylaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaargethirdelement
  ,
  )

oh, and we have not considered the multiline case yet :p ~~~~.hs myTupleSection = ( verylaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaargefirstelement , , verylaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaargethirdelement , ) ~~~~

matthew-piziak commented

2017-12-15 00:02:17 +01:00

(Migrated from github.com)

Well space-after-comma is done by default, so that's an advantage, haha.

😆 1

matthew-piziak commented

2017-12-15 00:05:08 +01:00

(Migrated from github.com)

Your long-elements example renders okay IMO:

myTupleSection =
  ( verylaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaargefirstelement
  ,
  , verylaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaargethirdelement
  ,
  )

Your long-elements example renders okay IMO: ``` myTupleSection = ( verylaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaargefirstelement , , verylaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaargethirdelement , ) ```

matthew-piziak commented

2017-12-15 00:05:21 +01:00

(Migrated from github.com)

Actually it seems to be exactly as you submitted it.

matthew-piziak commented

2017-12-15 00:15:02 +01:00

(Migrated from github.com)

@lspitzner I've put up a tentative PR. Thanks for making it easy to contribute. :)

❤️ 1

lspitzner commented

2017-12-15 00:15:55 +01:00

(Migrated from github.com)

yeah, it probably is the only reasonable choice. although for the equivalent of (verylaaaarge,,,,,) it takes up a lot of space. But I am inclined to say that's the user's problem for overusing tuples. (Also, any collapsing that I can think of would produce rather confusing layout.)

yeah, it probably is the only reasonable choice. although for the equivalent of `(verylaaaarge,,,,,)` it takes up a lot of space. But I am inclined to say that's the user's problem for overusing tuples. (Also, any collapsing that I can think of would produce rather confusing layout.)

👍 1

lspitzner commented

2017-12-15 01:10:55 +01:00

(Migrated from github.com)

btw, i have been toying with ghcjs and put up an experimental brittany web service today. I have updated to include this commit.

although i notice that i don't properly handle errors yet, but let them override the input. Good old -XDeleteSourceOnErrors.

btw, i have been toying with ghcjs and put up an experimental [brittany web service](https://hexagoxel.de/brittany/) today. I have updated to include this commit. although i notice that i don't properly handle errors yet, but let them override the input. Good old `-XDeleteSourceOnErrors`.

lspitzner commented

2017-12-15 01:13:24 +01:00

(Migrated from github.com)

(and no, it does not (and cannot) do the formatting on the clientside. dependency footprint is a bit too heavy for that, and ghcjs cannot build ghc-exactprint (yet).)

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#41

Brittany can't cope with -XTupleSections #41

Brittany can't cope with `-XTupleSections` #41