Avoid printing comments from AST.

[jordwalke]

Cristiano upgraded his compiler to 4.02.3, so this began storing comments in the AST. We print some of these, so we need to strip these attributes out and not print them (we already print comments so this is redundant).

[jordwalke]

cc @yunxing

[yunxing]

I can reproduce this issue by:

[yunxing]

The intermediate solution should be filter out all the attributes with "ocaml.text" until we can store comments into attributes. @dxu

[dxu]

cool, I can look into it. if my understanding is correct, I should be stripping out all the current comment code (including the interleaving), and just checking the actual structure item?

[dxu]

on second thought, i'll probably need to reuse some of the logic to handle inline comments. i'll have to look into it more

[dxu]

I tried modifying the matching on floating attributes to play with this, but I'm not sure if it's my lack of understanding or something else, but I can't seem to consistently reproduce this.

After taking out yunxing's preprocessor, I tested an ml file in different cases:

My understanding is that these are the AST's that are fed into the pretty printer - is this correct? Does anyone know why there are these inconsistencies? Is this a problem on ocaml's end?

[jordwalke]

The OCaml .ml parser stores comments in the AST as attributes (Reason doesn't yet). It looks like it's not storing the first comment in the AST. Perhaps that's because it treats the first docblock specially? Also, you don't need the -pp reasonfmt argument because there's nothing Reason about that file or the compilation command.
(Note OCaml only started parsing comments into the AST in version 4.02.3).

[dxu]

Ah, thanks for that, I probably should've noticed that.
Yeah, I'm not sure, I'd need to look more into what exactly is stored in 4.02.3. I also noticed that multiline comments don't seem to be stored.

I feel like there might be some issues if we try to convert over the reason parser to storing all comments in our AST, since there'd be inconsistencies between upgrading ocaml -> reason and just compiling reason, if the ocaml compiler doesn't store all comments in the AST.

[yunxing]

It also looks to me that only part of the comments are stored in the AST. It looks like my previous understanding of how ocaml stores comments in AST is not solid.

I would be interested to see why do they want to store comments in the AST at the first place and what's the current status.

[jordwalke]

Perhaps it only stores comments that it can associate with a particular AST node? If so, it's not really intuitive or clear what's currently happening. It's a good thing we have the current fallback.

[jordwalke]

@cristianoc I pushed a fix last night to ReasonSyntax which should fix the remaining issues with this. I haven't tested thoroughly, and I plan to spend the rest of the weekend finding edge cases in development. But if you were to rebase and re-run the converter, the comments should all be fixed.

[cristianoc]

@jordwalke looks like this fixes .re files but not .rei files for me.

[jordwalke]

I'll get a fix. I just missed one AST node type.

[jberdine]

4.03 is supposed to parse all ** comments into attributes. I think that 4.02.2 and 4.02.3 are in some intermediate situation. It the association rules trigger, then they are ocaml.doc attributes, otherwise they are ocaml.text attributes. The point of these attributes is for enabling documentation generation/browsing/etc tools to work from the cmt files. It seems to me that this also the right path for reason.

There is a warning (50) that identifies ** comments that could not be unambiguously associated with ast nodes. So it seems that the right plan for ocaml to reason conversions is to first enable warning 50, fix all of the warnings in the ocaml source, and then proceed with the conversion. This could be done with just a one off use of 4.03 before a full upgrade of a project. That ought to keep the conversion from being lossy wrt documentation comments. How to print them could vary or wait once the association in the ast is right. Thoughts?

[jordwalke]

We will definitely want to parse comments into the AST just as 4.03 does so that we get codoc support. We currently do a pretty good job of printing comments without storing them in the AST, however. We want to maintain full support for printing comments that couldn't be parsed into the AST (if someone choses not to enable warning 50), so we might need to always support that anyways. We can still also parse them into the AST solely for the purpose of doc generation.

[jberdine]

Yes, agreed. For the comments that do get parsed as attributes by 4.03, does reason maintain those attributes through printing and parsing? I don't know if there is enough information in those attributes to preserve things like keeping the comments before or after the expression through print/parse cycles? My motivation is that if so, it would be much easier to be confident that a ocaml to reason conversion is not lossy wrt doc comments if I first make the project warning 50 clean.

[jordwalke]

Sorry, I did not answer the question you asked. I answered it in depth here, but in short, the AST attributes will mostly be good for documentation generation, and warning 50 will be great for ensuring that your project is Reason clean, although we need to do more work to make that true.
Warning 50 will not make your project Reason clean due to the comments being stored in AST attributes, but rather due to just always being able to attribute comments to the right AST node.

[jaredly]

this is pretty stale. feed free to reopen if more discussion is needed