-
Notifications
You must be signed in to change notification settings - Fork 45
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 attribute access and call args on reference expressions #221
Conversation
super(); | ||
this.type = "CallArguments"; | ||
this.positional = positional; | ||
this.named = named; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CallArguments
is a separate SyntaxNode
for two reasons:
- The tooling parser can store the span of
(...)
on it. - In
TermReference
, theargs
field is nullable. This allows us to distinguish between-term
and-term()
, which are functionally the same, but require different serialization.
super(); | ||
this.type = "TermReference"; | ||
this.id = id; | ||
this.attribute = attribute; | ||
this.args = args; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Naming things... attribute
or attr
to make it consistent with id
and args
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Full name sounds about right to me. attr
is less obvious about the fact that this is exactly one or none.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this. In particular the ease in abstract.mjs
is nice, I've struggled with reviewing the Term.attr the first time around.
There's a nit about left-overs in abstract.mjs
for AttributeExpression
, make that 💥 ?
Independently of the r=me, we should talk about when to do this.
super(); | ||
this.type = "TermReference"; | ||
this.id = id; | ||
this.attribute = attribute; | ||
this.args = args; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Full name sounds about right to me. attr
is less obvious about the fact that this is exactly one or none.
Ah yes, thanks, I forgot about that part.
I think we should do this in 0.9, perhaps after #214 lands, so that we don't have to deal with |
Right now, we store complex member expressions as nested trees of AST nodes. As an example, a parameterized term attribute of the form
-term.attr(arg: "value")
parses as:This is a well established pattern in general-purpose languages. It is capable of handling deeply nested hierarchies of expressions. In Fluent, however, we only ever go so deep. All the possible nestings are known up-front and are defined by the grammar.
Furthermore, the above AST structure makes tooling harder. Checking for a
TermReference
means checking for four different AST structures (corresponding to-term
,-term()
,-term.attr
, and-term.attr()
).In this PR, I've tried a different approach: the attribute access and the call arguments are stored on the reference node itself:
I implemented this approach for
MessageReference
,TermReference
,VariableReference
andFunctionReference
.