[ClangImporter] On failing a module lookup, reset Clang's DiagnosticEngine, in addition to filtering out the "module not found" diagnostic

Even though 'ClangDiagnosticConsumer' filters out 'clang::diag::err_module_not_found' from being emitted by the Swift compiler, delegating to Swift's module-loading logic for error-handling, we must also ensure that we reset Clang's 'DiagnosticEngine''s error count. Otherwise, if we do not, final stages of IRGen will query Clang's code-gen to finalize its 'Module', which Clang will not do if its internal DiagnosticEngine contains errors. This will cause Swift's IRGen to fail, even though the only error emitted was one Swift intended to suppress.

Author: artemcm

lib/ClangImporter/ClangDiagnosticConsumer.cpp

@@ -118,6 +118,7 @@ void ClangDiagnosticConsumer::HandleDiagnostic(
   // we're looking for.
   if (clangDiag.getID() == clang::diag::err_module_not_found &&
       CurrentImport && clangDiag.getArgStdStr(0) == CurrentImport->getName()) {
+    CurrentImportNotFound = true;
     return;
   }
 

lib/ClangImporter/ClangDiagnosticConsumer.h

@@ -23,13 +23,19 @@ namespace swift {
 class ClangDiagnosticConsumer : public clang::TextDiagnosticPrinter {
   struct LoadModuleRAII {
     ClangDiagnosticConsumer *Consumer;
+    clang::DiagnosticsEngine *Engine;
+    bool DiagEngineClearPriorToLookup;
+
   public:
     LoadModuleRAII(ClangDiagnosticConsumer &consumer,
+                   clang::DiagnosticsEngine &engine,
                    const clang::IdentifierInfo *import)
-        : Consumer(&consumer) {
+        : Consumer(&consumer), Engine(&engine) {
       assert(import);
       assert(!Consumer->CurrentImport);
+      assert(!Consumer->CurrentImportNotFound);
       Consumer->CurrentImport = import;
+      DiagEngineClearPriorToLookup = !engine.hasErrorOccurred();
     }
 
     LoadModuleRAII(LoadModuleRAII &) = delete;
@@ -45,8 +51,21 @@ class ClangDiagnosticConsumer : public clang::TextDiagnosticPrinter {
     }
 
     ~LoadModuleRAII() {
-      if (Consumer)
+      if (Consumer) {
+        // We must reset Clang's diagnostic engine here since we know that only
+        // the module lookup errors have been emitted. While the ClangDiagnosticConsumer
+        // takes care of filtering out the diagnostics from the output and from
+        // being handled by Swift's DiagnosticEngine, we must ensure to also
+        // reset Clang's DiagnosticEngine because its state is queries in later
+        // stages of compilation and errors emitted on "module_not_found" should not
+        // be counted.
+        // FIXME: We must instead allow for module loading in Clang to fail without
+        // needing to emit a diagnostic.
+        if (Engine && Consumer->CurrentImportNotFound && DiagEngineClearPriorToLookup)
+            Engine->Reset();
         Consumer->CurrentImport = nullptr;
+        Consumer->CurrentImportNotFound = false;
+      }
     }
   };
 
@@ -56,6 +75,7 @@ class ClangDiagnosticConsumer : public clang::TextDiagnosticPrinter {
   ClangImporter::Implementation &ImporterImpl;
 
   const clang::IdentifierInfo *CurrentImport = nullptr;
+  bool CurrentImportNotFound = false;
   SourceLoc DiagLoc;
   const bool DumpToStderr;
 
@@ -65,9 +85,10 @@ class ClangDiagnosticConsumer : public clang::TextDiagnosticPrinter {
                           bool dumpToStderr);
 
   LoadModuleRAII handleImport(const clang::IdentifierInfo *name,
+                              clang::DiagnosticsEngine &engine,
                               SourceLoc diagLoc) {
     DiagLoc = diagLoc;
-    return LoadModuleRAII(*this, name);
+    return LoadModuleRAII(*this, engine, name);
   }
 
   void HandleDiagnostic(clang::DiagnosticsEngine::Level diagLevel,

lib/ClangImporter/ClangImporter.cpp

@@ -2058,14 +2058,16 @@ ModuleDecl *ClangImporter::Implementation::loadModuleClang(
         exportSourceLoc(component.Loc));
   }
 
-  auto &rawDiagClient = Instance->getDiagnosticClient();
+  auto &diagEngine = Instance->getDiagnostics();
+  auto &rawDiagClient = *diagEngine.getClient();
   auto &diagClient = static_cast<ClangDiagnosticConsumer &>(rawDiagClient);
 
   auto loadModule = [&](clang::ModuleIdPath path,
                         clang::Module::NameVisibilityKind visibility)
       -> clang::ModuleLoadResult {
     auto importRAII =
-        diagClient.handleImport(clangPath.front().first, importLoc);
+        diagClient.handleImport(clangPath.front().first, diagEngine,
+                                importLoc);
 
     std::string preservedIndexStorePathOption;
     auto &clangFEOpts = Instance->getFrontendOpts();
@@ -2079,7 +2081,6 @@ ModuleDecl *ClangImporter::Implementation::loadModuleClang(
     }
 
     clang::SourceLocation clangImportLoc = getNextIncludeLoc();
-
     clang::ModuleLoadResult result =
         Instance->loadModule(clangImportLoc, path, visibility,
                              /*IsInclusionDirective=*/false);

test/ScanDependencies/optional_transitive_dep_load_fail.swift

@@ -0,0 +1,49 @@
+// RUN: %empty-directory(%t)
+// RUN: %empty-directory(%t/clang-module-cache)
+// RUN: %empty-directory(%t/Foo.swiftmodule)
+// RUN: %empty-directory(%t/inputs)
+// RUN: echo "@_implementationOnly import A; public func foo() {}" > %t/Foo.swift
+// REQUIRES: executable_test
+// REQUIRES: objc_interop
+
+// Create the explicit module inputs map
+// RUN: echo "[{" > %/t/inputs/map.json
+// RUN: echo "\"moduleName\": \"Foo\"," >> %/t/inputs/map.json
+// RUN: echo "\"modulePath\": \"%/t/Foo.swiftmodule/%target-swiftmodule-name\"," >> %/t/inputs/map.json
+// RUN: echo "\"isFramework\": false," >> %/t/inputs/map.json
+// RUN: echo "}," >> %/t/inputs/map.json
+// RUN: echo "{" >> %/t/inputs/map.json
+// RUN: echo "\"moduleName\": \"Swift\"," >> %/t/inputs/map.json
+// RUN: echo "\"modulePath\": \"%/stdlib_module\"," >> %/t/inputs/map.json
+// RUN: echo "\"isFramework\": false" >> %/t/inputs/map.json
+// RUN: echo "}," >> %/t/inputs/map.json
+// RUN: echo "{" >> %/t/inputs/map.json
+// RUN: echo "\"moduleName\": \"SwiftOnoneSupport\"," >> %/t/inputs/map.json
+// RUN: echo "\"modulePath\": \"%/ononesupport_module\"," >> %/t/inputs/map.json
+// RUN: echo "\"isFramework\": false" >> %/t/inputs/map.json
+// RUN: echo "}," >> %/t/inputs/map.json
+// RUN: echo "{" >> %/t/inputs/map.json
+// RUN: echo "\"moduleName\": \"SwiftShims\"," >> %/t/inputs/map.json
+// RUN: echo "\"isFramework\": false," >> %/t/inputs/map.json
+// RUN: echo "\"clangModuleMapPath\": \"%swift-lib-dir/swift/shims/module.modulemap\"," >> %/t/inputs/map.json
+// RUN: echo "\"clangModulePath\": \"%t/inputs/SwiftShims.pcm\"" >> %/t/inputs/map.json
+// RUN: echo "}]" >> %/t/inputs/map.json
+
+// Emit the shims module PCM for explicit loading
+// RUN: %target-swift-emit-pcm -module-name SwiftShims %swift-lib-dir/swift/shims/module.modulemap -o %t/inputs/SwiftShims.pcm
+
+@testable import Foo
+
+// Step 1: build Foo Swift module
+// RUN: %target-swift-frontend -emit-module %t/Foo.swift -emit-module-path %t/Foo.swiftmodule/%target-swiftmodule-name -module-name Foo -emit-module-interface-path %t/Foo.swiftmodule/%target-swiftinterface-name -enable-library-evolution -I %S/Inputs/CHeaders -I %S/Inputs/Swift -enable-testing -swift-version 5 -disable-implicit-concurrency-module-import -disable-implicit-string-processing-module-import
+
+// Step 2: scan dependencies and ensure the transitive dependency on "A" is misssing
+// RUN: %target-swift-frontend -scan-dependencies %s -o %t/deps.json -I %t -sdk %t -prebuilt-module-cache-path %t/clang-module-cache
+// RUN: %validate-json %t/deps.json | %FileCheck -check-prefix CHECK_SCAN %s
+// CHECK_SCAN-NOT: "swift": "A"
+
+// Step 3: Run an explicit module compile of this file ensuring object files are produced
+// RUN: %target-swift-frontend -emit-object -o %t/optional_transitive_dep_load_fail.o -disable-implicit-swift-modules -disable-implicit-concurrency-module-import -disable-implicit-string-processing-module-import -module-cache-path %t.module-cache -explicit-swift-module-map-file %t/inputs/map.json %s
+
+// Step 4: Ensure the resulting object file exists
+// RUN: ls %t/optional_transitive_dep_load_fail.o > /dev/null