Skip to content

Add convenience initializers for simple expr values #288

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

Merged
merged 1 commit into from
Jun 13, 2021

Conversation

kimdv
Copy link
Contributor

@kimdv kimdv commented May 24, 2021

I have tried to make some initializers for the types I thought would be helpfull.

I could also try to generate all Expr that have a Keyword in the init parameter like NilLiteralExpr

Something like

extension NilLiteralExpr {
    public init(
        nilKeyword: TokenSyntax = Tokens.nil
    ) {
        self.init(nilKeyword: nilKeyword)
    }
}

@kimdv kimdv force-pushed the kimdv/add-convenience-inits-pt-2 branch from e7de2af to 7b3a8a3 Compare May 24, 2021 19:23
@kimdv kimdv force-pushed the kimdv/add-convenience-inits-pt-2 branch 3 times, most recently from 3dc69f8 to 075708a Compare May 25, 2021 19:13
@kimdv kimdv force-pushed the kimdv/add-convenience-inits-pt-2 branch 2 times, most recently from 2fa8c38 to d7d87aa Compare May 26, 2021 19:17
@ahoppen
Copy link
Member

ahoppen commented May 27, 2021

It’s not directly related to this PR, but I felt a bit uneasy about the BuildablesConvenienceInitializers and I understand why now: There are currently three areas through which we optimize the ergonomics of the initializers:

  1. Providing default values of nil and Tokens.something for optional children and token children without text
  2. Taking a String parameter for tokens with text and constructing a TokenSyntax from the string
  3. Supporting result builders for children that are syntax collections

I think 1. should already be implemented in Buildables.swift.gyb. Currently, we only default optional children to nil there. Also defaulting tokens would be nice, I think. Ideally, we could factor out the code that computes the default value and reuse it in BuildablesConvenienceInitializers.swift.gyb.

Regarding 2. and 3., I think it’s reasonable that we only provide an initializer that applies all optimizations and ignore use cases where a user might want to initialize a token by a string but not use the result builder. This is what we are doing already.

And just so recap, we need to be careful not to create a convenience initializer if neither 2. nor 3. apply because then we’re redeclaring the initializer defined in Buildables.swift.gyb. This, again, is what you’re already doing.

Long story short, I think the following changes would make the implementation more streamlined and easier to understand:

  • Factor out the code that determines default values for parameters and use them in Buildables.swift.gyb as well as BuildablesConvenienceInitializers.swift.gyb. Default values should be nil and Token.something where applicable.
  • In BuildablesConvenienceInitializers.swift.gyb, when doing the pre-scan whether a convenience initializer should be generated (link) just keep track of a single boolean variable should_create_convenience_initializer. Set it to True if either of the conditions in 2. or 3. are met
  • Always expose all parameters in the convenience initializer. If I understand correctly, with this PR you are sometimes omitting defaulted parameters in the convenience initializer to avoid redeclaring the one in Buildables.swift.gyb. If we discovered that the conditions of 2. or 3. are satisfied, we know that we aren’t redeclaring the initializer in Buildables.swift.gyb so it’s safe to include them.

Does this make sense to you @kimdv?

@ahoppen ahoppen self-assigned this May 27, 2021
@kimdv
Copy link
Contributor Author

kimdv commented May 27, 2021

I thinks it makes sense yes!
I think I will start with the first thing. Adding nil or Tokens.something in the Buildables.swift.gyb. 😁

@kimdv
Copy link
Contributor Author

kimdv commented May 31, 2021

@ahoppen I think I made what you wrote.

I can split change in Buildables.swift.gyb in another PR if you want?

The helper method is put in this PR: swiftlang/swift#37712

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.

This is looking very good overall. I’ve added a few remarks/questions inline.

@kimdv kimdv force-pushed the kimdv/add-convenience-inits-pt-2 branch from ba7c530 to 72463ed Compare June 3, 2021 15:46
@kimdv
Copy link
Contributor Author

kimdv commented Jun 3, 2021

@ahoppen I have resolved your comment and pushed.
Also rebased swiftlang/swift#37712 with latest main

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.

I’ve got a few suggestions on the wording for comments inline, otherwise looks good to me!

@kimdv kimdv force-pushed the kimdv/add-convenience-inits-pt-2 branch from 72463ed to 58fa415 Compare June 7, 2021 06:49
@kimdv
Copy link
Contributor Author

kimdv commented Jun 7, 2021

@ahoppen I have pushed the suggested changes. Also rebased swiftlang/swift#37712 with latest main

Happy WWDC days! 🎉

@ahoppen
Copy link
Member

ahoppen commented Jun 7, 2021

Happy WWDC days! 🎉

Same to you!

swiftlang/swift#37712
@swift-ci Please test

@ahoppen ahoppen merged commit 6d5968e into swiftlang:main Jun 13, 2021
@kimdv kimdv deleted the kimdv/add-convenience-inits-pt-2 branch March 24, 2022 10:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants