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

Full account of comments in the AST #13865

Open
ekpyron opened this issue Jan 11, 2023 · 2 comments
Open

Full account of comments in the AST #13865

ekpyron opened this issue Jan 11, 2023 · 2 comments
Assignees
Labels
low effort There is not much implementation work to be done. The task is very easy or tiny. low impact Changes are not very noticeable or potential benefits are limited. selected for development It's on our short-term development
Milestone

Comments

@ekpyron
Copy link
Member

ekpyron commented Jan 11, 2023

Tooling asked us to (potentially optionally) include all comments in the AST for being able to support pretty-printing ASTs to near-identical sources (except whitespace differences, accounting for which would definitely go too far).

There's related discussion already in e.g. #4559.

We'll need to weigh the complexity this would induce for scanning and parsing and check if this can be done in a reasonable amount of time (since this is non-critical).

We can consider removing comments on the expression level from the scope of this, since they may be a larger hassle than comments on the statement level.

We'd also need to decide whether to introduce full AST nodes (maybe that'd be the way to go for statement-level comments) or fields similar to the documentation field used for natspac comments (that may be the only choice for comments on the expression level, if we even consider those).

Some justification for spending time on this otherwise tangential issue is that we could use such comments for testing AST-based properties like the annotations produced by #13378 in isoltest by introducing special comments with test expectations. In that sense, this weakly relates to our roadmap task #13722

@ekpyron ekpyron added low effort There is not much implementation work to be done. The task is very easy or tiny. low impact Changes are not very noticeable or potential benefits are limited. nice to have We don’t see a good reason not to have it but won’t go out of our way to implement it. labels Jan 11, 2023
@ekpyron ekpyron added this to the 0.8.19 milestone Jan 11, 2023
@ekpyron
Copy link
Member Author

ekpyron commented Jan 11, 2023

Actually, thinking about this a bit further, for the requested use case, it's basically irrelevant in which structure the comments are stored - so this can actually be solved very easily:
We just have the scanner not skip over comments, but collect/accumulate them internally in a list/vector. Then whenever we create an AST node in the parser we move all the currently stored comments from the scanner into it.

In the AST, each node could then get an optional comment list field that plainly stores the comments and their source locations.

The only thing left would be where to store the comments past all other nodes of the AST, but they can just end up in the top-level source unit or something like that.

So yeah, if we only emit this somewhat weird addition to the AST conditionally upon explicit Standard-JSON request, this can actually be done rather smoothly, so we can indeed just go for it for 0.8.19 (especially since this should still allow using such comment annotations for testing new AST analysis properties a la #13378)

@coreyar
Copy link

coreyar commented Mar 17, 2023

@cameel This seems like an interesting issue to work on. What do you think about the approach that @ekpyron described?

@NunoFilipeSantos NunoFilipeSantos modified the milestones: 0.8.20, 0.8.21 Apr 17, 2023
@ekpyron ekpyron added selected for development It's on our short-term development and removed nice to have We don’t see a good reason not to have it but won’t go out of our way to implement it. labels May 15, 2023
@ekpyron ekpyron modified the milestones: 0.8.21, 0.8.22 Jun 26, 2023
@ekpyron ekpyron modified the milestones: 0.8.22, 0.8.23 Oct 16, 2023
@ekpyron ekpyron modified the milestones: 0.8.24, 0.8.25 Dec 20, 2023
@cameel cameel modified the milestones: 0.8.25, 0.8.26 Mar 4, 2024
@ekpyron ekpyron removed this from the 0.8.26 milestone May 13, 2024
@ekpyron ekpyron added this to the 0.8.27 milestone May 13, 2024
@cameel cameel modified the milestones: 0.8.27, 0.8.28 Sep 9, 2024
@ekpyron ekpyron modified the milestones: 0.8.28, 0.8.29 Oct 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
low effort There is not much implementation work to be done. The task is very easy or tiny. low impact Changes are not very noticeable or potential benefits are limited. selected for development It's on our short-term development
Projects
Development

No branches or pull requests

5 participants