Use 'prettyprinter' library

georgefst commented 4 years ago

This is a big change, and definitely isn't ready to go in as-is, but it's ready for feedback.

All tests should pass right now, but only because I changed some of the expected outputs...

This closes #25 as obsolete, and should make #43, #56 and #66 easier to implement. Indeed I'm going to go off and implement #66 on a separate branch now, seeing as that's what led me to do all this in the first place...

cdepillabout commented 4 years ago

@sjakobi Would you want to review this as well?

georgefst commented 4 years ago

As it happens, I just mentioned it to him on Reddit about two minutes ago.

georgefst commented 4 years ago

Thanks @sjakobi. I was mostly just looking for feedback on the bits I've commented on here for now. I appreciate the full code isn't too easy to follow.

I should probably separate more of exprsToDoc in to annotated and Haddock-ed top-level definitions.

And I'll try to match the old output fully, while keeping in mind how the rules on spacing could be made configurable.

georgefst commented 4 years ago

I've finally found the time to revisit this, but cabal test is failing with:

Running 1 test suites...
Test suite pretty-simple-doctest: RUNNING...
pretty-simple-doctest: /tmp/bios-wrapper9254-14: getPermissions:getFileStatus: does not exist (No such file or directory)
Test suite pretty-simple-doctest: FAIL

I don't think I've changed anything relevant since this worked two weeks ago (I haven't changed anything within pretty-simple at all)... But I'm not very familiar with Cabal-doctest so I don't know how to start debugging this.

@cdepillabout @sjakobi Have you ever seen this sort of failure?

sjakobi commented 4 years ago

@georgefst Sorry, no idea regarding that failure. I mostly use doctest with stack (and without Cabal-doctest).

georgefst commented 4 years ago

This should now be a lot simpler and more readable.

Changes:

3 of the 5 test cases that were previously changed have been reverted:
- We now have the old compact behaviour back with singleton lists and tuples (i.e. no unnecessary newlines).
- The printing of multiline strings is still slightly less compact. I'm sure I could find some way to return the old behaviour if anyone feels strongly about it, but it would complicate things a little for questionable gain.
I've ended up using ViewPatterns and RecordWildCards, but these could be avoided if required, especially the latter.
I dropped the Stream dependency, and implemented a Tape data structure from scratch (for storing the state when colouring brackets). There are libraries on Hackage that could provide this, but it seems that it's always tied up with a lot of other functionality, and brings in a lot of transitive dependencies.

87984b2f and related changes introduce some conflicts, but it looks fairly straightforward to solve.

Finally, the commit history is a little messy. I strongly recommend this be squashed.

cdepillabout commented 4 years ago

@georgefst Could you rebase this on the current master?

georgefst commented 4 years ago

Could you rebase this on the current master?

@cdepillabout

Done, but there's one test failure. It looks like pretty-simple was previously trimming trailing newlines from string literals, which this PR doesn't do.

Is there a particular reason for doing that, or shall I just change the expected test result?

cdepillabout commented 4 years ago

@georgefst I actually don't really remember, but let me take a look at it when I review this PR.

(Sorry, I've been quite busy these last few weeks. It might take me another week or two til I have enough time to look through this.)

georgefst commented 4 years ago

Sorry, I've been quite busy these last few weeks. It might take me another week or two til I have enough time to look through this.

No worries at all! The work I actually needed this for required a further change anyway, so I'm happily using my own fork for that (though I will open a further PR once this is merged).

cdepillabout commented 4 years ago

Oh, and don't worry too much about trying to support all sorts of older GHCs. I can always fix this up if I feel like it, but I don't want you to have to spend too much time on this.

It is probably unlikely anyone absolutely needs new versions of pretty-simple on like GHC-8.2, GHC-8.0, GHC-7.10, etc!

georgefst commented 4 years ago

don't worry too much about trying to support all sorts of older GHCs

Yeah, it's not something I'd want to sink much time into, but it was an easy enough fix in this case (once I'd remembered the syntax...).

Looks like all the failures now are from that one test.

cdepillabout commented 4 years ago

@georgefst Okay, sounds good. So I guess you're ready for me to review this and possibly merge it in?

georgefst commented 4 years ago

I haven't yet had a chance to look at the issue with the extra indentation at the start of muti-line strings and 'other's (corresponding to the two modified test outputs).

But apart from that, yes. It's possible it could still do with more comments, but there are now at least haddocks on everything top-level.

georgefst commented 4 years ago

there are now at least haddocks on everything top-level

Well now there are.

PS. is it possible to tell the CI to ignore that sort of change? Just seems like such a waste of energy

cdepillabout commented 4 years ago

Okay, sounds good. I'll try to get to a review of this in a week or two or so.

is it possible to tell the CI to ignore that sort of change?

I think if you have commit rights here on github, it should be possible to stop CI, but unfortunately I didn't see this fast enough to be able to get to it.

georgefst commented 4 years ago

How well-tested is this with larger AST-like expressions BTW? Are there any tests for that?

I've used it with some very heavily nested types e.g. SVG Documents, and output is the same as with the old version. But I don't think any of the existing test cases are particularly large.

georgefst commented 4 years ago

0d4f41e, which I thought should be pretty harmless, seems to be causing some tests not to terminate...

I'm sure I'll have fun debugging that tomorrow.

georgefst commented 4 years ago

0d4f41e, which I thought should be pretty harmless, seems to be causing some tests not to terminate...

Should have guessed - it's the laziness test (line 642 in Simple.hs). It seems that prettyprinter-ansi-terminal's renderLazy is in some sense less lazy than prettyprinter's renderLazy, the former being the one specialised to AnsiStyle, which inserts escape codes.

@sjakobi do you know anything about this?

Edit: originally got them the wrong way around.

sjakobi commented 4 years ago

@georgefst Yeah, Text.renderLazy doesn't force annotations. I haven't run into any problems due to that though.

Which renderer does the test use and what do the annotations look like?

I'd suspect that you're building up thunks somewhere in annotateAnsi. Maybe try to make that more strict!? I'm not very experienced with this though – I'd probably use a simple go function that is strict in the Tape state and forces the annotations it outputs. I'm not sure whether there are any laziness issues with the annotation handling on the prettyprinter[-ansi-terminal] side.

georgefst commented 4 years ago

@sjakobi To be clear, the problem here is that we actually need Terminal.renderLazy to be lazy. The test passed when we used Text.renderLazy, but hangs with Terminal.renderLazy.

Modifying the implementation slightly to make it use an RWST () Builder [AnsiStyle] instead of an ST monad, gives us the laziness we need. i.e. TL.putStrLn $ TL.take 1000 $ pStringOpt defaultOutputOptionsDarkBg $ show [1..] is able to terminate (and produce correctly highlighted output).

Presumably there's a reason it's written the way it is (performance?)? But what I'm really wondering is, despite its name, is there any sense in which Terminal.renderLazy is actually lazy?

sjakobi commented 4 years ago

To be clear, the problem here is that we actually need Terminal.renderLazy to be lazy.

Lazy in the annotations? But it's supposed to render the annotations! I don't understand how that's supposed to fit together.

The renderLazy name just refers to the output type: It's the lazy Text variant. renderStrict outputs the strict Text type.

georgefst commented 4 years ago

To be clear, the problem here is that we actually need Terminal.renderLazy to be lazy.

Lazy in the annotations? But it's supposed to render the annotations! I don't understand how that's supposed to fit together.

It's a stated goal of pretty-simple to be able to print partial results. e.g. pPrint [1,2,3, undefined] should still show the first few entries before failing.

The renderLazy name just refers to the output type: It's the lazy Text variant. renderStrict outputs the strict Text type.

Oh, okay. Then what's the purpose of having that variant if you can't take advantage of the laziness? Even something like TL.take 5 . renderLazy . fix $ SChar 'a' is ⊥, when, with a truly lazy renderLazy, it's "aaaaa".

georgefst commented 4 years ago

Anyway, we can fix this here with an actually-lazy renderLazy. I'm just surprised it's not the upstream behaviour.

As it happens, it's virtually a one-liner with quchen/prettyprinter#164, so perhaps I should reopen that...

sjakobi commented 4 years ago

It's a stated goal of pretty-simple to be able to print partial results. e.g. pPrint [1,2,3, undefined] should still show the first few entries before failing.

Oh, interesting! I wasn't aware of that.

The renderLazy name just refers to the output type: It's the lazy Text variant. renderStrict outputs the strict Text type.

Oh, okay. Then what's the purpose of having that variant if you can't take advantage of the laziness? Even something like TL.take 5 . renderLazy . fix $ SChar 'a' is ⊥, when, with a truly lazy renderLazy, it's "aaaaa".

I think it's just an exposed implementation detail that can be used when you don't need a strict Text. We make a Text Builder first, which is then converted to lazy Text. Maybe that conversion is less lazy than it ought to be!?

If you think that this is something that should be addressed, please do open an issue on the prettyprinter issue tracker.

cdepillabout commented 4 years ago

Hey, I haven't forgotten about this, I just haven't really been able to find time for it within the past week or so! Hopefully I'll get a chance to take a good look at this in the next two weeks.

georgefst commented 4 years ago

@cdepillabout No worries, it's probably worth waiting for quchen/prettyprinter#176 anyway.

I still need to fix up some minor things with that PR, and I'm also waiting to see if the prettyprinter author replies.

georgefst commented 4 years ago

it's probably worth waiting for quchen/prettyprinter#176

And that's up! Many thanks @sjakobi

georgefst commented 4 years ago

CI has failed on The program 'stack' is currently not installed - pretty sure that has nothing to do with me

cdepillabout commented 4 years ago

@georgefst Thanks for doing so much work on this! Sorry for taking so long to get to the review.

I think this is a big improvement for the output printing, and I agree that with the nice refactoring you have here, a bunch of other PRs will be much easier to implement.

Do you want me to release this on Hackage as pretty-simple-4.0.0.0? Or pretty-simple-3.4.0.0? This is the biggest change made to pretty-simple in a long time, so I'm leaning towards 4.0.0.0.

georgefst commented 4 years ago

Well it's a big change, but it's mostly an internal one.

Personally I'd wait until some of the features and fixes which can make use of this change are implemented, seeing as these will bring more breaking changes, albeit not big ones.

I intend to submit PRs for the ones I care about in the next few days.

cdepillabout / pretty-simple

Use 'prettyprinter' library #67