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 or Swift Forums DM. When contacting the review manager directly, please include "SE-0480" in the subject line.
This review touches on the evolution domains of both the Language Steering Group and the forthcoming Ecosystem Steering Group. Accordingly, it is being co-managed by myself for the LSG and @FranzBusch for the ESG. You may contact either of us with private feedback.
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
Important functionality, very much looking forward to this one.
In the pitch thread somebody suggested that the warning name be strongly typed (but still ExpressibleByStringLiteral). This would allow providing constants for well-known warning names. I still think this is preferable to the _ name: String in the proposed API. There could be 2 types, 1 for Swift names and 1 for C++ names.
Swift warning groups are a well-specified set that's under our control, but C/C++ warning groups are definitely compiler-specific. Do we have any sort of plan for handling differences between compilers here? Are we hardcoding an assumption that the C/C++ compiler is Clang? GCC at least uses broadly similar groups, but MSVC, ICC, etc. are all their own world.
The proposed API is designed with Clang in mind, but I believe most of it is actually compiler-agnostic. While the discrepancies between compilers are significant, we can likely find suitable mappings for most of the proposed functions. For example, with MSVC we could map the settings as follows:
SwiftPM Setting
Clang
MSVC
.treatAllWarnings(as: .error)
-Werror
/WX
.treatAllWarnings(as: .warning)
-Wno-error
/WX-
.treatWarning("name", as: .error)
-Werror=name
/we####
.treatWarning("name", as: .warning)
-Wno-error=name
No direct equivalent
.enableWarning("name")
-Wname
/w4####
.disableWarning("name")
-Wno-name
/wd####
If we find the mapping of .enableWarning(...) to /w4 insufficient for MSVC's warning level system, we could introduce additional MSVC-specific settings in the future that allow specifying the verbosity level explicitly.
We could also emit diagnostics when certain settings (like .treatWarning("name", as: .warning)) aren't supported by the current compiler.
The other challenge is handling warning names/codes. I don't think SwiftPM should attempt to maintain mappings between warning names across different compilers, as this would be complex and difficult to maintain, if possible at all.
If we add support for other compilers in the future, we could extend the conditions API to allow settings to be applied only for specific compilers. For example:
It's also worth noting that these settings only apply to local packages. Remote packages always compile with warnings suppressed. This means that if a dependency has warning flags designed for Clang but otherwise compiles fine with MSVC, when it's added to a project being built with MSVC, its warning control flags will be ignored and won't fail the compilation.
I'm looking forward to this proposal. It finishes the much-requested feature introduced in SE-0443, and I appreciate that it also covers the corresponding use case for C/C++ code.
Is the problem being addressed significant enough to warrant a change to Swift?
Yes! As I noted before, we get a lot of requests for better warning control like this.
Does this proposal fit well with the feel and direction of Swift?
Yes, it follows closely with other improvements we've made to the SwiftPM manifest, such as the experimental/upcoming features.
On this comment...
I understand the desire here, but it doesn't play well with the versioning of package manifests. Let's say a new warning group comes into version Swift X.Y. We add a strongly-typed symbol for that warning group. Folks that want to make that warning into an error will either have to move their package to only support Swift X.Y or newer (which isn't great for existing clients of their package) or will have to use the string literal version (which is a no-op for older Swift versions). I'd rather us pursue other ways to make these strings more discoverable.
I want to provide feedback on the two topics that have been brought up so far and add a new one that I thought about when reviewing the proposal.
String constants
I personally agree with @Douglas_Gregor's feedback here that maintaining this list of statics is problematic w.r.t. keeping a single Package.swift manifest working with multiple compiler versions. I would suggest that the documentation of these new types and methods link to the relevant compiler documentation that includes the list of valid strings to start with.
Conditionalizing warning groups based on C-compilers
I think this is a great future direction for BuildSettingCondition to allow conditionalizing a setting based on the used C compiler.
Dev only settings
As far as I can see, this proposal is the first that introduces settings that only apply during development mode of a package i.e. not when it is used as a dependency of another package. The proposal currently explains this behavior in the remote target behavior section.
Instead of making this behavior magical by building this logic into how Swift PM treats this specific setting, it might make sense to add new parameters to the .target method e.g. devSwiftSettings, devCSettings, devCXXSettings. This would make it very clear that those settings are only applied during local development and it would pave the way for developers to declare more settings as development only. We already see usage of development only settings that are passed during CI e.g. in swift-nio's CI. Furthermore, it would play nicely with an often requested dev dependencies feature.
Interested to hear what others think about taking such a direction.
Proposed API
This is a minor feedback regarding the proposed API section. Right now the proposal makes it look like the new methods are top-level, e.g.:
However, the implementation shows that all those methods are extensions on the existing build setting types. Can we update the proposal to make this clearer?
In private communication, the author of SE-0480 has indicated interest in exploring the idea of dev settings that Franz raised above. Since this would be a significant conceptual change to package manifests, it seems appropriate to bring SE-0480 back to the pitch stage so that the community can work through it in a less pressured setting than an active evolution review. Accordingly, SE-0480 has been returned for revision.
I'd like to thank the community for its participation in this review so far and invite it to engage in the forthcoming discussion about dev settings.
No, this is not the first case of SwiftSettings that apply only during development of a package. Strict concurrency checking and strict memory safety both introduce new warnings when developing the package, and they are suppressed when building the package as a dependency. I agree that having development-only settings is a good idea, but I would like to see one proposal that address all 3 use cases, and I do not think it belongs in this proposal. I would advocate for accepting this proposal as-is, especially because it makes the compiler settings already accepted in SE-0443: Precise Control Flags over Compiler Warnings usable in packages, and adding development-only settings as a future direction of this proposal / exploring it in a follow on proposal.
I agree with Franz here. We have a number of use cases that require the separation of settings between development time and consumer build time. One objective here would be to offer features like CMake that inform consuming targets what settings they need to add to their builds to properly consume published products. This certainly would apply to warning flags as well.
I look forward to co-ordinating with the community to bring these different interests together.
What Holly said :). Though what I want to make sure we don't end up in a situation where we need to change this proposal to meet a greater goal in a slightly different way.
By introducing a new condition within the existing BuildSettingCondition mechanism.
If we were to choose the first option, it would make sense to define the proposed warning control settings within these new Dev*Setting types from the outset. This would avoid the need to deprecate or move them from regular settings later.
However, the choice between these two approaches isn't straightforward, as both have their respective downsides. I've elaborated on these considerations in a new thread.
I think @hborla is raising good points here and there are already more existing settings that are specifically handled in development mode. While I personally want to see development settings in one way or another. I think for this proposal it is fine to defer that into a future direction. Getting warning control is an important enough feature.
As I mentioned on the other thread, I believe Swift Build will guide us in such a future direction. I think at the end of the day we may want to remove build settings from the package manifest and place them in build configuration files that Swift Build already knows how to integrate. I'd prefer we investigate that before making any manifest changes.
SE-0480 has been accepted. If you're trying to understand how a proposal that was returned for revision was accepted, please read the write-up in that thread.