haskell / pretty

Haskell Pretty-printer library
Other
69 stars 30 forks source link

Bugfix: overlap and f?(cat|sep) #28

Open thomie opened 8 years ago

thomie commented 8 years ago

The pretty source code currently contains two TODOs:

    -- XXX: TODO: PRETTY: Used to use True here (but GHC used False...)
     nilAboveNest False k (reduceDoc (vcat ys))
    -- XXX: TODO: PRETTY: Used to use True here (but GHC used False...)
     `mkUnion` nilAboveNest False k (fill g (y:ys))

I think we should go back to using True. From https://mail.haskell.org/pipermail/libraries/2008-June/009991.html (commit 1e50748beaa4bd2281d323b18ea51c786bba04a1):

2) Bugfix: overlap and f?(cat|sep)

The specification for cat/sep:
  * oneLiner (hcat/hsep ps)
   `union`
    vcat ps [*]

But currently cat, sep, fcat and fsep attempt to overlap the second  
line with the first one, i.e. they use
`foldr ($$) empty ps' instead of `foldr ($+$) empty ps' [*]. I assume  
this is a mistake.

This bug can lead to situations, where the line in the right argument  
of Union is actually longer:

 > prettyDoc$ cat [ text "a", nest 2 ( text "b") ]
 >> text "a"; union
 >>           (text "b"; empty)
 >>           (nilabove; nest 1; text "b"; empty)

 > renderStyle (Style PageMode 1 1) $ cat [ text "a", nest 2 ( text  
"b") ]
 >> "a b"

In the implementation, we call `nilAbove False' instead of `nilAbove  
True' (see patch).
dterei commented 8 years ago

The problem at this stage is that changing this value will affect the output of some existing programs. I think I'd be OK with the change if we had some good examples and understood what programs will change and why. Right now I don't have a good grasp of that.

Also, in the long run ideally we can unify with GHC. So changing away from GHC is an issue.