From 59cf32fa342a3e82c69d5f39deb12088f920b30d Mon Sep 17 00:00:00 2001 From: Steven Wu Date: Thu, 6 Jun 2024 19:43:02 -0700 Subject: [PATCH] [ScanDependencies] Make sure `canImport` resolution agrees with `import` 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 (cherry picked from commit 7d85aa423de39e4a8fa3b2dd5fea0a4a03361df0) --- include/swift/AST/ASTContext.h | 4 +- include/swift/AST/DiagnosticsSema.def | 3 + include/swift/AST/ModuleLoader.h | 2 +- include/swift/ClangImporter/ClangImporter.h | 2 +- .../swift/Frontend/ModuleInterfaceLoader.h | 4 +- include/swift/Sema/SourceLoader.h | 7 ++- .../Serialization/SerializedModuleLoader.h | 9 +-- lib/AST/ASTContext.cpp | 21 +++---- lib/ClangImporter/ClangImporter.cpp | 1 + lib/ClangImporter/ImportType.cpp | 2 +- lib/Frontend/ModuleInterfaceLoader.cpp | 14 ++--- lib/Parse/ParseIfConfig.cpp | 3 +- lib/Sema/SourceLoader.cpp | 2 +- lib/Serialization/ScanningLoaders.cpp | 2 +- lib/Serialization/SerializedModuleLoader.cpp | 56 +++++++++++++------ .../can_import_version_mismatch.swift | 30 ++++++++++ 16 files changed, 109 insertions(+), 53 deletions(-) create mode 100644 test/ScanDependencies/can_import_version_mismatch.swift diff --git a/include/swift/AST/ASTContext.h b/include/swift/AST/ASTContext.h index a435ceff672bc..f0622adfcd0cf 100644 --- 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 index 1d1f2acb4856f..9478298bc22ac 100644 --- a/include/swift/AST/DiagnosticsSema.def +++ b/include/swift/AST/DiagnosticsSema.def @@ -827,6 +827,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 index 1e4f234abeb20..f811454e70f69 100644 --- 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 index 83f68ed05c1f7..1b1350c7d12ad 100644 --- 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 index 8b2c6449f7004..4f82b217ae3e6 100644 --- 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 index dd27fa0c1d09a..d899a011908fc 100644 --- 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 index 433d7136edc33..d59620518a0ae 100644 --- 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 index cf1f478ea8018..caef3b73e6e92 100644 --- a/lib/AST/ASTContext.cpp +++ b/lib/AST/ASTContext.cpp @@ -2403,10 +2403,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(); @@ -2447,7 +2448,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; } @@ -2464,7 +2465,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()) @@ -2504,11 +2505,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; @@ -2522,8 +2523,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 index b13264878b205..27c01aca377c5 100644 --- a/lib/ClangImporter/ClangImporter.cpp +++ b/lib/ClangImporter/ClangImporter.cpp @@ -2133,6 +2133,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 index e2788b7ffa10f..87df6e2ea4b3b 100644 --- a/lib/ClangImporter/ImportType.cpp +++ b/lib/ClangImporter/ImportType.cpp @@ -3510,7 +3510,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 index 1bf23821c82a7..be564ec2395de 100644 --- a/lib/Frontend/ModuleInterfaceLoader.cpp +++ b/lib/Frontend/ModuleInterfaceLoader.cpp @@ -2383,7 +2383,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()) @@ -2409,7 +2409,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; } @@ -2420,8 +2420,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; } @@ -2734,7 +2733,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()) @@ -2763,12 +2762,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 index 278eb0b08e1c6..b5cfb865812b6 100644 --- 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 index f277b1dc1a1eb..5efd2ba259c04 100644 --- 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 index 1293acb794ca1..4145788bc5618 100644 --- a/lib/Serialization/ScanningLoaders.cpp +++ b/lib/Serialization/ScanningLoaders.cpp @@ -295,7 +295,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 index 1461b30f17567..1ff7a98443ac2 100644 --- 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" @@ -1373,7 +1374,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()) @@ -1396,36 +1397,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 new file mode 100644 index 0000000000000..3b1133c9cfe64 --- /dev/null +++ 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() {}