Reviews are an important part of the Swift evolution process. All review feedback should be either on this forum thread or, if you would like to keep your feedback private, directly to the review manager by email. When emailing the review manager directly, please keep the proposal link at the top of the message and put "SE-0394" in the subject line.
What goes into a review?
The goal of the review process is to improve the proposal under review through constructive criticism and, eventually, determine the direction of Swift. When writing your review, here are some questions you might want to answer in your review:
What is your evaluation of the proposal?
Is the problem being addressed significant enough to warrant a change to Swift?
Does this proposal fit well with the feel and direction of Swift?
If you have used other languages or libraries with a similar feature, how do you feel that this proposal compares to those?
How much effort did you put into your review? A glance, a quick reading, or an in-depth study?
More information about the Swift evolution process is available at
I have read the proposal and engaged with Boris directly regarding this feature. I'm looking forward to it!
The name .macro() seems a bit off, because it's used to define a macro target, not an individual macro. I'd suggest .macroTarget() for consistency with e.g. .testTarget().
One thing the proposed interface is missing is support for language settings (e.g. .define()) as is available for other target types. Although the macros are not necessarily compiled and executed on the same platform/architecture as other targets (i.e. cross-platform compilation), the ability to specify additional compiler settings for macro targets seems like it is necessary to allow for e.g. adoption of experimental features in a macro target, setting Swift compiler conditionals, etc.
(I'm a bit worried I'm the first person to reply—somebody wanna point me to the right thread if I'm replying in the wrong place? )
This is intentional, we're (slowly) trying to move away from having the target suffix since it is implied by the context. We did the same thing for plugins and eventually we'd like to have e.g. .test() as well.
This is a good point, it came up during the initial pitch and I neglected to add this.
Perfectly reasonable. I'd suggest including this info in the proposal somewhere (perhaps under Alternatives Considered) so the reasoning is documented.
Sorry that I am a bit late but I was on vacation. I gave very similar feedback in Pitch 2 but it was shortly before the review started and did not receive any answers. So I will repost it here:
I very much welcome the ability to declare a SwiftSyntax version range. However, I have some concerns:
I don't think it is the right decision to list macro targets under dependencies: because humans could be confused to think that the target can import the source definitions of the macro target. I would rather see macro targets listed under plugins: or macros:.
The requirement to explicitly list the macro implementations in the macro target looks exactly like this kind of thing where you forget to add a new macro to the list and then spent half an hour to find the error. Is there a specific need for this?
I think it's a reasonable point to want to separate build-only and linked dependencies explicitly, but I don't think re-using plugins would be appropriate and adding macros doesn't seem scalable to me (what if next year, there are linker plugins which become yet another distinct kind of target). It is also already the case that executables get listed under dependencies even though those dependencies are usually build-only (except when depending on one from a test target -- which is also true for macros). It could also be reasonable to not want to link a target that can be linked, e.g. you're making an executable that depends on a set of dynamic libraries which can be opened at runtime using dlopen.
With all that said, I think for the purpose of this proposal, it seems ok for macros to follow executables and be listed in dependencies, but I'd like to add a future direction on how a separation into build-only and linked dependencies could make sense to explore, at which point I would envision that the existing plugins API would also go away.
Sorry, I'm not 100% sure what you are referring to here:
I was a little on the fence about this, because I was trying to think of a reason that a macro program might want to be able to depend on and import the macro library—for example, if there was a data type publicly available in the library for clients that the macro program also wanted to use. As written today, that would be impossible because it would create a dependency cycle.
But, if the macro program could import the macro library and use its data types, that would mean it was also importing the macro declaration itself, but it can't be usable from within the same macro program that implements it. So that convinced me that what you have here is the correct structure, and calling it dependencies sort of self-enforces cycle prevention.
I don't know of a good way around it, because there aren't good facilities for finding all of the types that conform to a particular protocol. I was hoping that SE-0385 "Custom Reflection Metadata" would converge in time for us to adopt it here. I still think it's our best bet for the future, where providingMacros can get a default implementation that uses that custom reflection metadata:
extension CompilerPlugin {
public var providingMacros: [Macro.Type] {
// use custom reflection metadata to find all types that conform to `Macro`
}
}
)