group should be more idempotent

[sjakobi]

Currently we have:

> diag $ group . group . group $ "x" <> line
Union 
 ( Cat ( Char 'x' ) ( Char ' ' ) ) 
 ( Union 
 ( Cat ( Char 'x' ) ( Char ' ' ) ) 
 ( Union 
 ( Cat ( Char 'x' ) ( Char ' ' ) ) 
 ( Cat ( Char 'x' ) 
 ( FlatAlt Line ( Char ' ' ) )
 )
 )
 )

It would be nice if we could get just

Union 
 ( Cat ( Char 'x' ) ( Char ' ' ) ) 
 ( Cat ( Char 'x' ) 
 ( FlatAlt Line ( Char ' ' ) )
 )

instead. This patch should do the trick:

@@ -523,6 +523,7 @@ hardline = Line
 -- use of it.
 group :: Doc ann -> Doc ann
 -- See note [Group: special flattening]
+group x@Union{} = x
 group x = case changesUponFlattening x of
 Nothing -> x
 Just x' -> Union x' x

But maybe we could even get this?!

Union 
 ( Cat ( Char 'x' ) ( Char ' ' ) ) 
 ( Cat ( Char 'x' ) Line )

Related: #99, #112, #115.

[sjakobi]

But maybe we could even get this?!

Union 
 ( Cat ( Char 'x' ) ( Char ' ' ) ) 
 ( Cat ( Char 'x' ) Line )

And why not

Cat ( Char 'x' ) ( Union ( Char ' ' ) Line )

?

[sjakobi]

Regarding the ideas to move the Union constructor closer to the "division", I think this might cause work that e.g. layoutCompact wouldn't profit from.

[1Jajen1]

But maybe we could even get this?!

Union 
 ( Cat ( Char 'x' ) ( Char ' ' ) ) 
 ( Cat ( Char 'x' ) Line )

And why not

Cat ( Char 'x' ) ( Union ( Char ' ' ) Line )

?

I think that within changesUponFlattening one should have enough information present to do this.
Probably by creating the Union right at the point where we see flattening happen (e.g. on FlatAlt, Line) instead of letting the fact that we have changed bubble up. This would lead to a very compact doc without extra cost. Just an idea, I'll play around with that in the next few days if I have the time ^^

[1Jajen1]

So the difficulty here are cases like hardline <> linebreak. This cannot flatten, so treating both sides of Cat individually does not work. So this means as soon as we hit Cat we must verify that the left side does not try to flatten a Line. This removes the need for a Fail constructor entirely!

In my testing (in the kotlin version) I have two methods, one group which simply inserts Union as soon as it hits FlatAlt and recurses in almost any other case. The only interesting other cases are Union (where the Union is just returned as is) and Cat in which I evaluate the left side with groupNoLine :: Doc a -> Maybe (Doc a) where Nothing indicates that it contains a Line and Just x contains the flattened document x.

I am unsure on how this affects performance because I have no good benchmarks yet ^^
I'll try make a pr with this and you can try this with the benchmarks present here :)

[sjakobi]

So the difficulty here are cases like hardline <> linebreak.

What's linebreak here? prettyprinter doesn't export it…

I had previously also thought that we could get rid of the Fail constructor. See #116 (comment) and the following comments. But it's not fully clear to me why changesUponFlattening leaves some work to flatten, instead of really checking the whole document. I'm not sure how it could work out for Column and friends though…

I'm looking forward to your experiment in any case – even if it doesn't work out, we'll know more and can document it! :)

[1Jajen1]

What's linebreak here? prettyprinter doesn't export it…

My bad! Came from the kotlin version where I needed to rename line' because that doesn't play nice there ^^

[1Jajen1]

The function wrappers Column, Nesting and WithPageWidth made this a bit more complex, but overall I quite like this approach ^^

[sjakobi]

Closing since 51ec1d1 addressed the original issue.

I'm still excited to see whether #126 or similar approaches can improve things! :)