api-digester: Emit JSON errors during diagnose-sdk

The diagnostic consumers for swift-api-digester -diagnose-sdk were being set up after JSON files were read in, so errors in those files were silently dropped. In at least one case this led to a crash with no hint about what was happening. Set up the diagnostic consumer before doing any serious work to avoid this problem.

Fixing this required me to make `newCompilerInstance()` copy diagnostic consumers to the new instance’s diagnostic engine, since compiler instances were now being created after the consumers were added instead of before. I also had to make FilteringDiagnosticConsumer filter out clang warnings, remarks, and notes since those were now being emitted early enough to register in the digester.

Author: beccadax

include/swift/APIDigester/ModuleAnalyzerNodes.h

@@ -234,10 +234,7 @@ class SDKContext {
   ArrayRef<BreakingAttributeInfo> getBreakingAttributeInfo() const { return BreakingAttrs; }
   llvm::Optional<uint8_t> getFixedBinaryOrder(ValueDecl *VD) const;
 
-  CompilerInstance &newCompilerInstance() {
-    CIs.emplace_back(new CompilerInstance());
-    return *CIs.back();
-  }
+  CompilerInstance &newCompilerInstance();
   template<class YAMLNodeTy, typename ...ArgTypes>
   void diagnose(YAMLNodeTy node, Diag<ArgTypes...> ID,
                 typename detail::PassArgument<ArgTypes>::type... args) {

lib/APIDigester/ModuleAnalyzerNodes.cpp

@@ -89,6 +89,17 @@ void SDKContext::addDiagConsumer(DiagnosticConsumer &Consumer) {
   }
 }
 
+CompilerInstance &SDKContext::newCompilerInstance() {
+  CIs.emplace_back(new CompilerInstance());
+
+  // Add our existing diagnostic consumers to this new compiler instance.
+  // (Note that we do the opposite in addDiagConsumer().)
+  for (auto &consumer : Diags.getConsumers())
+    CIs.back()->addDiagnosticConsumer(consumer);
+
+  return *CIs.back();
+}
+
 void SDKNodeRoot::registerDescendant(SDKNode *D) {
   // Operator doesn't have usr
   if (isa<SDKNodeDeclOperator>(D))

lib/APIDigester/ModuleDiagsConsumer.cpp

@@ -16,6 +16,7 @@
 //===----------------------------------------------------------------------===//
 
 #include "swift/AST/DiagnosticEngine.h"
+#include "swift/AST/DiagnosticsClangImporter.h"
 #include "swift/AST/DiagnosticsModuleDiffer.h"
 #include "swift/APIDigester/ModuleDiagsConsumer.h"
 
@@ -145,6 +146,13 @@ bool swift::ide::api::FilteringDiagnosticConsumer::finishProcessing() {
 }
 
 bool swift::ide::api::FilteringDiagnosticConsumer::shouldProceed(const DiagnosticInfo &Info) {
+  // Suppress spurious warnings from imported headers. These were traditionally
+  // ignored because they were emitted before the diagnostic consumer was ready.
+  if (Info.ID == diag::warning_from_clang.ID ||
+      Info.ID == diag::remark_from_clang.ID ||
+      Info.ID == diag::note_from_clang.ID)
+    return false;
+
   if (allowedBreakages->empty()) {
     return true;
   }

lib/DriverTool/swift_api_digester_main.cpp

@@ -1915,42 +1915,20 @@ static bool readBreakageAllowlist(SDKContext &Ctx, llvm::StringSet<> &lines,
 }
 
 static int diagnoseModuleChange(SDKContext &Ctx, SDKNodeRoot *LeftModule,
-                                SDKNodeRoot *RightModule, StringRef OutputPath,
+                                SDKNodeRoot *RightModule,
                                 llvm::StringSet<> ProtocolReqAllowlist,
-                                bool DisableFailOnError,
-                                bool CompilerStyleDiags,
-                                StringRef SerializedDiagPath,
-                                StringRef BreakageAllowlistPath,
-                                bool DebugMapping) {
+                                bool DebugMapping,
+                                bool FailOnError,
+                                FilteringDiagnosticConsumer *pConsumer) {
   PrettyStackTraceSDKNodes trace(
        "diagnosing changes between modules", LeftModule, RightModule);
 
   assert(LeftModule);
   assert(RightModule);
-  llvm::raw_ostream *OS = &llvm::errs();
   if (!LeftModule || !RightModule) {
-    *OS << "Cannot diagnose null SDKNodeRoot";
+    llvm::errs() << "Cannot diagnose null SDKNodeRoot";
     exit(1);
   }
-  std::unique_ptr<llvm::raw_ostream> FileOS;
-  if (!OutputPath.empty()) {
-    std::error_code EC;
-    FileOS.reset(new llvm::raw_fd_ostream(OutputPath, EC, llvm::sys::fs::OF_None));
-    OS = FileOS.get();
-  }
-  bool FailOnError;
-  auto allowedBreakages = std::make_unique<llvm::StringSet<>>();
-  if (readBreakageAllowlist(Ctx, *allowedBreakages, BreakageAllowlistPath)) {
-    Ctx.getDiags().diagnose(SourceLoc(), diag::cannot_read_allowlist,
-                            BreakageAllowlistPath);
-  }
-  auto pConsumer = std::make_unique<FilteringDiagnosticConsumer>(
-      createDiagConsumer(*OS, FailOnError, DisableFailOnError, CompilerStyleDiags,
-                         SerializedDiagPath),
-      std::move(allowedBreakages),
-      /*DowngradeToWarning*/false);
-  SWIFT_DEFER { pConsumer->finishProcessing(); };
-  Ctx.addDiagConsumer(*pConsumer);
   Ctx.setCommonVersion(std::min(LeftModule->getJsonFormatVersion(),
                                 RightModule->getJsonFormatVersion()));
   TypeAliasDiffFinder(LeftModule, RightModule,
@@ -1965,14 +1943,12 @@ static int diagnoseModuleChange(SDKContext &Ctx, SDKNodeRoot *LeftModule,
   return FailOnError && pConsumer->hasError() ? 1 : 0;
 }
 
-static int diagnoseModuleChange(StringRef LeftPath, StringRef RightPath,
-                                StringRef OutputPath, CheckerOptions Opts,
+static int diagnoseModuleChange(SDKContext &Ctx, StringRef LeftPath,
+                                StringRef RightPath,
                                 llvm::StringSet<> ProtocolReqAllowlist,
-                                bool DisableFailOnError,
-                                bool CompilerStyleDiags,
-                                StringRef SerializedDiagPath,
-                                StringRef BreakageAllowlistPath,
-                                bool DebugMapping) {
+                                bool DebugMapping,
+                                bool FailOnError,
+                                FilteringDiagnosticConsumer *pConsumer) {
   if (!fs::exists(LeftPath)) {
     llvm::errs() << LeftPath << " does not exist\n";
     return 1;
@@ -1981,15 +1957,13 @@ static int diagnoseModuleChange(StringRef LeftPath, StringRef RightPath,
     llvm::errs() << RightPath << " does not exist\n";
     return 1;
   }
-  SDKContext Ctx(Opts);
   SwiftDeclCollector LeftCollector(Ctx);
   LeftCollector.deSerialize(LeftPath);
   SwiftDeclCollector RightCollector(Ctx);
   RightCollector.deSerialize(RightPath);
   return diagnoseModuleChange(
-      Ctx, LeftCollector.getSDKRoot(), RightCollector.getSDKRoot(), OutputPath,
-      std::move(ProtocolReqAllowlist), DisableFailOnError, CompilerStyleDiags, SerializedDiagPath,
-      BreakageAllowlistPath, DebugMapping);
+      Ctx, LeftCollector.getSDKRoot(), RightCollector.getSDKRoot(),
+      std::move(ProtocolReqAllowlist), DebugMapping, FailOnError, pConsumer);
 }
 
 static void populateAliasChanges(NodeMap &AliasMap, DiffVector &AllItems,
@@ -2579,26 +2553,46 @@ class SwiftAPIDigesterInvocation {
                                        OutputFile, IgnoredUsrs, CheckerOpts,
                                        OutputInJson, DebugMapping);
       }
+      SDKContext Ctx(CheckerOpts);
+
+      llvm::raw_ostream *OS = &llvm::errs();
+      std::unique_ptr<llvm::raw_ostream> FileOS;
+      if (!OutputFile.empty()) {
+        std::error_code EC;
+        FileOS.reset(new llvm::raw_fd_ostream(OutputFile, EC, llvm::sys::fs::OF_None));
+        OS = FileOS.get();
+      }
+      bool FailOnError;
+      auto allowedBreakages = std::make_unique<llvm::StringSet<>>();
+      if (readBreakageAllowlist(Ctx, *allowedBreakages, BreakageAllowlistPath)) {
+        Ctx.getDiags().diagnose(SourceLoc(), diag::cannot_read_allowlist,
+                                BreakageAllowlistPath);
+      }
+      auto pConsumer = std::make_unique<FilteringDiagnosticConsumer>(
+          createDiagConsumer(*OS, FailOnError, DisableFailOnError, CompilerStyleDiags,
+                             SerializedDiagPath),
+          std::move(allowedBreakages),
+          /*DowngradeToWarning*/false);
+      SWIFT_DEFER { pConsumer->finishProcessing(); };
+      Ctx.addDiagConsumer(*pConsumer);
+
       switch (Mode) {
       case ComparisonInputMode::BothJson: {
         return diagnoseModuleChange(
-            SDKJsonPaths[0], SDKJsonPaths[1], OutputFile, CheckerOpts,
-            std::move(protocolAllowlist), DisableFailOnError, CompilerStyleDiags,
-            SerializedDiagPath, BreakageAllowlistPath, DebugMapping);
+            Ctx, SDKJsonPaths[0], SDKJsonPaths[1], std::move(protocolAllowlist),
+            DebugMapping, FailOnError, pConsumer.get());
       }
       case ComparisonInputMode::BaselineJson: {
-        SDKContext Ctx(CheckerOpts);
         return diagnoseModuleChange(
-            Ctx, getBaselineFromJson(Ctx), getSDKRoot(Ctx, false), OutputFile,
-            std::move(protocolAllowlist), DisableFailOnError, CompilerStyleDiags,
-            SerializedDiagPath, BreakageAllowlistPath, DebugMapping);
+            Ctx, getBaselineFromJson(Ctx), getSDKRoot(Ctx, false),
+            std::move(protocolAllowlist), DebugMapping, FailOnError,
+            pConsumer.get());
       }
       case ComparisonInputMode::BothLoad: {
-        SDKContext Ctx(CheckerOpts);
         return diagnoseModuleChange(
-            Ctx, getSDKRoot(Ctx, true), getSDKRoot(Ctx, false), OutputFile,
-            std::move(protocolAllowlist), DisableFailOnError, CompilerStyleDiags,
-            SerializedDiagPath, BreakageAllowlistPath, DebugMapping);
+            Ctx, getSDKRoot(Ctx, true), getSDKRoot(Ctx, false),
+            std::move(protocolAllowlist), DebugMapping, FailOnError,
+            pConsumer.get());
       }
       }
     }

test/api-digester/diagnostics.swift

@@ -1,5 +1,6 @@
 // REQUIRES: OS=macosx
 // RUN: not %api-digester -deserialize-sdk -input-paths %S/diagnostics.json -o - 2>&1 | %FileCheck %s
+// RUN: not %api-digester -diagnose-sdk -input-paths %S/diagnostics.json -input-paths %S/diagnostics.json -compiler-style-diags -o - 2>&1 | %FileCheck %s
 
 // CHECK: diagnostics.json:5:3: error: unrecognized key 'badKey' in SDK node
 // CHECK: diagnostics.json:8:15: error: unrecognized SDK node kind 'Zyzyx'

validation-test/WholeSDK/api-digester.test

@@ -224,7 +224,7 @@ Social
 SoundAnalysis
 Speech
 SpriteKit
-// StoreKit - crash processing StoreKit.Transaction.environmentStringRepresentation in Swift CI
+StoreKit
 SwiftUI
 // SyncServices - error: module 'SyncServices' is incompatible with feature 'swift'
 SystemConfiguration