DiagnosticEngine: Add DiagGroupID to Diagnostic

DmT021 · AnthonyLatsis · commit 7934a16a6871 · 2025-01-09T14:30:33.000Z
This change addresses the following issue: when an error is being
wrapped in a warning, the diagnostic message will use the wrapper's
`DiagGroupID` as the warning's name. However, we want to retain the
original error's group for use. For example, in Swift 5,
`async_unavailable_decl` is wrapped in `error_in_future_swift_version`.
When we print a diagnostic of this kind, we want to keep the
`DiagGroupID` of `async_unavailable_decl`, not that of
`error_in_future_swift_version`.

To achieve this, we add `DiagGroupID` to the `Diagnostic` class. When an
active diagnostic is wrapped in `DiagnosticEngine`, we retain the
original `DiagGroupID`.
diff --git 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<DiagnosticArgument, 3> Args;
     SmallVector<CharSourceRange, 2> Ranges;
     SmallVector<FixIt, 2> FixIts;
@@ -510,7 +513,10 @@ namespace swift {
     friend DiagnosticEngine;
     friend class InFlightDiagnostic;
 
-    Diagnostic(DiagID ID) : ID(ID) {}
+    Diagnostic(DiagID ID, DiagGroupID GroupID) : ID(ID), GroupID(GroupID) {}
+
+    /// Constructs a Diagnostic with DiagGroupID infered from DiagID.
+    Diagnostic(DiagID ID);
 
   public:
     // All constructors are intentionally implicit.
@@ -535,6 +541,7 @@ namespace swift {
     
     // Accessors.
     DiagID getID() const { return ID; }
+    DiagGroupID getGroupID() const { return GroupID; }
     ArrayRef<DiagnosticArgument> getArgs() const { return Args; }
     ArrayRef<CharSourceRange> getRanges() const { return Ranges; }
     ArrayRef<FixIt> getFixIts() const { return FixIts; }
@@ -1434,7 +1441,8 @@ namespace swift {
 
     /// Generate DiagnosticInfo for a Diagnostic to be passed to consumers.
     std::optional<DiagnosticInfo>
-    diagnosticInfoForDiagnostic(const Diagnostic &diagnostic);
+    diagnosticInfoForDiagnostic(const Diagnostic &diagnostic,
+                                bool includeDiagnosticName);
 
     /// Send \c diag to all diagnostic consumers.
     void emitDiagnostic(const Diagnostic &diag);
@@ -1460,9 +1468,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/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
@@ -179,6 +179,9 @@ DiagnosticState::DiagnosticState() {
   warningsAsErrors.resize(LocalDiagID::NumDiags);
 }
 
+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);
@@ -1294,7 +1298,8 @@ void DiagnosticEngine::forwardTentativeDiagnosticsTo(
 }
 
 std::optional<DiagnosticInfo>
-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 +1352,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 +1474,9 @@ void DiagnosticEngine::emitDiagnostic(const Diagnostic &diagnostic) {
   ArrayRef<Diagnostic> childNotes = diagnostic.getChildNotes();
   std::vector<Diagnostic> 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 +1492,9 @@ void DiagnosticEngine::emitDiagnostic(const Diagnostic &diagnostic) {
 
     SmallVector<DiagnosticInfo, 1> 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 +1532,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 +1562,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
@@ -67,7 +67,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, 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
@@ -977,8 +977,8 @@ class ModuleWriter {
                       const_cast<ValueDecl *>(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";
