crystal-lang / crystal

The Crystal Programming Language
https://crystal-lang.org
Apache License 2.0
19.45k stars 1.62k forks source link

Store annotations within the node they annotate #14913

Open Blacksmoke16 opened 2 months ago

Blacksmoke16 commented 2 months ago

From https://github.com/crystal-lang/crystal/issues/9322#issuecomment-859676500:

@[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 https://github.com/crystal-lang/crystal/issues/9322#issuecomment-862129869 such that the annotation in this example would be stored within the Def node itself. Similar to how https://github.com/crystal-lang/crystal/pull/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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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.