[Macros] Introduce SwiftSyntaxMacroExpansion module #1654
Conversation
|
@swift-ci Please test |
rintaro
force-pushed
the
macro-expansion-module
branch
from
40a544e to
76aa581
Compare
This module is a unified macro expansion logic shared between 'swift' ASTGen and SwiftCompilerPluginMessageHandling
rintaro
force-pushed
the
macro-expansion-module
branch
from
76aa581 to
c57b410
Compare
|
@swift-ci Please test |
|
swiftlang/swift#65882 |
| .target( | ||
| name: "SwiftSyntaxMacroExpansion", | ||
| dependencies: ["SwiftSyntax", "SwiftSyntaxMacros"], | ||
| exclude: ["CMakeLists.txt"] | ||
| ), | ||
|
|
There was a problem hiding this comment.
Since this is a new target, could you add a new MARK section for it? Each MARK section only contains the target + its test target right now
There was a problem hiding this comment.
Huh, I didn't realize that, thank you, will do.
| declarationNode: DeclSyntax, | ||
| parentDeclNode: DeclSyntax?, | ||
| in context: Context | ||
| ) -> [String]? { |
There was a problem hiding this comment.
Would it maybe be cleaner to make this method throw instead of returning an optional? I think requiring callers to add the error message to the context’s diagnostics isn’t too much to ask.
There was a problem hiding this comment.
I think there's no reason not to catch exceptions, turn it to a diagnostic and emit it to the context in this function. The whole point of this PR is to minimize the duplicated logic. Why do you think making it throws is cleaner?
There was a problem hiding this comment.
I think I just have a disliking of making nil indicate the error case because it doesn’t allow the client to check what went wrong. Since this is public anyone could call it to try expanding a macro node and for their use case, it might be useful to know what went wrong, even though for our use cases it doesn’t matter.
That being said, I think I’m fine with the current approach if we add a comment saying that upon failure (aka. if this returns nil) a diagnostic is guaranteed to be added to context.
| } | ||
| } | ||
|
|
||
| /// Expand `@attached(XXX)` macros. |
There was a problem hiding this comment.
It would be nice to also document all the parameters.
| let parentDeclGroup = parentDeclNode?.asProtocol(DeclGroupSyntax.self) | ||
| else { | ||
| // Compiler error: 'parentDecl' is mandatory for MemberAttributeMacro. | ||
| throw MacroExpansionError(description: "parent decl group is nil") |
There was a problem hiding this comment.
Any reason why you went to string-based error messages instead of enum cases. I personally prefer enums cases because it doesn’t bury the message somewhere in the source code and enum’s you can actually check programmatically if you want.
This module is a unified macro expansion logic shared between 'swift' ASTGen and SwiftCompilerPluginMessageHandling