Skip to content

SyntaxNodes: DotSelfExpr should have optional base expression. #21926

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.

Sign up for GitHub

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 1 commit into from
Jan 17, 2019
Merged

SyntaxNodes: DotSelfExpr should have optional base expression. #21926

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion test/Syntax/Outputs/round_trip_parse_gen.swift.withkinds
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -532,7 +532,7 @@ struct S <TypeInheritanceClause>: <InheritedType><SimpleTypeIdentifier>Q</Simple
@_implements(<ImplementsAttributeArguments><SimpleTypeIdentifier>P</SimpleTypeIdentifier>, x</ImplementsAttributeArguments>)</Attribute>
var <PatternBinding><IdentifierPattern>y</IdentifierPattern><TypeAnnotation>: <SimpleTypeIdentifier>String</SimpleTypeIdentifier></TypeAnnotation></PatternBinding></VariableDecl></MemberDeclListItem><MemberDeclListItem><FunctionDecl><Attribute>
@_implements(<ImplementsAttributeArguments><SimpleTypeIdentifier>P</SimpleTypeIdentifier>, g<DeclNameArguments>()</DeclNameArguments></ImplementsAttributeArguments>)</Attribute>
func h<FunctionSignature><ParameterClause>() </ParameterClause></FunctionSignature><CodeBlock>{}</CodeBlock></FunctionDecl></MemberDeclListItem><MemberDeclListItem><VariableDecl><Attribute>
func h<FunctionSignature><ParameterClause>() </ParameterClause></FunctionSignature><CodeBlock>{ <SequenceExpr><DiscardAssignmentExpr>_ </DiscardAssignmentExpr><AssignmentExpr>= </AssignmentExpr><KeyPathExpr>\<DotSelfExpr>.self </DotSelfExpr></KeyPathExpr></SequenceExpr>}</CodeBlock></FunctionDecl></MemberDeclListItem><MemberDeclListItem><VariableDecl><Attribute>

@available(<AvailabilityArgument>*, </AvailabilityArgument><AvailabilityArgument><AvailabilityLabeledArgument>deprecated: <VersionTuple>1.2</VersionTuple></AvailabilityLabeledArgument>, </AvailabilityArgument><AvailabilityArgument><AvailabilityLabeledArgument>message: "ABC"</AvailabilityLabeledArgument></AvailabilityArgument>)</Attribute><DeclModifier>
fileprivate(set) </DeclModifier>var <PatternBinding><IdentifierPattern>x</IdentifierPattern><TypeAnnotation>: <SimpleTypeIdentifier>String</SimpleTypeIdentifier></TypeAnnotation></PatternBinding></VariableDecl></MemberDeclListItem>
Expand Down
2 changes: 1 addition & 1 deletion test/Syntax/round_trip_parse_gen.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -532,7 +532,7 @@ struct S : Q, Equatable {
@_implements(P, x)
var y: String
@_implements(P, g())
func h() {}
func h() { _ = \.self }
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a keypath, not a DotSelfExpr. I don't think its correct that DotSelfExpr's base is optional; you should instead parse keypaths differently. CC @jckarter

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We parse them as DotSelfExpr too for AST, see here.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see it parses like this:

        (keypath_expr type='<null>'
          <<null>>
          (dot_self_expr type='<null>'
            (key_path_dot_expr implicit type='<null>')))))))

So the base is a KeyPathDotExpr. Do you not have an equivalent in SwiftSyntax? It would be better if the base was never optional here and with MemberExpr also.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Syntax tree doesn't have equivalent implicit nodes in general. All nodes should cover at least one underlying token.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The only exception is collection syntax like CodeBlockItemList, which can be an empty set of items.

Copy link
Contributor

Choose a reason for hiding this comment

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

a.self is creating a DotSelfExpr syntax node while .self is creating a ImplicitMemberExpr one. If we want to be consistent here then shouldn't \.self be a KeyPathExpr wrapping an ImplicitMemberExpr ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also there is a difference in .a creating a ImplicitMemberExpr node while \.a is a MemberAccessExpr wrapped by KeyPathExpr.

Copy link
Contributor

Choose a reason for hiding this comment

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

a.self is creating a DotSelfExpr syntax node while .self is creating a ImplicitMemberExpr one. If we want to be consistent here then shouldn't \.self be a KeyPathExpr wrapping an ImplicitMemberExpr ?

After further discussion with Xi, this is explained by the parser treating these differently, .self being a unresolved_member_expr while \.self creating a dot_self_expr.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, makes sense. Since SwiftSyntax is trying to be more uniform and elegant perhaps it would make sense if .self in all contexts was a MemberRefExpr with an identifier of self? Does it make sense for it to be its own thing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, i think this suggestion makes sense. We could reuse existing MemberAccessExpr to cover DotSelfExpr. We could also use MemberAccessExpr to represent ImplicitMemberExpr to further reduce the type complexity.


@available(*, deprecated: 1.2, message: "ABC")
fileprivate(set) var x: String
Expand Down
4 changes: 2 additions & 2 deletions utils/gyb_syntax_support/ExprNodes.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -309,10 +309,10 @@
is_optional=True),
]),

# dot-self-expr -> expr '.' 'self'
# dot-self-expr -> expr? '.' 'self'
Node('DotSelfExpr', kind='Expr',
children=[
Child('Expression', kind='Expr'),
Child('Expression', kind='Expr', is_optional=True),
Child('Dot', kind='Token',
token_choices=[
'PeriodToken', 'PrefixPeriodToken'
Expand Down