Skip to content

Commit d8435e4

Browse files
AnthonyLatsisAnthonyLatsis
committed
DiagnosticEngine: Fix escalation for wrapped diagnostics
Wrapped diagnostics were not escalated to errors because the check was based on the diagnostic ID, which is that of the wrapper diagnostic in this case. Switch to tracking whether escalation was enabled for a given group instead.
1 parent 7934a16 commit d8435e4

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
@@ -513,11 +513,17 @@ namespace swift {
513513
friend DiagnosticEngine;
514514
friend class InFlightDiagnostic;
515515

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

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

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

909917
/// Whether a fatal error has occurred
@@ -945,16 +953,23 @@ namespace swift {
945953
void setSuppressRemarks(bool val) { suppressRemarks = val; }
946954
bool getSuppressRemarks() const { return suppressRemarks; }
947955

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

956-
/// Whether all warnings should be upgraded to errors or not
968+
/// Whether all warnings should be upgraded to errors or not.
957969
void setAllWarningsAsErrors(bool value) {
970+
// This works as intended because every diagnostic belongs to either a
971+
// custom group or the top-level `DiagGroupID::no_group`, which is also
972+
// a group.
958973
if (value) {
959974
warningsAsErrors.set();
960975
} 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)