Skip to content

[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

Merged
merged 3 commits into from
Jun 10, 2023

Conversation

DougGregor
Copy link
Member

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.

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.
@DougGregor DougGregor requested a review from ahoppen as a code owner June 9, 2023 20:39
@DougGregor
Copy link
Member Author

@swift-ci please test

Comment on lines +52 to +59
switch pluginMacroRole {
case .expression: macroRole = .expression
case .declaration: macroRole = .declaration
case .codeItem: macroRole = .codeItem

case .accessor, .conformance, .member, .memberAttribute, .peer:
throw MacroExpansionError.invalidMacroRole(pluginMacroRole)
}
Copy link
Member

@rintaro rintaro Jun 9, 2023

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.

Copy link
Member

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

Copy link
Member Author

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

Copy link
Member Author

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Cleanup is at #1762

@DougGregor DougGregor merged commit 4d8981f into swiftlang:main Jun 10, 2023
@DougGregor DougGregor deleted the freestanding-macro-role-message branch June 10, 2023 00:04
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.

2 participants