-
Notifications
You must be signed in to change notification settings - Fork 402
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
Generate BuildableCollectionNodes
with SwiftSyntaxBuilderGeneration
#506
Generate BuildableCollectionNodes
with SwiftSyntaxBuilderGeneration
#506
Conversation
@swift-ci please test |
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.
A note on the implementation and a few ideas for (future) improvements to the DSL below
Sources/SwiftSyntaxBuilderGeneration/Templates/BuildableCollectionNodesFile.swift
Outdated
Show resolved
Hide resolved
Sources/SwiftSyntaxBuilderGeneration/Templates/BuildableCollectionNodesFile.swift
Outdated
Show resolved
Hide resolved
Sources/SwiftSyntaxBuilderGeneration/Templates/BuildableCollectionNodesFile.swift
Outdated
Show resolved
Hide resolved
parameters: ParameterClause( | ||
parameterList: [ | ||
FunctionParameter( |
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.
Having Array: ExpressibleAsParameterClause
might be cool, this would be pretty analogous to #506 (comment).
Sources/SwiftSyntaxBuilderGeneration/Templates/BuildableCollectionNodesFile.swift
Outdated
Show resolved
Hide resolved
Sources/SwiftSyntaxBuilderGeneration/Templates/BuildableCollectionNodesFile.swift
Outdated
Show resolved
Hide resolved
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 thoughts inline.
Looking at this gigantic result builder, I’m wondering whether it makes sense to create the different initializers and functions in standalone functions. That way we would get rid of two nesting levels and could document the functions with what they generate.
Also, I think function calls are still pretty verbose with their TupleExprElement
s, but I haven’t come up with a better solution either…
Sources/SwiftSyntaxBuilderGeneration/Templates/BuildableCollectionNodesFile.swift
Outdated
Show resolved
Hide resolved
Sources/SwiftSyntaxBuilderGeneration/Templates/BuildableCollectionNodesFile.swift
Outdated
Show resolved
Hide resolved
Sources/SwiftSyntaxBuilderGeneration/Templates/BuildableCollectionNodesFile.swift
Outdated
Show resolved
Hide resolved
Sources/SwiftSyntaxBuilderGeneration/Templates/BuildableCollectionNodesFile.swift
Outdated
Show resolved
Hide resolved
Sources/SwiftSyntaxBuilderGeneration/Templates/BuildableCollectionNodesFile.swift
Outdated
Show resolved
Hide resolved
Sources/SwiftSyntaxBuilderGeneration/Templates/BuildableCollectionNodesFile.swift
Outdated
Show resolved
Hide resolved
ba56542
to
4b64757
Compare
4b64757
to
021df63
Compare
021df63
to
581042f
Compare
Whoops, didn't intend to auto-close this :D |
581042f
to
834ee19
Compare
528a58c
to
ce4f4f3
Compare
Sources/SwiftSyntaxBuilderGeneration/Templates/BuildableCollectionNodesFile.swift
Outdated
Show resolved
Hide resolved
ce4f4f3
to
76fdb9d
Compare
I have cleaned up the template and factored out the generated functions along with some doc comments, so it should be a bit more readable now. @swift-ci please test |
Sources/SwiftSyntaxBuilderGeneration/Templates/BuildableCollectionNodesFile.swift
Outdated
Show resolved
Hide resolved
f2a1094
to
ea88f57
Compare
IfStmt( | ||
conditions: OptionalBindingCondition( | ||
letOrVarKeyword: .let, | ||
pattern: "leadingTrivia", | ||
initializer: "leadingTrivia" | ||
) | ||
) { | ||
ReturnStmt(expression: FunctionCallExpr(MemberAccessExpr(base: "result", name: "withLeadingTrivia")) { | ||
TupleExprElement(expression: FunctionCallExpr(MemberAccessExpr( | ||
base: TupleExpr { | ||
SequenceExpr { | ||
"leadingTrivia" | ||
BinaryOperatorExpr("+") | ||
TupleExpr { | ||
SequenceExpr { | ||
MemberAccessExpr(base: "result", name: "leadingTrivia") | ||
BinaryOperatorExpr("??") | ||
ArrayExpr(elements: []) | ||
} | ||
} | ||
} | ||
}, | ||
name: "addingSpacingAfterNewlinesIfNeeded" | ||
))) | ||
}) |
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.
Contrary to what I proposed in #506 (comment), we may have to stick with the if-let
for now since the variant I proposed has slightly different semantics (the if
-version will only consider result.leadingTrivia
if the function parameter leadingTrivia
is set). This becomes relevant as otherwise .addingSpacingAfterNewlinesIfNeeded()
will generate duplicate indentation in some cases.
One alternative solution would be to use optional chaining and only apply the spacing-after-newlines to the function argument leadingTrivia
:
let combinedLeadingTrivia = (leadingTrivia?.addingSpacingAfterNewlinesIfNeeded() ?? []) + (result.leadingTrivia ?? [])
return combinedLeadingTrivia
instead of the current
if let leadingTrivia = leadingTrivia {
return result.withLeadingTrivia((leadingTrivia + (result.leadingTrivia ?? [])).addingSpacingAfterNewlinesIfNeeded())
} else {
return result
}
While that seems to yield the correct result, I am not sure whether there are some edge cases where this logic breaks down (or whether we make too many assumptions about the kind of leading trivia here, e.g. whitespace trivia is 'nice' in the sense that it commutes and therefore may not exercise certain logic errors).
For this reason I decided not to make this change until we have figured that out. Any thoughts on this?
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.
One alternative solution would be to use optional chaining and only apply the spacing-after-newlines to the function argument leadingTrivia
I think that sounds reasonable. I would suggest we discuss it in a separate PR.
@swift-ci please test |
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.
One minor comment regarding the generated documentation, otherwise LGTM.
Sources/SwiftSyntaxBuilderGeneration/Templates/BuildableCollectionNodesFile.swift
Outdated
Show resolved
Hide resolved
- Stub out BuildableCollectionNodes generation - Generate elements initializer in BuildableCollectionNodes - Generate combining initializer - Generate arrayLiteral initializer - Generate build method implementation - Add buildSyntax implementation - Add expressibleAs conformance method - Add createSyntaxBuildable - Generate Array conformances - Bootstrap BuildableCollectionNodes - Use FunctionSignatures in InitializerDecls - Use new IfStmt convenience init in BuildableCollectionNodesFile - Use new builder-based ExprList initializers - Use new builder-based ConditionElementList initializer - Use new VariableDecl convenience initializer - Use new SequenceExpr initializer - Factor out utility method for format-leadingTrivia params - Factor out generated functions in BuildableCollectionNodesFile - Parenthesize leading trivia correctly
fe98ad5
to
1b2af6f
Compare
@swift-ci please test |
This PR ports the currently gyb-generated
BuildableCollectionNodes
toSwiftSyntaxBuilder
's DSL as part of the ongoing effort to bootstrapSwiftSyntaxBuilder
withSwiftSyntaxBuilderGeneration
.