Idempotence bug involving moving comments

[mrkkrp]

From formatting parsec3:

Original input:

mergeErrorReply :: ParseError -> Reply s u a -> Reply s u a
mergeErrorReply err1 reply -- XXX where to put it?
    = case reply of
        Ok x state err2 -> Ok x state (mergeError err1 err2)
        Error err2      -> Error (mergeError err1 err2)

1 pass:

mergeErrorReply :: ParseError -> Reply s u a -> Reply s u a
mergeErrorReply err1 reply = -- XXX where to put it?
  case reply of
    Ok x state err2 -> Ok x state (mergeError err1 err2)
    Error err2 -> Error (mergeError err1 err2)

2 pass (fixpoint):

mergeErrorReply :: ParseError -> Reply s u a -> Reply s u a
mergeErrorReply err1 reply =
  -- XXX where to put it?
  case reply of
    Ok x state err2 -> Ok x state (mergeError err1 err2)
    Error err2 -> Error (mergeError err1 err2)

[mrkkrp]

This is actually pretty strange. My mental model is that in all cases the comment should be attached to reply and so it always should stay on the same line.

[utdemir]

Here is a smaller version:

foo reply -- XXX
  = foo

What happens is that when we are deciding whether a comment follows an element, one of the predicates we use is noEltBetween, which checks if the comment follows an element by checking whether the next element starts after the end of the comment. However, in this example, the next element is = foo, and after the first pass we move the = before the comment, so the predicate fails to attach the comment to the same element on the next run.

Similar issues also occur on almost everywhere we move something to a line above. Some examples I made up:

foo = \case
  True -- foo
    -> False
  False -> True

foo
  :: () -- bar
  = foo

a = b $ -- foo
  do
  foo

foo =
  let a -- foo
        = b
   in c

Everything above has idempotence issues.

I couldn't figure out an easy solution. Only thing I can come up with is adding a flag to the located combinator to only conditonally enable comments on some constructs.

[mrkkrp]

However, in this example, the next element is = foo, and after the first pass we move the = before the comment, so the predicate fails to attach the comment to the same element on the next run.

I do not understand is why = between those things would prevent attaching of the comment. I think = by itself is not in the AST and so it should not be in the "span stream" (it could be if we injected things from annotations there, but we currently do not do this).

One thing seems to be clear: while the current approach works decently in most cases, there are many marginal situations where it fails and from what I've seen and investigated, the idempotence issues we're facing right now are not at all easy to fix. Perhaps a completely different approach to comment rendering is necessary here.

[mrkkrp]

OK, more precisely it is not = per se is between the element and the comment, it's the entire match of the match group starting from the equal sign. The comment happens to be inside the match, but it starts after the span of the match, and this condition returns False:

ormolu/src/Ormolu/Printer/Comments.hs

Line 204 in 5c923a2

realSrcSpanStart nspn >= realSrcSpanEnd l

We cannot return True when for example the next element happens to enclose the comment because then other things break.

At least it's clear now what exactly is going on here.