-
Notifications
You must be signed in to change notification settings - Fork 10.5k
[Diagnostics] Add -no-warning-as-error to except a specific warning from being treated as an error #74466
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
@artemcm Can you take a look please? |
@hborla Can you take a look please? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
@@ -61,6 +62,9 @@ class DiagnosticOptions { | |||
/// Treat all warnings as errors | |||
bool WarningsAsErrors = false; | |||
|
|||
/// Don't treat these warnings as errors when WarningsAsErrors=true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please document that these are diagnosticID strings here.
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 |
Yes, please. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
This is a really important point. The addition of this feature would hamper further evolution of the compiler, because code that relies on 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. |
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.
Using id-based rules is still discouraged for source-distributed libraries, but is still an option for everyone else. |
It's necessary to allow us to improve diagnostics in the future, but providing more-specific diagnostics in certain cases.
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. |
@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 |
1e812ad
to
7314f01
Compare
@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:
What's left to do:
|
88702f0
to
32d599d
Compare
@artemcm @DougGregor ping? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll (separately) comment on the design aspects, but the implementation here looks good and I only have a few small comments.
#include "swift/AST/DefineDiagnosticGroupsMacros.h" | ||
|
||
#pragma clang diagnostic push | ||
#pragma clang diagnostic ignored "-Wgnu-zero-variadic-macro-arguments" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'll have to see if this causes us any trouble with other compilers.
// | ||
//===----------------------------------------------------------------------===// | ||
|
||
#define DEFINE_DIAGNOSTIC_GROUPS_MACROS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Outdated
llvm::ArrayRef<DiagID> diagnostics; | ||
|
||
void | ||
traverseDepthFirst(std::function<void(const DiagGroupInfo &)> func) const; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The std::function
could be llvm::function_ref
, but it's not a big deal.
@@ -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, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This specific warning is a bit tricky, because everything that goes through it started its life as an error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
return false; | ||
} | ||
|
||
static_assert(!hasCycle(), "Diagnostic groups graph has a cycle!"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is quite some clever metaprogramming.
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this forcing the LLVM diagnostic style?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
@swift-ci please smoke test |
MSVC failed on |
The changes:
@DougGregor Can you take a look and rerun the tests, please? |
@swift-ci please test |
The fix for broken Linux builds has been merged. Can someone rerun CI? |
@swift-ci please test Linux |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks really good; I have no further comments about the implementation that aren't "ooh, and then we can do !"
@@ -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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'll (separately) need to do this in the new swift-driver
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, here's a PR swiftlang/swift-driver#1678
@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.
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. |
Hey @DougGregor, do you have any updates here? It would be great to have this merged as soon as you come to an agreement. Also, would you be able to provide a rough estimate on when these changes would land on a RELEASE build of Swift? |
@mtzaquia This feature has recently expanded beyond what is implemented in this PR and will go through a review. |
The proposal has been accepted! |
I'm aligning the implementation here with what the proposal says. |
…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.
…trol over warning behavior
5dfd521
to
070c77e
Compare
I fixed the flag names, added a check to the old driver to prevent the new flags from being used with Someone, please rerun the tests 🙏 |
@swift-ci please test |
This commit adds support for the warning treating option group, including the following options: -warnings-as-errors, -no-warnings-as-errors, -Werror, and -Wwarning. 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.
The errors on macOS are rather strange. It doesn't look like it's because of this PR:
```
FAILED: stdlib/private/SwiftPrivate/FREESTANDING/arm64/SwiftPrivate.o /Users/ec2-user/jenkins/workspace/swift-PR-macos/branch-main/build/buildbot_incremental/minimalstdlib-macosx-x86_64/stdlib/private/SwiftPrivate/FREESTANDING/arm64/SwiftPrivate.o
...
/Applications/Xcode-beta.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX14.2.sdk/usr/lib/swift/Darwin.swiftmodule/arm64e-apple-macos.swiftinterface:29:34: error: 'Mirror' is unavailable
27 | }
28 | extension Darwin.DarwinBoolean : Swift.CustomReflectable {
29 | public var customMirror: Swift.Mirror {
| `- error: 'Mirror' is unavailable
30 | get
31 | }
Swift.Mirror:2:15: note: 'Mirror' has been explicitly marked unavailable here /Applications/Xcode-beta.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX14.2.sdk/usr/lib/swift/Darwin.swiftmodule/arm64e-apple-macos.swiftinterface:28:40: error: 'CustomReflectable' is unavailable Swift.CustomReflectable:2:17: note: 'CustomReflectable' has been explicitly marked unavailable here /Applications/Xcode-beta.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX14.2.sdk/usr/lib/swift/Darwin.swiftmodule/arm64e-apple-macos.swiftinterface:767:36: error: broken standard library: cannot find intrinsic operations on UnsafeMutablePointer /Applications/Xcode-beta.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX14.2.sdk/usr/lib/swift/Darwin.swiftmodule/arm64e-apple-macos.swiftinterface:772:36: error: broken standard library: cannot find intrinsic operations on UnsafeMutablePointer /Applications/Xcode-beta.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX14.2.sdk/usr/lib/swift/Darwin.swiftmodule/arm64e-apple-macos.swiftinterface:777:43: error: broken standard library: cannot find intrinsic operations on UnsafeMutablePointer /Applications/Xcode-beta.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX14.2.sdk/usr/lib/swift/Darwin.swiftmodule/arm64e-apple-macos.swiftinterface:782:44: error: broken standard library: cannot find intrinsic operations on UnsafeMutablePointer /Applications/Xcode-beta.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX14.2.sdk/usr/lib/swift/Darwin.swiftmodule/arm64e-apple-macos.swiftinterface:787:21: error: broken standard library: cannot find intrinsic operations on UnsafeMutablePointer /Users/ec2-user/jenkins/workspace/swift-PR-macos/branch-main/swift/stdlib/private/SwiftPrivate/IO.swift:21:8: error: failed to build module 'Darwin'; this SDK is not supported by the compiler (the SDK is built with 'Apple Swift version 5.9.2 (swiftlang-5.9.2.2.11 clang-1500.1.0.2.2)', while this compiler is 'Apple Swift version 6.1-dev effective-5.10 (LLVM 80d178d8cbb1cee, Swift 67ac5b9)'). Please select a toolchain which matches the SDK. /Applications/Xcode-beta.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX14.2.sdk/usr/lib/swift/Darwin.swiftmodule/arm64e-apple-macos.swiftinterface:29:34: error: 'Mirror' is unavailable
|
@swift-ci please smoke test macOS |
The macOS errors you are seeing do seem unrelated to your change. |
@swift-ci please smoke test macOS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All my remaining comments are cosmetic, and the proposal has been accepted. Let's merge this!
@@ -851,8 +852,8 @@ namespace swift { | |||
/// Don't emit any remarks | |||
bool suppressRemarks = false; | |||
|
|||
/// Emit all warnings as errors | |||
bool warningsAsErrors = false; | |||
/// Treat these warnings as errors. Indicies here corespond to DiagID enum |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typos "indicies" (for indices) and "corespond"
return state.getWarningsAsErrors(); | ||
} | ||
/// Apply rules specifing what warnings should or shouldn't be treated as | ||
/// errors. For group rules the string is either a group name defined by |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's no "or" to go with the "either here".
swiftc -warning-as-error availability_deprecated file.swift | ||
swiftc -warnings-as-errors -no-warning-as-error availability_deprecated file.swift |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These flag names have changed in SE-0443.
swiftc -warning-as-error deprecated file.swift | ||
swiftc -warnings-as-errors -no-warning-as-error deprecated file.swift |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Flag names have changed.
Such warnings are emitted after the behavior for all specified warning groups has been processed, which means their behavior can also be specified. For example: | ||
|
||
```sh | ||
swiftc -warning-as-error unknown_warning_group -warning-as-error non_existing_group file.swift |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Flag names again
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 ininclude/swift/AST/Diagnostics*.def
.The option can be added multiple times.