RFC: AST comments should be properly exposed

[krux02]

Problem

currently the AST has internally a field comment, so that documentation comments can be attached to everything. But this field is not exposed via macros, and it should not be. A NimNode (PNode in the compiler) has a cramped lineinfo with only a few bits for line and column information. People already hit it's limits. On the other hand, there is a comment field in every node that doesn't contain information at all. Most of the time it is unused.

Goal

This change should remove on one hand this general comment field from NimNode, and only allow documentation comments at certain positions in the AST. On the other hand, when there is a documentation comment, it should be easily visible in the AST, and therefore not just available to use for macros, but also visible with treeRepr. Illegal documentation comments should be detected as such (normal comments are still valid everywhere).

Solution

Documentation comments only make sense for identifiers that can potentially be exported: var, let, const, proc, template, etc. Here the identifier handling is already special cased. Sometimes the AST has only an identifier, sometimes it is the * postfix operator on the identifier. Nim doesn't have postfix operators, the postfix operator is only used to tag symbols as exported. Here the postfix operator can easily be substituted with a node of length 3 that contains the identifier, the information the tag, if the nodes is exported and the documentation comment, if it exists.

last words

This is a breaking change, so feedback is welcome. But it will be a solution for documentation comments. They will either be properly attached to the AST visibly, or they will cause an error/warning in the compiler. So when the compiler finds a documentation comment but can't attach it properly to the AST, it will no longer just disappear.

[cooldome]

+1 if you will remove comment field from the TNode definition and instead will wire it into the tree as string node of comment kind.

[Araq]

Furthermore the upcoming version 0.20 will be about making the AST stable, that includes a spec for typed ASTs (no more "for loop was inlined already" nonsense for typed ASTs) and this is a significant important milestone for this goal.

[timotheecour]

+1 for finally addressing this issue, it's a real enabler for a number of tools

• for further motivation on why we want to expose doc comments, see expose API to get NimNode doc comments, for tooling Nim#8516
• I had a PR that exposed the soon to be deprecated internal comment here: [TODO] add macros.comment(NimNode) magic to get node doc comment Nim#8903 ; my thinking was that even when comment internal node gets removed and replaced by an AST node, the change could be made transparently so users wouldn't have to patch their code: instead, macros.comment(n:NimNode) would've simply be rewritten to fetch the corresponding AST node, transparently, eg via AST accessor. I actually had coded up a similar logic to fetch a doc comment given a symbol (it'd fetch it's corresponding declaration, and walk the AST to retrieve the appropriate node that holds the comment field, it worked fine (eg for proc, type etc), and only failed in cases where the internal comment node itself didn't work properly, see my other bugs).

I'm not sure what was wrong with that logic?

In any case, if AST comments are coming soon, I'd be ok.

@krux02

Documentation comments only make sense for identifiers that can potentially be exported

what would be a counter example?

This is a breaking change

what type of user code could break, given that comment was internal? Wouldn't it only affect code that uses compilerapi?

Note that with accessors, we'd mitigate the risk of breaking changes since we'd be able to use myNimNode.getComment instead of a magic number myNimNode[8]

note

while we're talking about attaching comment nodes to AST, we should also look at fixing current defects in where comment places itself, eg see nim-lang/Nim#8929

[dom96]

I also wonder why we don't just expose a comment field (and perhaps enable it only for identifier or export marker nodes). This would prevent any breakage.

A new AST node is nice, but I don't think it really offers enough advantage to just having the existing comment field. Does it?

[krux02]

Documentation comments only make sense for identifiers that can potentially be exported

what would be a counter example?

This discussion is here, so you can post one if you find it. Then you would proof my assumption to be wrong.

This is a breaking change

what type of user code could break, given that comment was internal? Wouldn't it only affect code that uses compilerapi?

Macros would break. For example node.expectKind {nnkIdent, nnkPostfix} would crash and it would require to be fixed manually to take nnkExportDoc into account (the current name).

I also wonder why we don't just expose a comment field (and perhaps enable it only for identifier or export marker nodes). This would prevent any breakage.

The comment field in PNode was a lazy design mistake that needs to be reverted with the least amount of pain possible. It causen problems like the #8929 where comments are attached to nodes without it ever getting noticed. Then it bloats the memory with a ton of nil pointers. And people even create PR to export the comment field because they want it, engraving this design mistake further into the language.

[Araq]

I'm not sure what was wrong with that logic?

Nothing but Nim benefits from using the same AST that the compiler itself uses. It keeps us honest about what can be done with it, it keeps the bar for entering Nim compiler development low.

[Araq]

A new AST node is nice, but I don't think it really offers enough advantage to just having the existing comment field. Does it?

The clarity in the Nim grammar it brings and for docgen related tooling is enough of an advantage. Besides, the AST changed in 0.19.9 slightly because of other reasons, previously for-loop inlining was done in a typed AST context, a terrible design mistake that needed to be fixed (and was).

[arnetheduck]

very nice. it's interesting also to build custom tools for automated refactoring - an ideal AST would retain all information necessary to recreate the exact original source file byte by byte..

see for example clang-tidy and how it can be used to refactor old c++ into new c++ (unique_ptr, auto, etc).. imagine applying that to the std lib and all the crufty stuff in there..

[krux02]

@arnetheduck normal comments are still not part of the AST. They cannot be retained.

EDIT:
They are not retained.

[github-actions]

This RFC is stale because it has been open for 1095 days with no activity. Contribute a fix or comment on the issue, or it will be closed in 7 days.