Skip to content

buildExpression overload for ListBuilder proposal #2477

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
Feb 19, 2024

Conversation

RoBo-Inc
Copy link
Contributor

@RoBo-Inc RoBo-Inc commented Feb 6, 2024

There is an overload in ListBuilder:

@_disfavoredOverload
static func buildExpression(_ expression: FinalResult) -> Component {
  expression.map { $0 }
}

This overload isn't a "standard" function for a resultBuilder to have. It's a convenience overload for the "swift-syntax" library. If it happens that our expression is already of type FinalResult, we can still use the same syntax without needing to handle this scenario separately. I stumbled upon this overload accidentally while working on my pet project when was surprised to find that my code still compiles. So, yes, it's quite convenient.

In this PR, I propose adding one more buildExpression overload for convenience:

static func buildExpression(_ expression: Component) -> Component {
  expression
}

Its purpose is similar to the function:

static func buildArray(_ components: [Component]) -> Component {
  components.flatMap { $0 }
}

It allows to build arrays. While the above function facilitates array building using the for ... in ... syntax, the overload I propose enables array building using result of .map { } expression.
It has always felt strange to me that "standard" resultBuilders work with for ... in ... but not with .map { }. It seems counterintuitive. Personally, I prefer the .map { } syntax over for ... in .... I believe it would be beneficial to allow developers to use both syntaxes when working with resultBuilders.

This proposal is additive and has no impact on existing source code.

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.

Looks nice. Thank you!

Could you also add a short test case that uses this?

@RoBo-Inc RoBo-Inc force-pushed the build_expression_overload branch from 9551b50 to 0d6b731 Compare February 8, 2024 05:05
@RoBo-Inc
Copy link
Contributor Author

RoBo-Inc commented Feb 8, 2024

Updated ✅

@RoBo-Inc RoBo-Inc requested a review from ahoppen February 8, 2024 05:08
@RoBo-Inc
Copy link
Contributor Author

RoBo-Inc commented Feb 8, 2024

By the way, for some reason ./swift-syntax-dev-utils local-pr-precheck started to fail on my machine. On the main branch, so it's not related to my changes 🙂

error: couldn't build /Users/roman/Projects/swift-syntax/SwiftParserCLI/.build/x86_64-apple-macosx/debug/_AtomicBool.build/src/AtomicBool.c.o because of missing inputs: /Users/roman/Projects/swift-syntax/Sources/_AtomicBool/src/AtomicBool.c

@RoBo-Inc RoBo-Inc force-pushed the build_expression_overload branch from 0d6b731 to a6197d8 Compare February 8, 2024 09:43
@Matejkob
Copy link
Contributor

Matejkob commented Feb 8, 2024

By the way, for some reason ./swift-syntax-dev-utils local-pr-precheck started to fail on my machine. On the main branch, so it's not related to my changes 🙂

error: couldn't build /Users/roman/Projects/swift-syntax/SwiftParserCLI/.build/x86_64-apple-macosx/debug/_AtomicBool.build/src/AtomicBool.c.o because of missing inputs: /Users/roman/Projects/swift-syntax/Sources/_AtomicBool/src/AtomicBool.c

Hmm, that's odd. It works for me. Have you tried after @keith's changes: #2484

@RoBo-Inc
Copy link
Contributor Author

RoBo-Inc commented Feb 8, 2024

By the way, for some reason ./swift-syntax-dev-utils local-pr-precheck started to fail on my machine. On the main branch, so it's not related to my changes 🙂

error: couldn't build /Users/roman/Projects/swift-syntax/SwiftParserCLI/.build/x86_64-apple-macosx/debug/_AtomicBool.build/src/AtomicBool.c.o because of missing inputs: /Users/roman/Projects/swift-syntax/Sources/_AtomicBool/src/AtomicBool.c

Hmm, that's odd. It works for me. Have you tried after @keith's changes: #2484

Funny, looks like it depends on Terminal application (I'm using "Warp"). 🤯 But in "iTerm2" it seems everything works fine. ✅

@keith
Copy link
Member

keith commented Feb 8, 2024

(note my changes are unrelated unless you're using bazel)

looks like you just needed a clean build

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.

Thanks!

@ahoppen
Copy link
Member

ahoppen commented Feb 9, 2024

@swift-ci Please test

@RoBo-Inc
Copy link
Contributor Author

So can the PR be merged now? 😃

@ahoppen
Copy link
Member

ahoppen commented Feb 19, 2024

@swift-ci Please test Windows

@ahoppen
Copy link
Member

ahoppen commented Feb 19, 2024

Oh, sorry. Forgot about it. Thanks for the reminder.

@ahoppen ahoppen enabled auto-merge February 19, 2024 16:33
@ahoppen ahoppen merged commit c7b2500 into swiftlang:main Feb 19, 2024
@RoBo-Inc RoBo-Inc deleted the build_expression_overload branch February 20, 2024 01:28
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.

4 participants