Add initializer to `LabeledExprSyntax` that automatically adds a colon if the label exists

[ahoppen]

Currently, if you do the following

let node = EnumCaseParameterSyntax(firstName: "label", type: TypeSyntax("MyType"))
print(node.formatted())

you get label MyType, which is surprising because the colon is missing and you would expect the colon to be present if there is a first name of the enum case parameter.

To fix this, we should add a hand-written initializer of EnumCaseParameterSyntax, which takes a non-optional firstName and automatically adds a colon, because it knows that the firstName exists. The above call would pick that overload and thus add a colon.

And the same strategy can also be applied

• to other nodes that have an optional colon
• ClosureCaptureSyntax.equal
• to automatically add parentheses for the following nodes if arguments are present
  • AttributeSyntax
  • MacroExpansionDeclSyntax
  • ClosureCaptureModifierSyntax
  • FunctionCallExprSyntax
  • FreestandingMacroExpansionSyntax

rdar://107794232

[natikgadzhi]

I would love to tackle this after the isAt to at change. This looks a bit more involved, and I would love to dive in and learn a bit more about how things work.

[ahoppen]

I assigned the issue to you. Let me know if you have any questions.

[natikgadzhi]

I started looking into it. The first two PRs I was playing on easy, but for this one, I'll stop and read the documentation on the basic building blocks of swift-syntax — so that one will take a few days.

I hope to put together the first pull request with just the EnumCaseParameterSyntax and ask you for a review, so we check and make sure I got it right. I'll expand to the rest of the list then.

How does that sound, @ahoppen?

[ahoppen]

Sounds good to me 👍🏽

[natikgadzhi]

Found a little bug somewhere in swift-markdown or swift-docc: https://github.com/apple/swift-docc/issues/685

I'm continuing to work on this — hope to have some focus time on Sunday to figure out where the definitions are for the generated code, and how to edit them. Taking it slow to not cut too many corners.

[natikgadzhi]

@ahoppen, by handwritten, do you suggest that we add an extension for EnumCaseParameterSyntax with the initializer, and put it somewhere like Sources/SwiftSyntax/SyntaxNodes-overrides.swift? Is there an existing convention on how we add handwritten functions / properties to the generated structs?

[ahoppen]

Found a little bug somewhere in swift-markdown or swift-docc: https://github.com/apple/swift-docc/issues/685

Oh, interesting. This seems to be related to @Comment. I left a comment on the issue.

I'm continuing to work on this — hope to have some focus time on Sunday to figure out where the definitions are for the generated code, and how to edit them. Taking it slow to not cut too many corners.

Take your time. Thanks for contributing to swift-syntax in the first place.

by handwritten, do you suggest that we add an extension for EnumCaseParameterSyntax with the initializer, and put it somewhere like Sources/SwiftSyntax/SyntaxNodes-overrides.swift? Is there an existing convention on how we add handwritten functions / properties to the generated structs?

Exactly. We do already have similar convenience initializers in the SwiftSyntaxBuilder module. I would suggest that we create a similar file in the SwiftSyntax module.

https://github.com/apple/swift-syntax/blob/main/Sources/SwiftSyntaxBuilder/ConvenienceInitializers.swift

[natikgadzhi]

Ah, I missed them — there's SwiftSyntax/Convenience.swift already! I'll draft it up.

[natikgadzhi]

@ahoppen, quick gut check.

ClosureCaptureSyntax(name: "test", expression: ExprSyntax("123")) already produces [test = 123]. I think the equal sign should only be there if both the name and expression are set, right? And seemingly, it already pops in automatically.

Is there another scenario I'm missing with it?

[natikgadzhi]

Also, here's what I'm doing to get the other nodes with optional colon:

1. Go to CodeGeneration/Sources/*Nodes.swift
2. Read through each file, find Nodes that has a Child colon that isOptional:true.
3. Collect a list
4. Read through the list, gut-check prioritize it on how common those nodes are. For each node, consider what makes the colon show up.
5. Check if a custom initializer makes sense for that particular Node.

I think a good next step is for me to post a list that I'll get and get a thumbs up from you and @Matejkob ;)

[ahoppen]

ClosureCaptureSyntax(name: "test", expression: ExprSyntax("123")) already produces [test = 123]. I think the equal sign should only be there if both the name and expression are set, right? And seemingly, it already pops in automatically.

I just checked and ClosureCaptureSyntax(name: "test", expression: ExprSyntax("123")) produces test123 for me, which is what I was expecting from the current implementation.

But I agree with you that the equal sign should be added automatically if name != nil.

Also, here's what I'm doing to get the other nodes with optional colon

The plan sounds good 👍🏽 I’m curious how much else you find that’s not a colon.

[natikgadzhi]

I just checked and ClosureCaptureSyntax(name: "test", expression: ExprSyntax("123")) produces test123 for me, which is what I was expecting from the current implementation.

That's because I'm dumb, and I actually implemented the convenience init and then ran that test 🤦🏼

  public func testClosureCaptureSyntaxConvenienceInitWithEqual() {
    let noNameClosure = ClosureCaptureSyntax(name: "test", expression: ExprSyntax("123"))

    XCTAssertEqual(noNameClosure.formatted().description, "[test = 123]")
  }

So, you're right, it just picked up my convenience init instead of the generated one. I'll roll that into a little PR of its own.

I’m curious how much else you find that’s not a colon.

I love how you're pushing me to actually think and learn more, instead of just doing some "creative search and replace" ;)

[natikgadzhi]

Pushed up #2127, and realized that we could probably make this work with CodeGeneration.

For some Nodes, there's a rule that we know of that if a certain Child that is optional is not nil, then some other Child should have a default value, instead of also defaulting to nil:

• EnumCaseParamter: if firstName is not nil, add colon default to TokenSyntax.colonToken().
• ClosureCaptureSyntax: if name is not nil, add equal default value of TokenSyntax.equalToken().

I think we can express these rules in Node class, and add CodeGeneration step to generate both the init methods just like I'm writing them, and tests for them as well.

How does that look? As long as I've got the general idea right, I'll tinker with Node in CodeGeneration and see if I can get it to work.

[ahoppen]

If we can express this in CodeGeneration, I think it would be great. But to decide which way, I think it would be good if we had a list of all the cases where we want to automatically synthesize an optional token. It just makes it easier to spot patterns.

[natikgadzhi]

@ahoppen I fully agree. I've played around with CodeGeneration and drafted how the change could look like. Basically:

• In SyntaxNodesFile.swift
• Make a custom version of node.generateInitializerDocComment to clarify the applied rules and list out params.
• Extend generateInitializerDeclHeader() to support rule-based version where some parameters are not optional and have provided default values.
• Add an extra try! InitializerDeclSyntax() using the above doc and decl blocks.
• For the body of that initializer, we can just DeclSyntax or make a FunctionCallSyntax that would elegantly list all of the parameters for us, too.

I haven't looked into how to also generate tests for this, but overall seems doable at my exposure level.

I agree that the next step is to write down the list of SyntaxNodes and the rules that we want to apply, and then see if that simple scheme will work. I should have the list and a draft PR in a couple of days.

[ahoppen]

The approach sounds good in general. I’m looking forward to a PR.

[ahoppen]

Also, if we are generating these initializers, we don’t need to write a test case for every one since if one of them works, we assume that all the others should work as well.