tweag / ormolu

A formatter for Haskell source code
https://ormolu-live.tweag.io
Other
954 stars 83 forks source link

Differing AST on empty {- ^ -} Haddock markup #1065

Closed tomjaguarpaw closed 1 year ago

tomjaguarpaw commented 1 year ago

Describe the bug

If I have an empty {- ^ -} Haddock annotation then ormolu fails with an AST of input and AST of formatted code differ error.

To Reproduce

$ cat /tmp/example.hs
data T = T {- ^ -}
$ ormolu /tmp/example.hs
/tmp/example.hs
@@ -1,1 +1,3 @@
- data T = T {- ^ -}
+ data T
+   = -- |
+     T

  AST of input and AST of formatted code differ.
    at /tmp/example.hs:1:10
  Please, consider reporting the bug.
  To format anyway, use --unsafe.

Expected behavior

ormolu to produce the same reformatted code as above but to not complain that the ASTs differ (I don't think they do!). {- ^ ... -} is not mentioned in the Haddock documentation as valid Haddock syntax, but it does accept it. Note that if there is text in the markup then everything is fine:

$ cat /tmp/example2.hs
data T = T {- ^ text -}
$ ormolu /tmp/example2.hs
data T
  = -- | text
    T

Environment

$ ormolu --version
ormolu 0.7.1.0
using ghc-lib-parser 9.6.2.20230523

Additional context

ormolu is great, thanks!

Perhaps related: https://github.com/tweag/ormolu/issues/822

amesgen commented 1 year ago

Thanks for the report! The problem is that in the input, the AST contains a Haddock comment (HsDoc construct) with text " ", but the output does not contain a Haddock comment (you can eg see this on Ormolu Live with the "Show internal parse result" option), so the AST diffing reports an error. This somewhat surprising behavior of the GHC parser already came up in #726, and was fixed in #818 by removing all Haddock comments that are completely blank. #818 was missing that operation for Haddock comments in data declarations, which I added in #1068, which fixes this issue.

Hence, with #1068, your example

data T = T {- ^ -}

is formatted to

data T = T

Hope this makes sense!

tomjaguarpaw commented 1 year ago

Thanks for your response @amesgen.

Hope this makes sense!

It makes sense but I think it's the wrong thing to do. See https://github.com/tweag/ormolu/pull/1068#issuecomment-1707205055

tomjaguarpaw commented 1 year ago

Personally I would prefer the approach that AST diffing considers comments consisting of only of any amount of whitespace to be equivalent.

tomjaguarpaw commented 1 year ago

Or, if you want to keep AST diffing as it is, then I think that translating empty -- ^ to empty -- |, but deleting empty {- ^ -} would be satisfactory, because it would provide the escape hatch of using -- |/-- ^ as an alternative to {- ^ -}.

amesgen commented 1 year ago

(Just FTR: There is additional discussion in #1068)