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?