-
Notifications
You must be signed in to change notification settings - Fork 422
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
Add keypath method and initializer syntax under an experimental feature flag keypathWithMethodMembers
#2950
base: main
Are you sure you want to change the base?
Conversation
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.
The implementation looks good to me. I have a few small comments inline.
Child( | ||
name: "leftParen", | ||
kind: .token(choices: [.token(.leftParen)]), | ||
isOptional: true | ||
), |
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.
Why is the left parenthesis optional? Wouldn’t we just have a normal member component if we don’t have parentheses?
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 modeled this after .funcCallExpr
but you're right and I will make this non optional. @ahoppen I also have 2 related questions:
.keyPathPropertyComponent
has a .genericArgumentClause
but I don't see any examples of it being used in the tests. Do you happen to know when that is used? It seems like a generic on the root (eg: \Box<Int>.item
) is already being handled by KeyPathExpr
.
Is there a way to make .labeledExprList
optional? I forgot to account for this - partially applied methods parse their method name and arg list into DeclReferenceExpr
and do not have a LabeledExprList
. I'm inclined to create a second node to handle this (eg: .keyPathPartiallyAppliedComponent
).
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.
.keyPathPropertyComponent
has a.genericArgumentClause
but I don't see any examples of it being used in the tests. Do you happen to know when that is used? It seems like a generic on the root (eg:\Box<Int>.item
) is already being handled byKeyPathExpr
.
I don’t think a generic clause is actually valid in key path properties and maybe we should remove it. @rintaro Can you think of a reason why KeyPathPropertyComponentSyntax
should have a generic arguments clause?
Is there a way to make
.labeledExprList
optional? I forgot to account for this - partially applied methods parse their method name and arg list intoDeclReferenceExpr
and do not have aLabeledExprList
. I'm inclined to create a second node to handle this (eg:.keyPathPartiallyAppliedComponent
).
I’m not sure I understand. If there are no parentheses, we should be able to parse the component as a KeyPathPropertyComponentSyntax
. If there are parentheses and no arguments, arguments
can just be an empty list. And if there are arguments … then it doesn’t need to be optional.
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’m not sure I understand. If there are no parentheses, we should be able to parse the component as a KeyPathPropertyComponentSyntax. If there are parentheses and no arguments, arguments can just be an empty list. And if there are arguments … then it doesn’t need to be optional.
Should partial arguments should also be KeyPathPropertyComponentSyntax? method(_:)
and method(arg:)
for unapplied keypath methods are being parsed as DeclReferenceExpr
with DeclNameArgumentListSyntax
arguments instead of LabeledExprList
. I have pushed changes that reflect this, unsure if there are any ramifications of doing it this way, but the Swift and c++ parser outputs match.
fixedSource: #"\Foo.<#identifier#>()"#, | ||
experimentalFeatures: .keypathWithMethodMembers | ||
) | ||
} |
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.
Some suggestions for more test cases
\Foo.method<Int>()
\Foo.method<Int>(arg:)
\Foo.method(_:)
for unnamed arguments
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.
- \Foo.method<Int>()
- \Foo.method<Int>(arg:)
I'm getting an unable to round trip message for these 2 invalid test cases (and no diagnostics). How do I resolve this?
e0c6ccb
to
7762c01
Compare
This accompanies the compiler implementation for Method and Initializer Keypaths which extends keypath usage to include references to methods and initializers: