Significantly simplify SwiftSyntaxBuilder’s implementation

[ahoppen]

This significantly simplifies the implementation of SwiftSyntaxBuilder. The main cleanups are:

• Removing the separate syntax node hierarchy in SwiftSyntaxBuilder and instead re-using the SwiftSyntax hierarchy
  • This is possible since BasicFormat now formats an entire SwiftSyntax tree instead of requiring the careful intraction with SwiftSyntaxBuilder’s Buildable types
  • All convenience initializers from SwiftSyntaxBuilder are now extensions on the SwiftSyntax nodes. We can even consider sinking them down into SwiftSyntax again
• Removing the ExpressibleAs* types
  • With the integration of the parser into SwiftSyntaxBuilder, you create way fewer syntax nodes explicitly. Looking at the test cases, I think the regressions on the client side aren’t too significant and this hugely simplifies the SwiftSyntaxBuilder’s implementation
  • Having written a fair amount of SwiftSyntaxBuilder code myself now, I never found the ExpressibleAs* protocols super useful because it was non-trivial to discover which types are expressible as other types.
  • IMHO things are also clearer without the ExpressibleAs conformances because the conversion from ExprBuildable to InitializerClause implicitly inserted a =, which is a non-trivial fix up, I think.

I think the best way to review this is to skim over the new generated files (don’t bother looking at the diff because it’s way too noisy) as well as the updated test cases. ahoppen@f8af287 contains the changes that need to be made to CodeGeneration to adopt to this simplified SwiftSyntaxBuilder.

@fwcd and @kimdv I would really value your thoughts on this change and whether you think getting rid of ExpressibleAs is a change that we can make without making the library harder to use. You wrote the most SwiftSyntaxBuilder code and can probably judge this best.

[ahoppen]

@swift-ci Please test

[ahoppen]

@swift-ci Please test

[kimdv]

Hmm.. Not sure I like that we do this.
I think that FunctionCallExpr(calledExpression: "test", trailingClosure: ClosureExpr()) is a bit easier to to read and it's easier to get started with SwiftSyntaxBuilder

[ahoppen]

I thought the same at first as well but after writing this a few times, I think I mildly prefer the new syntax. Here’s why:

• When you create a FunctionCallExpr, code completion will add you a placeholder that has type ExprSyntaxProtocol. The natural step to do next is to try and create something that’s ExprSyntaxProtocol, so you probably start with Expr. Knowing that you can also pass a string literal requires more in-depth knowledge of SwiftSyntaxBuilder, which newcomers probably won’t have
• In the new design, you can jump-to-definition on Expr to see how the parser is invoked. This makes it easier to understand what’s actually going on IMO because there are less magic conversions going on behind the scenes.
• This also allows more complex expressions (like member access). The old design was only correct for identifiers (because it created an IdentifierExpr)

Thinking about it some more, I think we could get this working again by changing the parameter type from ExprSyntaxProtocol to ExprBuildable where ExprBuildable is defined as follows:

protocol ExprBuildable {
  func createExpr() -> Expr
}

extension ExprSyntaxProtocol: ExprBuildable {
  func createExpr() -> Expr { return Expr(self) }
}

extension String: ExprBuildable {
  func createExpr() -> Expr { return Expr(self) }
}

That’s probably not too terrible. WDYT?

[ahoppen]

OK, I just tried what I suggested above and it doesn’t work:

extension ExprSyntaxProtocol: ExprBuildable // error: Extension of protocol 'ExprSyntaxProtocol' cannot have an inheritance clause

Given that this simple approach doesn’t work, it would mean that we’d need to explicitly conform all Expr nodes go ExprBuildable, which defeats the beauty of the new implementation. Considering the arguments I gave above, I think that any implementation we find that makes it possible to pass strings here makes SwiftSyntaxBuilder harder to understand instead of easier.

[kimdv]

I see.
Let's go with the new design.
I think I also need to dive deeper into the usage of the new API. 😁

[kimdv]

Some initial toughts:

• I like the removal of ExpressibleAs* 🎉
• Some of the convince initialiser removals I'm not sure all are could. Specially those where we have to add Expr("someThing"). It makes a bit harder to get started in my opinion.

[ahoppen]

@swift-ci Please test

[ahoppen]

@swift-ci Please test

[ahoppen]

@swift-ci Please test

[ahoppen]

I’m merging this now as a starting point to reconsider which convenience initializer we still need now that we have the parser integration. We can always go and add those we want back in.

[fwcd]

Wow, you're moving quickly, sorry I didn't have time to look at this in detail. :D

Skimming over the proposal I'd say that I especially like the unification of the node hierarchies! These changes make things conceptually a lot simpler, which could be especially useful e.g. for tools that both parse and generate Swift code (e.g. source code transformation tools). IIUC, they could leverage the expressive power of SwiftSyntaxBuilder without the boilerplate of converting between different representations.

If we can address the added verboseness of explicit Expr wraps through protocols like ExprBuildable I see no downside there 👍

[ahoppen]

Yeah, sorry for merging this so quickly, I just have other PRs that depend on this and I wanted to make progress.

If we can address the added verboseness of explicit Expr wraps through protocols like ExprBuildable I see no downside there 👍

I’ve got an alternative proposal: We should provide more hand-written convenience initializers that use the parser interop to eliminate the need for most of the syntax tree construction altogether. With #1012 (and the corresponding CodeGeneration changes in ahoppen@fe30a08) we only have two places in CodeGeneration left where a call to Expr is now needed because we no longer have the implicit conversion from String.