Add result builder to decl blocks #305

kimdv · 2021-08-27T16:36:23Z

I have not forgotten you, just had a busy at work and then summer vacation!
And European Championship in football where Denmark did it very good ⚽️
But I'll get some time to look at somethings again.

From #291
This PR aims to A MemberDeclBlock should be directly creatable from a MemberDeclListBuilder result builder. This would probably take some hardcoding of MemberDeclBlock in the convenience initializers where the initialization of a MemberDeclBlock with a MemberDeclListBuilder is another convenience added (besides the current ones of: syntax collection through result builder, tokens by string literals and defaulting of tokens)

This will make it possible to make things like

StructDecl("Foo") {
    StructDecl("Bar")
}

I will next week start too look into it again, and catch up.
I will properly ask some questions

ahoppen · 2021-08-31T11:53:02Z

Cool. Thanks! And there’s no need to apologize, you are working on this in your free time after all. 😉

Let me know when the code is ready for review or if you have any questions. I just took a look at the code and it still appears to be in an incomplete state to me.

ahoppen · 2021-09-20T14:35:21Z

@kimdv What’s the state of this PR? Is it ready for review yet or still work in progress?

kimdv · 2021-09-20T15:58:29Z

@ahoppen I think it is in a state where it is ready for review.

I have rebased with latest master

ahoppen

I just looked through the changes and thought it was a little over-complicated to only generate the ExpressibleAs protocols for types that are children of syntax collections – why not have them for all syntax nodes as they might also come in handy later? I also thought it was a bit of a shame that ExprBuildable didn’t conform to ExpressibleAsExprBuildable.

I couldn’t resist the temptation to play around with your design myself and think it’s easiest to just implement the ExpressibleAs protocols in Buildables.swift.gyb. That way we can also conform protocols like ExprBuildable to their corresponding ExpressibleAs protocols. I pushed my draft idea to ahoppen@d6d4f12. What do you think?

kimdv · 2021-09-23T09:58:17Z

I just looked through the changes and thought it was a little over-complicated to only generate the ExpressibleAs protocols for types that are children of syntax collections – why not have them for all syntax nodes as they might also come in handy later?

Sure we can generate for them all. Did that initially, but went away because I wouldn't introduce a lot of protocols that wasn't used. Will add them again.

I also thought it was a bit of a shame that ExprBuildable didn’t conform to ExpressibleAsExprBuildable.

My thought was just as we have a ExprBuildable protocol it would be a bit duplicated to have a ExpressibleAsExprBuildable?
But maybe there is some scenarios I haven't thought of.

I couldn’t resist the temptation to play around with your design myself and think it’s easiest to just implement the ExpressibleAs protocols in Buildables.swift.gyb. That way we can also conform protocols like ExprBuildable to their corresponding ExpressibleAs protocols. I pushed my draft idea to ahoppen@d6d4f12. What do you think?

So we should move everything from ExpressibleBuildables.swift.gyb to Buildables.swift.gyb.

ahoppen · 2021-09-23T10:08:24Z

Sure we can generate for them all. Did that initially, but went away because I wouldn't introduce a lot of protocols that wasn't used. Will add them again.

My thought process was that we are exposing these protocols publicly and it’s hard to explain why certain syntax kinds have ExpressibleAs protocols and some don’t in case someone wants to use them in their own codebase. Generating all just eliminates that problem.

My thought was just as we have a ExprBuildable protocol it would be a bit duplicated to have a ExpressibleAsExprBuildable?
But maybe there is some scenarios I haven't thought of.

That’s a good point. I hadn’t thought of that. But I think that

if we have an ExressibleAsExprBuildable protocol, ExprBuildable should definitely conform to it (how is ExprBuildable not expressible as ExprBuildable 🤯) or
we should not have the ExpressibleAsExprBuildable protocol at all. In that case, ResultBuilders.swift.gyb would need to check whether element_type has an ExpressibleAs protocol or
typealias ExpressibleAsExprBuildable = ExprBuildable and add a comment that this type alias only exists to simplify code generation.

I don’t have a strong preference for either of these options.

So we should move everything from ExpressibleBuildables.swift.gyb to Buildables.swift.gyb.

Not necessarily. I only did so to conform ExprBuildable to ExpressibleAsExprBuildable (and reuse some of the loops). If we decide that we don’t need ExressibleAsExprBuildable as a protocol, everything can live inside ExpressibleBuildables.swift.gyb again.

kimdv · 2021-09-23T18:02:19Z

@ahoppen I have applied your suggestions 👍

I ended with adding ExressibleAsExprBuildable.
After some thoughts I think it would be strange that ExprBuildable would be the only one with no ExpressibleAs as protocol.

Tests/SwiftSyntaxBuilderTest/ExpressibleBuildablesTests.swift

ahoppen · 2021-09-24T09:18:19Z

After some thoughts I think it would be strange that ExprBuildable would be the only one with no ExpressibleAs as protocol.

It wouldn’t have been the only one. DeclBuildable also wouldn’t have had an ExpressibleAs protocol 😉 Still I agree it makes sense to add them for consistency.

ahoppen

Looking very good to me. And the test cases really start to look like what I’ve always envisioned SwiftSyntaxBuilder to look like 🤩

I’ve added some minor comments regarding the test cases inline.

Sources/SwiftSyntaxBuilder/StringLiteralExprConvenienceInitializers.swift

Tests/SwiftSyntaxBuilderTest/ExpressibleBuildablesTests.swift

ahoppen · 2021-09-24T09:23:49Z

Tests/SwiftSyntaxBuilderTest/ExpressibleBuildablesTests.swift

+final class ExpressibleBuildablesTests: XCTestCase {
+    func testExpressibleAsMemberDeclListItem() {
+        let myStruct = StructDecl(identifier: "MyStruct", members: MemberDeclBlock(membersBuilder: {
+            VariableDecl(.var, name: "myFirstVar", type: "Int")


This is really starting to look like you can generate real-world code with SwiftSyntaxBuilder 😍

Yes! It look really good and it gets more and more clean to read! 🎉

Tests/SwiftSyntaxBuilderTest/ExpressibleBuildablesTests.swift

ahoppen · 2021-09-24T09:27:12Z

Tests/SwiftSyntaxBuilderTest/ExpressibleBuildablesTests.swift

+    }
+
+    func testExpressibleAsCodeBlockItem() {
+        let myCodeBlock = SourceFile(eofToken: SyntaxFactory.makeToken(.eof, presence: .present)) {


Not part of this PR but would it make sense to add

extension TokenSyntax { static let eof = SyntaxFactory.makeToken(.eof, presence: .present) }

So we could improve this to

Suggested change

let myCodeBlock = SourceFile(eofToken: SyntaxFactory.makeToken(.eof, presence: .present)) {

let myCodeBlock = SourceFile(eofToken: .eof) {

And possibly try to remove the need to specify the eofToken in the SourceFile initializer altogether.

Added in #314

Tests/SwiftSyntaxBuilderTest/ExpressibleBuildablesTests.swift

ahoppen

Nice. Looks good to me. 🚀

ahoppen · 2021-09-24T12:21:57Z

@swift-ci Please test

swift-ci · 2021-09-24T12:22:13Z

Build failed
Swift Test Linux Platform
Git Sha - 633025d

swift-ci · 2021-09-24T12:22:56Z

Build failed
Swift Test OS X Platform
Git Sha - 633025d

kimdv · 2021-09-24T16:03:34Z

Rebased on latest master

ahoppen · 2021-09-24T18:16:28Z

@swift-ci Please test

swift-ci · 2021-09-24T18:16:46Z

Build failed
Swift Test Linux Platform
Git Sha - 5c28acc

swift-ci · 2021-09-24T18:17:23Z

Build failed
Swift Test OS X Platform
Git Sha - 5c28acc

kimdv force-pushed the kimdv/add-result-builder-to-decl-blocks branch 7 times, most recently from f0a1d02 to 882014e Compare September 5, 2021 13:32

kimdv force-pushed the kimdv/add-result-builder-to-decl-blocks branch from 882014e to 28d9e2f Compare September 20, 2021 15:48

kimdv marked this pull request as ready for review September 20, 2021 15:51

kimdv force-pushed the kimdv/add-result-builder-to-decl-blocks branch 2 times, most recently from 0527b7c to cad4f2b Compare September 20, 2021 15:57

kimdv force-pushed the kimdv/add-result-builder-to-decl-blocks branch from cad4f2b to 2a529d0 Compare September 22, 2021 18:23

ahoppen reviewed Sep 23, 2021

View reviewed changes

kimdv force-pushed the kimdv/add-result-builder-to-decl-blocks branch from 2a529d0 to c82e22e Compare September 23, 2021 17:41

kimdv force-pushed the kimdv/add-result-builder-to-decl-blocks branch from c82e22e to eae4100 Compare September 23, 2021 20:09

kimdv commented Sep 23, 2021

View reviewed changes

Tests/SwiftSyntaxBuilderTest/ExpressibleBuildablesTests.swift Outdated Show resolved Hide resolved

ahoppen reviewed Sep 24, 2021

View reviewed changes

kimdv force-pushed the kimdv/add-result-builder-to-decl-blocks branch 2 times, most recently from e7f75c0 to 5c28acc Compare September 24, 2021 10:37

kimdv requested a review from ahoppen September 24, 2021 10:39

ahoppen approved these changes Sep 24, 2021

View reviewed changes

ahoppen self-assigned this Sep 24, 2021

Introduce ExpressibleAs protocols

2228008

kimdv force-pushed the kimdv/add-result-builder-to-decl-blocks branch from 5c28acc to 2228008 Compare September 24, 2021 16:00

kimdv mentioned this pull request Sep 24, 2021

Add .eof for token syntax #314

Merged

ahoppen merged commit 7314cb0 into swiftlang:main Sep 28, 2021

kimdv deleted the kimdv/add-result-builder-to-decl-blocks branch September 28, 2021 15:05

	let myCodeBlock = SourceFile(eofToken: SyntaxFactory.makeToken(.eof, presence: .present)) {
	let myCodeBlock = SourceFile(eofToken: .eof) {

Add result builder to decl blocks #305

Add result builder to decl blocks #305

Uh oh!

Conversation

kimdv commented Aug 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ahoppen commented Aug 31, 2021

Uh oh!

ahoppen commented Sep 20, 2021

Uh oh!

kimdv commented Sep 20, 2021

Uh oh!

ahoppen left a comment

Choose a reason for hiding this comment

Uh oh!

kimdv commented Sep 23, 2021

Uh oh!

ahoppen commented Sep 23, 2021

Uh oh!

kimdv commented Sep 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

ahoppen commented Sep 24, 2021

Uh oh!

ahoppen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ahoppen Sep 24, 2021

Choose a reason for hiding this comment

Uh oh!

kimdv Sep 24, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ahoppen Sep 24, 2021

Choose a reason for hiding this comment

Uh oh!

kimdv Sep 24, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ahoppen left a comment

Choose a reason for hiding this comment

Uh oh!

ahoppen commented Sep 24, 2021

Uh oh!

swift-ci commented Sep 24, 2021

Uh oh!

swift-ci commented Sep 24, 2021

Uh oh!

kimdv commented Sep 24, 2021

Uh oh!

ahoppen commented Sep 24, 2021

Uh oh!

swift-ci commented Sep 24, 2021

Uh oh!

swift-ci commented Sep 24, 2021

Uh oh!

Uh oh!

kimdv commented Aug 27, 2021 •

edited

Loading

kimdv commented Sep 23, 2021 •

edited

Loading