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 me as the review manager by DM. When contacting the review manager directly, please put "SE-0540" 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:
This is a well-put-together proposal, but it still seems insufficiently motivated to me. The main reason not to have a helper binding is
A possible solution involves applying a constant array to each target. This is sufficient as long as all targets use identical settings. But, if a package author does happen to need slightly different settings for even one target, additional logic needs to be introduced.
But that's true even in the proposed solution, with the introduction of .inherited(). This seems like a bunch of extra behavior just to avoid one case of duplicated settings, without improving the situation for any other settings, or for sharing settings across packages.
I think you are right that the motivation needs more attention. But I would like to make sure I understand what could be satisfactory.
For many of my own packages, I use the following trick after the package description definition:
let swiftSettings: [SwiftSetting] = [
// list of settings goes here
]
for target in package.targets {
var settings = target.swiftSettings ?? []
settings.append(contentsOf: swiftSettings)
target.swiftSettings = settings
}
This helps to avoid forgetting a constant settings argument when targets are added and removed. But, when it comes down it, it's just a slight revision on the constant array solution.
Is your thinking that any duplication here is minor, there isn't enough duplication across packages to make it worth addressing, or both?
I'm curious why the default settings are stored in sets when the existing settings APIs are all arrays.
Also, unless added recently, I don't believe any the various settings types conform to Hashable which is required for the elements of sets. If this proposal includes adding a new protocol conformance for each of the settings types, I think that should be called out in the proposal as part of the detailed design.
I empathize with the motivation but also see @jrose's point.
My concern is more with the proposed design. It is not intuitive that A, B, and D inherit but not C in the example quoted below. The distinction in particular between A and C is subtle, not derivable from first principles, and feels like it cuts against the plain-language meaning of default.
When writing CSS (which this feature seems to deliberately evoke with its use of the "default/inherit" terminology), this isn't how defaults behave: by that analogy, you'd have to actively specify a different value for defaultIsolation in order not to inherit the otherwise default defaultIsolation:
On that note, it is worrying to me that the flagship use case shown here is setting a "default default isolation." We already have a compiler-default default isolation (which, I guess, makes it the default default default isolation?), Xcode-default default isolation (library versus executable), and now we're creating a package-default default isolation which can default the target-default default isolation.
Combining the two concerns above, the motivation text that shows the apparently unsatisfactory status quo seems...easier to me to understand than this. For a feature that's about improving expressivity, I think adding extra hoops to achieve correct understanding is less than ideal.
I wonder: since this proposal is adding a spelling for explicitly including a bunch of settings, perhaps the feature to emulate from CSS is not the cascade but instead CSS variables.
As an avid user of swiftSettings, I'm generally in favor of this proposal.
I think swiftSettings will only gain more importance over time as well, as Swift adds more and more settings, which I think is a good way to add new functionality without forcing code breakages or waiting on new major versions or such.
However I'd like to see a different impl at least considered.
A diff-based impl.
so if you have:
defaultSwiftSettings: [setting_1, setting_2]
then there would be no .inherited(), as that would be the default. Instead you can add or remove compared to the so-far-accumulated settings (including default settings):
swiftSettings: [
.excluding(.all), /// or e.g. .excluding(setting_1)
setting_3,
setting_4
]
The behavior would be that SwiftPM will walk down the path one by one and sequentially check each element. If anywhere it sees a .excluding(whatever), it tries to remove whatever if available. If .excluding(.all), it'll simply remove all previous settings.
It doesn't matter where in any of the arrays .excluding is. SwiftPM will just simply look backwards and try to apply the exclusion.
I think this will make for a more intuitive API, as it's rare that someone wants to just get rid of all defaultSwiftSettings at once, and start all over again. If they really do want that, they still have the choice.
Instead I expect that most users will need to just get rid of that 1 setting for that 1 specific target, which are incompatible with eachother.
This is a typo. These should all be arrays in the proposal, as they are in the implementation. When I was first beginning the feature, a set made much more sense to me. To my knowledge, all tool settings are mutually-exclusive and it does not make sense to have the same setting appear more than once.
However, as you noted, the types are not Hashable. Plus, it would make for a bit of an API asymmetry. So, even though it would have also been useful for the implementation, I just moved back to arrays.
Also a typo in the proposal document. Thanks for looking so closely.
Yes, we can! I opted for a function because I was worried about preserving source compatibility should conditions ever be supported. However, I was being overly-conservative, because it seems like a static var and function can overlap without issue.
I'm not opposed to this suggestion. At first glance, it feels more powerful. I also agree that it seems like it would be rare for a target to discard all settings.
The control mechanism has gone through multiple iterations based on feedback. I'd like to hear from others about this, as the current shape came almost entirely from discussion in the pitch thread.
I totally get this. Here's my rationale.
struct Object {
static let someDefault: [Int] = [1]
init(values: [Int] = someDefault) {
}
}
Object() // values == Object.someDefault
Object(values: []) // callsite overrides defaults
Object(values: [.someDefault]) // ok yes, this doesn't actually work
The implemented behavior isn't too far off from how default argument expressions work. But it is not the same.
In hindsight, I'm not sure selecting a setting with the word "default" it in as an example was wise. I'm going to revise this.
That might reduce a small amount of confusion, I'm not sure it'll go that far. I haven't thought too hard about this, but I believe that all tool settings have some value when unset. There is always a "default" behavior. And further, that behavior frequently changes depending on whether you are involving the tool directly (default isolation is nonisolated) or via Xcode (where default isolation is, in the most simplistic case, still dependent on what kind of target you are defining). It is highly nontrivial to determine what default applies in that case.
I was really trying to scope this down exclusively to what settings, if any, SwiftPM would supply to the tool. This even includes duplicate, incompatible settings, which SwiftPM accepts today.
In my original pitch, I went even further and didn't use the term "default". I opted to just go with swiftSettings, because the tooling involved already regularly exposes developers to cascading (to your other point) settings resolution. The true "default" is whatever the underlying build tool applies when a user setting is absent.
I'm not familiar enough with CSS to comment here. The inspiration for the current design was Xcode's configuration system, which came from feedback during the pitch.
I wasn't sure exactly how to proceed here. So, what I opted to do was take a look at the currently-featured packages on Swift.org and see if this feature could help them in any way.
Swift-otel's setting usage follows the constant-array/apply-array pattern. Adopting this feature would save a few lines of code and prevent an accidental omission when adding new targets. Very marginal wins.
swift-configuration's use is more complex. It uses the post-definition settings modification mechanism, and does so in a non-trivial way. Here's the current implementation:
for target in package.targets {
var settings = target.swiftSettings ?? []
// https://github.com/apple/swift-evolution/blob/main/proposals/0335-existential-any.md
// Require `any` for existential types.
settings.append(.enableUpcomingFeature("ExistentialAny"))
// https://github.com/swiftlang/swift-evolution/blob/main/proposals/0444-member-import-visibility.md
settings.append(.enableUpcomingFeature("MemberImportVisibility"))
// https://github.com/swiftlang/swift-evolution/blob/main/proposals/0409-access-level-on-imports.md
settings.append(.enableUpcomingFeature("InternalImportsByDefault"))
// https://docs.swift.org/compiler/documentation/diagnostics/nonisolated-nonsending-by-default/
settings.append(.enableUpcomingFeature("NonisolatedNonsendingByDefault"))
settings.append(
.enableExperimentalFeature(
"AvailabilityMacro=Configuration 1.0:macOS 15.0, iOS 18.0, watchOS 11.0, tvOS 18.0, visionOS 2.0"
)
)
if enableAllCIFlags {
// Ensure all public types are explicitly annotated as Sendable or not Sendable.
settings.append(.unsafeFlags(["-Xfrontend", "-require-explicit-sendable"]))
}
target.swiftSettings = settings
}
With this feature, I believe this could be simplified into the following:
I admit, I had to use a somewhat questionable ternary there.
But I still think this change is pretty valuable. It completely eliminates the array and target mutations. Plus, it helps to establish a more obvious connection between the definition and the settings. This makes the file feel much more declarative.
After a couple of read-throughs, I am somewhat neutral to this proposal. It's probably better than the current ad hoc solutions, and while any inheritance system is going to create certain confusion (it's just their nature), it's nice to have one, consistent (learnable) way things are confusing. So I lean gently towards it being worth it.
Given that this seems to be based on $(inherited), I expected all the examples to put .inherited() first, but they all put it last. That seems to work against your explanation:
This inheritance mechanism matches the existing behaivor of the settings definition APIs. This means that duplicates and invalid combinations are perimitted. This situations are handled either by later stages of package validation or by the build tools themselves. In many cases, this results in "last entry wins" semantics.
So, I assume this is expected to set default isolation to Main for D (since inherited() is overriding the D-specific settings):
Given the semantics, I recommend all examples put .inherited() first, and this be treated as best practice. I think it's more likely to do what you mean.
(That's my only strong suggestion. The rest of this is just musings.)
I think this proposal is going to run into a headache that some SwiftSettings have no "disable" form. So if enableUpcomingFeature is in defaultSwiftSetting, it is no way for a target to remove it. I doubt this will very often be a big problem, but it will be very annoying when it is. A future direction may be to create a disable... setting.
A similar issue comes up for BuildSettingConditions:
I assume DISABLE_SOMETHING will be defined here for D in Debug, even though it looks like it shouldn't be, and in fact D's cSettings is redundant. (I think this is true regardless of where inherited() is place in the list.) That's probably fine, but if this feature is used broadly, it does require more developers to have a deeper understanding of how build settings are actually applied.
My suggestion here would be making an undefine setting that overrides the previous define (and disableUpcomingFeature etc), rather than trying to remove the setting. I just don't know how you'd implement that in the general case. Do you have an example in mind?
What of the suggestion (perhaps half-baked, admittedly) to leave behind the cascading (defaulting) aspect of this proposal in a more CSS variable-like approach?
That is, let these settings be declared as top-level let myDefaultSwiftSettings = [ /* defaults */ ] and then write .target(name: "foo", swiftSettings: myDefaultSwiftSettings + [ /* target-specific settings */ ]). Are we missing any features currently that would allow such an approach which just leans on the fact that we're writing Swift?
Doesn't that get us 90% of the way there without having to contend with new rules about when defaults propagate, the order in which defaults have to be included, unset-like features, etc.?
I think this is a worthwhile counter-proposal (and matches what a lot of people do), but it does require that every target explicitly pass the settings parameter(s), which I think is the specific thing that this proposal is trying to remove. So the argument is whether getting rid of a bunch of swiftSettings: defaultSwiftSettings parameters is worth the trouble. I think "probably," but I agree it's the whole question.
Is there a strong reason to even allow the user to choose an ordering? It seems like an interface where the question of ordering is avoided altogether would be preferable:
For the same reason that you can put "$(inherited)" anywhere in an Xcode build setting: order matters for some settings, and you may need to preempt or append to the inherited values on a situational basis.
For example, passing -enable-upcoming-feature Foo -disable-upcoming-feature Foo to the compiler is effectively a no-op while -disable-upcoming-feature Foo -enable-upcoming-feature Foo enables the feature. Likewise if you specify -target arm64-apple-macos14 -target arm64-apple-macos13 the deployment target will be macOS 13, but reverse the order and it will be macOS 14 instead.
It's conventional to lead with $(inherited) in build settings because that ensures that the specified build settings override any inherited ones if applicable.
Possibly a variation of @xwu's suggestion that would also enable getting rid of the swiftSettings: defaultSwiftSettings parameters?
If it were static public var defaultSwiftSettings: [SwiftSettings]? (et al.) the default settings could be set separately from the Package initializer, but also be a known property. When no value is passed to swiftSettings: the static default settings, if defined, would be used.
But, when any value is passed to swiftSettings: it would be used instead of the default.
To incorporate the defaults, a variation of the existing common practice of swiftSettings: Package.defaultSwiftSettings! + [ /* target-specific settings */ ] could be used.
This would eliminate the need for inherited but still allow a target to intermingle the default settings with target-specific settings.
EDIT:
After posting, realized this might not work as suggested as static variables on Package, but possibly would work as an instance of a separate type, taking the definition of the default settings collection out of the Package initializer but present in a reliably named way.
"We have default settings at home."
So I lean gently towards it being worth it.