[Macros] Rework the ExpressionMacro API based on pitch feedback #1104
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…text. Having the source location converter in the context exposes the entire contents of the source file containing the macro expansion to the implementation of the macro. While convenient for the macro author, this breaks Swift's incremental build model, because Swift assumes that (e.g.) changing the body of a function cannot affect the result of type-checking the body of an unrelated function. Drop the implementations of some "built-in" macros that are no longer expressible, such as `#filePath`, `#line`, and `#column`.
We're talking about these as macro "expansions" uniformly, now.
This matches the current proposal, and makes it possible for mutations to the context in the future.
Providing the macro expansion syntax node directly to the macro implementation allowed those implementations to chase its parent pointer, which indirectly exposed the full source code of the enclosing file to the macro implementation. "Disconnect" the syntax node from its parent so the syntax node is independent of all other source code in the file. Another sample macro implementation, `#function`, is now unusable---but this is a better model for incremental builds.
Per pitch feedback, eliminate MacroResult. Instead, macro implementations can produce diagnostics by calling `diagnose()` on the macro evaluation context.
"Expand" better describes what the macro implementation is doing.
rxwei
reviewed
Dec 3, 2022
| static func apply( | ||
| _ macro: MacroExpansionExprSyntax, in context: MacroEvaluationContext | ||
| ) -> MacroResult<ExprSyntax> | ||
| static func expand( |
There was a problem hiding this comment.
IMO the method should start with a noun phrase to be in line with the API guidelines, e.g. expansion(for:in:), because the method is not meant to have side effects.
There was a problem hiding this comment.
A later commit introduces the potential for side effects in the macro evaluation context, e.g., for emitting diagnostics and creating new unique local names.
There was a problem hiding this comment.
I've thought about this more, and you're totally right. PR to call it expansion(for:in:): #1112
|
@swift-ci please test |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Make a number of changes to the API for expression macros:
MacroEvaluationContextgets renamed toMacroExpansionContextMacroExpansionContext, because one shouldn't have access to the whole source fileMacroExpansionContextinoutMacroResult; instead, allow diagnostics to be emitted via the macro expansion contextMacroExpansionContext.createUniqueLocalName()ExpressionMacro.applytoExpressionMacro.expand