Merge pull request #16529 from jrose-apple/4.2-batch-mode-diagnostics

[4.2] Fixes for batch mode diagnostics in Xcode

rdar://problem/40032762

Author: jrose-apple

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,12 @@ 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.
+  Optional<DiagnosticConsumer *> consumerForLocation(SourceManager &SM,
+                                                     SourceLoc loc) const;
 };
 
 } // end namespace swift

include/swift/AST/DiagnosticsSema.def

@@ -1498,7 +1498,7 @@ ERROR(witness_argument_name_mismatch,none,
       "%select{method|initializer}0 %1 has different argument labels from those "
       "required by protocol %2 (%3)", (bool, DeclName, Type, DeclName))
 ERROR(witness_initializer_not_required,none,
-      "initializer requirement %0 can only be satisfied by a `required` "
+      "initializer requirement %0 can only be satisfied by a 'required' "
       "initializer in%select{| the definition of}1 non-final class %2",
       (DeclName, bool, Type))
 ERROR(witness_initializer_failability,none,
@@ -1520,12 +1520,12 @@ NOTE(witness_self_weaken_same_type,none,
       "consider weakening the same-type requirement %0 == %1 to a superclass "
       "requirement", (Type, Type))
 ERROR(witness_requires_dynamic_self,none,
-      "method %0 in non-final class %1 must return `Self` to conform to "
+      "method %0 in non-final class %1 must return 'Self' to conform to "
       "protocol %2",
       (DeclName, Type, Type))
 ERROR(witness_requires_class_implementation,none,
       "method %0 in non-final class %1 cannot be implemented in a "
-      "protocol extension because it returns `Self` and has associated type "
+      "protocol extension because it returns 'Self' and has associated type "
       "requirements",
       (DeclName, Type))
 ERROR(witness_not_accessible_proto,none,
@@ -1553,6 +1553,9 @@ ERROR(type_witness_objc_generic_parameter,none,
       "type %0 involving Objective-C type parameter%select{| %1}2 cannot be "
       "used for associated type %3 of protocol %4",
       (Type, Type, bool, DeclName, DeclName))
+NOTE(witness_fix_access,none,
+     "mark the %0 as '%select{%error|fileprivate|internal|public|%error}1' to "
+     "satisfy the requirement", (DescriptiveDeclKind, AccessLevel))
 
 ERROR(protocol_refine_access,none,
       "%select{protocol must be declared %select{"
@@ -3380,10 +3383,10 @@ WARNING(objc_inference_swift3_objc_derived,none,
         "deprecated", ())
 
 NOTE(objc_inference_swift3_addobjc,none,
-     "add `@objc` to continue exposing an Objective-C entry point (Swift 3 "
+     "add '@objc' to continue exposing an Objective-C entry point (Swift 3 "
      "behavior)", ())
 NOTE(objc_inference_swift3_addnonobjc,none,
-     "add `@nonobjc` to suppress the Objective-C entry point (Swift 4 "
+     "add '@nonobjc' to suppress the Objective-C entry point (Swift 4 "
      "behavior)", ())
 
 ERROR(objc_for_generic_class,none,
@@ -3778,9 +3781,6 @@ ERROR(availability_protocol_requires_version,
 NOTE(availability_protocol_requirement_here, none,
      "protocol requirement here", ())
 
-NOTE(availability_conformance_introduced_here, none,
-     "conformance introduced here", ())
-
 // This doesn't display as an availability diagnostic, but it's
 // implemented there and fires when these subscripts are marked
 // unavailable, so it seems appropriate to put it here.

include/swift/Frontend/FrontendInputsAndOutputs.h

@@ -103,6 +103,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:

lib/AST/ASTContext.cpp

@@ -2268,6 +2268,9 @@ bool swift::fixDeclarationName(InFlightDiagnostic &diag, ValueDecl *decl,
 bool swift::fixDeclarationObjCName(InFlightDiagnostic &diag, ValueDecl *decl,
                                    Optional<ObjCSelector> targetNameOpt,
                                    bool ignoreImpliedName) {
+  if (decl->isImplicit())
+    return false;
+
   // Subscripts cannot be renamed, so handle them directly.
   if (isa<SubscriptDecl>(decl)) {
     diag.fixItInsert(decl->getAttributeInsertionLoc(/*forModifier=*/false),

lib/AST/Decl.cpp

@@ -2095,6 +2095,9 @@ bool ValueDecl::canInferObjCFromRequirement(ValueDecl *requirement) {
 }
 
 SourceLoc ValueDecl::getAttributeInsertionLoc(bool forModifier) const {
+  if (isImplicit())
+    return SourceLoc();
+
   if (auto var = dyn_cast<VarDecl>(this)) {
     // [attrs] var ...
     // The attributes are part of the VarDecl, but the 'var' is part of the PBD.

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,15 +121,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);
@@ -155,15 +155,15 @@ FileSpecificDiagnosticConsumer::consumerForLocation(SourceManager &SM,
     return possiblyContainingRangeIter->second;
   }
 
-  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,23 +176,25 @@ 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() {
   // 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->finishProcessing();
+    hadError |= subConsumer.second && subConsumer.second->finishProcessing();
   return hadError;
 }
 

lib/Driver/Compilation.cpp

@@ -528,6 +528,11 @@ namespace driver {
                               ReturnCode);
         }
 
+        // See how ContinueBuildingAfterErrors gets set up in Driver.cpp for
+        // more info.
+        assert((Comp.ContinueBuildingAfterErrors || !Comp.EnableBatchMode) &&
+               "batch mode diagnostics require ContinueBuildingAfterErrors");
+
         return Comp.ContinueBuildingAfterErrors ?
           TaskFinishedResponse::ContinueExecution :
           TaskFinishedResponse::StopExecution;

lib/Driver/Driver.cpp

@@ -667,8 +667,6 @@ Driver::buildCompilation(const ToolChain &TC,
   }
 
   bool SaveTemps = ArgList->hasArg(options::OPT_save_temps);
-  bool ContinueBuildingAfterErrors =
-    ArgList->hasArg(options::OPT_continue_building_after_errors);
   bool ShowDriverTimeCompilation =
     ArgList->hasArg(options::OPT_driver_time_compilation);
 
@@ -703,6 +701,21 @@ Driver::buildCompilation(const ToolChain &TC,
   OI.CompilerMode = computeCompilerMode(*TranslatedArgList, Inputs, BatchMode);
   buildOutputInfo(TC, *TranslatedArgList, BatchMode, Inputs, OI);
 
+  // Note: Batch mode handling of serialized diagnostics requires that all
+  // batches get to run, in order to make sure that all diagnostics emitted
+  // during the compilation end up in at least one serialized diagnostic file.
+  // Therefore, treat batch mode as implying -continue-building-after-errors.
+  // (This behavior could be limited to only when serialized diagnostics are
+  // being emitted, but this seems more consistent and less surprising for
+  // users.)
+  // FIXME: We don't really need (or want) a full ContinueBuildingAfterErrors.
+  // If we fail to precompile a bridging header, for example, there's no need
+  // to go on to compilation of source files, and if compilation of source files
+  // fails, we shouldn't try to link. Instead, we'd want to let all jobs finish
+  // but not schedule any new ones.
+  const bool ContinueBuildingAfterErrors =
+      BatchMode || ArgList->hasArg(options::OPT_continue_building_after_errors);
+
   if (Diags.hadAnyError())
     return nullptr;
 

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;