-
Notifications
You must be signed in to change notification settings - Fork 402
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
Conform TokenSyntax
to ExpressibleByStringLiteral
and ExpressibleByStringInterpolation
#507
Conversation
@swift-ci please test |
We have had this idea before and decided against it at the time and I think that argument still holds: #295 (comment) What do you think? |
Hm, I see your point. In my (limited) experience though, I think there is value in making the common case easy to express. The syntax builder DSL reads a lot better IMO, in many cases there is also an argument label that already states that the syntax is an identifier, so having to include |
@@ -30,7 +30,7 @@ final class StructTests: XCTestCase { | |||
genericWhereClause: GenericWhereClause { | |||
GenericRequirement(body: ConformanceRequirement(leftTypeIdentifier: "A", rightTypeIdentifier: "X")) | |||
GenericRequirement(body: SameTypeRequirement( | |||
leftTypeIdentifier: "A.P", equalityToken: .identifier("=="), rightTypeIdentifier: "D")) | |||
leftTypeIdentifier: "A.P", equalityToken: "==", rightTypeIdentifier: "D")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is mostly what I’m worried about. Technically, ==
should never have been an identifier
but .spacedBinaryOperator("==")
. This doesn’t really matter if all you do with the syntax tree is print it but e.g. a formatter might care about the distinction.
It’s not the most pretty solution but what do you think about the following: We introduce a new type IdentifierToken
and use that as a parameter for all syntax children that have type IdentifierToken
struct IdentifierToken: ExpressibleByStringLiteral {
let token: TokenSyntax
init(_ token: TokenSyntax) {
assert(token.tokenKind == .identifier)
self.token = token
}
// If necessary, also add these
func withLeadingTrivia(_ leadingTrivia: Trivia) -> IdentifierToken { /* … */ }
func withTrailingTrivia(_ trailing: Trivia) -> IdentifierToken { /* … */ }
}
That would cover the common case of just passing in a string for the identifier. Using any of the functions that are defined on TokenSyntax
becomes a little harder because you first need to create TokenSyntax.identifier
, then modify it and wrap it in IdentifierToken
, but maybe that’s not a big issue in practice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, yeah, we should fix that test.
I like the idea of statically encoding where we expect an identifier. The only thing I could think about is that there are a few places (like argument labels) where also other tokens like .wildcard
are 'allowed', but perhaps it would be fine to keep TokenSyntax
in these places to make this explicit at the use site.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I have experimented a bit with this idea over here, unfortunately fewer nodes than I expected are pure/arbitrary identifiers. Most prominently, FunctionDecl
still has to take a TokenSyntax
since it could e.g. represent a operator too. Since the vast majority of uses (at least in the test suite and among the SwiftSyntaxBuilderGeneration
templates is FunctionDecl
identifiers, I don't think such a workaround would be worth the hassle for now.
Perhaps we could just add a convenience initializer for FunctionDecl
that takes a String
parameter? It feels like there ought to be some more general solution, but apart from providing an ExpressibleAsStringLiteral
conformance on TokenSyntax
directly, I don't really see how we could provide this sugar without disproportionally complicating some other part of SwiftSyntaxBuilder
.
Since we decided to defer this, I'll close this PR. |
As mentioned in #506 (comment), we are using
TokenSyntax.identifier
quite a lot inSwiftSyntaxBuilderGeneration
.This PR therefore conforms
TokenSyntax
toExpressibleByStringLiteral
andExpressibleByStringInterpolation
, thereby making the use site more concise. This is also consistent with how we can useString
s to express many identifier-like nodes (seeStringConvenienceInitializers
).Previously:
Now:
Since the
.identifier
function still exists, this should be a fully backwards-compatible change.