-
Notifications
You must be signed in to change notification settings - Fork 439
[Macros] Pass macro role for freestanding macro expansion #1757
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] Pass macro role for freestanding macro expansion #1757
Conversation
Macros can only have a single freestanding macro role, so that their expansion is unambiguous. However, that restriction is at the level of a macro declaration, and is not reflected in a similar restriction on the set of macro protocols to which a macro implementation can conform. Therefore, have the compiler pass a freestanding macro role through to macro expansion in the same way that we do for attached macros, and use that to determine which macro protocol to use. Part of fixing rdar://110418969.
@swift-ci please test |
switch pluginMacroRole { | ||
case .expression: macroRole = .expression | ||
case .declaration: macroRole = .declaration | ||
case .codeItem: macroRole = .codeItem | ||
|
||
case .accessor, .conformance, .member, .memberAttribute, .peer: | ||
throw MacroExpansionError.invalidMacroRole(pluginMacroRole) | ||
} |
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.
(Not critical at all)
Could you use MacroRole.init(messageMacroRole: PluginMessage.MacroRole)
(at the bottom of this file) here?
Since expandFreestandingMacro
diagnoses unmatched macro role, and invalid macro role here would be a compiler error, diagnosing invalid is not necessary I think.
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.
But if we do diagnose invalid role here, we should do it in expandAttachedMacro()
too
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'd rather be defensive here, because we're on one side of an IPC barrier
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.
Nevermind, I see what you mean. It'll be checked and produce an error down in expandFreestandingMacro
, so we don't need the redundant check here.
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.
Cleanup is at #1762
Macros can only have a single freestanding macro role, so that their
expansion is unambiguous. However, that restriction is at the level of a
macro declaration, and is not reflected in a similar restriction on the
set of macro protocols to which a macro implementation can conform.
Therefore, have the compiler pass a freestanding macro role through to
macro expansion in the same way that we do for attached macros, and
use that to determine which macro protocol to use.
Part of fixing rdar://110418969.