Fix non-idempotent comment formatting

[Gabriella439]

The purpose of this change is to fix the following test failure:

    Formatting should be idempotent:                                                                                           FAIL (3.88s)
      *** Failed! Falsified (after 8396 tests and 12 shrinks):
      ASCII
      Header ""
      RecordCompletion (RecordCompletion (BoolLit False) (ImportAlt TextShow (Field (Pi "a" (Field (IntegerLit 0) (FieldSelection {fieldSelectionSrc0 = Nothing, fieldSelectionLabel = "", fieldSelectionSrc1 = Nothing})) Double) (FieldSelection {fieldSelectionSrc0 = Nothing, fieldSelectionLabel = "1aaa", fieldSelectionSrc1 = Nothing})))) (Let (Binding {bindingSrc0 = Just (Src {srcStart = SourcePos {sourceName = "", sourceLine = Pos 17, sourceColumn = Pos 8}, srcEnd = SourcePos {sourceName = "", sourceLine = Pos 6, sourceColumn = Pos 4}, srcText = "--\"zTep.;Bkr \n\n"}), variable = "", bindingSrc1 = Nothing, annotation = Nothing, bindingSrc2 = Nothing, value = NaturalOdd}) (Const Type))
      "( False::(Text/show ? (forall (a : +0.``) -> Double).`1aaa`)\n)::( let --\"zTep.;Bkr\n\n         `` =\n           Natural/odd\n\n     in  Type\n   )\n" /= "(False::(Text/show ? (forall (a : +0.``) -> Double).`1aaa`))::( let --\"zTep.;Bkr\n\n                                                                    `` =\n                                                                      Natural/odd\n\n                                                                in  Type\n                                                              )\n"
      Use --quickcheck-replay=344421 to reproduce.

What's happening is that the test attempts to format this expression

(False::(Text/show ? (forall (a : +0.``) -> Double).`1aaa`))::( let --"zTep.;Bkr 


                                                                    `` =
                                                                      Natural/odd

                                                                in  Type
                                                              )

… where there is a space at column 81 right after the ;Bkr followed by a newline.
The formatter takes that trailing space into account when formatting the expression,
by switching to the "long form" to format the record completion:

( False::(Text/show ? (forall (a : +0.``) -> Double).`1aaa`)
)::( let --"zTep.;Bkr

         `` =
           Natural/odd

     in  Type
   )

… but the formatted result then strips the trailing space from the comment after
formatting (because of the use of Pretty.removeTrailingWhitespace). Then
the second attempt to format the expression goes back to the first
form since the space is gone and now the original form fits in 80 columns.

Normally we would strip trailing whitespace on comments using stripSpaces,
which has hidden this issue in most cases, but the trailing newline after the space on
column 81 protects the space from being stripped, which is how the property test
caught this issue.

The way I worked around this was to strip trailing whitespace from each line of
the comment so that the formatting algorithm would correctly take that into
account while formatting.

[sjakobi]

Nice catch! The fix looks good to me.

I think we should eventually whitespace-strip all comments in a similar way. Then we'll be able to stop using removeTrailingWhitespace.

[Gabriella439]

@sjakobi: Not necessarily. Some whitespace is introduced by the formatting algorithm

[sjakobi]

@sjakobi: Not necessarily. Some whitespace is introduced by the formatting algorithm

That should be fixed as of prettyprinter v1.7.0.

removeTrailingWhitespace might even be removed in the future: quchen/prettyprinter#170