diff --git a/include/swift/AST/DiagnosticEngine.h b/include/swift/AST/DiagnosticEngine.h index 3319614217df7..50623747f267f 100644 --- a/include/swift/AST/DiagnosticEngine.h +++ b/include/swift/AST/DiagnosticEngine.h @@ -67,6 +67,8 @@ namespace swift { /// this enumeration type that uniquely identifies it. enum class DiagID : uint32_t; + enum class DiagGroupID : uint16_t; + /// Describes a diagnostic along with its argument types. /// /// The diagnostics header introduces instances of this type for each @@ -498,6 +500,7 @@ namespace swift { private: DiagID ID; + DiagGroupID GroupID; SmallVector Args; SmallVector Ranges; SmallVector FixIts; @@ -510,7 +513,16 @@ namespace swift { friend DiagnosticEngine; friend class InFlightDiagnostic; - Diagnostic(DiagID ID) : ID(ID) {} + /// Constructs a Diagnostic with DiagGroupID infered from DiagID. + Diagnostic(DiagID ID); + + protected: + /// Only use this constructor privately in this class or in unit tests by + /// subclassing. + /// In unit tests, it is used as a means for associating diagnostics with + /// groups and, thus, circumventing the need to otherwise define mock + /// diagnostics, which is not accounted for in the current design. + Diagnostic(DiagID ID, DiagGroupID GroupID) : ID(ID), GroupID(GroupID) {} public: // All constructors are intentionally implicit. @@ -522,8 +534,9 @@ namespace swift { gatherArgs(VArgs...); } - /*implicit*/Diagnostic(DiagID ID, ArrayRef Args) - : ID(ID), Args(Args.begin(), Args.end()) {} + Diagnostic(DiagID ID, ArrayRef Args) : Diagnostic(ID) { + this->Args.append(Args.begin(), Args.end()); + } template static Diagnostic fromTuple(Diag id, @@ -535,6 +548,7 @@ namespace swift { // Accessors. DiagID getID() const { return ID; } + DiagGroupID getGroupID() const { return GroupID; } ArrayRef getArgs() const { return Args; } ArrayRef getRanges() const { return Ranges; } ArrayRef getFixIts() const { return FixIts; } @@ -896,7 +910,9 @@ namespace swift { /// Don't emit any remarks bool suppressRemarks = false; - /// Treat these warnings as errors. Indices here correspond to DiagID enum + /// A mapping from `DiagGroupID` identifiers to Boolean values indicating + /// whether warnings belonging to the respective diagnostic groups should be + /// escalated to errors. llvm::BitVector warningsAsErrors; /// Whether a fatal error has occurred @@ -938,16 +954,23 @@ namespace swift { void setSuppressRemarks(bool val) { suppressRemarks = val; } bool getSuppressRemarks() const { return suppressRemarks; } - /// Whether a warning should be upgraded to an error or not - void setWarningAsErrorForDiagID(DiagID id, bool value) { + /// Sets whether warnings belonging to the diagnostic group identified by + /// `id` should be escalated to errors. + void setWarningsAsErrorsForDiagGroupID(DiagGroupID id, bool value) { warningsAsErrors[(unsigned)id] = value; } - bool getWarningAsErrorForDiagID(DiagID id) { + + /// Returns a Boolean value indicating whether warnings belonging to the + /// diagnostic group identified by `id` should be escalated to errors. + bool getWarningsAsErrorsForDiagGroupID(DiagGroupID id) { return warningsAsErrors[(unsigned)id]; } - /// Whether all warnings should be upgraded to errors or not + /// Whether all warnings should be upgraded to errors or not. void setAllWarningsAsErrors(bool value) { + // This works as intended because every diagnostic belongs to either a + // custom group or the top-level `DiagGroupID::no_group`, which is also + // a group. if (value) { warningsAsErrors.set(); } else { @@ -1434,7 +1457,8 @@ namespace swift { /// Generate DiagnosticInfo for a Diagnostic to be passed to consumers. std::optional - diagnosticInfoForDiagnostic(const Diagnostic &diagnostic); + diagnosticInfoForDiagnostic(const Diagnostic &diagnostic, + bool includeDiagnosticName); /// Send \c diag to all diagnostic consumers. void emitDiagnostic(const Diagnostic &diag); @@ -1460,9 +1484,16 @@ namespace swift { public: DiagnosticKind declaredDiagnosticKindFor(const DiagID id); - llvm::StringRef - diagnosticStringFor(const DiagID id, - PrintDiagnosticNamesMode printDiagnosticNamesMode); + /// Get a localized format string for a given `DiagID`. If no localization + /// available returns the default string for that `DiagID`. + llvm::StringRef diagnosticStringFor(DiagID id); + + /// Get a localized format string with an optional diagnostic name appended + /// to it. The diagnostic name type is defined by + /// `PrintDiagnosticNamesMode`. + llvm::StringRef diagnosticStringWithNameFor( + DiagID id, DiagGroupID groupID, + PrintDiagnosticNamesMode printDiagnosticNamesMode); static llvm::StringRef diagnosticIDStringFor(const DiagID id); diff --git a/include/swift/AST/DiagnosticGroups.h b/include/swift/AST/DiagnosticGroups.h index 744d4911594b0..ff8908c638546 100644 --- a/include/swift/AST/DiagnosticGroups.h +++ b/include/swift/AST/DiagnosticGroups.h @@ -33,7 +33,7 @@ enum class DiagGroupID : uint16_t { constexpr const auto DiagGroupsCount = [] { size_t count = 0; -#define GROUP(Name, Version) count++; +#define GROUP(Name, Version) ++count; #include "DiagnosticGroups.def" return count; }(); diff --git a/include/swift/AST/DiagnosticsCommon.def b/include/swift/AST/DiagnosticsCommon.def index 380608bd6c46a..f1ea5d7ca27fa 100644 --- a/include/swift/AST/DiagnosticsCommon.def +++ b/include/swift/AST/DiagnosticsCommon.def @@ -47,10 +47,14 @@ ERROR(error_no_group_info,none, NOTE(brace_stmt_suggest_do,none, "did you mean to use a 'do' statement?", ()) +// `error_in_future_swift_version` does not have a group because warnings of this kind +// inherit the group from the wrapped error. WARNING(error_in_future_swift_version,none, "%0; this is an error in the Swift %1 language mode", (DiagnosticInfo *, unsigned)) +// `error_in_a_future_swift_version` does not have a group because warnings of this kind +// inherit the group from the wrapped error. WARNING(error_in_a_future_swift_version,none, "%0; this will be an error in a future Swift language mode", (DiagnosticInfo *)) diff --git a/lib/AST/DiagnosticEngine.cpp b/lib/AST/DiagnosticEngine.cpp index f5bca58bd0c70..c840fec4ec337 100644 --- a/lib/AST/DiagnosticEngine.cpp +++ b/lib/AST/DiagnosticEngine.cpp @@ -176,9 +176,12 @@ DiagnosticState::DiagnosticState() { // Initialize our ignored diagnostics to default ignoredDiagnostics.resize(LocalDiagID::NumDiags); // Initialize warningsAsErrors to default - warningsAsErrors.resize(LocalDiagID::NumDiags); + warningsAsErrors.resize(DiagGroupsCount); } +Diagnostic::Diagnostic(DiagID ID) + : Diagnostic(ID, storedDiagnosticInfos[(unsigned)ID].groupID) {} + static CharSourceRange toCharSourceRange(SourceManager &SM, SourceRange SR) { return CharSourceRange(SM, SR.Start, Lexer::getLocForEndOfToken(SM, SR.End)); } @@ -464,8 +467,8 @@ InFlightDiagnostic::wrapIn(const Diagnostic &wrapper) { limit(Engine->getActiveDiagnostic().BehaviorLimit, DiagnosticBehavior::Unspecified); - Engine->WrappedDiagnostics.push_back( - *Engine->diagnosticInfoForDiagnostic(Engine->getActiveDiagnostic())); + Engine->WrappedDiagnostics.push_back(*Engine->diagnosticInfoForDiagnostic( + Engine->getActiveDiagnostic(), /* includeDiagnosticName= */ false)); Engine->state.swap(tempState); @@ -478,6 +481,7 @@ InFlightDiagnostic::wrapIn(const Diagnostic &wrapper) { // Overwrite the ID and argument with those from the wrapper. Engine->getActiveDiagnostic().ID = wrapper.ID; Engine->getActiveDiagnostic().Args = wrapper.Args; + // Intentionally keeping the original GroupID here // Set the argument to the diagnostic being wrapped. assert(wrapper.getArgs().front().getKind() == DiagnosticArgumentKind::Diagnostic); @@ -547,9 +551,7 @@ void DiagnosticEngine::setWarningsAsErrorsRules( if (auto groupID = getDiagGroupIDByName(name); groupID && *groupID != DiagGroupID::no_group) { getDiagGroupInfoByID(*groupID).traverseDepthFirst([&](auto group) { - for (DiagID diagID : group.diagnostics) { - state.setWarningAsErrorForDiagID(diagID, isEnabled); - } + state.setWarningsAsErrorsForDiagGroupID(*groupID, isEnabled); }); } else { unknownGroups.push_back(std::string(name)); @@ -1228,7 +1230,7 @@ DiagnosticBehavior DiagnosticState::determineBehavior(const Diagnostic &diag) { // 4) If the user substituted a different behavior for this behavior, apply // that change if (lvl == DiagnosticBehavior::Warning) { - if (getWarningAsErrorForDiagID(diag.getID())) + if (getWarningsAsErrorsForDiagGroupID(diag.getGroupID())) lvl = DiagnosticBehavior::Error; if (suppressWarnings) lvl = DiagnosticBehavior::Ignore; @@ -1294,7 +1296,8 @@ void DiagnosticEngine::forwardTentativeDiagnosticsTo( } std::optional -DiagnosticEngine::diagnosticInfoForDiagnostic(const Diagnostic &diagnostic) { +DiagnosticEngine::diagnosticInfoForDiagnostic(const Diagnostic &diagnostic, + bool includeDiagnosticName) { auto behavior = state.determineBehavior(diagnostic); if (behavior == DiagnosticBehavior::Ignore) return std::nullopt; @@ -1347,12 +1350,19 @@ DiagnosticEngine::diagnosticInfoForDiagnostic(const Diagnostic &diagnostic) { } } - return DiagnosticInfo( - diagnostic.getID(), loc, toDiagnosticKind(behavior), - diagnosticStringFor(diagnostic.getID(), getPrintDiagnosticNamesMode()), - diagnostic.getArgs(), Category, getDefaultDiagnosticLoc(), - /*child note info*/ {}, diagnostic.getRanges(), fixIts, - diagnostic.isChildNote()); + llvm::StringRef format; + if (includeDiagnosticName) + format = + diagnosticStringWithNameFor(diagnostic.getID(), diagnostic.getGroupID(), + getPrintDiagnosticNamesMode()); + else + format = diagnosticStringFor(diagnostic.getID()); + + return DiagnosticInfo(diagnostic.getID(), loc, toDiagnosticKind(behavior), + format, diagnostic.getArgs(), Category, + getDefaultDiagnosticLoc(), + /*child note info*/ {}, diagnostic.getRanges(), fixIts, + diagnostic.isChildNote()); } static DeclName @@ -1462,7 +1472,9 @@ void DiagnosticEngine::emitDiagnostic(const Diagnostic &diagnostic) { ArrayRef childNotes = diagnostic.getChildNotes(); std::vector extendedChildNotes; - if (auto info = diagnosticInfoForDiagnostic(diagnostic)) { + if (auto info = + diagnosticInfoForDiagnostic(diagnostic, + /* includeDiagnosticName= */ true)) { // If the diagnostic location is within a buffer containing generated // source code, add child notes showing where the generation occurred. // We need to avoid doing this if this is itself a child note, as otherwise @@ -1478,7 +1490,9 @@ void DiagnosticEngine::emitDiagnostic(const Diagnostic &diagnostic) { SmallVector childInfo; for (unsigned i : indices(childNotes)) { - auto child = diagnosticInfoForDiagnostic(childNotes[i]); + auto child = + diagnosticInfoForDiagnostic(childNotes[i], + /* includeDiagnosticName= */ true); assert(child); assert(child->Kind == DiagnosticKind::Note && "Expected child diagnostics to all be notes?!"); @@ -1516,12 +1530,18 @@ DiagnosticKind DiagnosticEngine::declaredDiagnosticKindFor(const DiagID id) { return storedDiagnosticInfos[(unsigned)id].kind; } -llvm::StringRef DiagnosticEngine::diagnosticStringFor( - const DiagID id, PrintDiagnosticNamesMode printDiagnosticNamesMode) { +llvm::StringRef DiagnosticEngine::diagnosticStringFor(DiagID id) { llvm::StringRef message = diagnosticStrings[(unsigned)id]; if (auto localizationProducer = localization.get()) { message = localizationProducer->getMessageOr(id, message); } + return message; +} + +llvm::StringRef DiagnosticEngine::diagnosticStringWithNameFor( + DiagID id, DiagGroupID groupID, + PrintDiagnosticNamesMode printDiagnosticNamesMode) { + auto message = diagnosticStringFor(id); auto formatMessageWithName = [&](StringRef message, StringRef name) { const int additionalCharsLength = 3; // ' ', '[', ']' std::string messageWithName; @@ -1540,7 +1560,6 @@ llvm::StringRef DiagnosticEngine::diagnosticStringFor( message = formatMessageWithName(message, diagnosticIDStringFor(id)); break; case PrintDiagnosticNamesMode::Group: - auto groupID = storedDiagnosticInfos[(unsigned)id].groupID; if (groupID != DiagGroupID::no_group) { message = formatMessageWithName(message, getDiagGroupInfoByID(groupID).name); diff --git a/lib/IDE/CodeCompletionDiagnostics.cpp b/lib/IDE/CodeCompletionDiagnostics.cpp index a399109dc35aa..1928245d963c6 100644 --- a/lib/IDE/CodeCompletionDiagnostics.cpp +++ b/lib/IDE/CodeCompletionDiagnostics.cpp @@ -67,7 +67,7 @@ bool CodeCompletionDiagnostics::getDiagnostics( typename swift::detail::PassArgument::type... VArgs) { DiagID id = ID.ID; std::vector DiagArgs{std::move(VArgs)...}; - auto format = Engine.diagnosticStringFor(id, PrintDiagnosticNamesMode::None); + auto format = Engine.diagnosticStringFor(id); DiagnosticEngine::formatDiagnosticText(Out, format, DiagArgs); severity = getSeverity(Engine.declaredDiagnosticKindFor(id)); diff --git a/lib/PrintAsClang/ModuleContentsWriter.cpp b/lib/PrintAsClang/ModuleContentsWriter.cpp index ec09f2ca97bd5..5deec8caf3346 100644 --- a/lib/PrintAsClang/ModuleContentsWriter.cpp +++ b/lib/PrintAsClang/ModuleContentsWriter.cpp @@ -977,8 +977,8 @@ class ModuleWriter { const_cast(vd)); // Emit a specific unavailable message when we know why a decl can't be // exposed, or a generic message otherwise. - auto diagString = M.getASTContext().Diags.diagnosticStringFor( - diag.getID(), PrintDiagnosticNamesMode::None); + auto diagString = + M.getASTContext().Diags.diagnosticStringFor(diag.getID()); DiagnosticEngine::formatDiagnosticText(os, diagString, diag.getArgs(), DiagnosticFormatOptions()); os << "\");\n"; diff --git a/unittests/AST/CMakeLists.txt b/unittests/AST/CMakeLists.txt index dcc91733edc6d..7e3b3305e8971 100644 --- a/unittests/AST/CMakeLists.txt +++ b/unittests/AST/CMakeLists.txt @@ -4,6 +4,7 @@ add_swift_unittest(SwiftASTTests ASTWalkerTests.cpp IndexSubsetTests.cpp DiagnosticConsumerTests.cpp + DiagnosticGroupsTests.cpp SourceLocTests.cpp TestContext.cpp TypeMatchTests.cpp diff --git a/unittests/AST/DiagnosticGroupsTests.cpp b/unittests/AST/DiagnosticGroupsTests.cpp new file mode 100644 index 0000000000000..395d9bb26797f --- /dev/null +++ b/unittests/AST/DiagnosticGroupsTests.cpp @@ -0,0 +1,198 @@ +//===--- DiagnosticGroupsTests.cpp ----------------------------------------===// +// +// This source file is part of the Swift.org open source project +// +// Copyright (c) 2025 Apple Inc. and the Swift project authors +// Licensed under Apache License v2.0 with Runtime Library Exception +// +// See https://swift.org/LICENSE.txt for license information +// See https://swift.org/CONTRIBUTORS.txt for the list of Swift project authors +// +//===----------------------------------------------------------------------===// +// +// NB: The correctness of the diagnostic group graph is verified in lib/AST +// ('namespace validation'). +// +//===----------------------------------------------------------------------===// + +#include "swift/AST/DiagnosticGroups.h" +#include "swift/AST/DiagnosticEngine.h" +#include "swift/AST/DiagnosticsFrontend.h" +#include "swift/Basic/SourceManager.h" +#include "gtest/gtest.h" + +using namespace swift; + +namespace { +class TestDiagnosticConsumer : public DiagnosticConsumer { + llvm::function_ref callback; + +public: + TestDiagnosticConsumer(decltype(callback) callback) : callback(callback) {} + + void handleDiagnostic(SourceManager &SM, + const DiagnosticInfo &Info) override { + this->callback(Info); + } +}; + +struct TestDiagnostic : public Diagnostic { + TestDiagnostic(DiagID ID, DiagGroupID GroupID) : Diagnostic(ID, GroupID) {} +}; +} // end anonymous namespace + +static void diagnosticGroupsTestCase( + llvm::function_ref diagnose, + llvm::function_ref callback, + unsigned expectedNumCallbackCalls) { + SourceManager sourceMgr; + DiagnosticEngine diags(sourceMgr); + + unsigned count = 0; + + const auto countingCallback = [&](const DiagnosticInfo &info) { + ++count; + callback(info); + }; + + TestDiagnosticConsumer consumer(countingCallback); + diags.addConsumer(consumer); + diagnose(diags); + diags.removeConsumer(consumer); + + EXPECT_EQ(count, expectedNumCallbackCalls); +} + +TEST(DiagnosticGroups, PrintDiagnosticGroups) { + // Test that we append the correct group in the format string. + diagnosticGroupsTestCase( + [](DiagnosticEngine &diags) { + diags.setPrintDiagnosticNamesMode(PrintDiagnosticNamesMode::Group); + + TestDiagnostic diagnostic(diag::error_immediate_mode_missing_stdlib.ID, + DiagGroupID::DeprecatedDeclaration); + diags.diagnose(SourceLoc(), diagnostic); + }, + [](const DiagnosticInfo &info) { + EXPECT_TRUE(info.FormatString.ends_with(" [DeprecatedDeclaration]")); + }, + /*expectedNumCallbackCalls=*/1); + + diagnosticGroupsTestCase( + [](DiagnosticEngine &diags) { + diags.setPrintDiagnosticNamesMode(PrintDiagnosticNamesMode::Group); + + TestDiagnostic diagnostic(diag::error_immediate_mode_missing_stdlib.ID, + DiagGroupID::no_group); + diags.diagnose(SourceLoc(), diagnostic); + }, + [](const DiagnosticInfo &info) { + EXPECT_FALSE(info.FormatString.ends_with("]")); + }, + /*expectedNumCallbackCalls=*/1); +} + +TEST(DiagnosticGroups, DiagnosticsWrappersInheritGroups) { + // Test that we don't loose the group of a diagnostic when it gets wrapped in + // another one. + diagnosticGroupsTestCase( + [](DiagnosticEngine &diags) { + diags.setPrintDiagnosticNamesMode(PrintDiagnosticNamesMode::Group); + + TestDiagnostic diagnostic(diag::error_immediate_mode_missing_stdlib.ID, + DiagGroupID::DeprecatedDeclaration); + diags.diagnose(SourceLoc(), diagnostic) + .limitBehaviorUntilSwiftVersion(DiagnosticBehavior::Warning, 99); + }, + [](const DiagnosticInfo &info) { + EXPECT_EQ(info.ID, diag::error_in_a_future_swift_version.ID); + EXPECT_TRUE(info.FormatString.ends_with(" [DeprecatedDeclaration]")); + }, + /*expectedNumCallbackCalls=*/1); +} + +TEST(DiagnosticGroups, TargetAll) { + // Test that uncategorized diagnostics are escalated when escalating all + // warnings. + diagnosticGroupsTestCase( + [](DiagnosticEngine &diags) { + const std::vector rules = { + WarningAsErrorRule(WarningAsErrorRule::Action::Enable)}; + diags.setWarningsAsErrorsRules(rules); + + TestDiagnostic diagnostic( + diag::warn_unsupported_module_interface_library_evolution.ID, + DiagGroupID::no_group); + diags.diagnose(SourceLoc(), diagnostic); + }, + [](const DiagnosticInfo &info) { + EXPECT_EQ(info.Kind, DiagnosticKind::Error); + }, + /*expectedNumCallbackCalls=*/1); +} + +TEST(DiagnosticGroups, OverrideBehaviorLimitations) { + // Test that escalating warnings to errors for *errors* in a diagnostic group + // overrides emission site behavior limitations. + { + TestDiagnostic diagnostic(diag::error_immediate_mode_missing_stdlib.ID, + DiagGroupID::DeprecatedDeclaration); + + // Make sure ID actually is an error by default. + diagnosticGroupsTestCase( + [&diagnostic](DiagnosticEngine &diags) { + diags.diagnose(SourceLoc(), diagnostic); + }, + [](const DiagnosticInfo &info) { + EXPECT_TRUE(info.Kind == DiagnosticKind::Error); + }, + /*expectedNumCallbackCalls=*/1); + + diagnosticGroupsTestCase( + [&diagnostic](DiagnosticEngine &diags) { + const std::vector rules = {WarningAsErrorRule( + WarningAsErrorRule::Action::Enable, "DeprecatedDeclaration")}; + diags.setWarningsAsErrorsRules(rules); + + diags.diagnose(SourceLoc(), diagnostic); + diags.diagnose(SourceLoc(), diagnostic) + .limitBehaviorUntilSwiftVersion(DiagnosticBehavior::Warning, 99); + }, + [](const DiagnosticInfo &info) { + EXPECT_EQ(info.Kind, DiagnosticKind::Error); + }, + /*expectedNumCallbackCalls=*/2); + } + + // Test that escalating warnings to errors for *warnings* in a diagnostic + // group overrides emission site behavior limitations. + { + TestDiagnostic diagnostic( + diag::warn_unsupported_module_interface_library_evolution.ID, + DiagGroupID::DeprecatedDeclaration); + + // Make sure ID actually is a warning by default. + diagnosticGroupsTestCase( + [&diagnostic](DiagnosticEngine &diags) { + diags.diagnose(SourceLoc(), diagnostic); + }, + [](const DiagnosticInfo &info) { + EXPECT_EQ(info.Kind, DiagnosticKind::Warning); + }, + /*expectedNumCallbackCalls=*/1); + + diagnosticGroupsTestCase( + [&diagnostic](DiagnosticEngine &diags) { + const std::vector rules = {WarningAsErrorRule( + WarningAsErrorRule::Action::Enable, "DeprecatedDeclaration")}; + diags.setWarningsAsErrorsRules(rules); + + diags.diagnose(SourceLoc(), diagnostic) + .limitBehaviorUntilSwiftVersion(DiagnosticBehavior::Warning, 99); + }, + [](const DiagnosticInfo &info) { + EXPECT_EQ(info.Kind, DiagnosticKind::Error); + }, + /*expectedNumCallbackCalls=*/1); + } +}