Retain comments in field parser

[leana8959]

This is the first part of the exact print parser. In this PR I changed the lexer so instead of dropping the comments it emits them to the parser which is further stored in GenericPackageDescription.

Please let me know your thoughts!

---

Checklist below:

This PR modifies behaviour or interface

Include the following checklist in your PR:

• Patches conform to the coding conventions.
• Any changes that could be relevant to users have been recorded in the changelog.
  • Is the change significant? If so, remember to add significance: significant in the changelog file.
• The documentation has been updated, if necessary.
• Manual QA notes have been included.
• Tests have been added. (Ask for help if you don’t know how to write them! Ask for an exemption if tests are too complex for too little coverage!)

[andreabedini]

Thank you for taking on this work. I think adding a Comment constructor to Field is not a good design. It modifies the meaning of the type (and indeed this forces you to make functions like elementInLayoutContext return [Fields]).

I think this has been already discussed before: we have the ann parameter which can be used to keep hold on the comments. E.g.

data Comment ann = Comment !ann !ByteString
type FieldWithComments ann = Field ([Comment ann], ann)

In this design each Field carries the comments preceding it, annotated with their position. An extra annotation marks the position of the field itself. Any comment at the end of the file would need to be captured separately.

This is already the practice of few packages developed by the community. Is there a reason to deviate from this?

[leana8959]

Here's a preliminary benchmark done with hyperfine running on my computer with as little other programs running as possible, done in the same condition (beside being run 6 hours apart).
It seems like we are within the standard deviation, so there's no noticeable degrade of performance.

Upstream:

~/r/haskell/cabal λ lts-18.28
$ hyperfine --runs 30 './validate.sh --partial-hackage-tests'
Benchmark 1: ./validate.sh --partial-hackage-tests
  Time (mean ± σ):     203.400 s ± 21.816 s    [User: 150.484 s, System: 39.487 s]
  Range (min … max):   183.805 s … 277.905 s    30 runs

This branch:

…/wt/haskell/cabal/exact-pp-leana λ lts-18.28
$ hyperfine --runs 30 './validate.sh --partial-hackage-tests'
Benchmark 1: ./validate.sh --partial-hackage-tests
  Time (mean ± σ):     199.168 s ± 10.540 s    [User: 156.373 s, System: 39.818 s]
  Range (min … max):   184.443 s … 242.563 s    30 runs

Thank you for your response Andrea, I'll write up a response and get back to you soon :)

[leana8959]

Thank you for your comment @andreabedini :)

I think this has been already discussed before: we have the ann parameter which can be used to keep hold on the comments.

That looks very interesting, but how would I deal with files that are just comments? To the point of view of readFields they should be valid yet we would have no Field to attach them to. Whether we should attach the comments above or below is yet another question. For example, in the sequence "comment element comment element comment", which element should grabs the comment in between?

I do think your model is very interesting so if you have the time to, please show working PR against mine so we can simply merge it in 🙏

This is already the practice of few packages developed by the community. Is there a reason to deviate from this?

Could you elaborate which packages are these? I would love to have more insight on how people solve similar problems.

Are there other design issues that needs to be addressed ?

[Bodigrim]

That looks very interesting, but how would I deal with files that are just comments?

Are they valid Cabal files if there is nothing but comments? I don't think so.

Could you elaborate which packages are these? I would love to have more insight on how people solve similar problems.

For instance, cabal-add has a function

annotateFieldsWithSource :: ByteString -> [Field Position] -> [Field ByteString]

which annotates each field with its source including all adjacent comments. It can be modified to return [Field (Position, ByteString) if you need both.

[Bodigrim]

Could you possibly outline how the output of this function is supposed to be used? Now that we detached comments from fields, how do we reconstruct the original document? How do we do it if [Field ann] is programmatically updated (say, adding or removing elements)?

[leana8959]

Could you possibly outline how the output of this function is supposed to be used?

This function recursively extracts the comments from the fields (the updated design variant does the same thing, albeit being more concise).

It is used to extract the comments out right before running the parseFieldGrammar parser in src/Distribution/PackageDescription/Parsec.hs. This allows us to not change the grammar and be able to directly inject the comments into the GenericPackageDescription.

Now that we detached comments from fields, how do we reconstruct the original document?

We can't yet. Following Jappie's proposal, we need to store the exact position of each field as a map (called exactPositions) indexed by the path in the rose tree (typed [NameSpace], a list of path segments). And then we will have the right information to construct the exact printer.

How do we do it if [Field ann] is programmatically updated (say, adding or removing elements)?

To quote Jappie:

The issue for addition is that you now have to invent exact positions. for removal, if it involves a line, you've to fix up all following lines, (and it has to know something was removed).

I haven't thought about this thoroughly because it's out of scope of this PR, but it should be very much feasible.

[geekosaur]

Are they valid Cabal files if there is nothing but comments? I don't think so.

Pretty sure you need at minimum name and one target.

[jappeace]

Hi friends, thanks for all your responses. Leana needs some time to read up on the exact proposal to see how it all fits together before replying.
After chatting with her, I think she wants to go with Andrea's design for the comment field parser. As you can see, she's deeply in the weeds about many of the details of the parser; she even corrected some of the field grammar comments!
I don't know what you guys think, but I think it's good progress 🚀

[Bodigrim]

Cabal is one of the toughest code bases I ever worked on, so I'm quite amazed by Leana making progress so quickly!

[leana8959]

Thank you Bodigrim and Jappie for your kind words! That means a lot to me, I'm glad to be on the right track.

I have started (and completed) to rewrite my PR using Andrea's approach.
Right now the behaviour is identical to my old approach, while comments are tracked in the annotation ann.

Are they valid Cabal files if there is nothing but comments? I don't think so.

Good to know. Currently the top level parser drops the comments consumed if there are no fields to attach them to.

Let me know what you think about the change :)

[leana8959]

Here are the benchmark results. The baseline has been rerun because I did these ones on a VPS machine, and they are not comparable to the last ones I ran on my machine.

# baseline
leana@Ubuntu-2404-noble-amd64-base:~/cabal$ hyperfine './validate.sh --partial-hackage-tests'
Benchmark 1: ./validate.sh --partial-hackage-tests
  Time (mean ± σ):     253.353 s ±  9.520 s    [User: 196.642 s, System: 60.881 s]
  Range (min … max):   241.649 s … 271.207 s    10 runs
  
# this PR
leana@Ubuntu-2404-noble-amd64-base:~/cabal$ hyperfine --setup "./validate.sh --partial-hackage-tests" "./validate.sh --partial-hackage-tests"
Benchmark 1: ./validate.sh --partial-hackage-tests
  Time (mean ± σ):     253.163 s ±  7.451 s    [User: 196.432 s, System: 58.507 s]
  Range (min … max):   239.373 s … 266.656 s    10 runs

[andreabedini]

@leana8959

That looks very interesting, but how would I deal with files that are just comments?

In my prototype I have replaced [Field ann] with something like

data File ann = File [Field ann] ann

Where the extra annotation is for anything coming after the last field.

Could you elaborate which packages are these? I would love to have more insight on how people solve similar problems.

In addition to @Bodigrim's cabal-add, @phadej's cabal-fields rewrites parser entirely (but dropping support for braces) but at the AST level does the same thing. I am sure he also left comments in some of the "exact printing" mega-threads.

After chatting with her, I think she wants to go with Andrea's design for the comment field parser. As you can see, she's deeply in the weeds about many of the details of the parser; she even corrected some of the field grammar comments!

I am available to discuss and support her effort. @leana8959 I'll reach out privately.

Cabal is one of the toughest code bases I ever worked on, so I'm quite amazed by Leana making progress so quickly!

I warmly second this!

[Mikolaj]

@leana8959, @andreabedini: how is the private communication going? We are interested too! Could we help somehow?

[jappeace]

for clarity: These parser changes are ready for review as far as leana and me are concerned, meanwhile we've moved over to import stanza retention in GenericPackageDescription (they currently get merged into the stanza's). This is an independent change of the parser changes here.

After that we can start on exact print propper.

[phadej]

import stanza retention in GenericPackageDescription (

Please don't. A lot of code wants an elaborated (= stripped down of syntactic convenience) representation of package description, and GPD serves that now.

Your parsing changes leak down the pipeline where they shouldn't. E.g. things like solver works with GPD, and it really shouldn't care about whether import stanzas were used to declare a package or not.

[jappeace]

This PR isn't about that,
and we're intending to keep everything working. 🙂

[phadej]

This PR does add

  , exactComments :: ExactComments Position

field to GPD. I don't see a point of having comments in GPD. (As noted in tests, it "breaks" equality)

[leana8959]

@phadej Indeed, I have added a newtype around GenericPackageDescription that doesn't have an Eq instance. This ensures GenericPackageDescription stays the same :)

[ulysses4ever]

I understand, the authors would like to get reviews for this PR. If this is so, please, squash the commit history. For the size of PR: a good part of the changes are test-suite changes, it seems. I don't think they have to be extracted into separate PR or even separate commits (because having commits that don't pass CI individually may be cumbersome in the future, for git-bisecting and alike).

[ulysses4ever]

@mpickering can you take a high-level look at the design in this PR and tell us your opinion? The gist of it is:

-- Cabal-syntax/src/Distribution/Fields/Field.hs

data Comment ann = Comment !ByteString !ann
  deriving (Show, Generic, Eq, Ord, Functor)

data WithComments ann = WithComments
  { justComments :: ![Comment ann]
  , unComments :: !ann
  }
  deriving (Show, Generic, Eq, Ord, Functor)

and then many parsing utilities that used to return ... Field Position now return Field (WithComments Position). With casual renamings like below:

-parseGenericPackageDescription'
+parseAnnotatedGenericPackageDescription'
  :: Maybe CabalSpecVersion
  -> [LexWarning]
  -> Maybe Int
-  -> [Field Position]
-  -> ParseResult src GenericPackageDescription
-parseGenericPackageDescription' scannedVer lexWarnings utf8WarnPos fs = do
+  -> [Field (WithComments Position)]
+  -> ParseResult src AnnotatedGenericPackageDescription
+parseAnnotatedGenericPackageDescription' scannedVer lexWarnings utf8WarnPos fs = do

[ulysses4ever]

One particular thing that bothers me is that clients will have to call unComment even if they don't care about comments (at least, it appears to be the case to me after looking at the diff in this patch). I feel like there should be a more graceful way to handle this. On the cabal meeting today @jappeace suggested that we could duplicate (I assume, some) parsing utilities so that some functions have two versions: one comment-bearing and another one, comment-less. I must say I quite like this idea. @geekosaur pointed out that this "just" moves the pressure from client's to our shoulders. But I feel like many clients aren't interested in comments anyway (they are surviving somehow today), and the cost of this sort of contained duplication (inside one module and very uniform) is low. So, overall, the cost/benefit ratio of this duplication-based idea is better than in the current proposal to me.

Any thoughts?

Meta-comment: I looked through the tech proposal again, and there doesn't appear to be a technical description of a solution for this particular comments issue. It is totally fine, because the proposal would turn into a foliant at that level of detail. Neveretheless, I wish that, before doing all the technical work in this PR, the authors had discussed the actual technical solution for the particular issue (like preserving comments; others are listed in the tech proposal) on the Cabal bug tracker (here).

[leana8959]

If this is so, please, squash the commit history. For the size of PR: a good part of the changes are test-suite changes, it seems. I don't think they have to be extracted into separate PR or even separate commits.

@ulysses4ever Are there some rule of thumb to squash my commits? I already went through the history once this morning and split out all changes that touch the testsuite into one commit.
Do I need to do something else for the feature commits? If it's all good please let me know, I'll force push to my PR branch.
Maybe you have an example PR that does it right?

[leana8959]

I cleaned up everything and made sure the test passes, this is ready for review!

[ulysses4ever]

@jappeace @leana8959 should we remove the needs-review label for now given #11227 (comment) ?

[Mikolaj]

Might this be a good moment for a (re-)review?

[jappeace]

@Mikolaj yes! I wanted to bring this up again in the cabal meeting.

[Bodigrim]

Could it be commentsAfter p = flip WithComments <$> p <*> many tokComment?

The real question though is: if comments in WithComments are the ones following and not preceding, should not they go to the second field instead of the first one?

[leana8959]

Could it be commentsAfter p = flip WithComments <$> p <*> many tokComment?

Indeed, nice catch.

It would need to be flip (fmap . WithComments) <$> p <*> many tokComment but I feel like it's less readable.

The real question though is: if comments in WithComments are the ones following and not preceding, should not they go to the second field instead of the first one

It is not that easy, because when we parse something with trailing comments, we don't yet know if there are fields that follow. For example, when parsing some field lines, we don't know if the comments we got are for this fieldline or if there are following fields and the comments belong there.

To implement what you meant (on which I agree is the "better" outcome), I'd need to guess that there's a following field, and then backtrack to attaching to current field line if I guessed wrong. Personally I think that obfuscates the flow of the parser and may cause obscure bugs that only manifest when user writes comments a certain way. Considering we don't anchor comments to AST nodes like haddock does and comments attachments to nodes aren't specified at all ¹, I think this is reasonable.

In case in the future we need to attach comments to the later field (should it exist), I also think it's a better idea to implement that with a function that post-processes the fields, just to keep the parser simple.

Footnotes

1. It is possible to give comments that have ambiguous attachments like the following.

   build-depends:
     foo
     
     -- some comments
   exposed-modules:
     Main
   

   ↩

[Bodigrim]

Perhaps I was not clear what I meant. When we have WithComments "foo" "bar", does it stand for foo -- bar or for --bar \n foo?

[leana8959]

Interesting question. The current semantic is "around", so we can't tell without inspecting the Position annotation. Do you think it'd be useful to make WithComments to have two fields, one for leading comments and one for trailing comments?

[Bodigrim]

I think I'm missing something fundamental here. If the current semantics is "around" then what is stored where? Is it the case that both preceding and following comments will be elements of the list justComments :: ![Comment ann]? Could there be less than two or more than two elements? Is the order of elements of the list predetermined or can one put them in any order, even if not matching locations?

I think having priorComment :: Comment ann and followingComment :: Comment ann would be handy and self-explanatory, if it is achievable.

[leana8959]

In cabal the lexer emits a Comment token for a line comment. Also, all comments are line comments. This is why the type is [Comment ann] instead of Comment ann.

I also think it might make manipulating the comments a bit easier during printing, I'll do that. Thanks for the suggestion! I'll keep you updated.

[Bodigrim]

To which states the lexer can never switch itself? On a brief inspection I didn't find any.

[leana8959]

I admit that this is not easy to spot. However I hesitate to add the specific states into the comments because I'm afraid that it might get out of sync. In this blog post I wrote about which state are not reachable. TL;DR: switching to in_field_layout and in_field_braces from in_section is only done in the parser.

Do you think I should detail this in the comments?

[Bodigrim]

Looking at Distribution.Fields.Lexer, I see both setStartCode in_field_layout and setStartCode in_field_braces, so it can switch to these states on its own. Am I missing something?

[leana8959]

Correct, bol_field_layout and in_field_layout can switch to each other, but without something that switches from in_section to in_field_layout the lexer would reach neither bol_field_layout nor in_field_layout states.

[Bodigrim]

OK, then I'd suggest to say exactly that, because the current wording is confusing. All states are reachable from the lexer itself, it's just that certain transitions of state are not.

[leana8959]

I can surely reword it, but

All states are reachable from the lexer itself, it's just that certain transitions of state are not.

is incorrect.

Here's an illustration stolen from my comment-preserving cabal parser article. The blue transitions are done by the field parser, while the red transitions are done by the lexer. You can see that without the field parser, you don't reach two whole classes of states, namely {in,bol}_field_layout and {in,bol}_field_braces.

[Bodigrim]

Thanks for the correction. I suggest to expand the comment (my confusion is a sure sign that other readers might have the same misunderstanding), maybe even with some ASCII art, but up to you.

[leana8959]

Indeed, I think adding a ASCII flow chart in the comments would help people :)