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

Store annotations within the node they annotate #14913

Open
Blacksmoke16 opened this issue Aug 15, 2024 · 3 comments
Open

Store annotations within the node they annotate #14913

Blacksmoke16 opened this issue Aug 15, 2024 · 3 comments

Comments

@Blacksmoke16
Copy link
Member

From #9322 (comment):

@[Bar]
def foo
end

Is currently parsed as:

Expressions.new([
  Annotation.new(Path.new(["Bar"])),
  Def.new("foo", [] of Arg),
] of ASTNode)

@straight-shoota suggested changing this in #9322 (comment) such that the annotation in this example would be stored within the Def node itself. Similar to how #12044 handled it for method/macro parameters. Which makes a lot more sense to me as well.

From an implementation perspective it's a bit tricky since unlike annotated parameters we do not necessarily know what specific node we're parsing until after the annotations have already been parsed. The best way I could think of is keeping track of the Annotation nodes within the parser itself, then apply them to an applicable node once we determine what it is, then clear the annotations array to prepare it for the next method/type/what have you.

Backwards compatibility also came up, which seems to suggest we would want to keep both APIs, meaning like we'd still represent the annotations as an Expressions, but also store a copy of them within the Def?

@Blacksmoke16
Copy link
Member Author

Blacksmoke16 commented Aug 15, 2024

So this may actually be a bit trickier than I thought. I got a prototype mostly working, however I'm unsure how to go about handling this:

@[Link("pcre2-8", pkg_config: "libpcre2-8")]
{% if compare_versions(Crystal::VERSION, "1.11.0-dev") >= 0 %}
  @[Link(dll: "pcre2-8.dll")]
{% end %}
lib LibPCRE2
end

With my working branch and some debug pp!:

$ ccrystal run --prelude=empty test.cr
name        # => LibPCRE2
annotations # => [@[Link("pcre2-8", pkg_config: "libpcre2-8")]]
...

It seems we can't actually do this purely in the parser due to macro code like this not yet being evaluated to allow adding the second Link annotation to the LibDef node directly. The current implementation works I guess because by the time the semantic visitor runs, the macro has expanded which results in def visit(node : Annotation) being called twice properly populating @annotations with both. Whereas the ones I parsed directly and added to the LibDef only has the first one. And AFAIK we wouldn't really have a way to know what node the macro generated nodes relate to?

@straight-shoota
Copy link
Member

Maybe the current implementation is actually best then. It all happens in TopLevelVisitor: macro expressions are expanded and annotations are collected and attached to the following node.

@Blacksmoke16
Copy link
Member Author

Yea, probably just going to shelve this for now in that case. Given there isn't a use case, yet at least, that really requires this refactor.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants