Skip to content

Deprecate SyntaxFactory #539

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Aug 11, 2022
Merged

Deprecate SyntaxFactory #539

merged 1 commit into from
Aug 11, 2022

Conversation

ahoppen
Copy link
Member

@ahoppen ahoppen commented Aug 2, 2022

Instead of always writing SyntaxFactory.make* we can just use the initializers of the nodes that should be created, which leads to a lot nicer code IMO.

CC @akyrtzi @harlanhaskins in case you remember any compelling reasons why we decided to use SyntaxFactory instead of initializers when this was implemented.

rdar://97910890

@ahoppen ahoppen requested review from rintaro, bnbarham and CodaFi August 2, 2022 06:56
@ahoppen
Copy link
Member Author

ahoppen commented Aug 2, 2022

@swift-ci Please test

@ahoppen ahoppen force-pushed the pr/no-syntax-factory branch from 64b17d4 to d7e2ccb Compare August 2, 2022 09:57
@ahoppen
Copy link
Member Author

ahoppen commented Aug 2, 2022

@swift-ci Please test

bnbarham
bnbarham approved these changes Aug 2, 2022
View reviewed changes
Copy link
Contributor

@bnbarham bnbarham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM assuming there was no good reason for it (I assume just an easy place to see all the nodes that could be created?)

@harlanhaskins
Copy link
Contributor

From what I remember in conversations with @bitjammer, the goal of SyntaxFactory was to make autocompletion much easier -- rather than needing to know the kinds of nodes available, you'd type SyntaxFactory.make and autocomplete would show you what's available.

I personally don't know if that's necessarily worth it, especially as we're moving toward a Result-builder DSL future for Swift

@ahoppen
Copy link
Member Author

ahoppen commented Aug 2, 2022

Yes, I agree that the verbosity of SyntaxFactory is not really worth the improved code completion behavior. Good documentation would probably serve a better job here.

@bitjammer
Copy link

That’s right. The motivation was that there were just an incredible number of node types. If the tools and documentation could be improved, or there is another approach that could provide scoped hints to code completion, there’s no need.

@rintaro
Copy link
Member

rintaro commented Aug 2, 2022 •
edited
Loading

I personally like factory methods on the type instead of init(...) and blank()

static func make(...) -> Self
static func blank() -> Self

One drawback of SyntaxFactory.makeXXX was that we couldn't use implicit member expression like

func genFunctionDecl() -> FunctionDeclSyntax {
   ...
   return .make(....)
}

I feel return .make(...) is more clean than return .init(...)

evnik
evnik reviewed Aug 5, 2022
View reviewed changes
Sources/SwiftSyntaxBuilder/TupleExprElementConvenienceInitializers.swift
@@ -17,6 +17,6 @@ public extension TupleExprElement {
/// The presence of the colon will be inferred based on the presence of the label.
init(label: String? = nil, expression: ExpressibleAsExprBuildable) {
self.init(
label: label.map(TokenSyntax.identifier), colon: label == nil ? nil : .colon, expression: expression)
label: label.map { TokenSyntax.identifier($0) }, colon: label == nil ? nil : .colon, expression: expression)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't it equivalent to what it was?

Copy link
Member Author

@ahoppen ahoppen Aug 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The old implementation doesn’t work anymore because TokenSyntax.identifier now takes three arguments, two of which are defaulted. But the argument default values are not used when referencing the function.

@ahoppen
Deprecate SyntaxFactory
0682c85 
Instead of always writing `SyntaxFactory.make*` we can just use the initializers of the nodes that should be created, which leads code that’s nicer to read.

rdar://97910890
@ahoppen ahoppen force-pushed the pr/no-syntax-factory branch 2 times, most recently from b93da38 to 3bb792d Compare August 11, 2022 13:50
@ahoppen
Copy link
Member Author

ahoppen commented Aug 11, 2022

@swift-ci Please test

harlanhaskins
harlanhaskins reviewed Aug 11, 2022
View reviewed changes
Sources/SwiftSyntax/SyntaxFactory.swift.gyb
@@ -119,6 +124,7 @@ public enum SyntaxFactory {
% leading_trivia = token_trivia(token.requires_leading_space)
% trailing_trivia = token_trivia(token.requires_trailing_space)
% if token.is_keyword:
@available(*, deprecated, message: "Use TokenSyntax.${token.swift_kind()}Keyword instead")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: update this deprecation and remove the Keyword suffix

@ahoppen ahoppen force-pushed the pr/no-syntax-factory branch from 3bb792d to c1eeac6 Compare August 11, 2022 15:29
rintaro
rintaro reviewed Aug 11, 2022
View reviewed changes
Sources/SwiftSyntax/Tokens.swift.gyb Outdated
% leading_trivia = '.space' if token.requires_leading_space else '[]'
% trailing_trivia = '.space' if token.requires_trailing_space else '[]'
% if token.is_keyword:
public static func `${lowercase_first_word(token.name)}`(
Copy link
Member

@rintaro rintaro Aug 11, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's wrong with token.swift_kind()? I feel, for example, TokenSyntax.is(presence: .missing) doesn't seem to be creating a new instance. ... And I'm not a fan of TokenSyntax.`init`()
I do think SwiftSyntaxBuilder should use token.swift_kind()

@ahoppen ahoppen force-pushed the pr/no-syntax-factory branch from c1eeac6 to 0682c85 Compare August 11, 2022 18:16
@ahoppen
Copy link
Member Author

ahoppen commented Aug 11, 2022

@swift-ci Please test

rintaro
rintaro approved these changes Aug 11, 2022
View reviewed changes
Copy link
Member

@rintaro rintaro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We decided to go with initializers, instead of factory methods.

@ahoppen ahoppen merged commit ccab159 into swiftlang:main Aug 11, 2022
@ahoppen ahoppen deleted the pr/no-syntax-factory branch August 11, 2022 19:40
ahoppen added a commit to ahoppen/swift-syntax that referenced this pull request Aug 12, 2022
@ahoppen
Fix build failure due to two PRs racing
c5aa935 
SwiftSyntax failed to build because swiftlang#539 and swiftlang#578 conflicted
@ahoppen ahoppen mentioned this pull request Aug 12, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@harlanhaskins harlanhaskins harlanhaskins left review comments

@evnik evnik evnik left review comments

@rintaro rintaro rintaro approved these changes

@bnbarham bnbarham bnbarham approved these changes

@CodaFi CodaFi Awaiting requested review from CodaFi

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@ahoppen @harlanhaskins @bitjammer @rintaro @bnbarham @evnik