[Diagnostics] Add -no-warning-as-error to except a specific warning from being treated as an error #74466

DmT021 · 2024-06-17T01:41:16Z

This commit adds a new compiler option -no-warning-as-error which allows users to specify exceptions from the -warnings-as-errors option.
The format of the option is -no-warning-as-error <diag_id>, where <diag_id> is one of the diagnostic IDs listed in include/swift/AST/Diagnostics*.def.
The option can be added multiple times.

DmT021 · 2024-06-25T14:27:27Z

@artemcm Can you take a look please?

DmT021 · 2024-07-01T19:26:56Z

@hborla Can you take a look please?

artemcm

Sorry for the delay @DmT021.
This change looks good, and I think it's good to keep this in HelpHidden.

Please also make a matching change in:
https://github.com/apple/swift-driver?tab=readme-ov-file#rebuilding-optionsswift

artemcm · 2024-07-01T19:57:11Z

include/swift/Basic/DiagnosticOptions.h

@@ -61,6 +62,9 @@ class DiagnosticOptions {
  /// Treat all warnings as errors
  bool WarningsAsErrors = false;

+  /// Don't treat these warnings as errors when WarningsAsErrors=true


Please document that these are diagnosticID strings here.

artemcm · 2024-07-01T20:31:51Z

It's worth noting that there are no guarantees that diagnostic IDs are stable API between different compiler versions, they have not historically been treated as such.

DmT021 · 2024-07-01T20:37:02Z

It's worth noting that there are no guarantees that diagnostic IDs are stable API between different compiler versions, they have not historically been treated as such.

Yeah, but when used with -debug-diagnostic-names it's pretty easy to see what the diagnosticID is for a particular warning. Should I include this note in the help text as well?

artemcm · 2024-07-01T20:39:26Z

It's worth noting that there are no guarantees that diagnostic IDs are stable API between different compiler versions, they have not historically been treated as such.

Yeah, but when used with -debug-diagnostic-names it's pretty easy to see what the diagnosticID is for a particular warning. Should I include this note in the help text as well?

Yes, please.

artemcm

After consulting with some more folks, I think the use of diagnostic IDs is a bigger concern.

Though the idea behind this change is worthwhile.
One possible approach to get the desired result is to implement something akin to Clang's diagnostic groups. Then warning diagnostics can be grouped into stable-named groups which can then be treated as exceptions to warning-as-error, for example.

DougGregor · 2024-07-01T20:51:04Z

It's worth noting that there are no guarantees that diagnostic IDs are stable API between different compiler versions, they have not historically been treated as such.

This is a really important point. The addition of this feature would hamper further evolution of the compiler, because code that relies on -no-warning-as-error <diag_id> to compile will stop working if the diagnostic ID changes. We can't just promise not to make changes, because it's very common for us to improve diagnostics by adding more special-case versions of existing warnings, which will necessitate new diagnostic IDs.

I support the general idea of more specific control over warnings, but not based on the diagnostic IDs. Clang's approach of having documented "warning groups" with stable IDs, where each warning is sorted into an appropriate warning group, would be a reasonable model to follow. It's definitely more upfront work to do this... along with building the infrastructure for warning groups, we'd also need to sort the existing ~350 warnings into groups. But if we're going to do this, it's worth doing it well so we can provide flexibility to developers while still being able to evolve the compiler.

DmT021 · 2024-07-01T21:50:18Z

Using groups will result in a loss of precision when defining exclusion rules, which isn't desirable either. Of course, I agree that control via diagnostic IDs isn't very rigid, and group-based rules are superior. But this is only important if the user wants to distribute the library. If they are building an end binary, it is quite acceptable.
This change addresses existing situations where a newly diagnosed warning by a new compiler led to a failed build because of the -warnings-as-errors rule.
What if we provide a generalized rule syntax that allows the user to specify a condition (either an ID or a group) to check a warning against? Something like

-no-warning-as-error ignored_import                     (group)
-no-warning-as-error id:sema_import_current_module      (id)

Using id-based rules is still discouraged for source-distributed libraries, but is still an option for everyone else.
And then we can categorize warnings into groups gradually.

DougGregor · 2024-07-02T02:12:48Z

Using groups will result in a loss of precision when defining exclusion rules, which isn't desirable either

It's necessary to allow us to improve diagnostics in the future, but providing more-specific diagnostics in certain cases.

But this is only important if the user wants to distribute the library. If they are building an end binary, it is quite acceptable.

That's not true. As I described in my original reply, we must preserve the ability to create more specific versions of a given warning that provide a better diagnosis for the user. Adding this feature based on diagnostic IDs makes that kind of change a source-breaking change.

DougGregor · 2024-07-03T16:54:40Z

@DmT021 there are a bunch of design and usability questions here, and it's grown beyond what's reasonable to figure out in the pull request. I think we'd benefit from taking the discussion over to forums.swift.org where we can get more feedback from others

DmT021 · 2024-07-03T20:03:32Z

https://forums.swift.org/t/warnings-as-errors-exceptions/72925

DmT021 · 2024-07-17T04:21:24Z

@artemcm @DougGregor This is still work-in-progress but I want to check if I'm moving in the right direction. Here's what I have so far:

DiagnosticGroups.def file defines diagnostic groups. I took clang's approach and made groups nested: a group can have subgroups.
To have something to begin with I declared several groups. In particular there are the deprecated group and availability_deprecated which is nested to deprecated. The idea here is to have all deprecation warnings nested in the deprecated group so they can be excluded together.
It compiles as constexpr.
WARNING and ERROR diagnostics now have GROUPED_ variants.
New compiler options -[no-]-warning-as-error <group>. <group> can be either a real group or a string _id:<diag_id>. And a new option group warning_treating_Group that aggregates them.
-[no-]-warning-as-error and -[no-]-warnings-as-errors apply in order they appear in the command line. If there're more than one rule affecting the behavior of the same DiagID - the last rule wins.
The Diagnostic Groups graph consistency expressed as static_asserts and not dedicated tests.
The diagnostic groups graph is checked for cycles.

What's left to do:

It seems reasonable to include the "warning suppression" rules in this mechanism. But I decided to avoid doing this until we agree on it.
There's a difficulty with testing that a particular warning is actually in the expected group(s).
For example there's a bunch of test for the @available(*, deprecated) annotation. We need to check that all of them are in the availability_deprecated and deprecated groups. We can invoke the compiler with all combinations of -[no-]-warning-as-error and -[no-]-warnings-as-errors flags for each test, but this will result in combinatorial explosion.
So I came up with an alternative: we can introduce a new flag -pring-diagnostic-groups which will add a suffix with [group:<group_name>] to all the warnings produced. We only need to print the group that directly includes this particular DiagID and not the whole hierarchy of groups. Then we test that warnings are suffixed with the expected groups.
(We should also test that groups are nested correctly, but that's already done with static_asserts.)
Is it ok?

…er warning behavior This commit adds new compiler options -no-warning-as-error/-warning-as-error which allows users to specify behavior for exact warnings and warning groups.

DmT021 · 2024-07-23T15:55:33Z

@artemcm @DougGregor ping?

DougGregor

I'll (separately) comment on the design aspects, but the implementation here looks good and I only have a few small comments.

DougGregor · 2024-07-23T18:45:44Z

include/swift/AST/DiagnosticGroups.def

+#include "swift/AST/DefineDiagnosticGroupsMacros.h"
+
+#pragma clang diagnostic push
+#pragma clang diagnostic ignored "-Wgnu-zero-variadic-macro-arguments"


We'll have to see if this causes us any trouble with other compilers.

DougGregor · 2024-07-23T18:47:03Z

include/swift/AST/DiagnosticGroups.def

+//
+//===----------------------------------------------------------------------===//
+
+#define DEFINE_DIAGNOSTIC_GROUPS_MACROS


It's not critical now, but at some point we'd want a big comment here documenting what it means to define a new group.

include/swift/AST/DiagnosticGroups.h

DougGregor · 2024-07-23T18:48:56Z

include/swift/AST/DiagnosticGroups.h

+  llvm::ArrayRef<DiagID> diagnostics;
+
+  void
+  traverseDepthFirst(std::function<void(const DiagGroupInfo &)> func) const;


The std::function could be llvm::function_ref, but it's not a big deal.

DougGregor · 2024-07-23T19:36:29Z

include/swift/AST/DiagnosticsCommon.def

@@ -47,7 +47,7 @@ ERROR(error_no_group_info,none,
 NOTE(brace_stmt_suggest_do,none,
     "did you mean to use a 'do' statement?", ())

-WARNING(error_in_future_swift_version,none,
+GROUPED_WARNING(error_in_future_swift_version,error_in_future_swift_version,none,


This specific warning is a bit tricky, because everything that goes through it started its life as an error.

Yep, it confuses me a little. It's not technically wrong - if a warning was upgraded to error because we enabled -swift 6 we want it to be an actual error and fail the build. But it would be nice if there was some cleaner solution for these warnings.

The unfortunate thing about this warning group is that it's going to mix unrelated diagnostics together into a single warning group, because we use it via the warnUntilSwiftVersion(N) on ERRORs of various flavors. The only other idea I have here is to (eventually) require ERRORs treated this way to be part of a warning group. For example, a "concurrency" warning group could encapsulate all of the concurrency diagnostics (which are ERRORs) when they get downgraded. But we'd have to disallow them from being downgraded in Swift 6 somehow, so there's a bit of work to get to that point.

It was my initial idea too, which is why both GROUPED_WARNING and GROUPED_ERROR exist. The issue is that, theoretically, a Diagnostic can be nested inside another Diagnostic at any place within the Args vector. And this tree can be of any depth. If we want to check nested diagnostics we have to traverse all the tree.
And then if at least one diagnostic with an excluded groupID is found - apply the behavior(error/warning/suppress?) to the root Diagnostic.
I wasn't sure if it's correct.

DougGregor · 2024-07-23T19:42:36Z

lib/AST/DiagnosticGroups.cpp

+  return false;
+}
+
+static_assert(!hasCycle(), "Diagnostic groups graph has a cycle!");


This is quite some clever metaprogramming.

DougGregor · 2024-07-23T19:43:28Z

test/diagnostics/warnings_as_errors_rules.swift

@@ -0,0 +1,65 @@
+// RUN: not %target-swift-frontend -typecheck -diagnostic-style llvm -warnings-as-errors %s 2>&1 | %FileCheck %s --check-prefix=CHECK-WAE-ALL


Why is this forcing the LLVM diagnostic style?

With the swift style the compiler produces a message like this:

$ ./swift-frontend -typecheck -diagnostic-style swift -warnings-as-errors ../../../../swift/test/diagnostics/warnings_as_errors_rules.swift ../../../../swift/test/diagnostics/warnings_as_errors_rules.swift:42:1: error: 'foo()' is deprecated 40 | // CHECK-WAE-GROUP-NWAE-DIAGID: warning: 'foo()' is deprecated [group:availability_deprecated] 41 | // CHECK-WAE-GROUP-NWAE-DIAGID-NOT: error: 'foo()' is deprecated [group:availability_deprecated] 42 | foo() | `- error: 'foo()' is deprecated 43 | 44 |

As you can see there are two lines more above and below the line 42. FileCheck sees it in its input and when it performs a -NOT check these comments actually produce a match (which fails the testcase).
Ideally I'd prefer if FileCheck ignored lines containing .*CHECK-.*, but I don't see a flag or option for this.

Ah, I understand. In our call through to the swift-syntax diagnostic mechanism, we're hard-coding the context size to 2 lines:

swift_ASTGen_renderQueuedDiagnostics(queuedDiagnostics, /*contextSize=*/2, forceColors ? 1 : 0, &bridgedRenderedString);

... we should have an option here to allow setting the context size (e.g., to 0), which would make FileCheck tests like this more feasible.

To be clear, I don't mean "you have to add this other command-line argument yourself." I'm fine with your PR using -diagnostic-style llvm until we get the other command-line argument. I suspect that's the reason for some of the other existing uses of -diagnostic-style llvm

DougGregor · 2024-07-23T19:49:35Z

@swift-ci please smoke test

DmT021 · 2024-07-23T20:50:23Z

MSVC failed on __VA_OPT__. I might need some time to find a windows machine and come up with an alternative.

…trol over warning behavior

DmT021 · 2024-07-27T04:37:01Z

The changes:

Removed __VA_OPT__
I couldn't find an acceptable way to iterate over __VA_ARGS__ without __VA_OPT__. Most solutions fail to compile on MSVC. So I changed the syntax for declaring groups by splitting it into two macros: GROUP and GROUP_LINK. Once we switch to C++20, we can bring __VA_OPT__ back.
Removed support for _id:
Used llvm::function_ref
Added docs and examples for each diagnostic group.
Removed support for error_in_future_swift_version. I'll work on it separately.
Removed #pragma clang and used [[maybe_unused]] attribute instead.

@DougGregor Can you take a look and rerun the tests, please?

tshortli · 2024-07-27T18:49:28Z

@swift-ci please test

DmT021 · 2024-07-29T16:14:30Z

The fix for broken Linux builds has been merged. Can someone rerun CI?

tshortli · 2024-07-29T16:15:37Z

@swift-ci please test Linux

DougGregor

This looks really good; I have no further comments about the implementation that aren't "ooh, and then we can do !"

DougGregor · 2024-08-02T22:14:22Z

lib/Driver/ToolChains.cpp

@@ -311,8 +311,7 @@ void ToolChain::addCommonFrontendArgs(const OutputInfo &OI,
  inputArgs.AddLastArg(arguments, options::OPT_profile_generate);
  inputArgs.AddLastArg(arguments, options::OPT_profile_use);
  inputArgs.AddLastArg(arguments, options::OPT_profile_coverage_mapping);
-  inputArgs.AddAllArgs(arguments, options::OPT_warnings_as_errors,
-                       options::OPT_no_warnings_as_errors);
+  inputArgs.AddAllArgs(arguments, options::OPT_warning_treating_Group);


We'll (separately) need to do this in the new swift-driver

Ok, here's a PR swiftlang/swift-driver#1678

DmT021 · 2024-08-03T02:32:58Z

This looks really good; I have no further comments about the implementation that aren't "ooh, and then we can do !"

@DougGregor Should I ping other folks to review this as well? Or I can just squash rebase and you'll merge it?

This commit adds support for the warning treating option group, including the following options: -warnings-as-errors, -no-warnings-as-errors, -warning-as-error, and -no-warning-as-error. Options in this group are now preserved as-is. It is forbidden to reorder or drop any of them. These changes reflect the modifications made to the frontend in swiftlang/swift#74466.

DougGregor · 2024-08-05T16:38:53Z

This looks really good; I have no further comments about the implementation that aren't "ooh, and then we can do !"

@DougGregor Should I ping other folks to review this as well? Or I can just squash rebase and you'll merge it?

To cover all of the bases, I've brought up this feature with the Language Steering Group to determine whether we should review it through the evolution process. I suspect we don't, because it's primarily a compiler feature that's not exposed through the language, but it's enough of a change in direction that I want to be sure to involve them.

DmT021 marked this pull request as ready for review June 17, 2024 01:44

DmT021 requested review from artemcm, hborla, slavapestov, xedin and tshortli as code owners June 17, 2024 01:44

artemcm approved these changes Jul 1, 2024

View reviewed changes

artemcm reviewed Jul 1, 2024

View reviewed changes

DmT021 force-pushed the wp/no-warning-as-error branch from 1e812ad to 7314f01 Compare July 17, 2024 03:38

DmT021 requested a review from DougGregor as a code owner July 17, 2024 03:38

DmT021 force-pushed the wp/no-warning-as-error branch from 7314f01 to 88702f0 Compare July 18, 2024 00:48

[Diagnostics] Add -[no-]warning-as-error flags for precise control ov…

32d599d

…er warning behavior This commit adds new compiler options -no-warning-as-error/-warning-as-error which allows users to specify behavior for exact warnings and warning groups.

DmT021 force-pushed the wp/no-warning-as-error branch from 88702f0 to 32d599d Compare July 18, 2024 02:19

DougGregor reviewed Jul 23, 2024

View reviewed changes

fixup! [Diagnostics] Add -[no-]warning-as-error flags for precise con…

5dfd521

…trol over warning behavior

DougGregor reviewed Aug 2, 2024

View reviewed changes

DmT021 mentioned this pull request Aug 3, 2024

[Diagnostics] Support for all the flags of the warning treating group swiftlang/swift-driver#1678

Open

		@@ -0,0 +1,65 @@
		// RUN: not %target-swift-frontend -typecheck -diagnostic-style llvm -warnings-as-errors %s 2>&1 \| %FileCheck %s --check-prefix=CHECK-WAE-ALL

[Diagnostics] Add -no-warning-as-error to except a specific warning from being treated as an error #74466

Are you sure you want to change the base?

[Diagnostics] Add -no-warning-as-error to except a specific warning from being treated as an error #74466

Conversation

DmT021 commented Jun 17, 2024

DmT021 commented Jun 25, 2024

DmT021 commented Jul 1, 2024

artemcm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

artemcm commented Jul 1, 2024

DmT021 commented Jul 1, 2024 • edited Loading

artemcm commented Jul 1, 2024

artemcm left a comment

Choose a reason for hiding this comment

DougGregor commented Jul 1, 2024

DmT021 commented Jul 1, 2024

DougGregor commented Jul 2, 2024

DougGregor commented Jul 3, 2024

DmT021 commented Jul 3, 2024

DmT021 commented Jul 17, 2024 • edited Loading

DmT021 commented Jul 23, 2024

DougGregor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DougGregor commented Jul 23, 2024

DmT021 commented Jul 23, 2024 • edited Loading

DmT021 commented Jul 27, 2024

tshortli commented Jul 27, 2024

DmT021 commented Jul 29, 2024

tshortli commented Jul 29, 2024

DougGregor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DmT021 commented Aug 3, 2024 • edited Loading

DougGregor commented Aug 5, 2024

DmT021 commented Jul 1, 2024 •

edited

Loading

DmT021 commented Jul 17, 2024 •

edited

Loading

DmT021 commented Jul 23, 2024 •

edited

Loading

DmT021 commented Aug 3, 2024 •

edited

Loading