Merge pull request swiftlang#78517 from AnthonyLatsis/micronecta-scholtzi-6.1

[6.1] DiagnosticEngine: Fix escalation for wrapped diagnostics

Author: AnthonyLatsis

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,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<DiagnosticArgument> Args)
-      : ID(ID), Args(Args.begin(), Args.end()) {}
+    Diagnostic(DiagID ID, ArrayRef<DiagnosticArgument> Args) : Diagnostic(ID) {
+      this->Args.append(Args.begin(), Args.end());
+    }
 
     template <class... ArgTypes>
     static Diagnostic fromTuple(Diag<ArgTypes...> id,
@@ -535,6 +548,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; }
@@ -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<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 +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);
 

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;
 }();

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 *))

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<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 +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<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 +1490,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 +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);

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));
 

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";

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