DiagnosticEngine: Print the ID of the wrapped, not wrapper, diagnostic

include/swift/AST/DiagnosticEngine.h

@@ -1530,16 +1530,17 @@ namespace swift {
   public:
     DiagnosticKind declaredDiagnosticKindFor(const DiagID id);
 
-    /// 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);
+    /// Get a localized format string for the given `DiagID`. If no localization
+    /// is available, returns the default string.
+    llvm::StringRef getFormatStringForDiagnostic(DiagID id);
+
+    /// Get a localized format string for the given diagnostic. If no
+    /// localization is available, returns the default string.
+    ///
+    /// \param includeDiagnosticName Whether to at all consider embedding the
+    /// name of the diagnostic identifier or group, per the setting.
+    llvm::StringRef getFormatStringForDiagnostic(const Diagnostic &diagnostic,
+                                                 bool includeDiagnosticName);
 
     static llvm::StringRef diagnosticIDStringFor(const DiagID id);
 

lib/AST/DiagnosticEngine.cpp

@@ -1429,17 +1429,11 @@ DiagnosticEngine::diagnosticInfoForDiagnostic(const Diagnostic &diagnostic,
     }
   }
 
-  llvm::StringRef format;
-  if (includeDiagnosticName) {
-    format =
-        diagnosticStringWithNameFor(diagnostic.getID(), groupID,
-                                    getPrintDiagnosticNamesMode());
-  } else {
-    format = diagnosticStringFor(diagnostic.getID());
-  }
+  auto formatString =
+      getFormatStringForDiagnostic(diagnostic, includeDiagnosticName);
 
   return DiagnosticInfo(diagnostic.getID(), loc, toDiagnosticKind(behavior),
-                        format, diagnostic.getArgs(), Category,
+                        formatString, diagnostic.getArgs(), Category,
                         getDefaultDiagnosticLoc(),
                         /*child note info*/ {}, diagnostic.getRanges(), fixIts,
                         diagnostic.isChildNote());
@@ -1622,18 +1616,24 @@ DiagnosticKind DiagnosticEngine::declaredDiagnosticKindFor(const DiagID id) {
   return storedDiagnosticInfos[(unsigned)id].kind;
 }
 
-llvm::StringRef DiagnosticEngine::diagnosticStringFor(DiagID id) {
+llvm::StringRef DiagnosticEngine::getFormatStringForDiagnostic(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);
+llvm::StringRef
+DiagnosticEngine::getFormatStringForDiagnostic(const Diagnostic &diagnostic,
+                                               bool includeDiagnosticName) {
+  auto diagID = diagnostic.getID();
+  auto message = getFormatStringForDiagnostic(diagID);
+
+  if (!includeDiagnosticName) {
+    return message;
+  }
+
   auto formatMessageWithName = [&](StringRef message, StringRef name) {
     const int additionalCharsLength = 3; // ' ', '[', ']'
     std::string messageWithName;
@@ -1648,16 +1648,28 @@ llvm::StringRef DiagnosticEngine::diagnosticStringWithNameFor(
   switch (printDiagnosticNamesMode) {
   case PrintDiagnosticNamesMode::None:
     break;
-  case PrintDiagnosticNamesMode::Identifier:
-    message = formatMessageWithName(message, diagnosticIDStringFor(id));
+  case PrintDiagnosticNamesMode::Identifier: {
+    // If this diagnostic is a wrapper for another diagnostic, use the ID of
+    // the wrapped diagnostic.
+    for (const auto &arg : diagnostic.getArgs()) {
+      if (arg.getKind() == DiagnosticArgumentKind::Diagnostic) {
+        diagID = arg.getAsDiagnostic()->ID;
+        break;
+      }
+    }
+
+    message = formatMessageWithName(message, diagnosticIDStringFor(diagID));
     break;
+  }
   case PrintDiagnosticNamesMode::Group:
+    auto groupID = diagnostic.getGroupID();
     if (groupID != DiagGroupID::no_group) {
       message =
           formatMessageWithName(message, getDiagGroupInfoByID(groupID).name);
     }
     break;
   }
+
   return message;
 }
 

lib/IDE/CodeCompletionDiagnostics.cpp

@@ -74,7 +74,7 @@ bool CodeCompletionDiagnostics::getDiagnostics(
     typename swift::detail::PassArgument<ArgTypes>::type... VArgs) {
   DiagID id = ID.ID;
   std::vector<DiagnosticArgument> DiagArgs{std::move(VArgs)...};
-  auto format = Engine.diagnosticStringFor(id);
+  auto format = Engine.getFormatStringForDiagnostic(id);
   DiagnosticEngine::formatDiagnosticText(Out, format, DiagArgs);
   severity = getSeverity(Engine.declaredDiagnosticKindFor(id));
 

lib/PrintAsClang/ModuleContentsWriter.cpp

@@ -1086,7 +1086,7 @@ class ModuleWriter {
         // 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());
+            M.getASTContext().Diags.getFormatStringForDiagnostic(diag.getID());
         DiagnosticEngine::formatDiagnosticText(os, diagString, diag.getArgs(),
                                                DiagnosticFormatOptions());
         os << "\");\n";

unittests/AST/CMakeLists.txt

@@ -8,6 +8,7 @@ add_swift_unittest(SwiftASTTests
   DiagnosticBehaviorTests.cpp
   DiagnosticConsumerTests.cpp
   DiagnosticGroupsTests.cpp
+  DiagnosticInfoTests.cpp
   SourceLocTests.cpp
   TestContext.cpp
   TypeMatchTests.cpp

unittests/AST/DiagnosticBehaviorTests.cpp

@@ -29,13 +29,12 @@ class TestDiagnosticConsumer : public DiagnosticConsumer {
     this->callback(Info);
   }
 };
-} // end anonymous namespace
 
-static void diagnosticBehaviorTestCase(
-    llvm::function_ref<void(DiagnosticEngine &)> diagnose,
-    llvm::function_ref<void(DiagnosticEngine &, const DiagnosticInfo &)>
-        callback,
-    unsigned expectedNumCallbackCalls) {
+static void
+testCase(llvm::function_ref<void(DiagnosticEngine &)> diagnose,
+         llvm::function_ref<void(DiagnosticEngine &, const DiagnosticInfo &)>
+             callback,
+         unsigned expectedNumCallbackCalls) {
   SourceManager sourceMgr;
   DiagnosticEngine diags(sourceMgr);
 
@@ -55,7 +54,7 @@ static void diagnosticBehaviorTestCase(
 }
 
 TEST(DiagnosticBehavior, WarnUntilSwiftLangMode) {
-  diagnosticBehaviorTestCase(
+  testCase(
       [](DiagnosticEngine &diags) {
         diags.setLanguageVersion(version::Version({5}));
         diags.diagnose(SourceLoc(), diag::error_immediate_mode_missing_stdlib)
@@ -64,30 +63,30 @@ TEST(DiagnosticBehavior, WarnUntilSwiftLangMode) {
       [](DiagnosticEngine &diags, const DiagnosticInfo &info) {
         EXPECT_EQ(info.Kind, DiagnosticKind::Error);
         EXPECT_EQ(info.FormatString,
-                  diags.diagnosticStringFor(
+                  diags.getFormatStringForDiagnostic(
                       diag::error_immediate_mode_missing_stdlib.ID));
       },
       /*expectedNumCallbackCalls=*/1);
 
-  diagnosticBehaviorTestCase(
+  testCase(
       [](DiagnosticEngine &diags) {
         diags.setLanguageVersion(version::Version({4}));
         diags.diagnose(SourceLoc(), diag::error_immediate_mode_missing_stdlib)
             .warnUntilSwiftVersion(5);
       },
       [](DiagnosticEngine &diags, const DiagnosticInfo &info) {
         EXPECT_EQ(info.Kind, DiagnosticKind::Warning);
-        EXPECT_EQ(info.FormatString,
-                  diags.diagnosticStringFor(diag::error_in_swift_lang_mode.ID));
+        EXPECT_EQ(info.FormatString, diags.getFormatStringForDiagnostic(
+                                         diag::error_in_swift_lang_mode.ID));
 
         auto wrappedDiagInfo = info.FormatArgs.front().getAsDiagnostic();
         EXPECT_EQ(wrappedDiagInfo->FormatString,
-                  diags.diagnosticStringFor(
+                  diags.getFormatStringForDiagnostic(
                       diag::error_immediate_mode_missing_stdlib.ID));
       },
       /*expectedNumCallbackCalls=*/1);
 
-  diagnosticBehaviorTestCase(
+  testCase(
       [](DiagnosticEngine &diags) {
         diags.setLanguageVersion(version::Version({4}));
         diags.diagnose(SourceLoc(), diag::error_immediate_mode_missing_stdlib)
@@ -96,13 +95,15 @@ TEST(DiagnosticBehavior, WarnUntilSwiftLangMode) {
       [](DiagnosticEngine &diags, const DiagnosticInfo &info) {
         EXPECT_EQ(info.Kind, DiagnosticKind::Warning);
         EXPECT_EQ(info.FormatString,
-                  diags.diagnosticStringFor(
+                  diags.getFormatStringForDiagnostic(
                       diag::error_in_a_future_swift_lang_mode.ID));
 
         auto wrappedDiagInfo = info.FormatArgs.front().getAsDiagnostic();
         EXPECT_EQ(wrappedDiagInfo->FormatString,
-                  diags.diagnosticStringFor(
+                  diags.getFormatStringForDiagnostic(
                       diag::error_immediate_mode_missing_stdlib.ID));
       },
       /*expectedNumCallbackCalls=*/1);
 }
+
+} // end anonymous namespace

unittests/AST/DiagnosticGroupsTests.cpp

@@ -39,12 +39,10 @@ class TestDiagnosticConsumer : public DiagnosticConsumer {
 struct TestDiagnostic : public Diagnostic {
   TestDiagnostic(DiagID ID, DiagGroupID GroupID) : Diagnostic(ID, GroupID) {}
 };
-} // end anonymous namespace
 
-static void diagnosticGroupsTestCase(
-    llvm::function_ref<void(DiagnosticEngine &)> diagnose,
-    llvm::function_ref<void(const DiagnosticInfo &)> callback,
-    unsigned expectedNumCallbackCalls) {
+static void testCase(llvm::function_ref<void(DiagnosticEngine &)> diagnose,
+                     llvm::function_ref<void(const DiagnosticInfo &)> callback,
+                     unsigned expectedNumCallbackCalls) {
   SourceManager sourceMgr;
   DiagnosticEngine diags(sourceMgr);
 
@@ -63,58 +61,10 @@ static void diagnosticGroupsTestCase(
   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_lang_mode.ID);
-        EXPECT_TRUE(info.FormatString.ends_with(" [DeprecatedDeclaration]"));
-      },
-      /*expectedNumCallbackCalls=*/1);
-}
-
 TEST(DiagnosticGroups, TargetAll) {
   // Test that uncategorized diagnostics are escalated when escalating all
   // warnings.
-  diagnosticGroupsTestCase(
+  testCase(
       [](DiagnosticEngine &diags) {
         const std::vector rules = {
             WarningAsErrorRule(WarningAsErrorRule::Action::Enable)};
@@ -139,16 +89,16 @@ TEST(DiagnosticGroups, OverrideBehaviorLimitations) {
                               DiagGroupID::DeprecatedDeclaration);
 
     // Make sure ID actually is an error by default.
-    diagnosticGroupsTestCase(
+    testCase(
         [&diagnostic](DiagnosticEngine &diags) {
           diags.diagnose(SourceLoc(), diagnostic);
         },
         [](const DiagnosticInfo &info) {
-          EXPECT_TRUE(info.Kind == DiagnosticKind::Error);
+          EXPECT_EQ(info.Kind, DiagnosticKind::Error);
         },
         /*expectedNumCallbackCalls=*/1);
 
-    diagnosticGroupsTestCase(
+    testCase(
         [&diagnostic](DiagnosticEngine &diags) {
           const std::vector rules = {WarningAsErrorRule(
               WarningAsErrorRule::Action::Enable, "DeprecatedDeclaration")};
@@ -172,7 +122,7 @@ TEST(DiagnosticGroups, OverrideBehaviorLimitations) {
         DiagGroupID::DeprecatedDeclaration);
 
     // Make sure ID actually is a warning by default.
-    diagnosticGroupsTestCase(
+    testCase(
         [&diagnostic](DiagnosticEngine &diags) {
           diags.diagnose(SourceLoc(), diagnostic);
         },
@@ -181,7 +131,7 @@ TEST(DiagnosticGroups, OverrideBehaviorLimitations) {
         },
         /*expectedNumCallbackCalls=*/1);
 
-    diagnosticGroupsTestCase(
+    testCase(
         [&diagnostic](DiagnosticEngine &diags) {
           const std::vector rules = {WarningAsErrorRule(
               WarningAsErrorRule::Action::Enable, "DeprecatedDeclaration")};
@@ -196,3 +146,5 @@ TEST(DiagnosticGroups, OverrideBehaviorLimitations) {
         /*expectedNumCallbackCalls=*/1);
   }
 }
+
+} // end anonymous namespace