Skip to content

Commit bbf189c

Browse files
committed
AST: Make the versioned variants of #if canImport() more reliable and consistent.
Previously, when evaluating a `#if canImport(Module, _version: 42)` directive the compiler could diagnose and ignore the directive under the following conditions: - The associated binary module is corrupt/bogus. - The .tbd for an underlying Clang module is missing a current-version field. This behavior is surprising when there is a valid `.swiftinterface` available and it only becomes apparent when building against an SDK with an old enough version of the module that the version in the `.swiftinterface` is too low, making this failure easy to miss. Some modules have different versioning systems for their Swift and Clang modules and it can also be intentional for a distributed binary `.swiftmodule` to contain bogus data (to force the compiler to recompile the `.swiftinterface`) so we need to handle both of these cases gracefully and predictably. Now the compiler will enumerate all module loaders, ask each of them to attempt to parse the module version and then consistently use the parsed version from a single source. The `.swiftinterface` is preferred if present, then the binary module if present, and then finally the `.tbd`. The `.tbd` is still always used exclusively for the `_underlyingVersion` variant of `canImport()`. Resolves rdar://88723492
1 parent 85bef69 commit bbf189c

17 files changed

+320
-87
lines changed

include/swift/AST/ModuleLoader.h

+39-2
Original file line numberDiff line numberDiff line change
@@ -196,14 +196,51 @@ class ModuleLoader {
196196
virtual void collectVisibleTopLevelModuleNames(
197197
SmallVectorImpl<Identifier> &names) const = 0;
198198

199+
/// The kind of source for a module's version.
200+
///
201+
/// NOTE: The order of the members is significant; they are declared in
202+
/// ascending priority order.
203+
enum class ModuleVersionSourceKind {
204+
ClangModuleTBD,
205+
SwiftBinaryModule,
206+
SwiftInterface,
207+
};
208+
209+
/// Represents a module version and the source it was parsed from.
210+
class ModuleVersionInfo {
211+
llvm::VersionTuple Version;
212+
llvm::Optional<ModuleVersionSourceKind> SourceKind;
213+
214+
public:
215+
/// Returns true if the version has a valid source kind.
216+
bool isValid() const { return SourceKind.hasValue(); }
217+
218+
/// Returns the version, which may be empty if a version was not present or
219+
/// was unparsable.
220+
llvm::VersionTuple getVersion() const { return Version; }
221+
222+
/// Returns the kind of source of the module version. Do not call if
223+
/// \c isValid() returns false.
224+
ModuleVersionSourceKind getSourceKind() const {
225+
return SourceKind.getValue();
226+
}
227+
228+
void setVersion(llvm::VersionTuple version, ModuleVersionSourceKind kind) {
229+
Version = version;
230+
SourceKind = kind;
231+
}
232+
};
233+
199234
/// Check whether the module with a given name can be imported without
200235
/// importing it.
201236
///
202237
/// Note that even if this check succeeds, errors may still occur if the
203238
/// module is loaded in full.
239+
///
240+
/// If a non-null \p versionInfo is provided, the module version will be
241+
/// parsed and populated.
204242
virtual bool canImportModule(ImportPath::Module named,
205-
llvm::VersionTuple version,
206-
bool underlyingVersion) = 0;
243+
ModuleVersionInfo *versionInfo) = 0;
207244

208245
/// Import a module with the given module path.
209246
///

include/swift/ClangImporter/ClangImporter.h

+4-2
Original file line numberDiff line numberDiff line change
@@ -204,9 +204,11 @@ class ClangImporter final : public ClangModuleLoader {
204204
///
205205
/// Note that even if this check succeeds, errors may still occur if the
206206
/// module is loaded in full.
207+
///
208+
/// If a non-null \p versionInfo is provided, the module version will be
209+
/// parsed and populated.
207210
virtual bool canImportModule(ImportPath::Module named,
208-
llvm::VersionTuple version,
209-
bool underlyingVersion) override;
211+
ModuleVersionInfo *versionInfo) override;
210212

211213
/// Import a module with the given module path.
212214
///

include/swift/Frontend/ModuleInterfaceLoader.h

+2-2
Original file line numberDiff line numberDiff line change
@@ -151,8 +151,8 @@ class ExplicitSwiftModuleLoader: public SerializedModuleLoaderBase {
151151
std::unique_ptr<llvm::MemoryBuffer> *ModuleSourceInfoBuffer,
152152
bool skipBuildingInterface, bool IsFramework) override;
153153

154-
bool canImportModule(ImportPath::Module named, llvm::VersionTuple version,
155-
bool underlyingVersion) override;
154+
bool canImportModule(ImportPath::Module named,
155+
ModuleVersionInfo *versionInfo) override;
156156

157157
bool isCached(StringRef DepPath) override { return false; };
158158

include/swift/Sema/SourceLoader.h

+4-2
Original file line numberDiff line numberDiff line change
@@ -56,9 +56,11 @@ class SourceLoader : public ModuleLoader {
5656
///
5757
/// Note that even if this check succeeds, errors may still occur if the
5858
/// module is loaded in full.
59+
///
60+
/// If a non-null \p versionInfo is provided, the module version will be
61+
/// parsed and populated.
5962
virtual bool canImportModule(ImportPath::Module named,
60-
llvm::VersionTuple version,
61-
bool underlyingVersion) override;
63+
ModuleVersionInfo *versionInfo) override;
6264

6365
/// Import a module with the given module path.
6466
///

include/swift/Serialization/SerializedModuleLoader.h

+7-4
Original file line numberDiff line numberDiff line change
@@ -168,9 +168,11 @@ class SerializedModuleLoaderBase : public ModuleLoader {
168168
///
169169
/// Note that even if this check succeeds, errors may still occur if the
170170
/// module is loaded in full.
171+
///
172+
/// If a non-null \p versionInfo is provided, the module version will be
173+
/// parsed and populated.
171174
virtual bool canImportModule(ImportPath::Module named,
172-
llvm::VersionTuple version,
173-
bool underlyingVersion) override;
175+
ModuleVersionInfo *versionInfo) override;
174176

175177
/// Import a module with the given module path.
176178
///
@@ -287,8 +289,9 @@ class MemoryBufferSerializedModuleLoader : public SerializedModuleLoaderBase {
287289
public:
288290
virtual ~MemoryBufferSerializedModuleLoader();
289291

290-
bool canImportModule(ImportPath::Module named, llvm::VersionTuple version,
291-
bool underlyingVersion) override;
292+
bool canImportModule(ImportPath::Module named,
293+
ModuleVersionInfo *versionInfo) override;
294+
292295
ModuleDecl *
293296
loadModule(SourceLoc importLoc,
294297
ImportPath::Module path) override;

lib/AST/ASTContext.cpp

+67-8
Original file line numberDiff line numberDiff line change
@@ -2184,6 +2184,27 @@ bool ASTContext::shouldPerformTypoCorrection() {
21842184
return NumTypoCorrections <= LangOpts.TypoCorrectionLimit;
21852185
}
21862186

2187+
static bool isClangModuleVersion(const ModuleLoader::ModuleVersionInfo &info) {
2188+
switch (info.getSourceKind()) {
2189+
case ModuleLoader::ModuleVersionSourceKind::ClangModuleTBD:
2190+
return true;
2191+
case ModuleLoader::ModuleVersionSourceKind::SwiftBinaryModule:
2192+
case ModuleLoader::ModuleVersionSourceKind::SwiftInterface:
2193+
return false;
2194+
}
2195+
}
2196+
2197+
static StringRef
2198+
getModuleVersionKindString(const ModuleLoader::ModuleVersionInfo &info) {
2199+
switch (info.getSourceKind()) {
2200+
case ModuleLoader::ModuleVersionSourceKind::ClangModuleTBD:
2201+
return "Clang";
2202+
case ModuleLoader::ModuleVersionSourceKind::SwiftBinaryModule:
2203+
case ModuleLoader::ModuleVersionSourceKind::SwiftInterface:
2204+
return "Swift";
2205+
}
2206+
}
2207+
21872208
bool ASTContext::canImportModuleImpl(ImportPath::Module ModuleName,
21882209
llvm::VersionTuple version,
21892210
bool underlyingVersion,
@@ -2195,22 +2216,60 @@ bool ASTContext::canImportModuleImpl(ImportPath::Module ModuleName,
21952216
// If we've failed loading this module before, don't look for it again.
21962217
if (FailedModuleImportNames.count(ModuleNameStr))
21972218
return false;
2198-
// If no specific version, the module is importable if it has already been imported.
2219+
21992220
if (version.empty()) {
22002221
// If this module has already been successfully imported, it is importable.
22012222
if (getLoadedModule(ModuleName) != nullptr)
22022223
return true;
2224+
2225+
// Otherwise, ask whether any module loader can load the module.
2226+
for (auto &importer : getImpl().ModuleLoaders) {
2227+
if (importer->canImportModule(ModuleName, nullptr))
2228+
return true;
2229+
}
2230+
2231+
if (updateFailingList)
2232+
FailedModuleImportNames.insert(ModuleNameStr);
2233+
2234+
return false;
22032235
}
2204-
// Otherwise, ask the module loaders.
2236+
2237+
// We need to check whether the version of the module is high enough.
2238+
// Retrieve a module version from each module loader that can find the module
2239+
// and use the best source available for the query.
2240+
ModuleLoader::ModuleVersionInfo bestVersionInfo;
22052241
for (auto &importer : getImpl().ModuleLoaders) {
2206-
if (importer->canImportModule(ModuleName, version, underlyingVersion)) {
2207-
return true;
2208-
}
2242+
ModuleLoader::ModuleVersionInfo versionInfo;
2243+
2244+
if (!importer->canImportModule(ModuleName, &versionInfo))
2245+
continue; // The loader can't find the module.
2246+
2247+
if (!versionInfo.isValid())
2248+
continue; // The loader didn't attempt to parse a version.
2249+
2250+
if (underlyingVersion && !isClangModuleVersion(versionInfo))
2251+
continue; // We're only matching Clang module versions.
2252+
2253+
if (bestVersionInfo.isValid() &&
2254+
versionInfo.getSourceKind() <= bestVersionInfo.getSourceKind())
2255+
continue; // This module version's source is lower priority.
2256+
2257+
bestVersionInfo = versionInfo;
22092258
}
2210-
if (updateFailingList && version.empty()) {
2211-
FailedModuleImportNames.insert(ModuleNameStr);
2259+
2260+
if (!bestVersionInfo.isValid())
2261+
return false;
2262+
2263+
if (bestVersionInfo.getVersion().empty()) {
2264+
// The module version could not be parsed from the preferred source for
2265+
// this query. Diagnose and treat the query as if it succeeded.
2266+
auto mID = ModuleName[0];
2267+
Diags.diagnose(mID.Loc, diag::cannot_find_project_version,
2268+
getModuleVersionKindString(bestVersionInfo), mID.Item.str());
2269+
return true;
22122270
}
2213-
return false;
2271+
2272+
return version <= bestVersionInfo.getVersion();
22142273
}
22152274

22162275
bool ASTContext::canImportModule(ImportPath::Module ModuleName,

lib/ClangImporter/ClangImporter.cpp

+7-14
Original file line numberDiff line numberDiff line change
@@ -1798,8 +1798,7 @@ static std::string getScalaNodeText(llvm::yaml::Node *N) {
17981798
}
17991799

18001800
bool ClangImporter::canImportModule(ImportPath::Module modulePath,
1801-
llvm::VersionTuple version,
1802-
bool underlyingVersion) {
1801+
ModuleVersionInfo *versionInfo) {
18031802
// Look up the top-level module to see if it exists.
18041803
auto &clangHeaderSearch = Impl.getClangPreprocessor().getHeaderSearchInfo();
18051804
auto topModule = modulePath.front();
@@ -1842,10 +1841,10 @@ bool ClangImporter::canImportModule(ImportPath::Module modulePath,
18421841
}
18431842
}
18441843

1845-
if (version.empty())
1844+
if (!versionInfo)
18461845
return true;
1846+
18471847
assert(available);
1848-
assert(!version.empty());
18491848
llvm::VersionTuple currentVersion;
18501849
StringRef path = getClangASTContext().getSourceManager()
18511850
.getFilename(clangModule->DefinitionLoc);
@@ -1888,16 +1887,10 @@ bool ClangImporter::canImportModule(ImportPath::Module modulePath,
18881887
}
18891888
break;
18901889
}
1891-
// Diagnose unable to checking the current version.
1892-
if (currentVersion.empty()) {
1893-
Impl.diagnose(topModule.Loc, diag::cannot_find_project_version, "Clang",
1894-
topModule.Item.str());
1895-
return true;
1896-
}
1897-
assert(!currentVersion.empty());
1898-
// Give a green light if the version on disk is greater or equal to the version
1899-
// specified in the canImport condition.
1900-
return currentVersion >= version;
1890+
1891+
versionInfo->setVersion(currentVersion,
1892+
ModuleVersionSourceKind::ClangModuleTBD);
1893+
return true;
19011894
}
19021895

19031896
ModuleDecl *ClangImporter::Implementation::loadModuleClang(

lib/Frontend/ModuleInterfaceLoader.cpp

+2-3
Original file line numberDiff line numberDiff line change
@@ -1947,9 +1947,8 @@ std::error_code ExplicitSwiftModuleLoader::findModuleFilesInDirectory(
19471947
return std::make_error_code(std::errc::not_supported);
19481948
}
19491949

1950-
bool ExplicitSwiftModuleLoader::canImportModule(ImportPath::Module path,
1951-
llvm::VersionTuple version,
1952-
bool underlyingVersion) {
1950+
bool ExplicitSwiftModuleLoader::canImportModule(
1951+
ImportPath::Module path, ModuleVersionInfo *versionInfo) {
19531952
// FIXME: Swift submodules?
19541953
if (path.hasSubmodule())
19551954
return false;

lib/Sema/SourceLoader.cpp

+2-2
Original file line numberDiff line numberDiff line change
@@ -71,8 +71,7 @@ void SourceLoader::collectVisibleTopLevelModuleNames(
7171
}
7272

7373
bool SourceLoader::canImportModule(ImportPath::Module path,
74-
llvm::VersionTuple version,
75-
bool underlyingVersion) {
74+
ModuleVersionInfo *versionInfo) {
7675
// FIXME: Swift submodules?
7776
if (path.hasSubmodule())
7877
return false;
@@ -90,6 +89,7 @@ bool SourceLoader::canImportModule(ImportPath::Module path,
9089

9190
return false;
9291
}
92+
9393
return true;
9494
}
9595

lib/Serialization/ModuleDependencyScanner.cpp

+1-1
Original file line numberDiff line numberDiff line change
@@ -199,7 +199,7 @@ Optional<ModuleDependencies> SerializedModuleLoaderBase::getModuleDependencies(
199199
assert(isa<PlaceholderSwiftModuleScanner>(scanners[0].get()) &&
200200
"Expected PlaceholderSwiftModuleScanner as the first dependency scanner loader.");
201201
for (auto &scanner : scanners) {
202-
if (scanner->canImportModule(modulePath, llvm::VersionTuple(), false)) {
202+
if (scanner->canImportModule(modulePath, nullptr)) {
203203
// Record the dependencies.
204204
cache.recordDependencies(moduleName, *(scanner->dependencies));
205205
return std::move(scanner->dependencies);

0 commit comments

Comments
 (0)