Pretty Printing with position logging #7

Closed
wants to merge 8 commits into
from

Conversation

Projects
None yet
3 participants

copton commented Jun 14, 2012

Hi

I have added a feature to the Pretty library and I think others could need this as well. That's why I want to show it to you and I am eager to here what you think about it. It's about collecting a log of which Docs ended up where in the output string.

Motivation: The Ocram compiler [1] is a source to source compiler that amongst other things needs to keep track of the mapping of input rows and columns to output rows and columns. It uses the Language.C library which in turn uses the Pretty library for printing.

To perform its task the Ocram compiler needs to know which parts of the AST have been rendered where in the output document. To this end I have patched the Pretty library to support augmenting Doc objects with callbacks. When such a Doc is rendered the callback is called with the current row and column of the output document. Now I can change the Pretty instances to augment the Docs of the AST objects in question and I get a nice, accurate log of which part of the output AST ended where.

The current status of the patch is complete, as I am already using it for Ocram. But it lacks more test cases and anything else you would premise in order to accept the patches for upstream. I am willing to perform the additional work, as I would like to avoid inofficial dependencies for my project.

What do you think?

Greetings

Alex

[1] http://github.com/copton/ocram

Alexander Be... added some commits May 14, 2012

Here, you'll probably run into problems, as you might loose here markers, if they are placed lift of an empty document. An easy way out would be to allow the following variant of here:

textWithPos :: (Position -> m) -> String -> Doc m

implemented similarly to text. This way you'd always have a TextBeside to attach the position callback to.

Owner

copton replied May 15, 2012

The thing is, with the here functions I can reuse existing Pretty instances by just prepending "here (log someid) $ ..." to its definition of the function pretty. With textWithPos I would have to dig into the implementation and find the first call to text to replace it with textWithPos. Furthermore, I will also need charWithLog, sizedTextWithLog, semiWithLog, intWithLog, etc...

So, when exactly do I lose a marker with the current implementation of here?

(here "your note" empty) <> text "what" will very likely loose your note.

Owner

copton replied May 17, 2012

Okay, this is an obvious case. One could argue, the user should know what he/she is doing here.

I still see your point and I don't like this case either, but when looking at the usage of here in [1] ff (wrapped by the function marker), I find its usage pretty convenient.

So, for me the question is, if there is a more subtle way of loosing notes which would be unexpected by the user. Something, one would consider unacceptable?

Or do you see an alternative way which avoids the problem of loosing notes and is still similarly convenient? AFAICS, textWithPos is not.

Thank you for your comments, anyway.

[1] https://github.com/copton/ocram/blob/314fddee6514eedf525a9b50c0f5f55382678579/ocram/src/Ocram/Print.hs#L128

Yes, I think there is a proper solution. Also add a "marker"-argument to the NoDoc and EmptyDoc constructors and merge these markers into TextBeside, if necessary.

You can probably wait with this in your case, as you are likely to always hit a TextBeside marker with your here functions. One way to ensure this during development is to define the NoDoc and EmptyDoc cases such that they throw an error "here applied to EmptyDoc". I'd actually do this, just to ensure that there are no unnecessary surprises.

Owner

copton replied May 18, 2012

I have chosen the error option for now. See recent commit. Thank you.

OK, good. You're welcome :-)

Why not use a Position -> m callback and parametrize over Monoid m in the rendering functions? This would require you to use the textWithPos approach outlined below, but I think that's anyway a good thing for your use case.

Owner

copton replied May 15, 2012

Done. See new commit. Thank you!

Member

dterei commented Jun 19, 2012

Thanks for the code!

So I'm snowed under right now and may be some time before I can really look at this. In general I have two issues:

  1. (unrelated to this pull request). There are some changes I want to make with the code and benchmark and testsuite I want to add. Until then I don't feel that confident accepting new features.

  2. I don't know how keen I am on changing the Doc type and all interfaces for this feature which 95% of users probably won't use but now will be affected by this code due to type changes. I'd have to think if there are better ways to do this which leave a lot of the core the same.

copton commented Jun 19, 2012

Hi David,

thanks for considering accepting my patches. Let me answer to your remarks.

  1. Just go ahead and finish your pipeline and I can re-create the patches accordingly. Just let me know when you are ready.

  2. Actually, I managed to keep downwards compatibility, as

  • type Doc = DocLog () [1]
  • fullRender ... = fst $ fullRenderWithLog ... [2]

Greetings,
Alex

[1] https://github.com/copton/pretty/blob/master/src/Text/PrettyPrint/HughesPJ.hs#L186
[2] https://github.com/copton/pretty/blob/master/src/Text/PrettyPrint/HughesPJ.hs#L879

copton commented Dec 26, 2012

Hi David,

I just wanted to ping you and ask for a status update.

I have some free time currently and I would love to get some productive work done :)
Maybe I could be of help in adding test cases and benchmarks to the library?

I would really love to see this pull request being accepted. With the compatibility issue out of the way, is there anything else that would prevent you from considering these patches?

Greetings,

Alex

Member

dterei commented Dec 28, 2012

Hi Alex,

yes very sorry about this, I just am not finding time to work on pretty at the moment.

So, what if we went with something a little more generic? I like the code and having logging, but pretty is a fairly used library that hasn't changed much a all in the last 5 years. What if rather than put logging into the core, we simply changed Doc to carry around an abstract type and the rest of the logging infrastructure was in a separate module. Basically, what is the min you need to change in the core to add logging in a another module?

copton commented Dec 29, 2012

Hi David,

thank you for your response.

As far as I can see, logging support needs to touch display and easy_display at least, because it is required to keep track of the row and column numbers while rendering. Reviewing the current pull request actually shows me little potential to reduce changes to the main module - at least as far as I can see. The patches touch many source code lines, but most changes are very systematic, like replacing Doc with DocL, for instance.

But I understand your reluctance. Maybe the following approach would be a good solution:

Let's add this test suite you have in your pipeline anyway first. Only when we feel comfortable about the test coverage, we will add the logging code and verify that the tests still pass. This should give us a strong indication that logging does not break existing code. The obvious next step would be to extend the tests to cover the logging aspect as well. Overall, its the same amount of work for me, because - as I already said previously - tests are still missing in the current pull request.

So, if you feel like this is a good way, let me know what you had in mind regarding the test suite and I will go ahead and do it.

Greetings,

Alex

Member

dterei commented Jan 2, 2013

Alex,

OK. So there is a folder already called "tests". It is a little messy right now but has the following:

  • whitebox version of pretty: PrettyTestVersion.hs
  • quickcheck based tests suite: Test.hs, TestGenerators.hs, TestStructures.hs
  • ghc test suite: all.T, pp1.hs, pp1.stdout, T3911.hs, T3911.stdout

The quickcheck stuff is pretty good and comprehensive. I'd like to see the Test.hs file split up a little though, the comments at the start of the file explain that it covers 3 different areas. I'd like each area to be its own file, especially the laws be in their own file.

The GHC testsuite checks should be incorporated into the quickcheck Test.hs framework. Either as quickcheck tests if this makes sense, or just straight hand-written test code otherwise.

Pretty also needs a good benchmark suite going forward. This is a concern for your pull request as we also want to make sure that it doesn't degrade performance. The only thing so far for this is a hastily written file in tests called Bench1.hs. It isn't very good. So if you have the time and motivation than a good benchmark suite would be awesome! Feel free to start with a clean slate here. Just custom code and lots of microbenchmarks may be the best, but you could also imagine testing with something more real. E.g., are there already some haskell packages that would be suitable for driving pretty as a benchmark? a library that features serialization of XML in a pretty form for example...

Finally. It would be great to get a simple example of how to use the logging code this pull requests adds. I'd like to see this to feel more confident in its purpose and the API you are exposing.

Thanks for working on this Alex!

copton commented Jan 4, 2013

Hi David,

sounds good to me. To sum it up, the individual steps are:

  • clean up and complete test suites
  • add a benchmark suite
  • incorporate position logging

The last step will in particular:

  • make sure that position logging sustains compatibility
  • measure the performance costs of position logging
  • show how to use the position logging API

Expect a pull request for the first item (hopefully) soon.

Greetings,

Alex

Member

dterei commented Dec 26, 2014

Alex,

I'm going to close this PR at this time as it's been a while now. Happy to reconsider this again if you want to rebase and open a new PR.

Cheers,
David

dterei closed this Dec 26, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment