Warnings-as-errors exceptions

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

10 Likes

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.

4 Likes

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).

9 Likes

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.

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

7 Likes

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.

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

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.

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, ...)