Skip to content

Commit 7d85aa4

Browse files
cachemeifyoucancachemeifyoucan
committed
[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
1 parent 8582828 commit 7d85aa4

16 files changed

+109
-53
lines changed

‎include/swift/AST/ASTContext.h

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1109,7 +1109,7 @@ class ASTContext final {
11091109
///
11101110
/// Note that even if this check succeeds, errors may still occur if the
11111111
/// module is loaded in full.
1112-
bool canImportModuleImpl(ImportPath::Module ModulePath,
1112+
bool canImportModuleImpl(ImportPath::Module ModulePath, SourceLoc loc,
11131113
llvm::VersionTuple version, bool underlyingVersion,
11141114
bool updateFailingList,
11151115
llvm::VersionTuple &foundVersion) const;
@@ -1146,7 +1146,7 @@ class ASTContext final {
11461146
///
11471147
/// Note that even if this check succeeds, errors may still occur if the
11481148
/// module is loaded in full.
1149-
bool canImportModule(ImportPath::Module ModulePath,
1149+
bool canImportModule(ImportPath::Module ModulePath, SourceLoc loc,
11501150
llvm::VersionTuple version = llvm::VersionTuple(),
11511151
bool underlyingVersion = false);
11521152

‎include/swift/AST/DiagnosticsSema.def

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -830,6 +830,9 @@ ERROR(serialization_failed,none,
830830
"serialization of module %0 failed due to the errors above",
831831
(const ModuleDecl *))
832832

833+
WARNING(can_import_invalid_swiftmodule,none,
834+
"canImport() evaluated to false due to invalid swiftmodule: %0", (StringRef))
835+
833836
ERROR(serialization_load_failed,Fatal,
834837
"failed to load module '%0'", (StringRef))
835838
ERROR(module_interface_build_failed,Fatal,

‎include/swift/AST/ModuleLoader.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -287,7 +287,7 @@ class ModuleLoader {
287287
///
288288
/// If a non-null \p versionInfo is provided, the module version will be
289289
/// parsed and populated.
290-
virtual bool canImportModule(ImportPath::Module named,
290+
virtual bool canImportModule(ImportPath::Module named, SourceLoc loc,
291291
ModuleVersionInfo *versionInfo,
292292
bool isTestableImport = false) = 0;
293293

‎include/swift/ClangImporter/ClangImporter.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -227,7 +227,7 @@ class ClangImporter final : public ClangModuleLoader {
227227
///
228228
/// If a non-null \p versionInfo is provided, the module version will be
229229
/// parsed and populated.
230-
virtual bool canImportModule(ImportPath::Module named,
230+
virtual bool canImportModule(ImportPath::Module named, SourceLoc loc,
231231
ModuleVersionInfo *versionInfo,
232232
bool isTestableImport = false) override;
233233

‎include/swift/Frontend/ModuleInterfaceLoader.h

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -163,7 +163,7 @@ class ExplicitSwiftModuleLoader: public SerializedModuleLoaderBase {
163163
bool SkipBuildingInterface, bool IsFramework,
164164
bool IsTestableDependencyLookup = false) override;
165165

166-
bool canImportModule(ImportPath::Module named,
166+
bool canImportModule(ImportPath::Module named, SourceLoc loc,
167167
ModuleVersionInfo *versionInfo,
168168
bool isTestableDependencyLookup = false) override;
169169

@@ -212,7 +212,7 @@ class ExplicitCASModuleLoader : public SerializedModuleLoaderBase {
212212
bool SkipBuildingInterface, bool IsFramework,
213213
bool IsTestableDependencyLookup = false) override;
214214

215-
bool canImportModule(ImportPath::Module named,
215+
bool canImportModule(ImportPath::Module named, SourceLoc loc,
216216
ModuleVersionInfo *versionInfo,
217217
bool isTestableDependencyLookup = false) override;
218218

‎include/swift/Sema/SourceLoader.h

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -59,9 +59,10 @@ class SourceLoader : public ModuleLoader {
5959
///
6060
/// If a non-null \p versionInfo is provided, the module version will be
6161
/// parsed and populated.
62-
virtual bool canImportModule(ImportPath::Module named,
63-
ModuleVersionInfo *versionInfo,
64-
bool isTestableDependencyLookup = false) override;
62+
virtual bool
63+
canImportModule(ImportPath::Module named, SourceLoc loc,
64+
ModuleVersionInfo *versionInfo,
65+
bool isTestableDependencyLookup = false) override;
6566

6667
/// Import a module with the given module path.
6768
///

‎include/swift/Serialization/SerializedModuleLoader.h

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -209,9 +209,10 @@ class SerializedModuleLoaderBase : public ModuleLoader {
209209
///
210210
/// If a non-null \p versionInfo is provided, the module version will be
211211
/// parsed and populated.
212-
virtual bool canImportModule(ImportPath::Module named,
213-
ModuleVersionInfo *versionInfo,
214-
bool isTestableDependencyLookup = false) override;
212+
virtual bool
213+
canImportModule(ImportPath::Module named, SourceLoc loc,
214+
ModuleVersionInfo *versionInfo,
215+
bool isTestableDependencyLookup = false) override;
215216

216217
/// Import a module with the given module path.
217218
///
@@ -339,7 +340,7 @@ class MemoryBufferSerializedModuleLoader : public SerializedModuleLoaderBase {
339340
public:
340341
virtual ~MemoryBufferSerializedModuleLoader();
341342

342-
bool canImportModule(ImportPath::Module named,
343+
bool canImportModule(ImportPath::Module named, SourceLoc loc,
343344
ModuleVersionInfo *versionInfo,
344345
bool isTestableDependencyLookup = false) override;
345346

‎lib/AST/ASTContext.cpp

Lines changed: 11 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -2404,10 +2404,11 @@ void ASTContext::addSucceededCanImportModule(
24042404
}
24052405
}
24062406

2407-
bool ASTContext::canImportModuleImpl(
2408-
ImportPath::Module ModuleName, llvm::VersionTuple version,
2409-
bool underlyingVersion, bool updateFailingList,
2410-
llvm::VersionTuple &foundVersion) const {
2407+
bool ASTContext::canImportModuleImpl(ImportPath::Module ModuleName,
2408+
SourceLoc loc, llvm::VersionTuple version,
2409+
bool underlyingVersion,
2410+
bool updateFailingList,
2411+
llvm::VersionTuple &foundVersion) const {
24112412
SmallString<64> FullModuleName;
24122413
ModuleName.getString(FullModuleName);
24132414
auto ModuleNameStr = FullModuleName.str().str();
@@ -2448,7 +2449,7 @@ bool ASTContext::canImportModuleImpl(
24482449

24492450
// Otherwise, ask whether any module loader can load the module.
24502451
for (auto &importer : getImpl().ModuleLoaders) {
2451-
if (importer->canImportModule(ModuleName, nullptr))
2452+
if (importer->canImportModule(ModuleName, loc, nullptr))
24522453
return true;
24532454
}
24542455

@@ -2465,7 +2466,7 @@ bool ASTContext::canImportModuleImpl(
24652466
for (auto &importer : getImpl().ModuleLoaders) {
24662467
ModuleLoader::ModuleVersionInfo versionInfo;
24672468

2468-
if (!importer->canImportModule(ModuleName, &versionInfo))
2469+
if (!importer->canImportModule(ModuleName, loc, &versionInfo))
24692470
continue; // The loader can't find the module.
24702471

24712472
if (!versionInfo.isValid())
@@ -2505,11 +2506,11 @@ void ASTContext::forEachCanImportVersionCheck(
25052506
Callback(entry.first, entry.second.Version, entry.second.UnderlyingVersion);
25062507
}
25072508

2508-
bool ASTContext::canImportModule(ImportPath::Module moduleName,
2509+
bool ASTContext::canImportModule(ImportPath::Module moduleName, SourceLoc loc,
25092510
llvm::VersionTuple version,
25102511
bool underlyingVersion) {
25112512
llvm::VersionTuple versionInfo;
2512-
if (!canImportModuleImpl(moduleName, version, underlyingVersion, true,
2513+
if (!canImportModuleImpl(moduleName, loc, version, underlyingVersion, true,
25132514
versionInfo))
25142515
return false;
25152516

@@ -2523,8 +2524,8 @@ bool ASTContext::testImportModule(ImportPath::Module ModuleName,
25232524
llvm::VersionTuple version,
25242525
bool underlyingVersion) const {
25252526
llvm::VersionTuple versionInfo;
2526-
return canImportModuleImpl(ModuleName, version, underlyingVersion, false,
2527-
versionInfo);
2527+
return canImportModuleImpl(ModuleName, SourceLoc(), version,
2528+
underlyingVersion, false, versionInfo);
25282529
}
25292530

25302531
ModuleDecl *

‎lib/ClangImporter/ClangImporter.cpp

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2147,6 +2147,7 @@ static llvm::VersionTuple getCurrentVersionFromTBD(llvm::vfs::FileSystem &FS,
21472147
}
21482148

21492149
bool ClangImporter::canImportModule(ImportPath::Module modulePath,
2150+
SourceLoc loc,
21502151
ModuleVersionInfo *versionInfo,
21512152
bool isTestableDependencyLookup) {
21522153
// Look up the top-level module to see if it exists.

‎lib/ClangImporter/ImportType.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3513,7 +3513,7 @@ ModuleDecl *ClangImporter::Implementation::tryLoadFoundationModule() {
35133513
bool ClangImporter::Implementation::canImportFoundationModule() {
35143514
ImportPath::Module::Builder builder(SwiftContext.Id_Foundation);
35153515
auto modulePath = builder.get();
3516-
return SwiftContext.canImportModule(modulePath);
3516+
return SwiftContext.canImportModule(modulePath, SourceLoc());
35173517
}
35183518

35193519
Type ClangImporter::Implementation::getNamedSwiftType(ModuleDecl *module,

‎lib/Frontend/ModuleInterfaceLoader.cpp

Lines changed: 6 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -2385,7 +2385,7 @@ std::error_code ExplicitSwiftModuleLoader::findModuleFilesInDirectory(
23852385
}
23862386

23872387
bool ExplicitSwiftModuleLoader::canImportModule(
2388-
ImportPath::Module path, ModuleVersionInfo *versionInfo,
2388+
ImportPath::Module path, SourceLoc loc, ModuleVersionInfo *versionInfo,
23892389
bool isTestableDependencyLookup) {
23902390
// FIXME: Swift submodules?
23912391
if (path.hasSubmodule())
@@ -2411,7 +2411,7 @@ bool ExplicitSwiftModuleLoader::canImportModule(
24112411
auto &fs = *Ctx.SourceMgr.getFileSystem();
24122412
auto moduleBuf = fs.getBufferForFile(it->second.modulePath);
24132413
if (!moduleBuf) {
2414-
Ctx.Diags.diagnose(SourceLoc(), diag::error_opening_explicit_module_file,
2414+
Ctx.Diags.diagnose(loc, diag::error_opening_explicit_module_file,
24152415
it->second.modulePath);
24162416
return false;
24172417
}
@@ -2422,8 +2422,7 @@ bool ExplicitSwiftModuleLoader::canImportModule(
24222422
if (auto forwardingModule = ForwardingModule::load(**moduleBuf)) {
24232423
moduleBuf = fs.getBufferForFile(forwardingModule->underlyingModulePath);
24242424
if (!moduleBuf) {
2425-
Ctx.Diags.diagnose(SourceLoc(),
2426-
diag::error_opening_explicit_module_file,
2425+
Ctx.Diags.diagnose(loc, diag::error_opening_explicit_module_file,
24272426
forwardingModule->underlyingModulePath);
24282427
return false;
24292428
}
@@ -2736,7 +2735,7 @@ std::error_code ExplicitCASModuleLoader::findModuleFilesInDirectory(
27362735
}
27372736

27382737
bool ExplicitCASModuleLoader::canImportModule(
2739-
ImportPath::Module path, ModuleVersionInfo *versionInfo,
2738+
ImportPath::Module path, SourceLoc loc, ModuleVersionInfo *versionInfo,
27402739
bool isTestableDependencyLookup) {
27412740
// FIXME: Swift submodules?
27422741
if (path.hasSubmodule())
@@ -2765,12 +2764,11 @@ bool ExplicitCASModuleLoader::canImportModule(
27652764
: it->second.modulePath;
27662765
auto moduleBuf = Impl.loadFileBuffer(moduleCASID, it->second.modulePath);
27672766
if (!moduleBuf) {
2768-
Ctx.Diags.diagnose(SourceLoc(), diag::error_cas,
2769-
toString(moduleBuf.takeError()));
2767+
Ctx.Diags.diagnose(loc, diag::error_cas, toString(moduleBuf.takeError()));
27702768
return false;
27712769
}
27722770
if (!*moduleBuf) {
2773-
Ctx.Diags.diagnose(SourceLoc(), diag::error_opening_explicit_module_file,
2771+
Ctx.Diags.diagnose(loc, diag::error_opening_explicit_module_file,
27742772
it->second.modulePath);
27752773
return false;
27762774
}

‎lib/Parse/ParseIfConfig.cpp

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -603,7 +603,8 @@ class EvaluateIfConfigCondition :
603603
}
604604
ImportPath::Module::Builder builder(Ctx, Str, /*separator=*/'.',
605605
Arg->getStartLoc());
606-
return Ctx.canImportModule(builder.get(), version, underlyingModule);
606+
return Ctx.canImportModule(builder.get(), E->getLoc(), version,
607+
underlyingModule);
607608
} else if (KindName == "hasFeature") {
608609
auto featureName = getDeclRefStr(Arg);
609610
return Ctx.LangOpts.hasFeature(featureName);

‎lib/Sema/SourceLoader.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -71,7 +71,7 @@ void SourceLoader::collectVisibleTopLevelModuleNames(
7171
// TODO: Implement?
7272
}
7373

74-
bool SourceLoader::canImportModule(ImportPath::Module path,
74+
bool SourceLoader::canImportModule(ImportPath::Module path, SourceLoc loc,
7575
ModuleVersionInfo *versionInfo,
7676
bool isTestableDependencyLookup) {
7777
// FIXME: Swift submodules?

‎lib/Serialization/ScanningLoaders.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -311,7 +311,7 @@ ModuleDependencyVector SerializedModuleLoaderBase::getModuleDependencies(
311311
"Expected PlaceholderSwiftModuleScanner as the first dependency "
312312
"scanner loader.");
313313
for (auto &scanner : scanners) {
314-
if (scanner->canImportModule(modulePath, nullptr,
314+
if (scanner->canImportModule(modulePath, SourceLoc(), nullptr,
315315
isTestableDependencyLookup)) {
316316

317317
ModuleDependencyVector moduleDependnecies;

‎lib/Serialization/SerializedModuleLoader.cpp

Lines changed: 38 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,7 @@
3333
#include "llvm/ADT/StringSet.h"
3434
#include "llvm/Support/Debug.h"
3535
#include "llvm/Support/FileSystem.h"
36+
#include "llvm/Support/VersionTuple.h"
3637
#include "llvm/TargetParser/Host.h"
3738
#include "llvm/Support/MemoryBuffer.h"
3839
#include "llvm/Support/Path.h"
@@ -1391,7 +1392,7 @@ swift::extractUserModuleVersionFromInterface(StringRef moduleInterfacePath) {
13911392
}
13921393

13931394
bool SerializedModuleLoaderBase::canImportModule(
1394-
ImportPath::Module path, ModuleVersionInfo *versionInfo,
1395+
ImportPath::Module path, SourceLoc loc, ModuleVersionInfo *versionInfo,
13951396
bool isTestableDependencyLookup) {
13961397
// FIXME: Swift submodules?
13971398
if (path.hasSubmodule())
@@ -1414,36 +1415,55 @@ bool SerializedModuleLoaderBase::canImportModule(
14141415
if (!found)
14151416
return false;
14161417

1417-
// If the caller doesn't want version info we're done.
1418-
if (!versionInfo)
1419-
return true;
1420-
1421-
assert(found);
1422-
llvm::VersionTuple swiftInterfaceVersion;
14231418
if (!moduleInterfaceSourcePath.empty()) {
1424-
swiftInterfaceVersion =
1419+
// If we found interface and version is not requested, we're done.
1420+
if (!versionInfo)
1421+
return true;
1422+
1423+
auto moduleVersion =
14251424
extractUserModuleVersionFromInterface(moduleInterfaceSourcePath);
1425+
1426+
// If version is requested and found in interface, return the version.
1427+
// Otherwise fallback to binary module handling.
1428+
if (!moduleVersion.empty()) {
1429+
versionInfo->setVersion(moduleVersion,
1430+
ModuleVersionSourceKind::SwiftInterface);
1431+
return true;
1432+
}
14261433
}
14271434

1428-
// If failing to extract the user version from the interface file, try the
1429-
// binary module format, if present.
1430-
if (swiftInterfaceVersion.empty() && moduleInputBuffer) {
1435+
if (moduleInputBuffer) {
14311436
auto metaData = serialization::validateSerializedAST(
1432-
moduleInputBuffer->getBuffer(),
1433-
Ctx.SILOpts.EnableOSSAModules,
1437+
moduleInputBuffer->getBuffer(), Ctx.SILOpts.EnableOSSAModules,
14341438
Ctx.LangOpts.SDKName);
1435-
versionInfo->setVersion(metaData.userModuleVersion,
1439+
1440+
// If we only found binary module, make sure that is valid.
1441+
if (metaData.status != serialization::Status::Valid &&
1442+
moduleInterfaceSourcePath.empty()) {
1443+
// Emit warning if the canImport check location is known.
1444+
if (loc.isValid())
1445+
Ctx.Diags.diagnose(loc, diag::can_import_invalid_swiftmodule,
1446+
moduleInputBuffer->getBufferIdentifier());
1447+
1448+
return false;
1449+
}
1450+
1451+
if (versionInfo)
1452+
versionInfo->setVersion(metaData.userModuleVersion,
1453+
ModuleVersionSourceKind::SwiftBinaryModule);
1454+
}
1455+
1456+
if (versionInfo && !versionInfo->isValid()) {
1457+
// If no version is found, set it to empty version.
1458+
versionInfo->setVersion(llvm::VersionTuple(),
14361459
ModuleVersionSourceKind::SwiftBinaryModule);
1437-
} else {
1438-
versionInfo->setVersion(swiftInterfaceVersion,
1439-
ModuleVersionSourceKind::SwiftInterface);
14401460
}
14411461

14421462
return true;
14431463
}
14441464

14451465
bool MemoryBufferSerializedModuleLoader::canImportModule(
1446-
ImportPath::Module path, ModuleVersionInfo *versionInfo,
1466+
ImportPath::Module path, SourceLoc loc, ModuleVersionInfo *versionInfo,
14471467
bool isTestableDependencyLookup) {
14481468
// FIXME: Swift submodules?
14491469
if (path.hasSubmodule())

‎test/ScanDependencies/can_import_version_mismatch.swift

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,30 @@
1+
// RUN: %empty-directory(%t)
2+
// RUN: split-file %s %t
3+
// RUN: cp %S/../Serialization/Inputs/too-old/Library.swiftmodule %t/include/Library.swiftmodule
4+
5+
/// No warning when there is a swiftinterface file even the binary module is invalid.
6+
// RUN: %target-swift-frontend -scan-dependencies -module-name Test %t/test.swift -module-cache-path %t/clang-module-cache \
7+
// RUN: -disable-implicit-string-processing-module-import -disable-implicit-concurrency-module-import -parse-stdlib \
8+
// RUN: -o %t/deps.json -I %t/include 2>&1 | %FileCheck %s --check-prefix=NOWARN --allow-empty
9+
10+
// NOWARN-NOT: warning:
11+
12+
/// Issue a warning when only found invalid binary module.
13+
// RUN: rm %t/include/Library.swiftinterface
14+
// RUN: %target-swift-frontend -scan-dependencies -module-name Test %t/test.swift -module-cache-path %t/clang-module-cache \
15+
// RUN: -disable-implicit-string-processing-module-import -disable-implicit-concurrency-module-import -parse-stdlib \
16+
// RUN: -o %t/deps.json -I %t/include 2>&1 | %FileCheck %s --check-prefix=WARN
17+
// WARN: warning: canImport() evaluated to false due to invalid swiftmodule
18+
19+
// RUN: %FileCheck %s --check-prefix=DEP --input-file=%t/deps.json
20+
// DEP-NOT: Library
21+
22+
//--- test.swift
23+
#if canImport(Library)
24+
import Library
25+
#endif
26+
27+
//--- include/Library.swiftinterface
28+
// swift-interface-format-version: 1.0
29+
// 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
30+
public func foo() {}

0 commit comments

Comments
 (0)