Skip to content

Use a dedicated Token type in SwiftSyntaxBuilder #647

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 5 commits into from
Aug 29, 2022

Conversation

ahoppen
Copy link
Member

@ahoppen ahoppen commented Aug 27, 2022

At the moment, tokens were a little bit of an oddity in SwiftSyntaxBuilder: All nodes were represented by dedicated types in SwiftSyntaxBuilder but tokens were represented by the TokenSyntax from the SwiftSyntax module. This has lead to a few problems:

  • We needed to add extensions on TokenSyntax to add convienience properties that are only designed for SwiftSyntaxBuilder
  • TokenSyntax was not SyntaxBuildable and thus couldn’t be used as elements in unexpected node collections

In general, it seems cleaner if we just add a custom Token type in SwiftSyntaxBuilder.

Also: Move the files that declare convenience initializers in SwiftSyntaxBuilder to their own directory.

rdar://99030016

@ahoppen ahoppen requested a review from fwcd August 27, 2022 08:37
@ahoppen
Copy link
Member Author

ahoppen commented Aug 27, 2022

@swift-ci Please test

@ahoppen
Copy link
Member Author

ahoppen commented Aug 27, 2022

@swift-ci Please test macOS

Copy link
Member

@fwcd fwcd left a comment

Choose a reason for hiding this comment

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

Looks good, a few minor notes/questions below

Copy link
Contributor

@kimdv kimdv left a comment

Choose a reason for hiding this comment

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

There is a space in the path name Convenience Initializers

Is that intended?

@fwcd
Copy link
Member

fwcd commented Aug 27, 2022

I stumbled on that too, I think in macOS-land it's not an uncommon convention to use spaces in folder names (see e.g. ~/Library/Application Support), but in the Linux world it's rather uncommon, mostly since it requires an extra escape and introduces a risk of subtle bugs in shell scripts that do not properly quote strings.

I think ConvenienceInitializers would look just as good and avoid these problems, wdyt?

@ahoppen ahoppen force-pushed the ahoppen/token-buildable branch from 4bab48e to 0d584d4 Compare August 28, 2022 08:06
At the moment, tokens were a little bit of an oddity in SwiftSyntaxBuilder: All nodes were represented by dedicated types in SwiftSyntaxBuilder but tokens were represented by the `TokenSyntax` from the SwiftSyntax module. This has lead to a few problems:
- We needed to add extensions on `TokenSyntax` to add convienience properties that are only designed for SwiftSyntaxBuilder
- `TokenSyntax` was not `SyntaxBuildable` and thus couldn’t be used as elements in unexpected node collections

In general, it seems cleaner if we just add a custom `Token` type in SwiftSyntaxBuilder.

rdar://99030016
This allows up to pick up the `ExpressibleAs` conformances from ExpressibleAsConformances.py
@ahoppen ahoppen force-pushed the ahoppen/token-buildable branch from 0d584d4 to 37508b8 Compare August 28, 2022 08:08
@ahoppen
Copy link
Member Author

ahoppen commented Aug 28, 2022

There is a space in the path name Convenience Initializers

Is that intended?

I didn’t put too much thought in it. But I agree that it’s easier for tooling in general if it doesn’t contain a space, so I changed it to ConvenienceIntializers. Thanks.

@ahoppen
Copy link
Member Author

ahoppen commented Aug 28, 2022

@swift-ci Please test

Copy link
Member

@fwcd fwcd left a comment

Choose a reason for hiding this comment

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

Some minor stylistic comments below, otherwise this looks very good, thanks! I also like how we could unify Tokens.swift, TokenSyntax.swift and the new Token type into a single file 👍

@ahoppen
Copy link
Member Author

ahoppen commented Aug 29, 2022

@swift-ci Please test

@ahoppen ahoppen merged commit c0f3642 into swiftlang:main Aug 29, 2022
@ahoppen ahoppen deleted the ahoppen/token-buildable branch August 29, 2022 12:50
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