Merge pull request #16526 from davidungar/compilation-failed

[Batch Mode] Emit a non-specific error for all primaries lacking specific errors when a batch has errors.

Author: David Ungar

include/swift/AST/DiagnosticConsumer.h

@@ -100,7 +100,7 @@ class DiagnosticConsumer {
                                 const DiagnosticInfo &Info) = 0;
 
   /// \returns true if an error occurred while finishing-up.
-  virtual bool finishProcessing() { return false; }
+  virtual bool finishProcessing(SourceManager &) { return false; }
 };
 
 /// \brief DiagnosticConsumer that discards all diagnostics.
@@ -140,11 +140,22 @@ class FileSpecificDiagnosticConsumer : public DiagnosticConsumer {
   /// All consumers owned by this FileSpecificDiagnosticConsumer.
   const SmallVector<ConsumerPair, 4> SubConsumers;
 
-  /// The DiagnosticConsumer may be empty if those diagnostics are not to be
-  /// emitted.
-  using ConsumersOrderedByRangeEntry =
-    std::pair<CharSourceRange, DiagnosticConsumer *>;
+public:
+  // The commented-out consts are there because the data does not change
+  // but the swap method gets called on this structure.
+  struct ConsumerSpecificInformation {
+    /*const*/ CharSourceRange range;
+    /// The DiagnosticConsumer may be empty if those diagnostics are not to be
+    /// emitted.
+    DiagnosticConsumer * /*const*/ consumer;
+    bool hasAnErrorBeenEmitted = false;
+
+    ConsumerSpecificInformation(const CharSourceRange range,
+                                DiagnosticConsumer *const consumer)
+        : range(range), consumer(consumer) {}
+  };
 
+private:
   /// The consumers owned by this FileSpecificDiagnosticConsumer, sorted by
   /// the end locations of each file so that a lookup by position can be done
   /// using binary search.
@@ -153,8 +164,8 @@ class FileSpecificDiagnosticConsumer : public DiagnosticConsumer {
   /// This allows diagnostics to be emitted before files are actually opened,
   /// as long as they don't have source locations.
   ///
-  /// \see #consumerForLocation
-  SmallVector<ConsumersOrderedByRangeEntry, 4> ConsumersOrderedByRange;
+  /// \see #consumerSpecificInformationForLocation
+  SmallVector<ConsumerSpecificInformation, 4> ConsumersOrderedByRange;
 
   /// Indicates which consumer to send Note diagnostics too.
   ///
@@ -163,7 +174,10 @@ class FileSpecificDiagnosticConsumer : public DiagnosticConsumer {
   ///
   /// If None, Note diagnostics are sent to every consumer.
   /// If null, diagnostics are suppressed.
-  Optional<DiagnosticConsumer *> ConsumerForSubsequentNotes = None;
+  Optional<ConsumerSpecificInformation *>
+      ConsumerSpecificInfoForSubsequentNotes = None;
+
+  bool HasAnErrorBeenConsumed = false;
 
 public:
   /// Takes ownership of the DiagnosticConsumers specified in \p consumers.
@@ -179,16 +193,22 @@ class FileSpecificDiagnosticConsumer : public DiagnosticConsumer {
                         ArrayRef<DiagnosticArgument> FormatArgs,
                         const DiagnosticInfo &Info) override;
 
-   bool finishProcessing() override;
+  bool finishProcessing(SourceManager &) override;
 
 private:
+  /// In batch mode, any error causes failure for all primary files, but
+  /// Xcode will only see an error for a particular primary in that primary's
+  /// serialized diagnostics file. So, emit errors for all other primaries here.
+  void addNonSpecificErrors(SourceManager &SM);
+
   void computeConsumersOrderedByRange(SourceManager &SM);
 
   /// Returns nullptr if diagnostic is to be suppressed,
   /// None if diagnostic is to be distributed to every consumer,
   /// a particular consumer if diagnostic goes there.
-  Optional<DiagnosticConsumer *> consumerForLocation(SourceManager &SM,
-                                                     SourceLoc loc) const;
+  Optional<ConsumerSpecificInformation *>
+  consumerSpecificInformationForLocation(SourceManager &SM,
+                                         SourceLoc loc) const;
 };
 
 } // end namespace swift

include/swift/AST/DiagnosticEngine.h

@@ -759,8 +759,8 @@ namespace swift {
 
     /// \returns true if any diagnostic consumer gave an error while invoking
     //// \c finishProcessing.
-    bool finishProcessing();
-    
+    bool finishProcessing(SourceManager &);
+
     /// \brief Format the given diagnostic text and place the result in the given
     /// buffer.
     static void formatDiagnosticText(
@@ -781,6 +781,9 @@ namespace swift {
     /// \brief Send all tentative diagnostics to all diagnostic consumers and
     /// delete them.
     void emitTentativeDiagnostics();
+
+  public:
+    static const char *diagnosticStringFor(const DiagID id);
   };
 
   /// \brief Represents a diagnostic transaction. While a transaction is

include/swift/AST/DiagnosticsFrontend.def

@@ -235,6 +235,8 @@ WARNING(cannot_assign_value_to_conditional_compilation_flag,none,
 ERROR(error_optimization_remark_pattern, none, "%0 in '%1'",
       (StringRef, StringRef))
 
+ERROR(error_compilation_stopped_by_errors_in_other_files,none, "compilation stopped by errors in other files", ())
+
 #ifndef DIAG_NO_UNDEF
 # if defined(DIAG)
 #  undef DIAG

lib/AST/DiagnosticConsumer.cpp

@@ -17,6 +17,7 @@
 #define DEBUG_TYPE "swift-ast"
 #include "swift/AST/DiagnosticConsumer.h"
 #include "swift/AST/DiagnosticEngine.h"
+#include "swift/AST/DiagnosticsFrontend.h"
 #include "swift/Basic/Defer.h"
 #include "swift/Basic/SourceManager.h"
 #include "llvm/ADT/STLExtras.h"
@@ -71,44 +72,38 @@ void FileSpecificDiagnosticConsumer::computeConsumersOrderedByRange(
     Optional<unsigned> bufferID = SM.getIDForBufferIdentifier(pair.first);
     assert(bufferID.hasValue() && "consumer registered for unknown file");
     CharSourceRange range = SM.getRangeForBuffer(bufferID.getValue());
-    ConsumersOrderedByRange.emplace_back(range, pair.second.get());
+    ConsumersOrderedByRange.emplace_back(
+        ConsumerSpecificInformation(range, pair.second.get()));
   }
 
   // Sort the "map" by buffer /end/ location, for use with std::lower_bound
   // later. (Sorting by start location would produce the same sort, since the
   // ranges must not be overlapping, but since we need to check end locations
   // later it's consistent to sort by that here.)
   std::sort(ConsumersOrderedByRange.begin(), ConsumersOrderedByRange.end(),
-            [](const ConsumersOrderedByRangeEntry &left,
-               const ConsumersOrderedByRangeEntry &right) -> bool {
-    auto compare = std::less<const char *>();
-    return compare(getRawLoc(left.first.getEnd()).getPointer(),
-                   getRawLoc(right.first.getEnd()).getPointer());
-  });
+            [](const ConsumerSpecificInformation &left,
+               const ConsumerSpecificInformation &right) -> bool {
+              auto compare = std::less<const char *>();
+              return compare(getRawLoc(left.range.getEnd()).getPointer(),
+                             getRawLoc(right.range.getEnd()).getPointer());
+            });
 
   // Check that the ranges are non-overlapping. If the files really are all
   // distinct, this should be trivially true, but if it's ever not we might end
   // up mis-filing diagnostics.
   assert(ConsumersOrderedByRange.end() ==
-           std::adjacent_find(ConsumersOrderedByRange.begin(),
-                              ConsumersOrderedByRange.end(),
-                              [](const ConsumersOrderedByRangeEntry &left,
-                                 const ConsumersOrderedByRangeEntry &right) {
-                                return left.first.overlaps(right.first);
-                              }) &&
+             std::adjacent_find(ConsumersOrderedByRange.begin(),
+                                ConsumersOrderedByRange.end(),
+                                [](const ConsumerSpecificInformation &left,
+                                   const ConsumerSpecificInformation &right) {
+                                  return left.range.overlaps(right.range);
+                                }) &&
          "overlapping ranges despite having distinct files");
 }
 
-Optional<DiagnosticConsumer *>
-FileSpecificDiagnosticConsumer::consumerForLocation(SourceManager &SM,
-                                                    SourceLoc loc) const {
-  // If there's only one consumer, we'll use it no matter what, because...
-  // - ...all diagnostics within the file will go to that consumer.
-  // - ...all diagnostics not within the file will not be claimed by any
-  //   consumer, and so will go to all (one) consumers.
-  if (SubConsumers.size() == 1)
-    return SubConsumers.front().second.get();
-
+Optional<FileSpecificDiagnosticConsumer::ConsumerSpecificInformation *>
+FileSpecificDiagnosticConsumer::consumerSpecificInformationForLocation(
+    SourceManager &SM, SourceLoc loc) const {
   // Diagnostics with invalid locations always go to every consumer.
   if (loc.isInvalid())
     return None;
@@ -139,20 +134,19 @@ FileSpecificDiagnosticConsumer::consumerForLocation(SourceManager &SM,
   // that /might/ contain 'loc'. Specifically, since the ranges are sorted
   // by end location, it's looking for the first range where the end location
   // is greater than or equal to 'loc'.
-  auto possiblyContainingRangeIter =
-      std::lower_bound(ConsumersOrderedByRange.begin(),
-                       ConsumersOrderedByRange.end(),
-                       loc,
-                       [](const ConsumersOrderedByRangeEntry &entry,
-                          SourceLoc loc) -> bool {
-    auto compare = std::less<const char *>();
-    return compare(getRawLoc(entry.first.getEnd()).getPointer(),
-                   getRawLoc(loc).getPointer());
-  });
+  const ConsumerSpecificInformation *possiblyContainingRangeIter =
+      std::lower_bound(
+          ConsumersOrderedByRange.begin(), ConsumersOrderedByRange.end(), loc,
+          [](const ConsumerSpecificInformation &entry, SourceLoc loc) -> bool {
+            auto compare = std::less<const char *>();
+            return compare(getRawLoc(entry.range.getEnd()).getPointer(),
+                           getRawLoc(loc).getPointer());
+          });
 
   if (possiblyContainingRangeIter != ConsumersOrderedByRange.end() &&
-      possiblyContainingRangeIter->first.contains(loc)) {
-    return possiblyContainingRangeIter->second;
+      possiblyContainingRangeIter->range.contains(loc)) {
+    return const_cast<ConsumerSpecificInformation *>(
+        possiblyContainingRangeIter);
   }
 
   return None;
@@ -163,41 +157,76 @@ void FileSpecificDiagnosticConsumer::handleDiagnostic(
     StringRef FormatString, ArrayRef<DiagnosticArgument> FormatArgs,
     const DiagnosticInfo &Info) {
 
-  Optional<DiagnosticConsumer *> specificConsumer;
+  HasAnErrorBeenConsumed |= Kind == DiagnosticKind::Error;
+
+  Optional<ConsumerSpecificInformation *> consumerSpecificInfo;
   switch (Kind) {
   case DiagnosticKind::Error:
   case DiagnosticKind::Warning:
   case DiagnosticKind::Remark:
-    specificConsumer = consumerForLocation(SM, Loc);
-    ConsumerForSubsequentNotes = specificConsumer;
+    consumerSpecificInfo = consumerSpecificInformationForLocation(SM, Loc);
+    ConsumerSpecificInfoForSubsequentNotes = consumerSpecificInfo;
     break;
   case DiagnosticKind::Note:
-    specificConsumer = ConsumerForSubsequentNotes;
+    consumerSpecificInfo = ConsumerSpecificInfoForSubsequentNotes;
     break;
   }
-
-  if (!specificConsumer.hasValue()) {
+  if (!consumerSpecificInfo.hasValue()) {
     for (auto &subConsumer : SubConsumers) {
       if (subConsumer.second) {
         subConsumer.second->handleDiagnostic(SM, Loc, Kind, FormatString,
                                              FormatArgs, Info);
       }
     }
-  } else if (DiagnosticConsumer *c = specificConsumer.getValue())
-    c->handleDiagnostic(SM, Loc, Kind, FormatString, FormatArgs, Info);
-  else
-    ; // Suppress non-primary diagnostic in batch mode.
+    return;
+  }
+  if (!consumerSpecificInfo.getValue()->consumer)
+    return; // Suppress non-primary diagnostic in batch mode.
+
+  consumerSpecificInfo.getValue()->consumer->handleDiagnostic(
+      SM, Loc, Kind, FormatString, FormatArgs, Info);
+  consumerSpecificInfo.getValue()->hasAnErrorBeenEmitted |=
+      Kind == DiagnosticKind::Error;
 }
 
-bool FileSpecificDiagnosticConsumer::finishProcessing() {
+bool FileSpecificDiagnosticConsumer::finishProcessing(SourceManager &SM) {
+  addNonSpecificErrors(SM);
+
   // Deliberately don't use std::any_of here because we don't want early-exit
   // behavior.
+
   bool hadError = false;
   for (auto &subConsumer : SubConsumers)
-    hadError |= subConsumer.second && subConsumer.second->finishProcessing();
+    hadError |= subConsumer.second && subConsumer.second->finishProcessing(SM);
   return hadError;
 }
 
+static void produceNonSpecificError(
+    FileSpecificDiagnosticConsumer::ConsumerSpecificInformation &info,
+    SourceManager &SM) {
+  Diagnostic diagnostic(
+      diag::error_compilation_stopped_by_errors_in_other_files);
+
+  // Stolen from DiagnosticEngine::emitDiagnostic
+  DiagnosticInfo Info;
+  Info.ID = diagnostic.getID();
+
+  info.consumer->handleDiagnostic(
+      SM, info.range.getStart(), DiagnosticKind::Error,
+      DiagnosticEngine::diagnosticStringFor(diagnostic.getID()), {}, Info);
+}
+
+void FileSpecificDiagnosticConsumer::addNonSpecificErrors(SourceManager &SM) {
+  if (!HasAnErrorBeenConsumed)
+    return;
+  for (auto &info : ConsumersOrderedByRange) {
+    if (!info.hasAnErrorBeenEmitted && info.consumer) {
+      produceNonSpecificError(info, SM);
+      info.hasAnErrorBeenEmitted = true;
+    }
+  }
+}
+
 void NullDiagnosticConsumer::handleDiagnostic(
     SourceManager &SM, SourceLoc Loc, DiagnosticKind Kind,
     StringRef FormatString, ArrayRef<DiagnosticArgument> FormatArgs,

lib/AST/DiagnosticEngine.cpp

@@ -251,10 +251,10 @@ bool DiagnosticEngine::isDiagnosticPointsToFirstBadToken(DiagID ID) const {
   return storedDiagnosticInfos[(unsigned) ID].pointsToFirstBadToken;
 }
 
-bool DiagnosticEngine::finishProcessing() {
+bool DiagnosticEngine::finishProcessing(SourceManager &SM) {
   bool hadError = false;
   for (auto &Consumer : Consumers) {
-    hadError |= Consumer->finishProcessing();
+    hadError |= Consumer->finishProcessing(SM);
   }
   return hadError;
 }
@@ -822,9 +822,11 @@ void DiagnosticEngine::emitDiagnostic(const Diagnostic &diagnostic) {
   Info.FixIts = diagnostic.getFixIts();
   for (auto &Consumer : Consumers) {
     Consumer->handleDiagnostic(SourceMgr, loc, toDiagnosticKind(behavior),
-                               diagnosticStrings[(unsigned)Info.ID],
-                               diagnostic.getArgs(),
-                               Info);
+                               diagnosticStringFor(Info.ID),
+                               diagnostic.getArgs(), Info);
   }
 }
 
+const char *DiagnosticEngine::diagnosticStringFor(const DiagID id) {
+  return diagnosticStrings[(unsigned)id];
+}

lib/Frontend/SerializedDiagnosticConsumer.cpp

@@ -140,7 +140,7 @@ class SerializedDiagnosticConsumer : public DiagnosticConsumer {
     assert(CalledFinishProcessing && "did not call finishProcessing()");
   }
 
-  bool finishProcessing() override {
+  bool finishProcessing(SourceManager &SM) override {
     assert(!CalledFinishProcessing &&
            "called finishProcessing() multiple times");
     CalledFinishProcessing = true;
@@ -160,8 +160,7 @@ class SerializedDiagnosticConsumer : public DiagnosticConsumer {
                                       llvm::sys::fs::F_None));
     if (EC) {
       // Create a temporary diagnostics engine to print the error to stderr.
-      SourceManager dummyMgr;
-      DiagnosticEngine DE(dummyMgr);
+      DiagnosticEngine DE(SM);
       PrintingDiagnosticConsumer PDC;
       DE.addConsumer(PDC);
       DE.diagnose(SourceLoc(), diag::cannot_open_serialized_file,

lib/FrontendTool/FrontendTool.cpp

@@ -404,7 +404,7 @@ class JSONFixitWriter
     }
   }
 
-  bool finishProcessing() override {
+  bool finishProcessing(SourceManager &) override {
     std::error_code EC;
     std::unique_ptr<llvm::raw_fd_ostream> OS;
     OS.reset(new llvm::raw_fd_ostream(FixitsOutputPath,
@@ -1651,7 +1651,7 @@ int swift::performFrontend(ArrayRef<const char *> Args,
 
   auto finishDiagProcessing = [&](int retValue) -> int {
     FinishDiagProcessingCheckRAII.CalledFinishDiagProcessing = true;
-    bool err = Instance->getDiags().finishProcessing();
+    bool err = Instance->getDiags().finishProcessing(Instance->getSourceMgr());
     return retValue ? retValue : err;
   };
 

test/Misc/serialized-diagnostics-batch-mode-nonprimary-errors.swift

@@ -0,0 +1,17 @@
+// Ensure that an error in a non-primary causes an error in the errorless primary.
+//
+// RUN: rm -f %t.*
+
+// RUN: not %target-swift-frontend -typecheck  -primary-file %s  -serialize-diagnostics-path %t.main.dia -primary-file %S/../Inputs/empty.swift  -serialize-diagnostics-path %t.empty.dia %S/Inputs/serialized-diagnostics-batch-mode-suppression-helper.swift  2> %t.stderr.txt
+// RUN: c-index-test -read-diagnostics %t.main.dia 2> %t.main.txt
+// RUN: c-index-test -read-diagnostics %t.empty.dia 2> %t.empty.txt
+
+// RUN: %FileCheck -check-prefix=NO-GENERAL-ERROR-OCCURRED %s <%t.main.txt
+// RUN: %FileCheck -check-prefix=GENERAL-ERROR-OCCURRED %s <%t.empty.txt
+// GENERAL-ERROR-OCCURRED: compilation stopped by errors in other files
+// NO-GENERAL-ERROR-OCCURRED-NOT: compilation stopped by errors in other files
+
+
+func test(x: SomeType) {
+    nonexistant()
+}