[ScanDependencies] Make sure canImport resolution agrees with import

cachemeifyoucan · cachemeifyoucan · commit 7d85aa423de3 · 2024-06-17T14:14:48.000-07:00
Fix the problem that when the only module can be found is an
invalid/out-of-date swift binary module, canImport and import statement
can have different view for if the module can be imported or not.

Now canImport will evaluate to false if the only module can be found for
name is an invalid swiftmodule, with a warning with the path to the
module so users will not be surprised by such behavior.

rdar://128876895
diff --git a/include/swift/AST/ASTContext.h b/include/swift/AST/ASTContext.h
@@ -1109,7 +1109,7 @@ class ASTContext final {
   ///
   /// Note that even if this check succeeds, errors may still occur if the
   /// module is loaded in full.
-  bool canImportModuleImpl(ImportPath::Module ModulePath,
+  bool canImportModuleImpl(ImportPath::Module ModulePath, SourceLoc loc,
                            llvm::VersionTuple version, bool underlyingVersion,
                            bool updateFailingList,
                            llvm::VersionTuple &foundVersion) const;
@@ -1146,7 +1146,7 @@ class ASTContext final {
   ///
   /// Note that even if this check succeeds, errors may still occur if the
   /// module is loaded in full.
-  bool canImportModule(ImportPath::Module ModulePath,
+  bool canImportModule(ImportPath::Module ModulePath, SourceLoc loc,
                        llvm::VersionTuple version = llvm::VersionTuple(),
                        bool underlyingVersion = false);
 
diff --git a/include/swift/AST/DiagnosticsSema.def b/include/swift/AST/DiagnosticsSema.def
@@ -830,6 +830,9 @@ ERROR(serialization_failed,none,
       "serialization of module %0 failed due to the errors above",
       (const ModuleDecl *))
 
+WARNING(can_import_invalid_swiftmodule,none,
+        "canImport() evaluated to false due to invalid swiftmodule: %0", (StringRef))
+
 ERROR(serialization_load_failed,Fatal,
       "failed to load module '%0'", (StringRef))
 ERROR(module_interface_build_failed,Fatal,
diff --git a/include/swift/AST/ModuleLoader.h b/include/swift/AST/ModuleLoader.h
@@ -287,7 +287,7 @@ class ModuleLoader {
   ///
   /// If a non-null \p versionInfo is provided, the module version will be
   /// parsed and populated.
-  virtual bool canImportModule(ImportPath::Module named,
+  virtual bool canImportModule(ImportPath::Module named, SourceLoc loc,
                                ModuleVersionInfo *versionInfo,
                                bool isTestableImport = false) = 0;
 
diff --git a/include/swift/ClangImporter/ClangImporter.h b/include/swift/ClangImporter/ClangImporter.h
@@ -227,7 +227,7 @@ class ClangImporter final : public ClangModuleLoader {
   ///
   /// If a non-null \p versionInfo is provided, the module version will be
   /// parsed and populated.
-  virtual bool canImportModule(ImportPath::Module named,
+  virtual bool canImportModule(ImportPath::Module named, SourceLoc loc,
                                ModuleVersionInfo *versionInfo,
                                bool isTestableImport = false) override;
 
diff --git a/include/swift/Frontend/ModuleInterfaceLoader.h b/include/swift/Frontend/ModuleInterfaceLoader.h
@@ -163,7 +163,7 @@ class ExplicitSwiftModuleLoader: public SerializedModuleLoaderBase {
       bool SkipBuildingInterface, bool IsFramework,
       bool IsTestableDependencyLookup = false) override;
 
-  bool canImportModule(ImportPath::Module named,
+  bool canImportModule(ImportPath::Module named, SourceLoc loc,
                        ModuleVersionInfo *versionInfo,
                        bool isTestableDependencyLookup = false) override;
 
@@ -212,7 +212,7 @@ class ExplicitCASModuleLoader : public SerializedModuleLoaderBase {
       bool SkipBuildingInterface, bool IsFramework,
       bool IsTestableDependencyLookup = false) override;
 
-  bool canImportModule(ImportPath::Module named,
+  bool canImportModule(ImportPath::Module named, SourceLoc loc,
                        ModuleVersionInfo *versionInfo,
                        bool isTestableDependencyLookup = false) override;
 
diff --git a/include/swift/Sema/SourceLoader.h b/include/swift/Sema/SourceLoader.h
@@ -59,9 +59,10 @@ class SourceLoader : public ModuleLoader {
   ///
   /// If a non-null \p versionInfo is provided, the module version will be
   /// parsed and populated.
-  virtual bool canImportModule(ImportPath::Module named,
-                               ModuleVersionInfo *versionInfo,
-                               bool isTestableDependencyLookup = false) override;
+  virtual bool
+  canImportModule(ImportPath::Module named, SourceLoc loc,
+                  ModuleVersionInfo *versionInfo,
+                  bool isTestableDependencyLookup = false) override;
 
   /// Import a module with the given module path.
   ///
diff --git a/include/swift/Serialization/SerializedModuleLoader.h b/include/swift/Serialization/SerializedModuleLoader.h
@@ -209,9 +209,10 @@ class SerializedModuleLoaderBase : public ModuleLoader {
   ///
   /// If a non-null \p versionInfo is provided, the module version will be
   /// parsed and populated.
-  virtual bool canImportModule(ImportPath::Module named,
-                               ModuleVersionInfo *versionInfo,
-                               bool isTestableDependencyLookup = false) override;
+  virtual bool
+  canImportModule(ImportPath::Module named, SourceLoc loc,
+                  ModuleVersionInfo *versionInfo,
+                  bool isTestableDependencyLookup = false) override;
 
   /// Import a module with the given module path.
   ///
@@ -339,7 +340,7 @@ class MemoryBufferSerializedModuleLoader : public SerializedModuleLoaderBase {
 public:
   virtual ~MemoryBufferSerializedModuleLoader();
 
-  bool canImportModule(ImportPath::Module named,
+  bool canImportModule(ImportPath::Module named, SourceLoc loc,
                        ModuleVersionInfo *versionInfo,
                        bool isTestableDependencyLookup = false) override;
 
diff --git a/lib/AST/ASTContext.cpp b/lib/AST/ASTContext.cpp
@@ -2404,10 +2404,11 @@ void ASTContext::addSucceededCanImportModule(
   }
 }
 
-bool ASTContext::canImportModuleImpl(
-    ImportPath::Module ModuleName, llvm::VersionTuple version,
-    bool underlyingVersion, bool updateFailingList,
-    llvm::VersionTuple &foundVersion) const {
+bool ASTContext::canImportModuleImpl(ImportPath::Module ModuleName,
+                                     SourceLoc loc, llvm::VersionTuple version,
+                                     bool underlyingVersion,
+                                     bool updateFailingList,
+                                     llvm::VersionTuple &foundVersion) const {
   SmallString<64> FullModuleName;
   ModuleName.getString(FullModuleName);
   auto ModuleNameStr = FullModuleName.str().str();
@@ -2448,7 +2449,7 @@ bool ASTContext::canImportModuleImpl(
 
     // Otherwise, ask whether any module loader can load the module.
     for (auto &importer : getImpl().ModuleLoaders) {
-      if (importer->canImportModule(ModuleName, nullptr))
+      if (importer->canImportModule(ModuleName, loc, nullptr))
         return true;
     }
 
@@ -2465,7 +2466,7 @@ bool ASTContext::canImportModuleImpl(
   for (auto &importer : getImpl().ModuleLoaders) {
     ModuleLoader::ModuleVersionInfo versionInfo;
 
-    if (!importer->canImportModule(ModuleName, &versionInfo))
+    if (!importer->canImportModule(ModuleName, loc, &versionInfo))
       continue; // The loader can't find the module.
 
     if (!versionInfo.isValid())
@@ -2505,11 +2506,11 @@ void ASTContext::forEachCanImportVersionCheck(
     Callback(entry.first, entry.second.Version, entry.second.UnderlyingVersion);
 }
 
-bool ASTContext::canImportModule(ImportPath::Module moduleName,
+bool ASTContext::canImportModule(ImportPath::Module moduleName, SourceLoc loc,
                                  llvm::VersionTuple version,
                                  bool underlyingVersion) {
   llvm::VersionTuple versionInfo;
-  if (!canImportModuleImpl(moduleName, version, underlyingVersion, true,
+  if (!canImportModuleImpl(moduleName, loc, version, underlyingVersion, true,
                            versionInfo))
     return false;
 
@@ -2523,8 +2524,8 @@ bool ASTContext::testImportModule(ImportPath::Module ModuleName,
                                   llvm::VersionTuple version,
                                   bool underlyingVersion) const {
   llvm::VersionTuple versionInfo;
-  return canImportModuleImpl(ModuleName, version, underlyingVersion, false,
-                             versionInfo);
+  return canImportModuleImpl(ModuleName, SourceLoc(), version,
+                             underlyingVersion, false, versionInfo);
 }
 
 ModuleDecl *
diff --git a/lib/ClangImporter/ClangImporter.cpp b/lib/ClangImporter/ClangImporter.cpp
@@ -2147,6 +2147,7 @@ static llvm::VersionTuple getCurrentVersionFromTBD(llvm::vfs::FileSystem &FS,
 }
 
 bool ClangImporter::canImportModule(ImportPath::Module modulePath,
+                                    SourceLoc loc,
                                     ModuleVersionInfo *versionInfo,
                                     bool isTestableDependencyLookup) {
   // Look up the top-level module to see if it exists.
diff --git a/lib/ClangImporter/ImportType.cpp b/lib/ClangImporter/ImportType.cpp
@@ -3513,7 +3513,7 @@ ModuleDecl *ClangImporter::Implementation::tryLoadFoundationModule() {
 bool ClangImporter::Implementation::canImportFoundationModule() {
   ImportPath::Module::Builder builder(SwiftContext.Id_Foundation);
   auto modulePath = builder.get();
-  return SwiftContext.canImportModule(modulePath);
+  return SwiftContext.canImportModule(modulePath, SourceLoc());
 }
 
 Type ClangImporter::Implementation::getNamedSwiftType(ModuleDecl *module,
diff --git a/lib/Frontend/ModuleInterfaceLoader.cpp b/lib/Frontend/ModuleInterfaceLoader.cpp
@@ -2385,7 +2385,7 @@ std::error_code ExplicitSwiftModuleLoader::findModuleFilesInDirectory(
 }
 
 bool ExplicitSwiftModuleLoader::canImportModule(
-    ImportPath::Module path, ModuleVersionInfo *versionInfo,
+    ImportPath::Module path, SourceLoc loc, ModuleVersionInfo *versionInfo,
     bool isTestableDependencyLookup) {
   // FIXME: Swift submodules?
   if (path.hasSubmodule())
@@ -2411,7 +2411,7 @@ bool ExplicitSwiftModuleLoader::canImportModule(
   auto &fs = *Ctx.SourceMgr.getFileSystem();
   auto moduleBuf = fs.getBufferForFile(it->second.modulePath);
   if (!moduleBuf) {
-    Ctx.Diags.diagnose(SourceLoc(), diag::error_opening_explicit_module_file,
+    Ctx.Diags.diagnose(loc, diag::error_opening_explicit_module_file,
                        it->second.modulePath);
     return false;
   }
@@ -2422,8 +2422,7 @@ bool ExplicitSwiftModuleLoader::canImportModule(
     if (auto forwardingModule = ForwardingModule::load(**moduleBuf)) {
       moduleBuf = fs.getBufferForFile(forwardingModule->underlyingModulePath);
       if (!moduleBuf) {
-        Ctx.Diags.diagnose(SourceLoc(),
-                           diag::error_opening_explicit_module_file,
+        Ctx.Diags.diagnose(loc, diag::error_opening_explicit_module_file,
                            forwardingModule->underlyingModulePath);
         return false;
       }
@@ -2736,7 +2735,7 @@ std::error_code ExplicitCASModuleLoader::findModuleFilesInDirectory(
 }
 
 bool ExplicitCASModuleLoader::canImportModule(
-    ImportPath::Module path, ModuleVersionInfo *versionInfo,
+    ImportPath::Module path, SourceLoc loc, ModuleVersionInfo *versionInfo,
     bool isTestableDependencyLookup) {
   // FIXME: Swift submodules?
   if (path.hasSubmodule())
@@ -2765,12 +2764,11 @@ bool ExplicitCASModuleLoader::canImportModule(
                                 : it->second.modulePath;
   auto moduleBuf = Impl.loadFileBuffer(moduleCASID, it->second.modulePath);
   if (!moduleBuf) {
-    Ctx.Diags.diagnose(SourceLoc(), diag::error_cas,
-                       toString(moduleBuf.takeError()));
+    Ctx.Diags.diagnose(loc, diag::error_cas, toString(moduleBuf.takeError()));
     return false;
   }
   if (!*moduleBuf) {
-    Ctx.Diags.diagnose(SourceLoc(), diag::error_opening_explicit_module_file,
+    Ctx.Diags.diagnose(loc, diag::error_opening_explicit_module_file,
                        it->second.modulePath);
     return false;
   }
diff --git a/lib/Parse/ParseIfConfig.cpp b/lib/Parse/ParseIfConfig.cpp
@@ -603,7 +603,8 @@ class EvaluateIfConfigCondition :
       }
       ImportPath::Module::Builder builder(Ctx, Str, /*separator=*/'.',
                                           Arg->getStartLoc());
-      return Ctx.canImportModule(builder.get(), version, underlyingModule);
+      return Ctx.canImportModule(builder.get(), E->getLoc(), version,
+                                 underlyingModule);
     } else if (KindName == "hasFeature") {
       auto featureName = getDeclRefStr(Arg);
       return Ctx.LangOpts.hasFeature(featureName);
diff --git a/lib/Sema/SourceLoader.cpp b/lib/Sema/SourceLoader.cpp
@@ -71,7 +71,7 @@ void SourceLoader::collectVisibleTopLevelModuleNames(
   // TODO: Implement?
 }
 
-bool SourceLoader::canImportModule(ImportPath::Module path,
+bool SourceLoader::canImportModule(ImportPath::Module path, SourceLoc loc,
                                    ModuleVersionInfo *versionInfo,
                                    bool isTestableDependencyLookup) {
   // FIXME: Swift submodules?
diff --git a/lib/Serialization/ScanningLoaders.cpp b/lib/Serialization/ScanningLoaders.cpp
@@ -311,7 +311,7 @@ ModuleDependencyVector SerializedModuleLoaderBase::getModuleDependencies(
          "Expected PlaceholderSwiftModuleScanner as the first dependency "
          "scanner loader.");
   for (auto &scanner : scanners) {
-    if (scanner->canImportModule(modulePath, nullptr,
+    if (scanner->canImportModule(modulePath, SourceLoc(), nullptr,
                                  isTestableDependencyLookup)) {
 
       ModuleDependencyVector moduleDependnecies;
diff --git a/lib/Serialization/SerializedModuleLoader.cpp b/lib/Serialization/SerializedModuleLoader.cpp
@@ -33,6 +33,7 @@
 #include "llvm/ADT/StringSet.h"
 #include "llvm/Support/Debug.h"
 #include "llvm/Support/FileSystem.h"
+#include "llvm/Support/VersionTuple.h"
 #include "llvm/TargetParser/Host.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/Path.h"
@@ -1391,7 +1392,7 @@ swift::extractUserModuleVersionFromInterface(StringRef moduleInterfacePath) {
 }
 
 bool SerializedModuleLoaderBase::canImportModule(
-    ImportPath::Module path, ModuleVersionInfo *versionInfo,
+    ImportPath::Module path, SourceLoc loc, ModuleVersionInfo *versionInfo,
     bool isTestableDependencyLookup) {
   // FIXME: Swift submodules?
   if (path.hasSubmodule())
@@ -1414,36 +1415,55 @@ bool SerializedModuleLoaderBase::canImportModule(
   if (!found)
     return false;
 
-  // If the caller doesn't want version info we're done.
-  if (!versionInfo)
-    return true;
-
-  assert(found);
-  llvm::VersionTuple swiftInterfaceVersion;
   if (!moduleInterfaceSourcePath.empty()) {
-    swiftInterfaceVersion =
+    // If we found interface and version is not requested, we're done.
+    if (!versionInfo)
+      return true;
+
+    auto moduleVersion =
         extractUserModuleVersionFromInterface(moduleInterfaceSourcePath);
+
+    // If version is requested and found in interface, return the version.
+    // Otherwise fallback to binary module handling.
+    if (!moduleVersion.empty()) {
+      versionInfo->setVersion(moduleVersion,
+                              ModuleVersionSourceKind::SwiftInterface);
+      return true;
+    }
   }
 
-  // If failing to extract the user version from the interface file, try the
-  // binary module format, if present.
-  if (swiftInterfaceVersion.empty() && moduleInputBuffer) {
+  if (moduleInputBuffer) {
     auto metaData = serialization::validateSerializedAST(
-        moduleInputBuffer->getBuffer(),
-        Ctx.SILOpts.EnableOSSAModules,
+        moduleInputBuffer->getBuffer(), Ctx.SILOpts.EnableOSSAModules,
         Ctx.LangOpts.SDKName);
-    versionInfo->setVersion(metaData.userModuleVersion,
+
+    // If we only found binary module, make sure that is valid.
+    if (metaData.status != serialization::Status::Valid &&
+        moduleInterfaceSourcePath.empty()) {
+      // Emit warning if the canImport check location is known.
+      if (loc.isValid())
+        Ctx.Diags.diagnose(loc, diag::can_import_invalid_swiftmodule,
+                           moduleInputBuffer->getBufferIdentifier());
+
+      return false;
+    }
+
+    if (versionInfo)
+      versionInfo->setVersion(metaData.userModuleVersion,
+                              ModuleVersionSourceKind::SwiftBinaryModule);
+  }
+
+  if (versionInfo && !versionInfo->isValid()) {
+    // If no version is found, set it to empty version.
+    versionInfo->setVersion(llvm::VersionTuple(),
                             ModuleVersionSourceKind::SwiftBinaryModule);
-  } else {
-    versionInfo->setVersion(swiftInterfaceVersion,
-                            ModuleVersionSourceKind::SwiftInterface);
   }
 
   return true;
 }
 
 bool MemoryBufferSerializedModuleLoader::canImportModule(
-    ImportPath::Module path, ModuleVersionInfo *versionInfo,
+    ImportPath::Module path, SourceLoc loc, ModuleVersionInfo *versionInfo,
     bool isTestableDependencyLookup) {
   // FIXME: Swift submodules?
   if (path.hasSubmodule())
diff --git a/test/ScanDependencies/can_import_version_mismatch.swift b/test/ScanDependencies/can_import_version_mismatch.swift
@@ -0,0 +1,30 @@
+// RUN: %empty-directory(%t)
+// RUN: split-file %s %t
+// RUN: cp %S/../Serialization/Inputs/too-old/Library.swiftmodule %t/include/Library.swiftmodule
+
+/// No warning when there is a swiftinterface file even the binary module is invalid.
+// RUN: %target-swift-frontend -scan-dependencies -module-name Test %t/test.swift -module-cache-path %t/clang-module-cache \
+// RUN:   -disable-implicit-string-processing-module-import -disable-implicit-concurrency-module-import -parse-stdlib \
+// RUN:   -o %t/deps.json -I %t/include 2>&1 | %FileCheck %s --check-prefix=NOWARN --allow-empty
+
+// NOWARN-NOT: warning:
+
+/// Issue a warning when only found invalid binary module.
+// RUN: rm %t/include/Library.swiftinterface
+// RUN: %target-swift-frontend -scan-dependencies -module-name Test %t/test.swift -module-cache-path %t/clang-module-cache \
+// RUN:   -disable-implicit-string-processing-module-import -disable-implicit-concurrency-module-import -parse-stdlib \
+// RUN:   -o %t/deps.json -I %t/include 2>&1 | %FileCheck %s --check-prefix=WARN
+// WARN: warning: canImport() evaluated to false due to invalid swiftmodule
+
+// RUN: %FileCheck %s --check-prefix=DEP --input-file=%t/deps.json
+// DEP-NOT: Library
+
+//--- test.swift
+#if canImport(Library)
+import Library
+#endif
+
+//--- include/Library.swiftinterface
+// swift-interface-format-version: 1.0
+// swift-module-flags: -module-name Library -O -disable-implicit-string-processing-module-import -disable-implicit-concurrency-module-import -parse-stdlib -user-module-version 1.0
+public func foo() {}

Original file line number	Diff line number	Diff line change
`@@ -2147,6 +2147,7 @@ static llvm::VersionTuple getCurrentVersionFromTBD(llvm::vfs::FileSystem &FS,`
`2147`	`2147`	`}`
`2148`	`2148`
`2149`	`2149`	`bool ClangImporter::canImportModule(ImportPath::Module modulePath,`
	`2150`	`+ SourceLoc loc,`
`2150`	`2151`	`ModuleVersionInfo *versionInfo,`
`2151`	`2152`	`bool isTestableDependencyLookup) {`
`2152`	`2153`	`// Look up the top-level module to see if it exists.`
Original file line number	Diff line number	Diff line change
`@@ -3513,7 +3513,7 @@ ModuleDecl *ClangImporter::Implementation::tryLoadFoundationModule() {`
`3513`	`3513`	`bool ClangImporter::Implementation::canImportFoundationModule() {`
`3514`	`3514`	`ImportPath::Module::Builder builder(SwiftContext.Id_Foundation);`
`3515`	`3515`	`auto modulePath = builder.get();`
`3516`		`- return SwiftContext.canImportModule(modulePath);`
	`3516`	`+ return SwiftContext.canImportModule(modulePath, SourceLoc());`
`3517`	`3517`	`}`
`3518`	`3518`
`3519`	`3519`	`Type ClangImporter::Implementation::getNamedSwiftType(ModuleDecl *module,`