prettyprinter: generalise alterAnnotationsS to lists

[ners]

We can very simply support reannotating to a list of annotations if we store a counter instead of a boolean in the stack.

[sjakobi]

Since this is a breaking change, it would be good to understand the motivation behind it. Could you explain why you want this change?

[ners]

Hey @sjakobi, thank you for the speedy reply.

The background of the motivation is a little bit involved:

• I maintain the following library which uses a combination of prettyprinter and terminal: https://github.com/ners/terminal-widgets
  The prettyprinter annotation in use here is Attribute m, an associated data type to the MonadMarkupPrinter typeclass.
• I've authored another library which produces SimpleDocStream ann from skylighting output.
  The intention of this library is having skylighted inputs in terminal-widgets.
• Finally, I've also written a CLI tool that uses terminal-widgets to construct an API query and then pretty-prints the JSON result using the aforementioned skylighting prettyprinter and aeson-pretty.
• To print the result, I use prettyprinter-ansi-terminal, which has a different annotation type: AnsiStyle
• The mapping from terminal's Attribute m to AnsiStyle is nontrivial because you don't really know what Attribute m is.
• The mapping from AnsiStyle to Attribute m is easy to implement if we are able to have this fanout.

I've implemented the function first in my own codebase, to confirm that it works as intended.

Then I researched the Prettyprinter codebase to see why such a function does not yet exist. The alterAnnotationsS is a natural fit, but for some reason it is implemented with a boolean stack. This change gives it power parity with alterAnnotations, which means we can remove all the comments that warn users that it's less powerful.

I think it's worth the breaking change to generalise this function. That's what semver is for after all. Another possibility is exposing this a separate function, but then I'd question the need for alterAnnotationsS.

[sjakobi]

I agree that this would improve the API.

What I'd like to be more sure of:

• Is this change correct? Unfortunately there's only a single test for alterAnnotationsS in the testsuite. Are there any properties that we could test for alterAnnotationsS?
• Is the performance good? In particular I'm suspicious of the length bit – it seems a bit unfortunate that the result list has to be traversed twice. I don't see an obvious way to avoid that though. Maybe n should be strict?!

[sjakobi]

• Are there any properties that we could test for alterAnnotationsS?

I think one property that we'd want is alterAnnotations f . render === render . alterAnnotationsS f. Could you add a test for this, @ners?

[ners]

Hey @sjakobi

Apologies for the long wait, I haven't forgotten about this PR. 🙂

I've added three property tests that I could think of. I wasn't sure where to put them, so I made a new test group for them.

I've played around with ways to remove the double traversal, making n strict, etc. but could not get any measurable difference in performance, just in code complexity.

I've also rebased the PR to the latest master.

---

When running my properties with many thousands of QuickCheck runs, I came across a potential bug in how layoutWadlerLeijen picks the indentation of blank lines. If an annotation sits right after a line break, the indentation it picks can be wrong.

It's in layoutWadlerLeijen.best, the Line case. It decides a line is blank (and uses indentation 0) by checking if SEmpty or another SLine comes next. It doesn't look through SAnnPush, so if an annotation comes right after the line break, it falls back to the real nesting level instead, even if the annotation wraps nothing.

This is not caused by this PR, you can try this repro on master:

ghci> layoutPretty defaultLayoutOptions (nest 4 (line <> annotate () mempty))
SLine 4 (SAnnPush () (SAnnPop SEmpty))

ghci> layoutPretty defaultLayoutOptions (nest 4 (line <> mempty))
SLine 0 SEmpty

In both cases the result should have been SLine 0 SEmpty.

I can open a PR with a fix, but I'd like to base it on this one when it is merged.

[sjakobi]

Regarding the bug, do you think this is something that could cause "real" problems in real code?

I believe the same buggy logic is the source of of a bad perf bug: #205. I'm still unsure what the best fix would be. I'll try to lay out the options and maybe I could then get your feedback on it?

Meanwhile please do file an issue for that bug. I think it might be helpful to decide which approach to take for the perf fix.

[sjakobi]

Thank you! I've found some minor perf tweaks that I might add in a follow-up.