optional pointer to consumer replaces NullDiagnosticConsumer placeholder

Author: David Ungar

include/swift/AST/DiagnosticConsumer.h

@@ -131,13 +131,17 @@ class FileSpecificDiagnosticConsumer : public DiagnosticConsumer {
   /// 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>>;
 
 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 @@ class FileSpecificDiagnosticConsumer : public DiagnosticConsumer {
   /// 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,15 @@ class FileSpecificDiagnosticConsumer : public DiagnosticConsumer {
 
 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.
+  ///
+  /// (For Xcode compatibility, batch mode must suppress diagnostics to
+  /// non-primaries.)
+  Optional<DiagnosticConsumer *> consumerForLocation(SourceManager &SM,
+                                                     SourceLoc loc) const;
 };
 
 } // end namespace swift

include/swift/Frontend/FrontendInputsAndOutputs.h

@@ -104,6 +104,10 @@ class FrontendInputsAndOutputs {
   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:
@@ -211,6 +215,10 @@ class FrontendInputsAndOutputs {
   bool forEachInputProducingSupplementaryOutput(
       llvm::function_ref<bool(const InputFile &)> fn) const;
 
+  /// If \p fn returns true, exit early and return true.
+  bool forEachInputNotProducingSupplementaryOutput(
+      llvm::function_ref<bool(const InputFile &)> fn) const;
+
   /// Assumes there is not more than one primary input file, if any.
   /// Otherwise, you would need to call getPrimarySpecificPathsForPrimary
   /// to tell it which primary input you wanted the outputs for.

lib/AST/DiagnosticConsumer.cpp

@@ -71,7 +71,8 @@ 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(range, pair.second ? pair.second.get()
+                                                            : nullptr);
   }
 
   // Sort the "map" by buffer /end/ location, for use with std::lower_bound
@@ -99,7 +100,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 +112,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,15 +122,15 @@ 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());
     if (!SM.getIDForBufferIdentifier(SubConsumers.begin()->first).hasValue()) {
       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);
@@ -152,18 +153,21 @@ FileSpecificDiagnosticConsumer::consumerForLocation(SourceManager &SM,
 
   if (possiblyContainingRangeIter != ConsumersOrderedByRange.end() &&
       possiblyContainingRangeIter->first.contains(loc)) {
-    return possiblyContainingRangeIter->second;
+    DiagnosticConsumer *consumerIfDiagnosticIsToBeEmitted =
+        possiblyContainingRangeIter->second;
+    return consumerIfDiagnosticIsToBeEmitted ? consumerIfDiagnosticIsToBeEmitted
+                                             : nullptr;
   }
 
-  return nullptr;
+  return None;
 }
 
 void FileSpecificDiagnosticConsumer::handleDiagnostic(
     SourceManager &SM, SourceLoc Loc, DiagnosticKind Kind,
     StringRef FormatString, ArrayRef<DiagnosticArgument> FormatArgs,
     const DiagnosticInfo &Info) {
 
-  DiagnosticConsumer *specificConsumer;
+  Optional<DiagnosticConsumer *> specificConsumer;
   switch (Kind) {
   case DiagnosticKind::Error:
   case DiagnosticKind::Warning:
@@ -176,14 +180,15 @@ 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);
     }
+  } else if (DiagnosticConsumer *c = specificConsumer.getValue())
+    c->handleDiagnostic(SM, Loc, Kind, FormatString, FormatArgs, Info);
+  else {
+    /// suppress non-primary diagnostic in batch mode
   }
 }
 

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");
@@ -364,6 +371,11 @@ bool FrontendInputsAndOutputs::forEachInputProducingSupplementaryOutput(
                             : hasInputs() ? fn(firstInput()) : false;
 }
 
+bool FrontendInputsAndOutputs::forEachInputNotProducingSupplementaryOutput(
+    llvm::function_ref<bool(const InputFile &)> fn) const {
+  return hasPrimaryInputs() ? forEachNonPrimaryInput(fn) : false;
+}
+
 bool FrontendInputsAndOutputs::hasSupplementaryOutputPath(
     llvm::function_ref<const std::string &(const SupplementaryOutputPaths &)>
         extractorFn) const {

lib/FrontendTool/FrontendTool.cpp

@@ -1527,6 +1527,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.forEachInputNotProducingSupplementaryOutput(
+        [&](const InputFile &input) -> bool {
+          subConsumers.emplace_back(input.file(), nullptr);
+          return false;
+        });
+  }
 
   if (subConsumers.empty())
     return nullptr;