Merge pull request swiftlang#68549 from artemcm/NoErrorOnMissingOptionalTranstiveDep

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

Author: swift-ci

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 queried 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