Skip to content

Generate BuildableCollectionNodes with SwiftSyntaxBuilderGeneration #506

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

Jump to bottom
Merged
merged 2 commits into from
Aug 4, 2022
Merged

Generate BuildableCollectionNodes with SwiftSyntaxBuilderGeneration #506

merged 2 commits into from
Aug 4, 2022

Conversation

fwcd
Copy link
Member

@fwcd fwcd commented Jul 20, 2022

This PR ports the currently gyb-generated BuildableCollectionNodes to SwiftSyntaxBuilder's DSL as part of the ongoing effort to bootstrap SwiftSyntaxBuilder with SwiftSyntaxBuilderGeneration.

@fwcd fwcd requested a review from ahoppen as a code owner July 20, 2022 18:55
@fwcd
Copy link
Member Author

fwcd commented Jul 20, 2022

@swift-ci please test

fwcd
fwcd commented Jul 20, 2022
View reviewed changes
Copy link
Member Author

@fwcd fwcd left a 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
Comment on lines 95 to 97
parameters: ParameterClause(
parameterList: [
FunctionParameter(
Copy link
Member Author

@fwcd fwcd Jul 20, 2022 •
edited
Loading

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).

@fwcd fwcd mentioned this pull request Jul 20, 2022
Closed
ahoppen
ahoppen reviewed Jul 20, 2022
View reviewed changes
Copy link
Member

@ahoppen ahoppen left a 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 TupleExprElements, but I haven’t come up with a better solution either…

@fwcd fwcd force-pushed the buildable-collection-nodes-swift-syntax-gen branch from ba56542 to 4b64757 Compare July 26, 2022 14:00
This was referenced Jul 26, 2022
Merged
Merged
Merged
@fwcd fwcd marked this pull request as draft July 27, 2022 15:04
@fwcd fwcd force-pushed the buildable-collection-nodes-swift-syntax-gen branch from 4b64757 to 021df63 Compare July 27, 2022 15:23
@fwcd fwcd mentioned this pull request Jul 27, 2022
Merged
@fwcd fwcd force-pushed the buildable-collection-nodes-swift-syntax-gen branch from 021df63 to 581042f Compare July 27, 2022 15:53
@fwcd fwcd closed this in #524 Jul 27, 2022
@fwcd
Copy link
Member Author

fwcd commented Jul 27, 2022

Whoops, didn't intend to auto-close this :D

@fwcd fwcd reopened this Jul 27, 2022
@fwcd fwcd force-pushed the buildable-collection-nodes-swift-syntax-gen branch from 581042f to 834ee19 Compare July 27, 2022 16:10
@fwcd fwcd mentioned this pull request Jul 27, 2022
Closed
@fwcd fwcd force-pushed the buildable-collection-nodes-swift-syntax-gen branch 2 times, most recently from 528a58c to ce4f4f3 Compare July 29, 2022 22:59
@fwcd fwcd force-pushed the buildable-collection-nodes-swift-syntax-gen branch from ce4f4f3 to 76fdb9d Compare August 2, 2022 20:02
@fwcd fwcd marked this pull request as ready for review August 2, 2022 20:21
@fwcd
Copy link
Member Author

fwcd commented Aug 2, 2022

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

@fwcd fwcd force-pushed the buildable-collection-nodes-swift-syntax-gen branch from f2a1094 to ea88f57 Compare August 4, 2022 02:04
fwcd
fwcd commented Aug 4, 2022
View reviewed changes
Sources/SwiftSyntaxBuilderGeneration/Templates/BuildableCollectionNodesFile.swift Outdated
Comment on lines 211 to 235
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"
)))
})
Copy link
Member Author

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?

Copy link
Member

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.

@fwcd
Copy link
Member Author

fwcd commented Aug 4, 2022

@swift-ci please test

ahoppen
ahoppen approved these changes Aug 4, 2022
View reviewed changes
Copy link
Member

@ahoppen ahoppen left a 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.

fwcd added 2 commits August 4, 2022 15:00
@fwcd
Fix ArrayExpr in SyntaxBuildableChild
cb49936
@fwcd
Generate BuildableCollectionNodes with SwiftSyntaxBuilder
1b2af6f 
- 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
@fwcd fwcd force-pushed the buildable-collection-nodes-swift-syntax-gen branch from fe98ad5 to 1b2af6f Compare August 4, 2022 13:14
@fwcd
Copy link
Member Author

fwcd commented Aug 4, 2022

@swift-ci please test

@fwcd fwcd merged commit cbdc9a4 into swiftlang:main Aug 4, 2022
@fwcd fwcd deleted the buildable-collection-nodes-swift-syntax-gen branch August 4, 2022 14:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ahoppen ahoppen ahoppen approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@fwcd @ahoppen