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

Preserve comments in AST #1

Closed
wants to merge 12 commits into from
Closed

Preserve comments in AST #1

wants to merge 12 commits into from

Conversation

bartolomej
Copy link
Owner

@bartolomej bartolomej commented Jul 26, 2024

Description

POC for preserving comments in Cadence AST. When complete, this should enable us to make a fully working a 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 onflow#308

Design

I decided to start tracking trivia tokens in the form of leading/trailing trivia structs attached to Token's as discussed in onflow#308 (comment).

I believe this could simplify some parsing logic, as the lexer already computes which tokens are associated with which comments + it removes the need to skip comments when parsing.

At the AST level, I added leading/trailing comments fields to some AST nodes. The other trivia types such as spaces or newlines are just discarded for now, as I don't think we'd need them. Maybe that also means we don't even need to track them at the lexer level, but I still think that is a good think for checking correctness in unit testing if nothing else.

Note that I've currently implemented this only for FunctionDeclaration and Block AST nodes, but I'll keep expanding that to other AST nodes.

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
  • I'm not sure if old parser is still being used at all. If not, should that package be removed from source code (I saw there was already some related work here, but the source wasn't removed yet: Remove old parser onflow/cadence#243)? For now, I'm updating it so that the tests pass.

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 (WIP)
    • 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
Copy link
Owner Author

Closing in favor of onflow#3509

@bartolomej bartolomej closed this Aug 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Retain comments in AST
1 participant