Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] Retain comments in AST #3509

Draft
wants to merge 39 commits into
base: master
Choose a base branch
from

Conversation

bartolomej
Copy link

@bartolomej bartolomej commented Aug 4, 2024

Description

POC for preserving comments in Cadence AST. This should enable us to make a fully working, pretty printer for Cadence.

This is a follow-up to my discussion in Discord some time ago: https://discord.com/channels/613813861610684416/1108479699732152503/1240749220357607475

Closes #308

Design

I decided to start tracking comments in the form of leading/trailing comment structs attached to lexer tokens and AST nodes as discussed in #308 (comment).

I believe this approach (instead of the old one where comments were tracked as AST nodes) could simplify comment parsing logic, as the lexer already computes which tokens are associated with which comments + it removes the need to skip comments when parsing.

Notes

  • This change will touch quite a lot of the parsing code, as we'd need to update how we parse declaration/statement/expression (and possibly other) AST nodes to attach leading/trailing comments to them. These changes shouldn't be difficult to implement but are pretty sensitive regardless, so updating and expanding the test suite will be important.
  • We should also test that the comments in the real-world code on mainnet/testnet are preserved as expected + possibly add a runtime check to see if all the comments are correctly preserved when doing prettifying, similar as it's done in prettier: https://github.com/prettier/prettier/blob/251ecabbdee509954f7b0d33b38da9ec4ae93ad8/src/main/comments/print.js#L206-L222
  • I'm tracking some of the leftover work with TODO(preserve-comments) in code

Definition of Done

  • Add AST elements for comments:
    • Block comment
    • Line comment
  • Add a leading/trailing comments property
    • Declarations: Line and block comments
    • Statements: Line and block comments
    • Expressions: Block comments
  • Add comments to AST elements when parsing
    • Declarations
    • Statements
    • Expressions
  • Tests

Description


  • Targeted PR against master branch
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work
  • Code follows the standards mentioned here
  • Updated relevant documentation
  • Re-reviewed Files changed in the Github PR explorer
  • Added appropriate labels

@bartolomej bartolomej changed the title Retain comments in AST [WIP] Retain comments in AST Aug 4, 2024
@RZhang05
Copy link
Contributor

Is this still being worked on? If not, I may try to finish what remains

@bartolomej
Copy link
Author

Is this still being worked on? If not, I may try to finish what remains

Hi, yes I'm still working on this and plan to finish it including some follow up work, but I've just been busy with other stuff in the past few weeks.

@SupunS
Copy link
Member

SupunS commented Oct 18, 2024

hey @bartolomej , we recently restructured the cadence repo and as part of that, moved all the subdirectories under runtime package (e.g: runtime/ast, runtime/interpreter, etc) to the root/top level. Please take a pull and update the branch. Github should be able to identify the changes to the moved files, and correctly map them to the new locations. So hopefully there shouldn't be much conflicts.

Let me know if you run into any issues/conflicts, I can help with resolving those.

@SupunS SupunS added the Feature label Oct 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Retain comments in AST
3 participants