8000 Pass comments to AST nodes during parsing by upedd · Pull Request #1005 · teal-language/tl · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Pass comments to AST nodes during parsing #1005

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

Merged
merged 4 commits into from
Jun 10, 2025
Merged

Conversation

upedd
Copy link
Contributor
@upedd upedd commented Jun 6, 2025

Part of tealdoc project. Passes comments to AST nodes of all declarations, record/interface fields and enum values.

Copy link
8000
github-actions bot commented Jun 6, 2025

Teal Playground URL: https://1005--teal-playground-preview.netlify.app

meta_fields: {string: Type}
meta_field_order: {string}
meta_field_comments: {string: {{Comment}}}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@upedd It's unfortunate that we have the storage of comments mixed across Node and Type, but I understand why you did it this way.

There is a backstory as to why the parser mixes the creation of Node and Type objects — back in the beginning, it felt like a small optimization. But then when I tried to change everything into Node for a better separation of concerns, it became apparent that the checker now had a dependency on this behavior: this is because the type-checker runs as a single pass of the Node tree, and for some situations it becomes suspiciously convenient that the Type objects are already there during the "first pass" (that is, the processing of Type objects as part of the parser functions like a "hidden zero-th pass").

This is eventually fixable, but not something I want us to do right away because changing that has other consequences to the later checking stage (there are other reasons why I'm considering turning the type-checker into a more (nowadays) common multi-pass process).

So we'll keep it the way you did it for now — this was more for sharing and "documenting" the context behind it.

(cc @Frityet since this is also important for you to know)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For me, this mixing between the Node and Type felt a bit "off". However, it's the best we can do without a major refactor of the parser codebase.

Good to know the context behind it.

Copy link
Member
@hishamhm hishamhm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is in the right direction! I think there's one problem spot I caught on review, and the other big thing is a suggestion for a further improvement.

@hishamhm
Copy link
Member
hishamhm commented Jun 9, 2025

@upedd Ah, another thing: I think for consistency's sake we should take any comments that you're attaching to Type objects, and copy them in the map_type function as well (since I think those might end up eventually being exposed in the tl types function in the future for IDE completions etc.)

@upedd
Copy link
Contributor Author
upedd commented Jun 9, 2025

Thank you for your thorough review, @hishamhm. I’ve fixed all the issues you pointed out.

Also noticed and fixed some flawed logic in the test code.

@hishamhm hishamhm merged commit 4ccded2 into teal-language:master Jun 10, 2025
8 checks passed
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.

2 participants
0