Merge pull request #16485 from davidungar/oddball-diags

[Batch Mode] Suppressing diagnostics for non-primaries - rdar://40032762

include/swift/AST/DiagnosticConsumer.h

@@ -131,6 +131,8 @@ public:
   /// be associated with. An empty string means that a consumer is not
   /// associated with any particular buffer, and should only receive diagnostics
   /// that are not in any of the other consumers' files.
+  /// A null pointer for the DiagnosticConsumer means that diagnostics for this
+  /// file should not be emitted.
   using ConsumerPair =
       std::pair<std::string, std::unique_ptr<DiagnosticConsumer>>;
 
@@ -138,6 +140,8 @@ private:
   /// 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 *>;
 
@@ -157,8 +161,9 @@ private:
   /// Notes are always considered attached to the error, warning, or remark
   /// that was most recently emitted.
   ///
-  /// If null, Note diagnostics are sent to every consumer.
-  DiagnosticConsumer *ConsumerForSubsequentNotes = nullptr;
+  /// If None, Note diagnostics are sent to every consumer.
+  /// If null, diagnostics are suppressed.
+  Optional<DiagnosticConsumer *> ConsumerForSubsequentNotes = None;
 
 public:
   /// Takes ownership of the DiagnosticConsumers specified in \p consumers.
@@ -178,8 +183,12 @@ public:
 
 private:
   void computeConsumersOrderedByRange(SourceManager &SM);
-  DiagnosticConsumer *consumerForLocation(SourceManager &SM,
-                                          SourceLoc loc) const;
+
+  /// 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;
 };
   
 } // end namespace swift

include/swift/Frontend/FrontendInputsAndOutputs.h

@@ -104,6 +104,10 @@ public:
   bool
   forEachPrimaryInput(llvm::function_ref<bool(const InputFile &)> fn) const;
 
+  /// If \p fn returns true, exit early and return true.
+  bool
+  forEachNonPrimaryInput(llvm::function_ref<bool(const InputFile &)> fn) const;
+
   unsigned primaryInputCount() const { return PrimaryInputsInOrder.size(); }
 
   // Primary count readers:

lib/AST/DiagnosticConsumer.cpp

@@ -99,7 +99,7 @@ void FileSpecificDiagnosticConsumer::computeConsumersOrderedByRange(
          "overlapping ranges despite having distinct files");
 }
 
-DiagnosticConsumer *
+Optional<DiagnosticConsumer *>
 FileSpecificDiagnosticConsumer::consumerForLocation(SourceManager &SM,
                                                     SourceLoc loc) const {
   // If there's only one consumer, we'll use it no matter what, because...
@@ -111,7 +111,7 @@ FileSpecificDiagnosticConsumer::consumerForLocation(SourceManager &SM,
 
   // Diagnostics with invalid locations always go to every consumer.
   if (loc.isInvalid())
-    return nullptr;
+    return None;
 
   // This map is generated on first use and cached, to allow the
   // FileSpecificDiagnosticConsumer to be set up before the source files are
@@ -121,7 +121,7 @@ FileSpecificDiagnosticConsumer::consumerForLocation(SourceManager &SM,
     // It's possible to get here while a bridging header PCH is being
     // attached-to, if there's some sort of AST-reader warning or error, which
     // happens before CompilerInstance::setUpInputs(), at which point _no_
-    // source buffers are loaded in yet. In that case we return nullptr, rather
+    // source buffers are loaded in yet. In that case we return None, rather
     // than trying to build a nonsensical map (and actually crashing since we
     // can't find buffers for the inputs).
     assert(!SubConsumers.empty());
@@ -129,7 +129,7 @@ FileSpecificDiagnosticConsumer::consumerForLocation(SourceManager &SM,
       assert(llvm::none_of(SubConsumers, [&](const ConsumerPair &pair) {
             return SM.getIDForBufferIdentifier(pair.first).hasValue();
           }));
-      return nullptr;
+      return None;
     }
     auto *mutableThis = const_cast<FileSpecificDiagnosticConsumer*>(this);
     mutableThis->computeConsumersOrderedByRange(SM);
@@ -155,7 +155,7 @@ FileSpecificDiagnosticConsumer::consumerForLocation(SourceManager &SM,
     return possiblyContainingRangeIter->second;
   }
 
-  return nullptr;
+  return None;
 }
 
 void FileSpecificDiagnosticConsumer::handleDiagnostic(
@@ -163,7 +163,7 @@ void FileSpecificDiagnosticConsumer::handleDiagnostic(
     StringRef FormatString, ArrayRef<DiagnosticArgument> FormatArgs,
     const DiagnosticInfo &Info) {
 
-  DiagnosticConsumer *specificConsumer;
+  Optional<DiagnosticConsumer *> specificConsumer;
   switch (Kind) {
   case DiagnosticKind::Error:
   case DiagnosticKind::Warning:
@@ -176,15 +176,17 @@ void FileSpecificDiagnosticConsumer::handleDiagnostic(
     break;
   }
 
-  if (specificConsumer) {
-    specificConsumer->handleDiagnostic(SM, Loc, Kind, FormatString, FormatArgs,
-                                       Info);
-  } else {
+  if (!specificConsumer.hasValue()) {
     for (auto &subConsumer : SubConsumers) {
-      subConsumer.second->handleDiagnostic(SM, Loc, Kind, FormatString,
-                                           FormatArgs, Info);
+      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.
 }
 
 bool FileSpecificDiagnosticConsumer::finishProcessing() {
@@ -192,7 +194,7 @@ bool FileSpecificDiagnosticConsumer::finishProcessing() {
   // behavior.
   bool hadError = false;
   for (auto &subConsumer : SubConsumers)
-    hadError |= subConsumer.second->finishProcessing();
+    hadError |= subConsumer.second && subConsumer.second->finishProcessing();
   return hadError;
 }
 

lib/Frontend/FrontendInputsAndOutputs.cpp

@@ -97,6 +97,13 @@ bool FrontendInputsAndOutputs::forEachPrimaryInput(
   return false;
 }
 
+bool FrontendInputsAndOutputs::forEachNonPrimaryInput(
+    llvm::function_ref<bool(const InputFile &)> fn) const {
+  return forEachInput([&](const InputFile &f) -> bool {
+    return f.isPrimary() ? false : fn(f);
+  });
+}
+
 void FrontendInputsAndOutputs::assertMustNotBeMoreThanOnePrimaryInput() const {
   assert(!hasMultiplePrimaryInputs() &&
          "have not implemented >1 primary input yet");

lib/FrontendTool/FrontendTool.cpp

@@ -948,9 +948,13 @@ static bool performCompile(CompilerInstance &Instance,
 
   // We've just been told to perform a typecheck, so we can return now.
   if (Action == FrontendOptions::ActionType::Typecheck) {
-    const bool hadPrintAsObjCError = printAsObjCIfNeeded(
-        Invocation.getObjCHeaderOutputPathForAtMostOnePrimary(),
-        Instance.getMainModule(), opts.ImplicitObjCHeaderPath, moduleIsPublic);
+    const bool hadPrintAsObjCError =
+        Invocation.getFrontendOptions()
+            .InputsAndOutputs.hasObjCHeaderOutputPath() &&
+        printAsObjCIfNeeded(
+            Invocation.getObjCHeaderOutputPathForAtMostOnePrimary(),
+            Instance.getMainModule(), opts.ImplicitObjCHeaderPath,
+            moduleIsPublic);
 
     const bool hadEmitIndexDataError = emitIndexData(Invocation, Instance);
 
@@ -1527,6 +1531,19 @@ createDispatchingDiagnosticConsumerIfNeeded(
       subConsumers.emplace_back(input.file(), std::move(subConsumer));
     return false;
   });
+  // For batch mode, the compiler must swallow diagnostics pertaining to
+  // non-primary files in order to avoid Xcode showing the same diagnostic
+  // multiple times. So, create a diagnostic "eater" for those non-primary
+  // files.
+  // To avoid introducing bugs into WMO or single-file modes, test for multiple
+  // primaries.
+  if (inputsAndOutputs.hasMultiplePrimaryInputs()) {
+    inputsAndOutputs.forEachNonPrimaryInput(
+        [&](const InputFile &input) -> bool {
+          subConsumers.emplace_back(input.file(), nullptr);
+          return false;
+        });
+  }
 
   if (subConsumers.empty())
     return nullptr;

test/Misc/Inputs/serialized-diagnostics-batch-mode-suppression-helper.swift

@@ -0,0 +1 @@
+typealias SomeType = NonExistentType

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

@@ -0,0 +1,26 @@
+// To avoid redundant diagnostics showing up in Xcode, batch-mode must suppress diagnostics in
+// non-primary files.
+//
+// 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
+
+// Ensure there was an error:
+
+// RUN: %FileCheck -check-prefix=ERROR %s <%t.stderr.txt
+// ERROR: error:
+
+// Ensure the error is not in the serialized diagnostics:
+
+// RUN: %FileCheck -check-prefix=NO-DIAGNOSTICS %s <%t.main.txt
+// RUN: %FileCheck -check-prefix=NO-DIAGNOSTICS %s <%t.empty.txt
+// NO-DIAGNOSTICS: Number of diagnostics: 0
+
+// RUN: %FileCheck -check-prefix=NO-ERROR %s <%t.main.txt
+// RUN: %FileCheck -check-prefix=NO-ERROR %s <%t.empty.txt
+// NO-ERROR-NOT: error:
+
+func test(x: SomeType) {
+}