Skip to content
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

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

Open
ahoppen opened this issue Aug 2, 2023 · 17 comments · May be fixed by #2142
Open

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

ahoppen opened this issue Aug 2, 2023 · 17 comments · May be fixed by #2142
Assignees
Labels
enhancement New feature or request good first issue Good for newcomers

Comments

@ahoppen
Copy link
Member

ahoppen commented Aug 2, 2023

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

@ahoppen ahoppen added enhancement New feature or request good first issue Good for newcomers labels Aug 2, 2023
@natikgadzhi
Copy link
Contributor

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
Copy link
Member Author

ahoppen commented Aug 3, 2023

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

@natikgadzhi
Copy link
Contributor

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
Copy link
Member Author

ahoppen commented Aug 4, 2023

Sounds good to me 👍🏽

@natikgadzhi
Copy link
Contributor

Found a little bug somewhere in swift-markdown or swift-docc: swiftlang/swift-docc#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
Copy link
Contributor

@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
Copy link
Member Author

ahoppen commented Aug 5, 2023

Found a little bug somewhere in swift-markdown or swift-docc: swiftlang/swift-docc#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
Copy link
Contributor

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

@natikgadzhi
Copy link
Contributor

@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
Copy link
Contributor

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
Copy link
Member Author

ahoppen commented Aug 31, 2023

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
Copy link
Contributor

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 added a commit to natikgadzhi/swift-syntax that referenced this issue Aug 31, 2023
**Summary**
This adds a convenience init to ClosureCaptureSyntax that adds equal
token if the provided `name` is not nil.

It's supposed to be used by developers, so it omits the unexpected
tokens.

Related: swiftlang#1984
natikgadzhi added a commit to natikgadzhi/swift-syntax that referenced this issue Aug 31, 2023
**Summary**
This adds a convenience init to ClosureCaptureSyntax that adds equal
token if the provided `name` is not nil.

It's supposed to be used by developers, so it omits the unexpected
tokens.

Related: swiftlang#1984
@natikgadzhi
Copy link
Contributor

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
Copy link
Member Author

ahoppen commented Aug 31, 2023

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
Copy link
Contributor

@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
Copy link
Member Author

ahoppen commented Sep 1, 2023

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

@ahoppen
Copy link
Member Author

ahoppen commented Sep 1, 2023

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.

natikgadzhi added a commit to natikgadzhi/swift-syntax that referenced this issue Sep 1, 2023
**Summary**
This adds a convenience init to ClosureCaptureSyntax that adds equal
token if the provided `name` is not nil.

It's supposed to be used by developers, so it omits the unexpected
tokens.

Related: swiftlang#1984
@natikgadzhi natikgadzhi linked a pull request Sep 1, 2023 that will close this issue
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants