Skip to content

Commit c3fed61

Browse files
AnthonyLatsisAnthonyLatsis
committed
DiagnosticEngine: Fix diagnostic groups behavior for wrapped diagnostics
1 parent 55189ba commit c3fed61

File tree

5 files changed

+225
-13
lines changed

5 files changed

+225
-13
lines changed

‎include/swift/AST/DiagnosticEngine.h

Lines changed: 22 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -512,11 +512,17 @@ namespace swift {
512512
friend DiagnosticEngine;
513513
friend class InFlightDiagnostic;
514514

515-
Diagnostic(DiagID ID, DiagGroupID GroupID) : ID(ID), GroupID(GroupID) {}
516-
517515
/// Constructs a Diagnostic with DiagGroupID infered from DiagID.
518516
Diagnostic(DiagID ID);
519517

518+
protected:
519+
/// Only use this constructor privately in this class or in unit tests by
520+
/// subclassing.
521+
/// In unit tests, it is used as a means for associating diagnostics with
522+
/// groups and, thus, circumventing the need to otherwise define mock
523+
/// diagnostics, which is not accounted for in the current design.
524+
Diagnostic(DiagID ID, DiagGroupID GroupID) : ID(ID), GroupID(GroupID) {}
525+
520526
public:
521527
// All constructors are intentionally implicit.
522528
template<typename ...ArgTypes>
@@ -902,7 +908,9 @@ namespace swift {
902908
/// Don't emit any remarks
903909
bool suppressRemarks = false;
904910

905-
/// Treat these warnings as errors. Indices here correspond to DiagID enum
911+
/// A mapping from `DiagGroupID` identifiers to Boolean values indicating
912+
/// whether warnings belonging to the respective diagnostic groups should be
913+
/// escalated to errors.
906914
llvm::BitVector warningsAsErrors;
907915

908916
/// Whether a fatal error has occurred
@@ -944,16 +952,23 @@ namespace swift {
944952
void setSuppressRemarks(bool val) { suppressRemarks = val; }
945953
bool getSuppressRemarks() const { return suppressRemarks; }
946954

947-
/// Whether a warning should be upgraded to an error or not
948-
void setWarningAsErrorForDiagID(DiagID id, bool value) {
955+
/// Sets whether warnings belonging to the diagnostic group identified by
956+
/// `id` should be escalated to errors.
957+
void setWarningsAsErrorsForDiagGroupID(DiagGroupID id, bool value) {
949958
warningsAsErrors[(unsigned)id] = value;
950959
}
951-
bool getWarningAsErrorForDiagID(DiagID id) {
960+
961+
/// Returns a Boolean value indicating whether warnings belonging to the
962+
/// diagnostic group identified by `id` should be escalated to errors.
963+
bool getWarningsAsErrorsForDiagGroupID(DiagGroupID id) {
952964
return warningsAsErrors[(unsigned)id];
953965
}
954966

955-
/// Whether all warnings should be upgraded to errors or not
967+
/// Whether all warnings should be upgraded to errors or not.
956968
void setAllWarningsAsErrors(bool value) {
969+
// This works as intended because every diagnostic belongs to either a
970+
// custom group or the top-level `DiagGroupID::no_group`, which is also
971+
// a group.
957972
if (value) {
958973
warningsAsErrors.set();
959974
} else {

‎include/swift/AST/DiagnosticGroups.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,7 @@ enum class DiagGroupID : uint16_t {
3333

3434
constexpr const auto DiagGroupsCount = [] {
3535
size_t count = 0;
36-
#define GROUP(Name, Version) count++;
36+
#define GROUP(Name, Version) ++count;
3737
#include "DiagnosticGroups.def"
3838
return count;
3939
}();

‎lib/AST/DiagnosticEngine.cpp

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -176,7 +176,7 @@ DiagnosticState::DiagnosticState() {
176176
// Initialize our ignored diagnostics to default
177177
ignoredDiagnostics.resize(LocalDiagID::NumDiags);
178178
// Initialize warningsAsErrors to default
179-
warningsAsErrors.resize(LocalDiagID::NumDiags);
179+
warningsAsErrors.resize(DiagGroupsCount);
180180
}
181181

182182
Diagnostic::Diagnostic(DiagID ID)
@@ -551,9 +551,7 @@ void DiagnosticEngine::setWarningsAsErrorsRules(
551551
if (auto groupID = getDiagGroupIDByName(name);
552552
groupID && *groupID != DiagGroupID::no_group) {
553553
getDiagGroupInfoByID(*groupID).traverseDepthFirst([&](auto group) {
554-
for (DiagID diagID : group.diagnostics) {
555-
state.setWarningAsErrorForDiagID(diagID, isEnabled);
556-
}
554+
state.setWarningsAsErrorsForDiagGroupID(*groupID, isEnabled);
557555
});
558556
} else {
559557
unknownGroups.push_back(std::string(name));
@@ -1232,7 +1230,7 @@ DiagnosticBehavior DiagnosticState::determineBehavior(const Diagnostic &diag) {
12321230
// 4) If the user substituted a different behavior for this behavior, apply
12331231
// that change
12341232
if (lvl == DiagnosticBehavior::Warning) {
1235-
if (getWarningAsErrorForDiagID(diag.getID()))
1233+
if (getWarningsAsErrorsForDiagGroupID(diag.getGroupID()))
12361234
lvl = DiagnosticBehavior::Error;
12371235
if (suppressWarnings)
12381236
lvl = DiagnosticBehavior::Ignore;

‎unittests/AST/CMakeLists.txt

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ add_swift_unittest(SwiftASTTests
44
ASTWalkerTests.cpp
55
IndexSubsetTests.cpp
66
DiagnosticConsumerTests.cpp
7+
DiagnosticGroupsTests.cpp
78
SourceLocTests.cpp
89
TestContext.cpp
910
TypeMatchTests.cpp

‎unittests/AST/DiagnosticGroupsTests.cpp

Lines changed: 198 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,198 @@
1+
//===--- DiagnosticGroupsTests.cpp ----------------------------------------===//
2+
//
3+
// This source file is part of the Swift.org open source project
4+
//
5+
// Copyright (c) 2025 Apple Inc. and the Swift project authors
6+
// Licensed under Apache License v2.0 with Runtime Library Exception
7+
//
8+
// See https://swift.org/LICENSE.txt for license information
9+
// See https://swift.org/CONTRIBUTORS.txt for the list of Swift project authors
10+
//
11+
//===----------------------------------------------------------------------===//
12+
//
13+
// NB: The correctness of the diagnostic group graph is verified in lib/AST
14+
// ('namespace validation').
15+
//
16+
//===----------------------------------------------------------------------===//
17+
18+
#include "swift/AST/DiagnosticGroups.h"
19+
#include "swift/AST/DiagnosticEngine.h"
20+
#include "swift/AST/DiagnosticsFrontend.h"
21+
#include "swift/Basic/SourceManager.h"
22+
#include "gtest/gtest.h"
23+
24+
using namespace swift;
25+
26+
namespace {
27+
class TestDiagnosticConsumer : public DiagnosticConsumer {
28+
llvm::function_ref<void(const DiagnosticInfo &)> callback;
29+
30+
public:
31+
TestDiagnosticConsumer(decltype(callback) callback) : callback(callback) {}
32+
33+
void handleDiagnostic(SourceManager &SM,
34+
const DiagnosticInfo &Info) override {
35+
this->callback(Info);
36+
}
37+
};
38+
39+
struct TestDiagnostic : public Diagnostic {
40+
TestDiagnostic(DiagID ID, DiagGroupID GroupID) : Diagnostic(ID, GroupID) {}
41+
};
42+
} // end anonymous namespace
43+
44+
static void diagnosticGroupsTestCase(
45+
llvm::function_ref<void(DiagnosticEngine &)> diagnose,
46+
llvm::function_ref<void(const DiagnosticInfo &)> callback,
47+
unsigned expectedNumCallbackCalls) {
48+
SourceManager sourceMgr;
49+
DiagnosticEngine diags(sourceMgr);
50+
51+
unsigned count = 0;
52+
53+
const auto countingCallback = [&](const DiagnosticInfo &info) {
54+
++count;
55+
callback(info);
56+
};
57+
58+
TestDiagnosticConsumer consumer(countingCallback);
59+
diags.addConsumer(consumer);
60+
diagnose(diags);
61+
diags.removeConsumer(consumer);
62+
63+
EXPECT_EQ(count, expectedNumCallbackCalls);
64+
}
65+
66+
TEST(DiagnosticGroups, PrintDiagnosticGroups) {
67+
// Test that we append the correct group in the format string.
68+
diagnosticGroupsTestCase(
69+
[](DiagnosticEngine &diags) {
70+
diags.setPrintDiagnosticNamesMode(PrintDiagnosticNamesMode::Group);
71+
72+
TestDiagnostic diagnostic(diag::error_immediate_mode_missing_stdlib.ID,
73+
DiagGroupID::DeprecatedDeclaration);
74+
diags.diagnose(SourceLoc(), diagnostic);
75+
},
76+
[](const DiagnosticInfo &info) {
77+
EXPECT_TRUE(info.FormatString.ends_with(" [DeprecatedDeclaration]"));
78+
},
79+
/*expectedNumCallbackCalls=*/1);
80+
81+
diagnosticGroupsTestCase(
82+
[](DiagnosticEngine &diags) {
83+
diags.setPrintDiagnosticNamesMode(PrintDiagnosticNamesMode::Group);
84+
85+
TestDiagnostic diagnostic(diag::error_immediate_mode_missing_stdlib.ID,
86+
DiagGroupID::no_group);
87+
diags.diagnose(SourceLoc(), diagnostic);
88+
},
89+
[](const DiagnosticInfo &info) {
90+
EXPECT_FALSE(info.FormatString.ends_with("]"));
91+
},
92+
/*expectedNumCallbackCalls=*/1);
93+
}
94+
95+
TEST(DiagnosticGroups, DiagnosticsWrappersInheritGroups) {
96+
// Test that we don't loose the group of a diagnostic when it gets wrapped in
97+
// another one.
98+
diagnosticGroupsTestCase(
99+
[](DiagnosticEngine &diags) {
100+
diags.setPrintDiagnosticNamesMode(PrintDiagnosticNamesMode::Group);
101+
102+
TestDiagnostic diagnostic(diag::error_immediate_mode_missing_stdlib.ID,
103+
DiagGroupID::DeprecatedDeclaration);
104+
diags.diagnose(SourceLoc(), diagnostic)
105+
.limitBehaviorUntilSwiftVersion(DiagnosticBehavior::Warning, 99);
106+
},
107+
[](const DiagnosticInfo &info) {
108+
EXPECT_EQ(info.ID, diag::error_in_a_future_swift_version.ID);
109+
EXPECT_TRUE(info.FormatString.ends_with(" [DeprecatedDeclaration]"));
110+
},
111+
/*expectedNumCallbackCalls=*/1);
112+
}
113+
114+
TEST(DiagnosticGroups, TargetAll) {
115+
// Test that uncategorized diagnostics are escalated when escalating all
116+
// warnings.
117+
diagnosticGroupsTestCase(
118+
[](DiagnosticEngine &diags) {
119+
const std::vector rules = {
120+
WarningAsErrorRule(WarningAsErrorRule::Action::Enable)};
121+
diags.setWarningsAsErrorsRules(rules);
122+
123+
TestDiagnostic diagnostic(
124+
diag::warn_unsupported_module_interface_library_evolution.ID,
125+
DiagGroupID::no_group);
126+
diags.diagnose(SourceLoc(), diagnostic);
127+
},
128+
[](const DiagnosticInfo &info) {
129+
EXPECT_EQ(info.Kind, DiagnosticKind::Error);
130+
},
131+
/*expectedNumCallbackCalls=*/1);
132+
}
133+
134+
TEST(DiagnosticGroups, OverrideBehaviorLimitations) {
135+
// Test that escalating warnings to errors for *errors* in a diagnostic group
136+
// overrides emission site behavior limitations.
137+
{
138+
TestDiagnostic diagnostic(diag::error_immediate_mode_missing_stdlib.ID,
139+
DiagGroupID::DeprecatedDeclaration);
140+
141+
// Make sure ID actually is an error by default.
142+
diagnosticGroupsTestCase(
143+
[&diagnostic](DiagnosticEngine &diags) {
144+
diags.diagnose(SourceLoc(), diagnostic);
145+
},
146+
[](const DiagnosticInfo &info) {
147+
EXPECT_TRUE(info.Kind == DiagnosticKind::Error);
148+
},
149+
/*expectedNumCallbackCalls=*/1);
150+
151+
diagnosticGroupsTestCase(
152+
[&diagnostic](DiagnosticEngine &diags) {
153+
const std::vector rules = {WarningAsErrorRule(
154+
WarningAsErrorRule::Action::Enable, "DeprecatedDeclaration")};
155+
diags.setWarningsAsErrorsRules(rules);
156+
157+
diags.diagnose(SourceLoc(), diagnostic);
158+
diags.diagnose(SourceLoc(), diagnostic)
159+
.limitBehaviorUntilSwiftVersion(DiagnosticBehavior::Warning, 99);
160+
},
161+
[](const DiagnosticInfo &info) {
162+
EXPECT_EQ(info.Kind, DiagnosticKind::Error);
163+
},
164+
/*expectedNumCallbackCalls=*/2);
165+
}
166+
167+
// Test that escalating warnings to errors for *warnings* in a diagnostic
168+
// group overrides emission site behavior limitations.
169+
{
170+
TestDiagnostic diagnostic(
171+
diag::warn_unsupported_module_interface_library_evolution.ID,
172+
DiagGroupID::DeprecatedDeclaration);
173+
174+
// Make sure ID actually is a warning by default.
175+
diagnosticGroupsTestCase(
176+
[&diagnostic](DiagnosticEngine &diags) {
177+
diags.diagnose(SourceLoc(), diagnostic);
178+
},
179+
[](const DiagnosticInfo &info) {
180+
EXPECT_EQ(info.Kind, DiagnosticKind::Warning);
181+
},
182+
/*expectedNumCallbackCalls=*/1);
183+
184+
diagnosticGroupsTestCase(
185+
[&diagnostic](DiagnosticEngine &diags) {
186+
const std::vector rules = {WarningAsErrorRule(
187+
WarningAsErrorRule::Action::Enable, "DeprecatedDeclaration")};
188+
diags.setWarningsAsErrorsRules(rules);
189+
190+
diags.diagnose(SourceLoc(), diagnostic)
191+
.limitBehaviorUntilSwiftVersion(DiagnosticBehavior::Warning, 99);
192+
},
193+
[](const DiagnosticInfo &info) {
194+
EXPECT_EQ(info.Kind, DiagnosticKind::Error);
195+
},
196+
/*expectedNumCallbackCalls=*/1);
197+
}
198+
}

0 commit comments

Comments
 (0)