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. When emailing the review manager directly, please keep the proposal link at the top of the message.
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?
Logistical request: Can you please add a link to the original pitch thread to the proposal header? I wanted to reference some of the original discussion and had to search the forums to find it.
Overall, the requirements that traits be purely additive eases many of the concerns I had about build scalability. I still disagree with the idea presented in the original pitch thread that it's more desirable to use traits to control, for example, whether different data structures in Collections are compiled as part of the same module as opposed to just putting them in separate modules and letting users depend on the precise ones they want. But, that's mainly a criticism of specific proposed uses of traits and not of the overall feature, which has other valid uses.
Regarding the proposal itself:
There seems to be an inconsistency in the write-up about allowed characters in names of traits. It claims that they must be valid Swift identifiers, but then goes on to list possibilities that aren't valid Swift identifiers:
These rules would allow for trait names like 1foo, hello-there, and foo+bar. If these traits are getting passed to the compiler as simple -D flags so that they can be checked in #if blocks, how is a user expected to test for any of those which aren't valid Swift identifiers?
Just to make sure I'm on the right page:
A module can write #if TraitName to check internally if a trait is enabled, to control the presence of some API or logic (because the module is compiled with -DTraitName).
Do these propagate up the build graph? Can someone who depends on that package/target also write #if TraitName to control whether something is conditionally done based on whether they've requested that trait for the dependency? I assume the answer is no, because (1) it's not necessary, the author of that code knows whether they've enabled the trait or not on the dependency, and (2) it means that the traits would not actually be namespaced per package as described above.
Would it make sense to allow a different compiler condition name for a trait? e.g. if I want to name my trait "Performance" but don't want to pollute the global compiler condition namespace with such a generic name.
The doc calls out traits in Swift Testing and says this feature is "consistent" with them, but I'm honestly not sure what that means—there doesn't really appear to be any overlap between the two that I can see. Can you clarify what you mean here?
This and the fact that a package must support that all traits are enabled at the same time guarantees that this doesn’t cause problems for builds.
You are right the rules are not correct. I mistakenly assumed that the Unicode XID start characters are the same as legal character identifier heads. I would propose to just drop the rules except the default/defaults one and say that traits must be legal Swift identifiers.
Yes your understanding is correct. A package cannot detect if a trait of its dependencies is active. Otherwise they would need to be globally namespaced. However, a package can define a trait which toggles one or multiple traits in a dependency and can check for that trait in its own code.
Initially, I started off with prefixing all compiler conditions with TRAIT_; however, I decided against that since you as a package author are in complete control here. Do you have a specific case in mind where such a generic name would be problematic?
What we meant with this is just that swift-testing traits lets you customise a test or a suite. Similar to this package traits lets you customise the whole package. I know that might be a bit far fetched but we feel like the name trait is fitting for what we want to enable here.
And if needs be, one can use an empty array to make sure that no traits are enabled.
But, I'd argue this is not what makes the most sense in real world.
I think optimally the API would allow users to provide the diff of what they want compared to the default traits, not overriding all default traits all at once.
Imagine this instead:
traits: [
.disabled("Foo"),
.enabled("Bar")
]
With this API, the users can keep relying on the package's default traits. If there is a new trait added to the package in the future, which is enabled by default, users will have that enabled as well automatically.
With the current API, users will have no choice but to manually declare all traits they want and manually check to see what new traits are added or are enabled by default. Or let go of manually defining traits and just move on with the default traits.
An API similar to what I proposed will also make it possible for package authors to extract certain APIs into traits after they are already released, and enable those traits by default to avoid code breakages. Other users who wish to not rely on or compile those APIs, will be able disable those traits later.
How does this square with the explicit acknowledgement of "mutually exclusive traits" in the proposal? Seems like a package with that pattern wouldn't build with all traits enabled.
My main use case is for compiling a package that supports high-level audio operations on multiple operating systems — it needs to use different “backends” on different operating systems. On Apple systems it will use CoreAudio; on Linux systems it will use some open source C libraries. I think SwiftCrypto has a similar setup.
But it’s helpful to be able to run the Linux backend on macOS for development, even if that’s not what I usually want by default. Or, for example, if building a server based on my library, I probably always want to use the Linux backend — even when running builds on macOS — because that’s how the server will actually be deployed.
We’ve used environment variables + .define(…) for this so far, but I’d be glad to have a better solution.
This seems like a perfect case for mutually exclusive traits.
"Namespacing" is one of the hard problems in computer science.
Swift Testing has a number of knobs and levers that we enable or disable on a platform-specific basis. For instance, WASI doesn't support process spawning, so exit tests (experimental) are disabled there, and the code is guarded by a compiler condition named SWT_NO_EXIT_TESTS that's specified in our Package.swift file.
All these flags are currently opt-out, but it'd be interesting to be able to enable features in the package on an opt-in basis. In theory that would let us start linking to libraries like Swift Argument Parser. We can't do that today because if we did, it would make it much more difficult for SAP to adopt Swift Testing, but if we had the ability to disable our use of SAP via a package trait, they could adopt and enable the trait and opt out of the dependency cycle.
But what would we name that trait? Probably something like "EnableSwiftArgumentParser" (or "DisableSAP" as it were.) Of course, if somebody else has a trait with the same name that does something different (subtly or grossly) then we would have a potential naming conflict wherever we write #if EnableSAP in our code. It'd be better if we could use the same naming scheme we use today internally, i.e. SWT_ENABLE_SAP or similar, while presenting a "nice" or "pretty" name to package clients.
I hope I explained that clearly without getting too far off in the weeds!
I wonder if there's room here to actually build a feature that's more like Swift Testing's traits, i.e. something that uses a protocol and the Swift type system. Just spitballing but:
protocol Trait {
/// The name of the compile-time condition to set when this trait is enabled.
///
/// If `nil`, the name of the trait is used.
var customCompilerConditionName: String? { get }
/// Update a target when this trait is enabled, e.g. by adding Swift settings or
/// dependencies.
func updateTarget(_ target: inout Target)
}
struct EnableSAPTrait: Trait { ... }
extension Trait where Self == EnableSAPTrait {
static var enableSAP: Self { ... }
}
I understand where you are coming from and initially I thought about this the same way. However, after looking at what other ecosystems do and thinking about a few use-cases this didn't really work nicely.
As a consumer of a package it must be possible to say all traits of a dependency are disabled. This allows you to say that you really want the bare-bones API of that package. This is incredibly important if you want to ensure that you really are only using the minimum API surface. If we would allow to providing diffs as a consumer it becomes almost impossible to do this since you would to constantly keep updating your diff with the default traits of the package.
To understand why it's so important to disable all default traits we need to look at a few potential use-cases. Let's imagine NIO would adopt traits. NIO offers different "backends" on various platforms for the eventing and I/O mechanisms. Right now those different backends are in separate modules. Hypothetically NIO might decide to use a single module with traits instead to make the discoverability of these APIs easier. Furthermore, NIO wants to offer the Posix trait by default since this is what 99% of the users need.
As a library author of a package that is platform agnostic and depends on just the NIOCore API you would want to disable all default traits to make sure you are sticking to the core APIs.
While I sympathise with allowing package authors to extract existing APIs behind new traits in and making them a default trait. I think it is more important to allow consumers to disable all default traits and consider moving an API behind a trait a SemVer breaking change.
You are right that there are certain scenarios where a package might want to offer mutually exclusive traits. I consider this an exception and not something that the standard package should do. Once traits become mutually exclusive they must be aligned across the whole dependency graph and most often only make sense to enable/disable on an executable level.
Now there are good scenarios such as the ones you describe but I would encourage if possible to try any other way including platform conditionals before trying to reach for mutually exclusive traits.
I think some of your knobs can definitely be turned into traits. However, traits aren't able to replace platform checks. If you know a certain trait is not available on a platform it is better to use #if os() and similar checks instead. Not saying this applies to your knobs but just a general note.
I played around with various ways to spell this including enums and protocols; however, I decided to avoid all of that since it doesn't really fit into how the manifest is currently declared. It is all string based and doesn't require any special types or generics. While I agree that the manifest could do with an overhaul I think that's out of scope for this particular proposal. It's better to stay consistent and have little less compile time safety than mixing idioms in my opinion.
Right, but I don't think that's what really most Swift users care about.
If we cared about absolute minimum dependencies and minimum binary size and maximum performance, there were more appropriate languages we could choose.
I think the ergonomics are more important than making sure all traits can be disabled altogether, just for the sake of those power-users that want to maximally optimize their Swift apps. I imagine the vast majority of the users will prefer an API like what I proposed, over being able to disable all traits.
What I like about Swift is that it gives pretty good performance and ergonomics without me needing to think too much. If I wanted maximum performance and minimum binary size and all, I could have moved to a language like Rust which is generally more verbose since you need to spell out everything.
I appreciate Swift moving towards better performance and all, but I'm not willing to risk losing the ergonomics.
If needs be, there can be a special case for disabling all traits, but at user's risk of finding out that the API of a dependency broke, over a minor version.
something like:
enum Traits: ExpressibleByArrayLiteral {
case _disableAll
case _enableAll
case diff([Diff])
enum Diff {
case enabled(String)
case disabled(String)
}
init(arrayLiteral: Diff...) {
self = .diff(arrayLiteral)
}
}
Disabling default traits is not about binary size at all. It is about being able to state that you only require the minimum set of APIs and functionality. It is important to step back what default traits provide to package authors. They allow them to say that this set of functionality is what they expect most users want and gives them a great first experience with the package. As a consumer if you disable all default traits you most likely don't care if a new trait is added since you don't need it.
If I start to consume a package I can make a choice do I want the default traits or a custom set or maybe the default traits + some custom ones. After that my package must continue to work when I update to the next SemVer minor or patch of that package. A package author can freely introduce new traits and make them default if they want to. This will never break any body as long as no API gets moved between traits or moved to a trait.
The only thing that a diff based approach that doesn't allow to disable default traits enables, over the here proposed one, is that it you can move existing API behind a new trait. Not behind another existing one since that might be disabled just behind a new trait. As I said I sympathise that as a package author this seems practical but I don't think that trade-off is worth.
Now, there is another argument to make. Currently, if you want to enable traits of a package and you want to keep the default traits you have to spell it like this: [.default, "SomeTrait"]. This might be unexpected that you have to explicitly state that you want to keep the default traits. In an initial design I separated this out. There was a separate bool that triggered the default traits enableDefaultTraits and a set of traits that are enabled explicitly. In the end, I decided it was better to have one single set that controls everything than a set and a boolean.
That's not the only thing, although It's an important one.
With the current APIs you can't express disabling a single trait while having all the default ones enabled, including those that will come in the future.
If we can enable package authors to extract existing features into traits with a better set of Trait APIs which I imagine most users will find better anyway, then why not.
I think most users will just start using a package. Then they notice that compared to the default traits, they need a specific trait enabled or disabled, and they'll use the package traits to express that.
There will be a lot less users who start using a package and immediately decide that they want to use the minimum set of APIs and perhaps enable a trait or two that they still need.
Not to mention there is a special case default, which can be avoided with the APIs similar to what I proposed. There will be no need of special case handlings.
With a more-complicated API, there could be a way that package authors can control whether or not they allow disabling all traits. If they do want to allow disabling all traits, then they have chosen to lose the ability of extracting existing APIs into features in SemVer minor versions, which I imagine a decent amount of package authors will want to do. This "whether or not they allow disabling all traits" should be set to "We don't allow" by default to allow maximum flexibility, unless the package author makes a conscious decision.
This will also allow incremental adoption of Traits, instead of having to outright go for a new major version.
Even if we decide that the ability to extract current APIs into features is not worth it, I think that's the lesser problem with the proposed Trait APIs.
The bigger problem is the fact that you can't use the default traits, but disable a single trait that you find useless or harmful. To make sure that you'll still receive the new features if the package authors deem them worth enabling by default (whether those features are existing-APIs-extracted-into-features, or just new APIs) you need to be able to express having the default traits but disabling certain ones.
I think that the default behaviour is excellent. Having both a disable and enable is much more complicated in my eyes than having a .default set of enabled values, and being able to provide your own set composed of default values or otherwise. And I mean this from every point of view - the user, SwiftPM, and even tools that want to integrate with SwiftPM.
As a wider review, I'm completely in favour of this proposal. I think it'll allow me, as a library author, to provide features in a more accessible and integrated way. Things that come to mind include HTTP/2 support in Hummingbird, which could be enabled with a trait. But I also think that this extends throughout almost every package I've worked on or worked with.
While I have no real concerns or complaints, I'd like to emphasize the massive impact this will have on how packages are built and consumed. As such I highly recommend that people add their two cents like @MahdiBM has been doing.
i think the current API proposal is fair enough for both package authors and package consumers. if i am not mistaken, @MahdiBM concern's is in the situation where a package author decided to enable a set of traits (say 4 traits) as default, but the consumer only wants 3. with the current API, the consumer would have to either use the default traits, and have the unwanted trait added, or explicitly set the 3 traits they want. but if a package author adds a fifth trait to the default set, and the consumer wants it too, well now they (the consumer) have to also explicitly enable it.
and i think that's fine.
and even though this could cumbersome, we also have to keep in mind that there is some work in managing packages in our codebases. there could always be breaking changes, or new features added. the package author decides what is new and what is breaking, and they need to make sure that they introduce those changes in a way that doesn't break existing consumers (major release) , and we as consumers need to keep up to date with the packages changes and releases. if a new API is introduced or deprecated, we have to make sure our codebase can adapt to that.
and this new trait system means we also have to maintain our Package.swift to support the latest changes of the packages we are using and their traits.
and this is what sets good open source projects and packages apart. if a new default trait is added, which is not something that will happen as often as we think, then it should be part of a release that the consumers can see and decide if they want to use it or not, and if not, explicitly enable the ones they want.
the package author is responsible for defining what the default traits are, and the consumer can decide to go all one with the default ones, and accept that in the future this set can change, or be explicit with what they want.
This is exactly the behavior I'd like to see (disclosure: I made some contributions to the proposal's design and implementation, so consider me biased).
Providing a "diff-based" API at this early stage introduces unexpected behavior where you're opted in for newly added traits in a dependency without an explicit acknowledgement. We already have too much implicit stuff to worry about, traits with more magic added on top would make things more confusing.
IMO explicit building blocks should come first, with implicit "magic" added later only if absolutely necessary. What's written in the pitch doesn't preclude us from adding some implicit (diff-based or not) behavior later. But we need to get the basics right before proceeding with higher level abstractions, those can always come in a later pitch.
@Max_Desiatov the Package.swift is designed to explicitly set everything we want, from packages, dependencies, to targets and plugins, so to introduce a new behavior is just going to make things confusing.
