1

The project I'm working on, uses Treat Warnings as Errors so that warnings aren't ignored. This works really well except for deprecations. A framework we are using has made a core function deprecated and the task to update the code isn't quick or small. If we could set deprecations to be excluded from the "Treat warnings as errors" then the task could be split up over time (and developers). However swift has nothing like the -Wn-error=deprecated that Objective-C had as far as I can see?

Is there an alternative to having to turn off "Treat Warnings as Errors"? If not why hasn't this option been added, to give developers more control over their own projects?

2 Likes
2

At first this reminded me of the “Doctor, it hurts when I do this” joke, but on further reflection I guess there could be a distinction made here about warnings that you produce due to your own code and warnings due to your dependencies changing. I can see why someone might want to upgrade the former to errors but not the latter. Swift and the standard library aren't shy about introducing new warnings, because it allows changes to be made while maintaining source compatibility, but this only works if warnings-as-errors is off. I have seen people complaining about this exact issue when new Swift versions are released.

If this distinction is valuable to make, you might want to consider:

  • Is “all deprecation warnings” the right level of granularity? If the deprecated API is in your own code, not a dependency, should it be a warning or upgraded to an error?
  • In the future there will probably be a compiler mode to compile all your dependencies together, to eliminate cross-module issues (annotations, inlining, etc) in common cases. If there are warnings other than deprecations in your dependencies, should they be upgraded to errors?
  • In an ideal world you would probably want writing new code that used deprecated API to be an error, but this is too difficult to implement.
3 Likes
3

I strongly believe in having warnings as errors enabled for CI builds whenever possible, but have also seen deprecation warnings make that tough.

I work on a very large project and we have used depreciation as a way to transition our own APIs incrementally. In these cases specifically we would not want the deprecation to be an error immediately. If we were ready to make the deprecation an error we would just remove the API altogether. So in the case of internal APIs specifically, it makes no sense at all to make deprecation an error. The entire point is to discourage use and point people to a new API without having to break everything right away. Ideally we could do that in a way that is orthogonal to Treat Warnings as Errors.

As for 3rd party deprecations, ideally our team would have the ability to set a per-module policy. This would allow us to upgrade a dependency that has new deprecations without having to disable Treat Warnings as Errors. When we are ready we would modify the policy to make deprecations from that module errors. The best way to do this might be a whitelist of modules where deprecations are exempted from Treat Warnings as Errors which implicitly always includes the current module.

No, each module should have it's own build configuration. I want to be able to use Treat Warnings as Errors without being limited to dependencies that compile without warnings. I should still be free to choose a dependency that produces build warnings. Further, I should be able to update my dependencies without having to disable Treat Warnings as Errors because it now produces warnings and previously did not.

We have also run into cases where compiler bugs lead to warnings (usually redundant constraint warnings) that cannot be avoided without breaking the design. When the "redundant" constraint is removed the code fails to compile. I would really like to have a way to suppress these warnings explicitly as they also break Treat Warnings as Errors. Further, even if the code did compile without the warning, there are cases where a strong stylistic argument can be made that the "redundant" constraint adds clarity and should be allowed to be stated without a warning (even if that requires suppressing the warning explicitly).

I strongly believe it should be possible to adopt Treat Warnings as Errors while still having stylistic freedom and the ability to migrate to new APIs at a pace appropriate to our project.

2 Likes
4

-warnings-as-errors is fine for development and incoming pull request hygiene, but we don't recommend using it for production builds, since we add new warnings in new releases for deprecations and new safety issues.

6 Likes
5

Warnings as errors is most important to us on the CI build that must be clean before a pull request can be merged. My team would really like to use it there but the deprecation issues discussed above as well as the inability to suppress warnings caused by or necessitated by compiler bugs make that tough to do. IMO that's pretty unfortunate. Warnings as errors is an important policy a teams should be able to opt into, especially when there is a large team and a complex codebase.

If we had finer-grained control over which warnings are treated as errors this wouldn't be an issue. I don't understand all of the motivation but I know that this is something the core team does not want to do. I'm ok with that as long we solve the real world issues that make it tough for large teams to use -warnings-as-errors in practice.

Frankly, I don't understand the reluctance to do so. It would be nice to at least have a more complete rationale that acknowledges the challenges of large-scale projects.

1 Like
6

The policy you want isn't "warnings as errors", though. It's "your PR must not add any new warnings (without a darn good reason)". That's not necessarily a reason to stop the build, and it's something that accounts for "I know there's a problem here but I won't (or can't) fix it immediately".

I'm one of the people against adding compilation-wide warning flags, for a few reasons:

  • The same warning might fire in multiple files, but only be spurious in some of them. Turning it off compilation-wide means you miss those other issues, including any that might be introduced in the future.

  • There ought to be a way to silence any potentially-spurious warning within the language (think "add 'as Foo' to silence this warning" notes). I know we don't have that for a whole bunch of cases, but I'd like to see people push on the ones that are important rather than just giving up.

  • It creates language dialects.

6 Likes
7

In service to the "don't add new warnings" policy, maybe what we really want here is tooling support for diagnostic history, so that new issues get prioritized and highlighted even with existing warning-worthy behavior. The danger with "no new warnings" is that, if the number of "accepted" warnings increases, it becomes hard to even notice new warnings to begin with. We've certainly had this issue ourselves maintaining the standard library, which has had to live with warning-laden code for extended periods for various compatibility reasons.

2 Likes
8

There is nothing in any language or practice that can make it foolproof to develop (well beside subcontracting it to developers who are much better than you are at the moment... it get you closer to that ;)) and resist or prevent abuse. I do think that making coding life bearable and safe can be achieved if you make treating warnings as errors manageable (helps you out more times than not) instead of being stuck with a terribly restrictive punishingly paranoid setup with no escape hatches or a laissez fairy sea of yellow :warning: signs ;).

From a practical point of view a strategy that has worked for us quite well overall was relatively straightforward.

1.) For all the code we compile ourselves (Cocoapod’s source pods that are not private development ones we own can and should be bound by their own warnings) turn on: -Weverything and treat compilation time warning as errors.

2.) Observe the ocean of red marks/errors thrown by the compiler and see the most common errors and study the warnings that are hitting you.

3.) As a team discuss the warnings that really really really really can be safely ignored (bad memory here perhaps, but there was a warning about actually making use of auto synthesised properties which was odd for a post Objective-C 2.0 codebase at least :)).

4.) As a team decide guidelines around which warnings could be suppressed locally with #pragma clang diagnostic push/pop [...] blocks (“no... I do not like my build to fail because the headers of this third party library I am including do not have proper documentation and you are throwing warnings for that, thank you :)”)

5.) Happy days are here again? ;)

9

I disgree, the whole point of my post is that during development we don't want deprecations to be treated as errors as we want to incrementally update the code. If anything having it turned on for production builds would be fine as by then we should have transitioned to the new API completely (and updated to the latest swift, etc)

10

The solution to this problem is simple and has been solved already, allow developers to choose which warnings aren't treated as errors using a flag like -Wno-error=deprecated

imo the compiler and language should be there to help developers, which means flexibility, not the compiler team imposing things from the top down.

@jawbroken

I think all of these are solved by other processes like Peer Reviews, if there is a warning then the reviews will see it and determine whether it is something that should be resolved before merge or can be left in. The compiler can only do so much and if it tries to be too clever and imposing with things like warnings then it will just end up causing problems for someone somewhere who has a slightly different use case than what was imagined.

2 Likes
11

Why are deprecations different from any other warnings as you incrementally update code?

1 Like
12

Yeah, this is a real problem. It's important that a developer can see locally (without much trouble) whether or not their code will violate the policy before they push up a PR and run a CI build.

Interesting. I imagine nobody was too happy with this and it would have been nice to have a little more flexibility in handling the unavoidable warnings, right?

The policy we would like is closer to "your PR must not add any new warnings, period" (but with a backdoor that can only be opened by a couple of people who ensure exceptions are accepted by our core team). Ideally, when we do choose to accept warnings we would have the option of suppressing the diagnostic for that instance if we choose to keep the list of reported warnings short or zero.

For sure! This is why it would be ideal to be able to suppress a warning in a specific location, or at least have it omitted from error promotion. This obviously creates a hole in -warnings-as-errors but there a number of reasonable approaches to addressing that. Short of the ability to do that, we may decide that we're willing to accept a specific class of warnings and / or rely on social pressure and code review to keep the unnecessary instances out of our codebase.

Right now, as soon as we have a single necessary warning -warnings-as-errors becomes useless. That is an extremely blunt instrument.

I wholeheartedly agree! However, this is not the current reality and I don't see evidence of it becoming a priority. Even if the language did fill this gap there it is not likely to address deprecations or compiler bugs which force warnings on us. I'd rather not have to wait until the language is sufficiently mature to solve the issue this way (although I would like to see this happen eventually). As far as I can tell we're still years away from reaching this level of maturity.

I know this has been stated before and the Swift team is very sensitive to this issue, but I don't think it is completely accurate in this case. Promoting warnings to errors does not change anything about the semantics of the language, it only changes how issues are reported by the compiler. It's completely reasonable for a team to want to define policies around how different kinds of issues are handled.

Is -warnings-as-errors a dialect? If so, why is that flag acceptable? If not, why does finer-grained control produce dialects when that is not a dialect?

Further, if you consider the context in which Swift is often used you will find that SwiftLint is extremely popular. The configurability (including custom rules!) of this tool has been an incredible asset to our team. I haven't heard any complaints about SwiftLint creating language dialects. Perhaps the community's experience with a tool like this is sufficient to revisit the idea that simply promoting a warning to an error creates a language dialect.

+1. I understand if a solution has just not been a priority yet, but my impression is that there is resistance to solving this problem, especially because of the "language dialect" issue. I have not yet seen anything resembling a sound argument for the current state of a blunt all-or-nothing -warnings-as-errors (paired with the eventual capability to silence all potentially spurious warnings) as a final solution. (Again, I understand if this is simply not a priority yet, but I think it should be soon - Swift is used by some very large teams these days)

Theoretically they aren't, but pragmatically they are a tool that helps a large team evolve / refactor a large codebase incrementally. I don't think we should have to choose between using this tool and using -warnings-as-errors.

I want to emphasize that I don't have any strong opinions about how this problem should be solved but I do think that it is crucial to provide more flexibility in handling warnings and deprecation than we currently have available.

4 Likes
13

The standard library is a special case, especially regarding compatibility support. Most of these warnings were not things we'd normally want people to ignore for long periods of time.

I'm interested to know why you feel this way. Most warnings do come with a fixit or message telling you how to quiet the warning, and having such a mechanism is an important consideration we take into account before adding any new warning.

14

If this is a priority I apologize. In my experience though, the implementation isn't mature enough for this approach to be reliable yet in practice.

For example, when I push the type system I not infrequently run into warnings that are necessary for the code to work but difficult to silence. I have spent a nontrivial amount of time trying to silence these warnings. I don't recall ever seeing a fixit that solves the problem for warnings in this class.

The latest example is Redundant conformance constraint which can pop in cases where removing the constraint causes the code to not compile. This warning is potentially spurious by definition IMO as it's really a stylistic suggestion. If I decide that the constraint adds clarity to my code how can I silence the warning? It appears in cases where the constraint is not obvious unless you know a lot about the constraints on other types.

15

Yeah, that exact warning has been contentious even among us due to the lack of an escape hatch. IMO, warnings should be for practical safety or ambiguity issues and support lifecycle management, and things like the "redundant constraint" warning are style issues more appropriate to optional linters. That line isn't black and white, of course.

16

They're different because they come from code you didn't necessarily write. If you update a dependency (3rd party, or in house owned by someone else) the dependent affects your code. But if you're writing code yourself and create a warning then it's your own fault and you can more easily change it.

1 Like
17

Sure, it's more complex when the API is both public and used internally, though, where you conceivably might want internal use to be an error.

If there was a targeted way to stop warnings from being upgraded to errors on a particular line or region of code would that be sufficient to address a lot of use cases? Or would it still be considered too onerous to have to e.g. tag every site where a deprecated API from a dependency is used. Would something simple like a special tag/comment appended to lines where warnings should not be upgraded to errors suffice? Or would something more complex be desired (e.g. marking entire regions instead of just lines, only applying to certain classes of warning, etc)?

18

In general I don't mind stylistic warnings with strong rationale, especially when they require deep semantic understanding of the language that is probably best done in the compiler (at least right now). This one in particular actually alerted me to inference behavior that I was not previously aware of.

func takesDictionary<Key, Value>(_ dictionary: [Key: Value]) {} is valid! I was under the impression that constraints behaved more like a type system for generic parameters and that without a Key: Hashable constraint this would produce a compiler error. It appears that Swift now infers constraints based on how generic parameters are used.

In the case of the dictionary example nobody would be surprised, but cases where user-defined types are involved this can easily result in constraints that are not immediately clear unless one is already intimately familiar with all of the generic types involved. I'm not sure how I feel about this degree of inference but I definitely think it should be possible to write out the constraints manually if desired.

So in this case the warning was very useful and informative, but at the same time it is a little bit too aggressive, at least in some cases (IMO). I think we would be better off with more options for handling warnings than we would without this warning.

That's fair when the API is public.

This is the most flexible solution. We often use swiftlint:disable:next comments to suppress individual SwiftLint diagnostics. It isn't pretty but that is an acceptable tradeoff for the project given the alternatives. We also occasionally use file-level swiftlint:disable comments but that is much less common.

For deprecations specifically I think it would be preferable to have a whitelist that is not promoted to errors. When we allow a deprecated symbol to remain in use it is generally because we have decided not to migrate off of that specific API immediately for a specific reason. When that reason becomes no longer valid we migrate all uses and no longer allow use of the symbol. Tagging every site would work and is a tradeoff we would make if necessary but is somewhat a verbose way to express the policy we would want in this case.

It would be an improvement on our current options, although I think it would be much better if it required disabling promotion of specific warnings. If the same line contains other warnings those would still be promoted. This would also give us the ability to use simple custom linter rules to ban this tag / comment for warnings we always want treated as errors.

My experience with SwiftLint suggest that individual lines is the predominant use case, but whole files is occasionally extremely useful. I don't recall ever seeing swiftlint:enable paired with swiftlint:disable to mark regions smaller than a file in projects I have worked on. Similarly, in the Objective-C days the pattern was almost always to push a specific warning suppression and immediately pop. In both cases disabling specific rules / warnings is an essential flexibility.

If we were to introduce a tool like this my suggestion would be to allow us to specify the reported level for a specific warning that is honored regardless of build flags at the file and at the line level (next non-comment line and perhaps same line as well). Setting this to none would allow suppression of a specific warning. Setting it to warn would allow a line to be omitted from promotion when -warnings-as-errors is enabled. Setting it to error would always report as an error even without -warnings-as-errors. The last option would be pretty unusual, but could be useful in rare circumstances. I'm not exactly sure what those might be, but I don't see why it should be omitted as an option.

I can't say for sure that the above solution is the best one but it is a tried and true technique that worked reasonably well in Objective-C and works reasonably well in SwiftLint. I would be happy to see us explore the design space if other ideas emerge. I'm sure there are other approaches that would also work reasonably well.

19

This is an issue that I care a good deal about because of protocol buffers. A protocol buffer message can list messages, fields, etc. as being deprecated, and some generators (including the Swift implementation) map that to the target language's deprecation annotation.

Unfortunately, the code that gets generated for the implementations of the messages themselves also needs to touch the deprecated properties in order to serialize them; the implementation can't just drop the deprecated fields if a client or server still sets them or expects them. So separate from any concerns about warning external clients about deprecated usage, compiling the generated protobuf code itself produces a slew of deprecation warnings that aren't actionable without jumping through some hoops to change the code that we generate (to add some indirection to separate the underlying storage from the public properties, which could have performance impacts, but those might be lessened now due to some other language enhancements that happened recently). But what's worse is that protocol buffer generated code can't be successfully compiled at all if it contains deprecated fields and the user wants to use -warnings-as-errors.

I pitched some ideas a little over a year ago to address the specific issue of deprecations, initially to just suggest suppressing deprecation warnings from references in the same file, but other folks responded with valid scenarios where they wanted the module to be the boundary. @xwu's idea of adding an access level to deprecated that specifies where uses of that symbol should actually be considered deprecated is an interesting one.

It seems like an approach that uses such semantic information to eliminate spurious or non-actionable warnings at the source would be preferable to letting users just suppress individual occurrences and would be in the spirit of how the Swift team views these diagnostics.

1 Like
20

allevato Tony Allevato
July 25
This is an issue that I care a good deal about because of protocol buffers. A protocol buffer message can list messages, fields, etc. as being deprecated, and some generators (including the Swift implementation) map that to the target language's deprecation annotation.

Unfortunately, the code that gets generated for the implementations of the messages themselves also needs to touch the deprecated properties in order to serialize them; the implementation can't just drop the deprecated fields if a client or server still sets them or expects them. So separate from any concerns about warning external clients about deprecated usage, compiling the generated protobuf code itself produces a slew of deprecation warnings that aren't actionable without jumping through some hoops to change the code that we generate (to add some indirection to separate the underlying storage from the public properties, which could have performance impacts, but those might be lessened now due to some other language enhancements that happened recently). But what's worse is that protocol buffer generated code can't be successfully compiled at all if it contains deprecated fields and the user wants to use -warnings-as-errors.

I pitched some ideas a little over a year ago to address the specific issue of deprecations, initially to just suggest suppressing deprecation warnings from references in the same file, but other folks responded with valid scenarios where they wanted the module to be the boundary. @xwu's idea of adding an access level to deprecated that specifies where uses of that symbol should actually be considered deprecated is an interesting one.

It seems like an approach that uses such semantic information to eliminate spurious or non-actionable warnings at the source would be preferable to letting users just suppress individual occurrences and would be in the spirit of how the Swift team views these diagnostics.

Is there a problem allowing old school style pragma’s added by the developer as we have always been used to do in the clang/Objective-C days as in my previous message on this thread (sorry di I rambled a bit there)?

1 Like