SE-0443: Precise Control Flags over Compiler Warnings

Evolution Proposal Reviews
23

For what it's worth, the only reason we depend on the format of -debug-diagnostic-names is to implement the feature that this proposal would do even better. We could gate our implementation on which version of the compiler is being used so that we stop using the diagnostic names if we have a compiler that supports this instead.

It would be nice to not break the format of those strings right away so that our clients would have an opportunity to smoothly transition between my hacked up feature and this real one, but people who know me have heard me say "people who depend on parsing debug strings shouldn't be surprised when their format changes" and I can't hold myself to a different standard here.

9 Likes
24

AFAICT Swift nightly rejects the combination of -suppress-warnings and -warnings-as-errors, and has since before Swift 4:

% swiftc test.swift -suppress-warnings -warnings-as-errors
error: conflicting options '-warnings-as-errors' and '-suppress-warnings'

IMO this is the correct behavior and I would similarly want the compiler to reject the combination of -suppress-warnings with any of these fine-grained options. That should leave us with full flexibility if we ever want to make a different choice.

Warning suppression is IMO a very different topic than fine-grained control for -warnings-as-errors and I think it should receive separate treatment in a future proposal.

4 Likes
25

Yes, but it allows -suppress-warnings -warnings-as-errors -no-warnings-as-errors. Do you suggest forbidding this combination?

And if we choose this path what should we call the flag that makes a warning keep its level: -no-warning-as-error or -Wwarning? The differences are outlined in this section.

26

I agree with the other comments about this feeling like overkill. I thought the point of the proposal was to silence specific deprecation warnings in specific places in code, not have a massive hammer to kill warnings at the file level.

IMO the perfect thing would be a single "marker" function in the standard library, something like:

public func silenceWarnings<T, E: Error>(in code: () throws(E) -> T) throws(E) -> T

(with maybe an async version because because we don't have reasync yet)

The function wouldn't do anything beyond tell the compiler "don't warn about anything inside this closure".

3 Likes
27

It's a bit silly but I'm not sure it rises to the level where we ought to break anyone who happens to be relying on it for esoteric reasons. I would prohibit the combination of -suppress-warnings and any of the new command line options being introduced in this proposal (and also do away with the option to suppress individual groups).

I think this section (particularly the point about off-by-default warnings) makes a decent case that -no-warning-as-error is not the best name. I don't feel super strongly about the naming as long as it isn't misleading, but if I'm being asked to bikeshed perhaps something like -warning-group-warn and -warning-group-error would be sufficiently clear/accurate?

28
  • What is your evaluation of the proposal?

Like others, I feel it is incomplete with only a build flag that impacts things module wide. I've seen too many large modules, so it would be safer to have some way to turn things on/off on regions of code instead to ensure new issues aren't introduced while temporarily allowing some.

I'd also like to see explicit call out to how this will work with macros (or any other future language feature that might involve code generation). If the macro expansion results in code that causes warnings/errors, how are those reported/controlled? The authors of the source file using the macro has no control over that generated code; but someone needs to be able to find out about then, but it also shouldn't break/clutter results of compiling the sources using the macro.

  • Is the problem being addressed significant enough to warrant a change to Swift?

Like the example in the proposal, sdks can't really use deprecated annotations for their own apis as just implementing them tends to result in warnings which cause some consumers of the packages concerns. So yes, this is important and improvements in this space are very much needed.

  • Does this proposal fit well with the feel and direction of Swift?

It's moving in the right direction, but needs more granular annotation support. Swift allows the marking of individual apis as deprecated, we should be able to mark individual sections of code as ok to suppress warnings/errors.

  • If you have used other languages or libraries with a similar feature, how do you feel that this proposal compares to those?

This is similar to what other C/C++/ObjC compilers have, but lacks the control for control within a source file they those also have.

  • How much effort did you put into your review? A glance, a quick reading, or an in-depth study?

Quick read, but it is a problem space I've been tracking ever since we opened the first forum topic on this with SwiftProtobuf as an example for the need for something in this space.

1 Like
29

Like others, I am surprised to see this proposal come to review with a feature to silence warnings.

As pitched, it was about fine-grained control over upgrading warnings to errors and then downgrading the upgraded errors back to warnings. This makes a lot of sense: since users are permitted to opt into something more strict, it makes sense to allow them to opt in selectively. It also comports with our approach to staging in diagnostics that would otherwise break source—with a warning first, which then gets upgraded to an error. Makes sense that users can declare their readiness before we decide to impose it across the board.

I have no problem with the existing -suppress-warnings as an across-the-board compile flag: if you're going to let me compile the code, it's fine not to see the warnings. It's basically redirecting the diagnostic output to /dev/null.

Selectively silencing warnings, whether by flag or by language feature, is a different ballgame altogether. It was also not what was pitched to begin with, nor does the proposal reckon with the myriad considerations previously discussed on this topic. What this means, practically, is that this review thread is going to be a rehashing of all of that prior conversation rather than a focused review of the part of the proposal which was actually pitched. Procedurally, I'm not sure that flies.

14 Likes
30
  • What is your evaluation of the proposal?

+1.

Even if it doesn't provide all the flexibility that other more advanced solutions might provide, it is a very welcome addition to large codebases that today are forced to disable -warnings-as-errors in a per-module basis because there's is no other way to do be specific about which warning-as-error needs to be disabled today.

I am not sure about the complete suppression of warnings, but I understand that it is part of the proposal as a parallel to the already existing --suppress-warnings, which has a dubious value.

I will love to see a later SE proposal augmenting this to allow fine grained disabling of warnings-as-errors in code, but I understand that keeping the scope of this SE limited is to provide a workable solution to experiment with and get more information about how such feature can be implemented in the future.

  • Is the problem being addressed significant enough to warrant a change to Swift?

Yes.

With every compiler version we find new warnings. Some of them are easily handled and can be done quickly, but others require more thinking, studying the alternatives and deciding for one. The only option today for an iterative approach of fixing those warnings-as-errors is to completely disable warnings-as-errors and have the affected module less "protected" for a time.

With every new SDK the deprecation warnings creep in and, in large enough codebases, forces the disabling of warnings-as-errors in order to allow the code to compile, and let the groups that work with the actual code fix it as best as they can on their own timeline, without blocking the adoption of the new SDK for the rest of the application(s).

Finally, during the beta releases, it is necessary to maintain compatibility with two compiler versions and two SDK versions, which makes the above even more complicated. Allowing reducing the severity of warnings-as-errors in the newer compiler and ignoring the unknown group warning in the older compiler will allow keep using both in the safest way.

  • Does this proposal fit well with the feel and direction of Swift?

I think it should be the new direction of Swift.

For the longest time the policy has been that there's not way of disabling one warning and that Swift did not want dialects. For the longest time proposals of changing this have been rejected on those grounds, sometimes without a lot more discussion that using the "no dialects" reasoning. This might have been a good decision in the past, but in the real world adopting newer compiler versions and SDKs has need to disable warnings-as-errors for the smallest reasons.

I honestly don't think a "dialect" is created by the user wanting to have the safest experience (warnings-as-errors), but voluntarily selecting to delay treating some of those warnings-as-errors. When not using warnings-as-errors those are warnings, and let the user keep working. There will not be a "dialect" created. The language is the same. We allow the user to decide what is important for them.

  • If you have used other languages or libraries with a similar feature, how do you feel that this proposal compares to those?

The usage of warnings-as-errors in our codebase is encouraged, but in some cases we need to opt-in some modules into -no-warnings-as-errors to avoid some specific warning-as-error and sadly missing on the safety net of the rest of warnings in the process.

In comparison, for Clang, we can use the command line arguments or the pragmas to selectively disable some warnings-as-errors while keeping the rest of the errors/warnings enabled.

  • How much effort did you put into your review? A glance, a quick reading, or an in-depth study?

I reviewed the SE carefully and skimmed over the implementation. I was looking for the possibility of ignoring unknown warning groups, which is not pointed out in the SE, but it is part of the implementation. This is a very welcomed forward-looking addition.

31

If we leave this combination allowed, then we have to accept and reconcile two unpleasant facts:

  1. In the future, we will not be able to make the -suppress-warnings flag positional
  2. We introduce a subtle inconsistency between the flags. -Werror and -warnings-as-errors behave identically with respect to -suppress-warnings, but -Wwarning and -no-warnings-as-errors behave differently.
32

I'm not sure I understand the issues you're laying out. If we prohibit the combination of -suppress-warnings with any of the newly-proposed flags, what issues does that introduce for future evolution which would not already be confronted head-on by SE-0443 as written?

In any case, I don't think blanket warning suppression is a good idea even in the long term, so I'm not really troubled by (1) or (2) above. I also don't really understand the use case (if any) for -suppress-warnings -warnings-as-errors -no-warnings-as-errors as it exists today, so wouldn't be heartbroken to see it be prohibited, but I'm not sure the minor inconsistency is worth any potential breakage.

2 Likes
33

I'm supportive of this proposal in general. Having the ability to control warnings-as-errors behavior by diagnostic group, module-wide, makes perfect sense to me. Having the ability to turn on -warnings-as-errors, but then leave deprecation warnings as warnings, is something many teams want.

A common sentiment I'm reading in other reviews of this proposal is that folks would rather see scoped control over diagnostics, e.g. #pragma diagnostic. I think that feature is also a very important part of the entire picture, but to me that does not seem like an alternative to this proposal. Having both the capability proposed here and also having the ability to suppress specific warnings in specific scopes in source simultaneously seems like the ideal end result to me. All of the following can be true for the owners of some project:

  1. They want to treat most diagnostics as errors, so that those diagnostics are never introduced in the first place.
  2. They don't want to be blocked by some categories of diagnostics, like deprecations, since those are not always straightforward to address immediately, and can arise out of the team's control.
  3. As the team investigates the specific warnings that still remains, they want to suppress individual instances of a warning in source once they have acknowledged the problem and decided what to do about it (e.g. file a bug to track fixing later, ignore forever, etc.).

For the team I'm describing above, the goal is to get their build to zero diagnostics, but there will be periods where they temporarily have some diagnostics which they want to consider non-blocking. For instance, adopting a brand new SDK with new deprecation warnings shouldn't completely block this team; they're going to look at the new deprecations asynchronously. This proposal is the first step in allowing them to do that.

A proposal that only introduces scoped control over diagnostics, with no ability to control module-wide warning as errors behavior, would not be very ergonomic for this purpose. The task of individually suppressing every new deprecation warning all at once would potentially be too tedious to be realistic in this workflow.

Although I'm in favor of control over warnings-as-errors behavior, I am skeptical of the affordance to completely suppress a diagnostic group module wide. I think that controlling warnings as errors behavior is as far as it should go. I think completely suppressing a diagnostic should be left to a future proposal that introduces scoped control in source.

4 Likes
34

This sums up my position as well. The design space for source-level silencing annotations is big and I don't think it makes any sense to try to tackle it here. I know you and I have gone back and forth about it at length, @tshortli. :slightly_smiling_face:

I would still love to see @Serena's pitch come to review someday!

4 Likes
35

My vote is for source code compiler directives rather than new compiler command-line options, which add complexity and could result in unintended behavior, like hiding desired warnings. Something like this (just pseudocode):

#warnings off
// piece of code where I know what I'm doing
#warnings on

Not sure if Swift already has something like that.
Could be helpful in scenarios like here.

1 Like
36

To be blunt, I think -suppress-warnings is an attractive nuisance that should almost never be used, and I think that suppressing certain diagnostic groups globally would also be an attractive nuisance that should never be used. I don’t really care much about finding a super elegant interaction for the former (suppressing whatever warnings the compiler would have otherwise emitted seems fine to me!), and I don’t want us to add the latter.

Suppressing certain diagnostic groups at certain use sites is a different story—that feature would have important use cases—but that’s not what’s being proposed here.

13 Likes
37

I have a fairly lengthy response to this proposal so please bear with me.

At Meta we’ve worked with the pain of the inflexibility of -warnings-as-errors (WAE) as well as the security and certainty with which it allows us to work in Swift, and provides a marked contrast to the granularity offered by clang, and the ability to foot-gun your own project with that granularity.

Until recently, we had a haphazard application of compiler flags that differed per-app and often per-library within those apps. We managed to go 7y without using -Wdeprecated-declarations and several other flags in ObjcC code which required immense effort to clean up, as we’d racked up thousands of unique deprecation warnings throughout the codebase, and would make it nearly impossible to enable the diagnostic.

Also due to the scale at which we build apps at meta we have taken advantage of @_implementationOnly imports throughout the project, and actually disable this warning. I have a proposal for hidden private imports in non-resilient modules to address this warning as well that I’ll be posting soon.

For deprecations however we’ve had to either drop back down to ObjectiveC where we can more selectively apply deprecation diagnostics or pull product developers off to address stable-yet-deprecated API at scale. Being more selective about these will help the speed with which we can develop product without sacrificing the stability of every other warning upgraded by WAE.

I think we can actually go farther, and I’ve written up a pitch for clang/LLVM to enable diagnostics filtering, which I’ll be pitching in swift-evolution soon as well. We take the diagnostic text, and instead of selecting all diagnostics by group ID or diagnostic ID, selectively enforce them based on a regex-match of the text itself. The application, ordering and downstream implications can be discussed on that proposal but we’ve had tremendous success in adopting -Wdeprecated-declarations to cover much more of the codebase with this tool.

That’s all to say: I believe that the granularity offered by these diagnostics are great at providing the flexibility to adopt new diagnostics and the changes to them piecemeal, whereas we have been operating in Swift in an all-or-nothing way of WAE, and it’s on one hand difficult to use WAE so coarsely at scale, even for individual diagnostics and requires some vigilance to prevent divergence of dialects.

The answer to the latter has to be that these granular application of warning diagnostics is temporary, that any downgrading of errors or filtering is temporary, and that code authors should do their best to return to using WAE as soon as possible once a new set of warnings is introduced.

+1. For these reasons, and the complexities of working on code at this scale, I eagerly await using this feature and plan to pitch these same diagnostic filters in Swift in a subsequent proposal to mirror the clang/LLVM one.

4 Likes
38

Hello, Swift community.

The proposal authors have reviewed the feedback and chosen to remove the new features for suppressing warnings from SE-0334. The proposal document has been updated to reflect this revision.

John McCall
Review Manager

9 Likes
39

Without suppression, I sorta feel like this isn't " Precise Control Flags over Compiler Warnings" and instead it is just a means to "Selectively Promote Warnings to Errors".

For the SwiftProtobuf use case, where we'd like to be able to expose the fact that a message/enums/field was annotated at deprecated, this won't help, because the generated files (which could end up in different modules) will still need to reference deprecated things, so there will always be warnings. And history from other projects has taught me developers want your projects to compile completely clean (no warnings).

I do also fear with suppression support, this won't help SDK authors in general, as once you deprecate something, some other modules that builds on top of it might also need use the deprecated apis while things migrated, so their library will have to produce warnings even thought the package authors have confirmed the code is correct in its continued usage.

1 Like
40

With the removal of -Wsuppress from the proposal, I think this is a good step forward for Swift. More fine-grained control over warnings-as-errors has been requested often over the years, and helps users dial in to ensuring those things they care about most are treated as errors while other warnings don't break the build.

Yes, I think it is. We have had the warnings-as-errors functionality for many years, and have heard repeatedly that it isn't fine-grained enough to be generally useful.

I'm used to Clang's warning groups, which this is patterned after. Clang has a reasonable approach that has worked for folks for a lot of years in that C/C++ ecosystem, and I'm glad we're bringing it over.

Well... I'm a co-author, and have reviewed the implementation and encouraged the direction here.

Doug

8 Likes
41

Very good feature!

Diagnostic groups may expand over time, but they can never become narrower.

I'd add a clarification to this rule. For example is removing one warning from a group while adding two new unrelated warnings allowed or not. It will expand the group "overall", but it seems it should not be allowed still.

With SwiftLint we could control individual warnings/errors via SwiftLint comments, is something like this desirable to have with normal warnings?

2 Likes
42

SE-0443 has been accepted. Thank you, all, for your contributions in this review.

John McCall
Review Manager