Merge pull request #74199 from cachemeifyoucan/eng/PR-128876895

cachemeifyoucan · web-flow · commit fef4ee493579 · 2024-06-18T09:14:19.000-07:00
[ScanDependencies] Make sure `canImport` resolution agrees with `import`
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
@@ -3512,7 +3512,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
`@@ -3512,7 +3512,7 @@ ModuleDecl *ClangImporter::Implementation::tryLoadFoundationModule() {`
`3512`	`3512`	`bool ClangImporter::Implementation::canImportFoundationModule() {`
`3513`	`3513`	`ImportPath::Module::Builder builder(SwiftContext.Id_Foundation);`
`3514`	`3514`	`auto modulePath = builder.get();`
`3515`		`- return SwiftContext.canImportModule(modulePath);`
	`3515`	`+ return SwiftContext.canImportModule(modulePath, SourceLoc());`
`3516`	`3516`	`}`
`3517`	`3517`
`3518`	`3518`	`Type ClangImporter::Implementation::getNamedSwiftType(ModuleDecl *module,`