Warnings-as-errors exceptions

Evolution Discussion
1

TLDR
I'd like to discuss a design for defining exceptions to the -warnings-as-errors rule.

Background
There is a lack of flexibility in how the compiler can be configured to deal with warnings. Specifically, when the -warnings-as-errors option is enabled, all warnings are treated as errors unless the -no-warnings-as-errors option is also applied, which turns off the rule completely.
Although having -warnings-as-errors enabled is a good hygiene practice, it also has some downsides when it comes to compiler and SDK updates.
The most obvious example is when a function or type is declared deprecated in an update. Additionally, there are the new concurrency diagnostics introduced in versions 5.9, 5.10, and probably 6.0. There are many more examples as well.
The problem escalates as the codebase grows. In some projects, the people responsible for the code and those responsible for the infrastructure are different groups. When it comes to a compiler update, the infrastructure team can sometimes face an overwhelming amount of work to address all the newly diagnosed warnings at once.

Proposed solution
I propose a new compiler option -no-warning-as-error, which allows users to specify exceptions to the -warnings-as-errors option.
Originally, the format of the option was -no-warning-as-error <diag_id>, where <diag_id> is one of the diagnostic IDs listed in include/swift/AST/Diagnostics*.def.
However, @ArtemC and @Douglas_Gregor rightfully pointed out that the diagnostic IDs aren't stable and will never be stable, as the compiler continuously improves diagnostics and introduces more specific versions for existing messages, necessitating the creation of new diagnostic IDs.
So instead it was suggested to follow Clang's model of diagnostic groups whose name are stable across the compiler's versions. And then sort all the existing warnings into these groups.
For example, these two warnings can be grouped into a single group named ignored_import:

WARNING(sema_import_current_module,none,
        "this file is part of module %0; ignoring import", (Identifier))
WARNING(sema_import_current_module_with_file,none,
        "file '%0' is part of module %1; ignoring import",
        (StringRef, Identifier))

Both of these warnings can then be exempted from being upgraded to errors by specifying -no-warning-as-error ignored_import.

It's a lot of work
It would require grouping hundreds of warnings and errors into appropriate groups and properly naming these groups. This process involves considerable work, verification, and approval, which may be too much to accomplish all at once.
So I think it'd be more approachable to do this gradually:

  1. Introduce support for group names

    • Create two versions of the WARNING and ERROR macros: one with a group name argument and one without.
    • Address some of the most popular warnings first.
    • Implement support for the -no-warning-as-error <group_name> option.

  2. Temporary Support for Diagnostic IDs

    • Implement a temporary special syntax for the remaining warnings: -no-warning-as-error _id:<diag_id>, which accepts diagnostic IDs as declared in the Diagnostics*.def files.
    • Note that this syntax will not be stable and should not be used in build settings for modules built in unknown environments (e.g., distributed libraries). Its existence at all isn't guaranteed either.

  3. Complete the Grouping

    • Gradually sort all diagnostic messages into appropriate groups.
    • Discard support for the temporary diagnostic ID syntax introduced in Step 2.
    • Make the group name parameter mandatory in the WARNING and ERROR macros.

The original discussion is in this PR: [Diagnostics] Add -no-warning-as-error to except a specific warning from being treated as an error by DmT021 · Pull Request #74466 · swiftlang/swift · GitHub

15 Likes
2

I'm guilty of grafting this exact feature on top of our builds in Bazel using those very same unstable IDs, adding -debug-diagnostic-names to our compilation commands and scraping the output to upgrade matching warnings into errors, because our teams want some form of warning prohibition that excludes deprecations (implementation). compiler devs: please don't break us :pray:

(The other motivation was to let our users opt-in to a stricter simulation of Swift 6 mode on Swift 5.9-10 compilers, by having a single build setting that flipped on all the Swift 6 upcoming features and making any warnings with the ID error_in_future_swift_version into errors instead. It's not perfect since there are known issues with a couple of the checks on older compiler versions, but for 90-95% of the cases we've used it on so far, it's good enough.)

All the past discussions on this topic (typically centered around deprecation warnings) have had a lot of strong opinions (including my own!) in various directions and different ways to envision deprecation suppression that didn't involve a broad compiler flag. Those discussions have ended up hamstrung on details so we haven't made any progress, and I think it's a really unfortunate state to be in.

So I've come around to think that we should just do what you're suggesting here. It's a proven technique that works with other compilers, and at the very least if we only added a single deprecation group at the start, the feature would be significantly useful.

6 Likes
3

I suppose an additional footnote to this is the following: I consider the -warnings-as-errors flag in its current state to be unusable without the capability to refine what you enforce. To quote a much older post, it's not even recommended for use in all cases:

But I'd go further than this—there are often warnings that simply can't be removed from code easily. Some of the new concurrency features have produced diagnostics that we can't immediately drop everything to fix when a new compiler version comes out. Sometimes a bug slips through and a released compiler produces false warnings and we have to wait a release for a fix. Sometimes the alternative to a deprecated API doesn't work exactly the same way and we need some more time to migrate off of it. There's no development vs. production distinction there, in my mind—and if I have extra enforcement on my dev builds, I'd also want it on my prod builds specifically to ensure that nothing sneaks through (maybe I have a large #if DEBUG ... #else ... block that wouldn't get checked otherwise).

11 Likes
4

I updated the pull request to include support for diagnostic groups, flags to control the behavior of warnings, and several use cases. I defined the groups to include the availability_deprecated and error_in_future_swift_version warnings, just to have some examples.

5

The pull request describes the approach in more detail, following the way Clang did it, and I think it's a good model. To summarize for the thread here, the options are:

  • -warning-as-error <group name>: escalates warnings in the named group to errors
  • -no-warning-as-error <group name>: downgrades warnings in the named group from errors to warnings
  • -warnings-as-errors: continues to escalate all warnings into errors.
  • -no-warnings-as-errors: cancels out -warnings-as-errors, making all warnings that were escalated to errors warnings again.

This all makes sense to me. I think I'd like to ensure that we're thinking ahead to all of the information we want to include in each group, so we don't have to come back and revisit every group.

The only bit of information I know of that I'd like each group to be documented with a snippet of Markdown associated with it (could be in a separate file). Then we can pull these together to document the warning groups in some central way; like Clang's documentation for diagnostic flags but with an actual human-written description of "this is the kind of warning you'd see" rather than the diagnostic message diagram's Clang did.

I'd rather us not have this feature in the first place, because we might find it hard to take it back once we've added it. We have time before the main compiler gets stabilized for release: perhaps we can get far enough with adding warning groups before then that we don't need this feature.

We can probably make this third bullet stronger: once we land the support for warning groups, we can have a static assert on the number of warnings that have no group. If you add a warning without a group, it fails and the comment tells you to go put it in a group. That'll prevent regressions as we work toward driving the number to zero.

Other things we probably want to consider, although they don't have to happen immediately:

  • SwiftPM's SwiftSettings should get some settings to correspond to these compiler flags

  • We could pass the diagnostic group name into the serialized diagnostics file as a "category" (Clang does this with warning groups)

  • Should we design warning suppression at the same time? -no-warning <group name> or similar?

    Doug

9 Likes
6

In case we don't provide support for the _id: option we should support the most frequent warnings first. And I'd like hear from the community what are those.

That's a nice idea! I'll try to add such assert.

I was thinking about this, and it would be easy to implement. I can gather -[no-]warning-as-error, -[no-]warnings-as-errors, -suppress-warnings and -suppress-warning(new) together into WarningAsErrorRule (renamed to something like WarningBehaviorRule) and process them together in the DiagnosticEngine.

I'll see what I can do. I'd prefer the group and its description to be in the same place. But I'm afraid it would be too awkward to implement with the macro-based approach I used in the DiagnosticGroups.def file.

7

I think I'd suggest not doing this in your initial PR, or commenting it out, because it'll be a source of conflicts until it lands.

Yeah. I guess my hope is that we could start with not having this feature, and only add it if we don't make sufficient progress with the warning groups.

FWIW, there's some prior art and infrastructure here with educational notes.

Doug

1 Like
8

Fantastic to see this :slight_smile:

In server libraries I've maintained I've quite frequently had to give up on warnings-as-errors because of maintaining multiple Swift versions and sometimes the warnings flip-flopping between some functions and making it hard to be "clean" on all versions without too much of a hassle.

The other problem we hit frequently is deprecations, even within the same package -- because "new" APIs call "old" APIs sometimes internally, and get deprecation warnings inside the lib itself which we can't solve.

I like the diagnostic "group" approach and also the one-by-one control of -warning-as-error / -no-warning-as-error and the other flags @Douglas_Gregor proposed. I can definitely see enabling all warnings and selectively disabling a few, or the other way around - enabling a few.

Thoughts on diagnostic groups:

For deprecations we may want to consider having a group that is about "all" but also allow some -no-warning-as-error=deprecated-same-module.

Example:

Sources/Tracing/TracerProtocol+Legacy.swift:84:11: warning: 'LegacyTracer' is deprecated: renamed to 'Tracer'
extension LegacyTracer {
          ^
Sources/Tracing/TracerProtocol+Legacy.swift:84:11: note: use 'Tracer' instead
extension LegacyTracer {
          ^~~~~~~~~~~~
          Tracer

Sources/Tracing/TracerProtocol.swift:25:25: warning: 'LegacyTracer' is deprecated: renamed to 'Tracer'
public protocol Tracer: LegacyTracer {
                        ^
Sources/Tracing/TracerProtocol.swift:25:25: note: use 'Tracer' instead
public protocol Tracer: LegacyTracer {
                        ^~~~~~~~~~~~
                        Tracer

So e.g. I might want to disable all such warnings in my module, I can't get rid of them after all. But keep being warned about deprecations in other libraries...

Anyway, this goes outside of the scope of this post -- groups, yes! And just that we'll have to figure out good groupings :slight_smile:

:+1: I'd be happy to help out grouping up concurrency warnings into some categories and talking this through with the team so we have categories we're happy with. At least concurrency and deprecations could quickly adopt this I think.

:question: One question I thought we should consider: Do we need multiple groups...? like concurrency but also concurrency_some_specific_issue. I think probably NO may be an okey answer here, just in order to avoid making this too complicated -- if someone wants to disable errors-as-warnings having them to explicitly call out those they care about sounds ok to me. But figured it's a question worth considering as a group.

2 Likes
9

This is already in the implementation in the PR: Groups can be nested into some other groups. Like here the deprecated contains the availability_deprecated group. So changing the behavior of the depracated group will affect all the warnings associated with the depracated group itself and all its children.
It might seem like an overkill, but the deprecated group is actually a huge group. It would be a nightmare to list all of the deprecation warnings in the compiler invocation. I didn't want to blow up the initial PR, but I have a complete list of "depracation-related" groups:

The list 
GROUP(deprecated, 6.0, 
      availability_deprecated,
      enable_experimental_cxx_interop_flag_deprecated,
      flag_deprecated,
      implicit_bridging_header_imported_from_module_deprecated,
      use_filelists_deprecated,
      darwin_link_objc_deprecated,
      old_driver_deprecated,
      ide_availability_softdeprecated,
      borrowing_in_pattern_matches_deprecated,
      writable_keypath_to_readonly_property_deprecated,
      selector_literal_deprecated,
      invalid_redecl_swift5_deprecated,
      implementation_only_deprecated,
      unlabeled_trailing_closure_deprecated,
      attr_objcImplementation_deprecated,
      witness_deprecated,
      anyobject_class_inheritance_deprecated,
      attr_ApplicationMain_deprecated,
      conditional_conformance_outer_access_deprecated,
      one_pattern_for_several_associated_values_deprecated,
      unsafe_global_actor_deprecated,
      implicitly_unwrapped_optional_deprecated,
      conformance_availability_deprecated,
      hashvalue_implementation_deprecated,
      executor_enqueue_owned_job_implementation_deprecated,
      property_wrapper_wrapperValue_deprecated,
      property_wrapper_init_initialValue_deprecated,
      reordering_tuple_shuffle_deprecated)
GROUP(availability_deprecated, 6.0)
GROUP(enable_experimental_cxx_interop_flag_deprecated, 6.0)
GROUP(flag_deprecated, 6.0)
GROUP(implicit_bridging_header_imported_from_module_deprecated, 6.0)
GROUP(use_filelists_deprecated, 6.0)
GROUP(darwin_link_objc_deprecated, 6.0)
GROUP(old_driver_deprecated, 6.0)
GROUP(ide_availability_softdeprecated, 6.0)
GROUP(borrowing_in_pattern_matches_deprecated, 6.0)
GROUP(writable_keypath_to_readonly_property_deprecated, 6.0)
GROUP(selector_literal_deprecated, 6.0)
GROUP(invalid_redecl_swift5_deprecated, 6.0)
GROUP(implementation_only_deprecated, 6.0)
GROUP(unlabeled_trailing_closure_deprecated, 6.0)
GROUP(attr_objcImplementation_deprecated, 6.0)
GROUP(witness_deprecated, 6.0)
GROUP(anyobject_class_inheritance_deprecated, 6.0)
GROUP(attr_ApplicationMain_deprecated, 6.0)
GROUP(conditional_conformance_outer_access_deprecated, 6.0)
GROUP(one_pattern_for_several_associated_values_deprecated, 6.0)
GROUP(unsafe_global_actor_deprecated, 6.0)
GROUP(implicitly_unwrapped_optional_deprecated, 6.0)
GROUP(conformance_availability_deprecated, 6.0)
GROUP(hashvalue_implementation_deprecated, 6.0)
GROUP(executor_enqueue_owned_job_implementation_deprecated, 6.0)
GROUP(property_wrapper_wrapperValue_deprecated, 6.0)
GROUP(property_wrapper_init_initialValue_deprecated, 6.0)
GROUP(reordering_tuple_shuffle_deprecated, 6.0)

Likewise there could be

GROUP(concurrency, 6.x, concurrency_some_specific_issue)
GROUP(concurrency_some_specific_issue, 6.x)

I think this can be expressed as a group, and doesn't require doing this upfront. We need to add some new warnings corresponding to the existing conformance_availability_deprecated, availability_deprecated and availability_deprecated_rename warnings, and use them when the deprecated function/type is in the same module.
With that we can build a tree where the availability_deprecated_same_module group is nested into the regular availability_deprecated group:

Groups:
GROUP(deprecated, 6.0, 
      availability_deprecated, ...)
GROUP(availability_deprecated, 6.0,
      availability_deprecated_same_module)
GROUP(availability_deprecated_same_module, 6.0)

Warnings:
GROUPED_WARNING(availability_deprecated, availability_deprecated, ...)
GROUPED_WARNING(availability_deprecated_rename, availability_deprecated, ...)
GROUPED_WARNING(conformance_availability_deprecated, availability_deprecated, ...)
GROUPED_WARNING(availability_deprecated_same_module, availability_deprecated_same_module, ...)
GROUPED_WARNING(availability_deprecated_rename_same_module, availability_deprecated_same_module, ...)
GROUPED_WARNING(conformance_availability_deprecated_same_module, availability_deprecated_same_module, ...)
10

Oh even better then! Sorry I didn't check the PR before posting :slight_smile: That's exactly what I was thinking about, great to see.

Sounds good to me! Since we have hierarchies that'll do the job here. I think this works here since one is clearly "wider" -- no deprecation warnings at all, and the other one more specific, within module... We may need to emit them using different IDs but that's doable I think.

This is looking quite neat, thank you @dmt :-)

1 Like
11

Yes, but even when we can't decide which one is wider, we can introduce a new supergroup and include both of them.
Moreover, a group can have any number of parents, so we can have several supergroups with partially intersecting sets of subgroups.
For example, some warnings might be related to both the 'concurrency' group and the 'deprecated' group simultaneously. Suppose we want to treat concurrency warnings as errors, but keep deprecation warnings as warnings. This would be achieved with the options: -warning-as-error concurrency -no-warning-as-error deprecated.

2 Likes
12

@ArtemC What will happen with [no-]warning-as-error options when compiling a Swift interface? Do we get to drop them from the module compilation, or do they apply also when compiling the interface? In clang -W options apply to the module compilation, which causes us to build more module variants.

13

This is amazing. +1000.

Same here for iOS apps. There are deprecations every year and we can't get to all of them right away. It means turning off warnings-as-errors which leads us to tons and tons of warnings.

2 Likes
14

Any improvements on this aspect will be welcomed.

What I’ve implemented on my team is to not treat them as errors but to block PRs with warnings. For a team it works very well. Crucially this allows is to ignore certain warnings for a period of time while we fix then (usually new iOS or Xcode versions) while at the same time not allowing new ones to be merged. And i think this is the key, we want to allow certain “known warnings “ for a while, but we don’t want to let others slip trough. Errors as warnings is too extreme to be useful on these scenarios.

15

I think we should drop these from the .swiftinterface so that we don't end up building more module variants.

Doug

3 Likes
16

@Douglas_Gregor thanks, that was the consensus from others I talked to about this offline as well.

17

How do warnings-as-errors affect .swiftinterface generation? Are the compiler options written into the .swiftinterface?

18

Opinion: Swiftinterfaces are always dependencies and thus swiftinterfaces should never result in warnings. Yes, it’s possible the swiftinterface contains broken code in an inlinable function body and you could conceivably want to fix that, or at least know about it. But in practice that is not the point of swiftinterfaces and it is much simpler if they do not produce warnings. Which, I believe, is the behavior they have today.

5 Likes
19

I think the recent steps to make the compiler aware of SwiftPM packages might impact that assessment. Other packages are certainly dependencies, but are other modules in the same package true dependencies? If not, should resilient builds affect whether a warning or error is generated by a declaration in a sibling module?

20

Warnings for other modules in the same package will be produced when those modules are compiled. Whether or not the warning is emitted only depends on whether the module is compiled, not how it's used.

(Also, swiftinterfaces are only used for binary libraries, so it doesn't actually apply to this case.)

1 Like