[Diagnostics][NFC] Optionally add notes as explicit children of an error/warning

include/swift/AST/DiagnosticConsumer.h

@@ -38,10 +38,17 @@ enum class DiagnosticKind : uint8_t {
   Note
 };
 
-/// Extra information carried along with a diagnostic, which may or
-/// may not be of interest to a given diagnostic consumer.
+/// Information about a diagnostic passed to DiagnosticConsumers.
 struct DiagnosticInfo {
   DiagID ID = DiagID(0);
+  SourceLoc Loc;
+  DiagnosticKind Kind;
+  StringRef FormatString;
+  ArrayRef<DiagnosticArgument> FormatArgs;
+  SourceLoc BufferIndirectlyCausingDiagnostic;
+
+  /// DiagnosticInfo of notes which are children of this diagnostic, if any
+  ArrayRef<DiagnosticInfo *> ChildDiagnosticInfo;
 
   /// Represents a fix-it, a replacement of one range of text with another.
   class FixIt {
@@ -61,6 +68,24 @@ struct DiagnosticInfo {
 
   /// Extra source ranges that are attached to the diagnostic.
   ArrayRef<FixIt> FixIts;
+
+  /// This is a note which has a parent error or warning
+  bool IsChildNote = false;
+
+  DiagnosticInfo() {}
+
+  DiagnosticInfo(DiagID ID, SourceLoc Loc, DiagnosticKind Kind,
+                 StringRef FormatString,
+                 ArrayRef<DiagnosticArgument> FormatArgs,
+                 SourceLoc BufferIndirectlyCausingDiagnostic,
+                 ArrayRef<DiagnosticInfo *> ChildDiagnosticInfo,
+                 ArrayRef<CharSourceRange> Ranges, ArrayRef<FixIt> FixIts,
+                 bool IsChildNote)
+      : ID(ID), Loc(Loc), Kind(Kind), FormatString(FormatString),
+        FormatArgs(FormatArgs),
+        BufferIndirectlyCausingDiagnostic(BufferIndirectlyCausingDiagnostic),
+        ChildDiagnosticInfo(ChildDiagnosticInfo), Ranges(Ranges),
+        FixIts(FixIts), IsChildNote(IsChildNote) {}
 };
 
 /// Abstract interface for classes that present diagnostics to the user.

include/swift/AST/DiagnosticEngine.h

@@ -322,7 +322,9 @@ namespace swift {
     SmallVector<DiagnosticArgument, 3> Args;
     SmallVector<CharSourceRange, 2> Ranges;
     SmallVector<FixIt, 2> FixIts;
+    std::vector<Diagnostic> ChildNotes;
     SourceLoc Loc;
+    bool IsChildNote = false;
     const Decl *Decl = nullptr;
 
     friend DiagnosticEngine;
@@ -347,10 +349,13 @@ namespace swift {
     ArrayRef<DiagnosticArgument> getArgs() const { return Args; }
     ArrayRef<CharSourceRange> getRanges() const { return Ranges; }
     ArrayRef<FixIt> getFixIts() const { return FixIts; }
+    ArrayRef<Diagnostic> getChildNotes() const { return ChildNotes; }
+    bool isChildNote() const { return IsChildNote; }
     SourceLoc getLoc() const { return Loc; }
     const class Decl *getDecl() const { return Decl; }
 
     void setLoc(SourceLoc loc) { Loc = loc; }
+    void setIsChildNote(bool isChildNote) { IsChildNote = isChildNote; }
     void setDecl(const class Decl *decl) { Decl = decl; }
 
     /// Returns true if this object represents a particular diagnostic.
@@ -371,6 +376,8 @@ namespace swift {
     void addFixIt(FixIt &&F) {
       FixIts.push_back(std::move(F));
     }
+
+    void addChildNote(Diagnostic &&D);
   };
 
   /// Describes an in-flight diagnostic, which is currently active
@@ -589,7 +596,8 @@ namespace swift {
 
     friend class InFlightDiagnostic;
     friend class DiagnosticTransaction;
-
+    friend class CompoundDiagnosticTransaction;
+
   public:
     explicit DiagnosticEngine(SourceManager &SourceMgr)
         : SourceMgr(SourceMgr), ActiveDiagnostic(),
@@ -802,6 +810,15 @@ namespace swift {
       return diagnose(decl, Diagnostic(id, std::move(args)...));
     }
 
+    /// Emit a parent diagnostic and attached notes.
+    ///
+    /// \param parentDiag An InFlightDiagnostic representing the parent diag.
+    ///
+    /// \param builder A closure which builds and emits notes to be attached to
+    /// the parent diag.
+    void diagnoseWithNotes(InFlightDiagnostic parentDiag,
+                           llvm::function_ref<void(void)> builder);
+
     /// \returns true if diagnostic is marked with PointsToFirstBadToken
     /// option.
     bool isDiagnosticPointsToFirstBadToken(DiagID id) const;
@@ -829,6 +846,10 @@ namespace swift {
     /// Retrieve the active diagnostic.
     Diagnostic &getActiveDiagnostic() { return *ActiveDiagnostic; }
 
+    /// Generate DiagnosticInfo for a Diagnostic to be passed to consumers.
+    Optional<DiagnosticInfo>
+    diagnosticInfoForDiagnostic(const Diagnostic &diagnostic);
+
     /// Send \c diag to all diagnostic consumers.
     void emitDiagnostic(const Diagnostic &diag);
 
@@ -869,6 +890,7 @@ namespace swift {
   /// in LIFO order. An open transaction is implicitly committed upon
   /// destruction.
   class DiagnosticTransaction {
+  protected:
     DiagnosticEngine &Engine;
 
     /// How many tentative diagnostics there were when the transaction
@@ -892,7 +914,6 @@ namespace swift {
         Depth(Engine.TransactionCount),
         IsOpen(true)
     {
-      assert(!Engine.ActiveDiagnostic);
       Engine.TransactionCount++;
     }
 
@@ -935,6 +956,61 @@ namespace swift {
              "transactions must be closed LIFO");
     }
   };
+
+  /// Represents a diagnostic transaction which constructs a compound diagnostic
+  /// from any diagnostics emitted inside. A compound diagnostic consists of a
+  /// parent error, warning, or remark followed by a variable number of child
+  /// notes. The semantics are otherwise the same as a regular
+  /// DiagnosticTransaction.
+  class CompoundDiagnosticTransaction : public DiagnosticTransaction {
+  public:
+    explicit CompoundDiagnosticTransaction(DiagnosticEngine &engine)
+        : DiagnosticTransaction(engine) {}
+
+    ~CompoundDiagnosticTransaction() {
+      if (IsOpen) {
+        commit();
+      }
+
+      if (Depth == 0) {
+        Engine.TransactionStrings.clear();
+        Engine.TransactionAllocator.Reset();
+      }
+    }
+
+    void commit() {
+      assert(PrevDiagnostics < Engine.TentativeDiagnostics.size() &&
+             "CompoundDiagnosticTransaction must contain at least one diag");
+
+      // The first diagnostic is assumed to be the parent. If this is not an
+      // error or warning, we'll assert later when trying to add children.
+      Diagnostic &parent = Engine.TentativeDiagnostics[PrevDiagnostics];
+
+      // Associate the children with the parent.
+      for (auto diag =
+               Engine.TentativeDiagnostics.begin() + PrevDiagnostics + 1;
+           diag != Engine.TentativeDiagnostics.end(); ++diag) {
+        diag->setIsChildNote(true);
+        parent.addChildNote(std::move(*diag));
+      }
+
+      // Erase the children, they'll be emitted alongside their parent.
+      Engine.TentativeDiagnostics.erase(Engine.TentativeDiagnostics.begin() +
+                                            PrevDiagnostics + 1,
+                                        Engine.TentativeDiagnostics.end());
+
+      DiagnosticTransaction::commit();
+    }
+  };
+
+  inline void
+  DiagnosticEngine::diagnoseWithNotes(InFlightDiagnostic parentDiag,
+                                      llvm::function_ref<void(void)> builder) {
+    CompoundDiagnosticTransaction transaction(*this);
+    parentDiag.flush();
+    builder();
+  }
+
 } // end namespace swift
 
 #endif

include/swift/Frontend/PrintingDiagnosticConsumer.h

@@ -50,6 +50,13 @@ class PrintingDiagnosticConsumer : public DiagnosticConsumer {
   bool didErrorOccur() {
     return DidErrorOccur;
   }
+
+private:
+  void printDiagnostic(SourceManager &SM, SourceLoc Loc, DiagnosticKind Kind,
+                       StringRef FormatString,
+                       ArrayRef<DiagnosticArgument> FormatArgs,
+                       const DiagnosticInfo &Info,
+                       SourceLoc bufferIndirectlyCausingDiagnostic);
 };
 
 }

lib/AST/DiagnosticEngine.cpp

@@ -258,6 +258,14 @@ void InFlightDiagnostic::flush() {
     Engine->flushActiveDiagnostic();
 }
 
+void Diagnostic::addChildNote(Diagnostic &&D) {
+  assert(storedDiagnosticInfos[(unsigned)D.ID].kind == DiagnosticKind::Note &&
+         "Only notes can have a parent.");
+  assert(storedDiagnosticInfos[(unsigned)ID].kind != DiagnosticKind::Note &&
+         "Notes can't have children.");
+  ChildNotes.push_back(std::move(D));
+}
+
 bool DiagnosticEngine::isDiagnosticPointsToFirstBadToken(DiagID ID) const {
   return storedDiagnosticInfos[(unsigned) ID].pointsToFirstBadToken;
 }
@@ -776,10 +784,11 @@ void DiagnosticEngine::emitTentativeDiagnostics() {
   TentativeDiagnostics.clear();
 }
 
-void DiagnosticEngine::emitDiagnostic(const Diagnostic &diagnostic) {
+Optional<DiagnosticInfo>
+DiagnosticEngine::diagnosticInfoForDiagnostic(const Diagnostic &diagnostic) {
   auto behavior = state.determineBehavior(diagnostic.getID());
   if (behavior == DiagnosticState::Behavior::Ignore)
-    return;
+    return None;
 
   // Figure out the source location.
   SourceLoc loc = diagnostic.getLoc();
@@ -819,7 +828,7 @@ void DiagnosticEngine::emitDiagnostic(const Diagnostic &diagnostic) {
           // FIXME: Horrible, horrible hackaround. We're not getting a
           // DeclContext everywhere we should.
           if (!dc) {
-            return;
+            return None;
           }
 
           while (!dc->isModuleContext()) {
@@ -896,17 +905,37 @@ void DiagnosticEngine::emitDiagnostic(const Diagnostic &diagnostic) {
     }
   }
 
-  // Pass the diagnostic off to the consumer.
-  DiagnosticInfo Info;
-  Info.ID = diagnostic.getID();
-  Info.Ranges = diagnostic.getRanges();
-  Info.FixIts = diagnostic.getFixIts();
-  for (auto &Consumer : Consumers) {
-    Consumer->handleDiagnostic(
-        SourceMgr, loc, toDiagnosticKind(behavior),
-        diagnosticStringFor(Info.ID, getPrintDiagnosticNames()),
-        diagnostic.getArgs(), Info, getDefaultDiagnosticLoc());
+  return DiagnosticInfo(
+      diagnostic.getID(), loc, toDiagnosticKind(behavior),
+      diagnosticStringFor(diagnostic.getID(), getPrintDiagnosticNames()),
+      diagnostic.getArgs(), getDefaultDiagnosticLoc(), /*child note info*/ {},
+      diagnostic.getRanges(), diagnostic.getFixIts(), diagnostic.isChildNote());
+}
+
+void DiagnosticEngine::emitDiagnostic(const Diagnostic &diagnostic) {
+  if (auto info = diagnosticInfoForDiagnostic(diagnostic)) {
+    SmallVector<DiagnosticInfo, 1> childInfo;
+    TinyPtrVector<DiagnosticInfo *> childInfoPtrs;
+    auto childNotes = diagnostic.getChildNotes();
+    for (unsigned idx = 0; idx < childNotes.size(); ++idx) {
+      if (auto child = diagnosticInfoForDiagnostic(childNotes[idx])) {
+        childInfo.push_back(*child);
+        childInfoPtrs.push_back(&childInfo[idx]);
+      }
+    }
+    info->ChildDiagnosticInfo = childInfoPtrs;
+    for (auto &consumer : Consumers) {
+      consumer->handleDiagnostic(SourceMgr, info->Loc, info->Kind,
+                                 info->FormatString, info->FormatArgs, *info,
+                                 info->BufferIndirectlyCausingDiagnostic);
+    }
   }
+
+  // For compatibility with DiagnosticConsumers which don't know about child
+  // notes. These can be ignored by consumers which do take advantage of the
+  // grouping.
+  for (auto &childNote : diagnostic.getChildNotes())
+    emitDiagnostic(childNote);
 }
 
 const char *DiagnosticEngine::diagnosticStringFor(const DiagID id,

lib/Frontend/PrintingDiagnosticConsumer.cpp

@@ -68,6 +68,25 @@ void PrintingDiagnosticConsumer::handleDiagnostic(
     StringRef FormatString, ArrayRef<DiagnosticArgument> FormatArgs,
     const DiagnosticInfo &Info,
     const SourceLoc bufferIndirectlyCausingDiagnostic) {
+  if (Info.IsChildNote)
+    return;
+
+  printDiagnostic(SM, Loc, Kind, FormatString, FormatArgs, Info,
+                  bufferIndirectlyCausingDiagnostic);
+
+  for (auto ChildInfo : Info.ChildDiagnosticInfo) {
+    printDiagnostic(SM, ChildInfo->Loc, ChildInfo->Kind,
+                    ChildInfo->FormatString, ChildInfo->FormatArgs, *ChildInfo,
+                    ChildInfo->BufferIndirectlyCausingDiagnostic);
+  }
+}
+
+void PrintingDiagnosticConsumer::printDiagnostic(
+    SourceManager &SM, SourceLoc Loc, DiagnosticKind Kind,
+    StringRef FormatString, ArrayRef<DiagnosticArgument> FormatArgs,
+    const DiagnosticInfo &Info,
+    const SourceLoc bufferIndirectlyCausingDiagnostic) {
+
   // Determine what kind of diagnostic we're emitting.
   llvm::SourceMgr::DiagKind SMKind;
   switch (Kind) {

lib/Sema/CSApply.cpp

@@ -2506,15 +2506,19 @@ namespace {
             auto baseTyUnwrappedName = baseTyUnwrapped->getString();
             auto loc = DSCE->getLoc();
             auto startLoc = DSCE->getStartLoc();
-
-            tc.diagnose(loc, swift::diag::optional_ambiguous_case_ref,
-                        baseTyName, baseTyUnwrappedName, memberName.str());
-
-            tc.diagnose(loc, swift::diag::optional_fixit_ambiguous_case_ref)
-              .fixItInsert(startLoc, "Optional");
-            tc.diagnose(loc, swift::diag::type_fixit_optional_ambiguous_case_ref,
+            tc.diagnoseWithNotes(
+                tc.diagnose(loc, swift::diag::optional_ambiguous_case_ref,
+                            baseTyName, baseTyUnwrappedName, memberName.str()),
+                [&]() {
+                  tc.diagnose(loc,
+                              swift::diag::optional_fixit_ambiguous_case_ref)
+                      .fixItInsert(startLoc, "Optional");
+                  tc.diagnose(
+                        loc,
+                        swift::diag::type_fixit_optional_ambiguous_case_ref,
                         baseTyUnwrappedName, memberName.str())
-              .fixItInsert(startLoc, baseTyUnwrappedName);
+                      .fixItInsert(startLoc, baseTyUnwrappedName);
+                });
           }
         }
       }

lib/Sema/TypeCheckDecl.cpp

@@ -874,8 +874,10 @@ static void checkRedeclaration(TypeChecker &tc, ValueDecl *current) {
                         current->getFullName(),
                         otherInit->isMemberwiseInitializer());
         } else {
-          tc.diagnose(current, diag::invalid_redecl, current->getFullName());
-          tc.diagnose(other, diag::invalid_redecl_prev, other->getFullName());
+          tc.diagnoseWithNotes(tc.diagnose(current, diag::invalid_redecl,
+                                           current->getFullName()), [&]() {
+            tc.diagnose(other, diag::invalid_redecl_prev, other->getFullName());
+          });
         }
         markInvalid();
       }

lib/Sema/TypeChecker.h

@@ -723,6 +723,13 @@ class TypeChecker final : public LazyResolver {
     return Diags.diagnose(std::forward<ArgTypes>(Args)...);
   }
 
+  void diagnoseWithNotes(InFlightDiagnostic parentDiag,
+                         llvm::function_ref<void(void)> builder) {
+    CompoundDiagnosticTransaction transaction(Diags);
+    parentDiag.flush();
+    builder();
+  }
+
   static Type getArraySliceType(SourceLoc loc, Type elementType);
   static Type getDictionaryType(SourceLoc loc, Type keyType, Type valueType);
   static Type getOptionalType(SourceLoc loc, Type elementType);