-
Notifications
You must be signed in to change notification settings - Fork 420
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
[macros] prepare generic arguments for replacement in macros #2450
Conversation
@swift-ci please smoke test |
2d4ea46
to
e861835
Compare
Thanks folks! I added the notes, and a test covering the new functionality as well. Please have a look 👀 |
@swift-ci please smoke test |
@swift-ci Please test |
Tests/SwiftSyntaxMacroExpansionTest/MacroReplacementTests.swift
Outdated
Show resolved
Hide resolved
Tests/SwiftSyntaxMacroExpansionTest/MacroReplacementTests.swift
Outdated
Show resolved
Hide resolved
Thanks a lot for the review! I'll follow up shortly |
b696a64
to
d449bac
Compare
@swift-ci Please test |
swiftlang/swift#71271 |
|
||
override func visit(_ node: GenericArgumentSyntax) -> SyntaxVisitorContinueKind { | ||
guard let baseName = node.argument.as(IdentifierTypeSyntax.self)?.name else { | ||
// Handle error |
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.
Is this a leftover todo?
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.
Wasn't sure what to do here, but actually best might be to just .skipChildren and call it a day
Tests/SwiftSyntaxMacroExpansionTest/MacroReplacementTests.swift
Outdated
Show resolved
Hide resolved
Tests/SwiftSyntaxMacroExpansionTest/MacroReplacementTests.swift
Outdated
Show resolved
Hide resolved
Tests/SwiftSyntaxMacroExpansionTest/MacroReplacementTests.swift
Outdated
Show resolved
Hide resolved
Tests/SwiftSyntaxMacroExpansionTest/MacroReplacementTests.swift
Outdated
Show resolved
Hide resolved
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.
Just an open question: Would it make sense to specialize macros with types that aren’t part of the second-level macro? Ie. would the following make sense?
macro gen<T>(a: T) = #externalMacro …
macro genString(a: String) = #gen<String>(a: a)
And then #genString(a: "x")
would have the intermediate expansion step #gen<String>(a: "x")
Similarly, would it make sense to allow generic nesting in a second-level macro definition? Ie. would something like the following make sense?
macro gen<T>(a: T) = #externalMacro …
macro genArray<T>(a: [T]) = #gen<Array<T>>(a: a)
And then #genArray<Int>(a: [1])
would have the intermediate expansion step #gen<Array<Int>>(a: [1])
AFAICT neither of those are currently supported.
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.
Yeah these may be worth supporting as well -- I made a ticket for it #2476
Thanks for the thorough review Alex. I think we're good now, if CI gods are willing 😇
Co-authored-by: Alex Hoppen <[email protected]>
5af31e5
to
9d22af9
Compare
swiftlang/swift#71271 |
swiftlang/swift#71271 |
Swift-syntax changes necessary of allowing the following code:
I didn't quite implement it entirely right and need to add a test here as well.
Resolves rdar://122004157