Skip to content

Commit fb8a090

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

File tree

5 files changed

+194
-11
lines changed

5 files changed

+194
-11
lines changed

‎include/swift/AST/DiagnosticEngine.h

Lines changed: 18 additions & 6 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,11 +952,15 @@ 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

‎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: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -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: 172 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,172 @@
1+
//===--- DiagnosticConsumerTests.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+
#include "swift/AST/DiagnosticEngine.h"
14+
#include "swift/AST/DiagnosticsFrontend.h"
15+
#include "swift/Basic/SourceManager.h"
16+
#include "gtest/gtest.h"
17+
18+
using namespace swift;
19+
20+
namespace {
21+
class TestDiagnosticConsumer : public DiagnosticConsumer {
22+
llvm::function_ref<void(const DiagnosticInfo &)> callback;
23+
24+
public:
25+
TestDiagnosticConsumer(decltype(callback) callback) : callback(callback) {}
26+
27+
void handleDiagnostic(SourceManager &SM,
28+
const DiagnosticInfo &Info) override {
29+
this->callback(Info);
30+
}
31+
};
32+
33+
struct TestDiagnostic : public Diagnostic {
34+
TestDiagnostic(DiagID ID, DiagGroupID GroupID) : Diagnostic(ID, GroupID) {}
35+
};
36+
} // end anonymous namespace
37+
38+
static void diagnosticGroupsTestCase(
39+
llvm::function_ref<void(DiagnosticEngine &)> diagnose,
40+
llvm::function_ref<void(const DiagnosticInfo &)> callback,
41+
unsigned expectedNumCallbackCalls) {
42+
SourceManager sourceMgr;
43+
DiagnosticEngine diags(sourceMgr);
44+
45+
unsigned count = 0;
46+
47+
const auto countingCallback = [&](const DiagnosticInfo &info) {
48+
++count;
49+
callback(info);
50+
};
51+
52+
TestDiagnosticConsumer consumer(countingCallback);
53+
diags.addConsumer(consumer);
54+
diagnose(diags);
55+
diags.removeConsumer(consumer);
56+
57+
EXPECT_EQ(count, expectedNumCallbackCalls);
58+
}
59+
60+
TEST(DiagnosticGroups, PrintDiagnosticGroups) {
61+
// Test that we append the correct group in the format string.
62+
diagnosticGroupsTestCase(
63+
[](DiagnosticEngine &diags) {
64+
diags.setPrintDiagnosticNamesMode(PrintDiagnosticNamesMode::Group);
65+
66+
TestDiagnostic diagnostic(diag::error_immediate_mode_missing_stdlib.ID,
67+
DiagGroupID::DeprecatedDeclaration);
68+
diags.diagnose(SourceLoc(), diagnostic);
69+
},
70+
[](const DiagnosticInfo &info) {
71+
EXPECT_TRUE(info.FormatString.ends_with(" [DeprecatedDeclaration]"));
72+
},
73+
/*expectedNumCallbackCalls=*/1);
74+
75+
diagnosticGroupsTestCase(
76+
[](DiagnosticEngine &diags) {
77+
diags.setPrintDiagnosticNamesMode(PrintDiagnosticNamesMode::Group);
78+
79+
TestDiagnostic diagnostic(diag::error_immediate_mode_missing_stdlib.ID,
80+
DiagGroupID::no_group);
81+
diags.diagnose(SourceLoc(), diagnostic);
82+
},
83+
[](const DiagnosticInfo &info) {
84+
EXPECT_FALSE(info.FormatString.ends_with("]"));
85+
},
86+
/*expectedNumCallbackCalls=*/1);
87+
}
88+
89+
TEST(DiagnosticGroups, DiagnosticsWrappersInheritGroups) {
90+
// Test that we don't loose the group of a diagnostic when it gets wrapped in
91+
// another one.
92+
diagnosticGroupsTestCase(
93+
[](DiagnosticEngine &diags) {
94+
diags.setPrintDiagnosticNamesMode(PrintDiagnosticNamesMode::Group);
95+
96+
TestDiagnostic diagnostic(diag::error_immediate_mode_missing_stdlib.ID,
97+
DiagGroupID::DeprecatedDeclaration);
98+
diags.diagnose(SourceLoc(), diagnostic)
99+
.limitBehaviorUntilSwiftVersion(DiagnosticBehavior::Warning, 99);
100+
},
101+
[](const DiagnosticInfo &info) {
102+
EXPECT_EQ(info.ID, diag::error_in_a_future_swift_version.ID);
103+
EXPECT_TRUE(info.FormatString.ends_with(" [DeprecatedDeclaration]"));
104+
},
105+
/*expectedNumCallbackCalls=*/1);
106+
}
107+
108+
TEST(DiagnosticGroups, OverrideBehaviorLimitations) {
109+
// Test that escalating warnings to errors for *errors* in a diagnostic group
110+
// overrides emission site behavior limitations.
111+
{
112+
TestDiagnostic diagnostic(diag::error_immediate_mode_missing_stdlib.ID,
113+
DiagGroupID::DeprecatedDeclaration);
114+
115+
// Make sure ID actually is an error by default.
116+
diagnosticGroupsTestCase(
117+
[&diagnostic](DiagnosticEngine &diags) {
118+
diags.diagnose(SourceLoc(), diagnostic);
119+
},
120+
[](const DiagnosticInfo &info) {
121+
EXPECT_TRUE(info.Kind == DiagnosticKind::Error);
122+
},
123+
/*expectedNumCallbackCalls=*/1);
124+
125+
diagnosticGroupsTestCase(
126+
[&diagnostic](DiagnosticEngine &diags) {
127+
const std::vector rules = {WarningAsErrorRule(
128+
WarningAsErrorRule::Action::Enable, "DeprecatedDeclaration")};
129+
diags.setWarningsAsErrorsRules(rules);
130+
131+
diags.diagnose(SourceLoc(), diagnostic);
132+
diags.diagnose(SourceLoc(), diagnostic)
133+
.limitBehaviorUntilSwiftVersion(DiagnosticBehavior::Warning, 99);
134+
},
135+
[](const DiagnosticInfo &info) {
136+
EXPECT_TRUE(info.Kind == DiagnosticKind::Error);
137+
},
138+
/*expectedNumCallbackCalls=*/2);
139+
}
140+
141+
// Test that escalating warnings to errors for *warnings* in a diagnostic
142+
// group overrides emission site behavior limitations.
143+
{
144+
TestDiagnostic diagnostic(
145+
diag::warn_unsupported_module_interface_library_evolution.ID,
146+
DiagGroupID::DeprecatedDeclaration);
147+
148+
// Make sure ID actually is a warning by default.
149+
diagnosticGroupsTestCase(
150+
[&diagnostic](DiagnosticEngine &diags) {
151+
diags.diagnose(SourceLoc(), diagnostic);
152+
},
153+
[](const DiagnosticInfo &info) {
154+
EXPECT_TRUE(info.Kind == DiagnosticKind::Warning);
155+
},
156+
/*expectedNumCallbackCalls=*/1);
157+
158+
diagnosticGroupsTestCase(
159+
[&diagnostic](DiagnosticEngine &diags) {
160+
const std::vector rules = {WarningAsErrorRule(
161+
WarningAsErrorRule::Action::Enable, "DeprecatedDeclaration")};
162+
diags.setWarningsAsErrorsRules(rules);
163+
164+
diags.diagnose(SourceLoc(), diagnostic)
165+
.limitBehaviorUntilSwiftVersion(DiagnosticBehavior::Warning, 99);
166+
},
167+
[](const DiagnosticInfo &info) {
168+
EXPECT_TRUE(info.Kind == DiagnosticKind::Error);
169+
},
170+
/*expectedNumCallbackCalls=*/1);
171+
}
172+
}

0 commit comments

Comments
 (0)