-
Notifications
You must be signed in to change notification settings - Fork 10.5k
[ScanDependency][canImport] Improve canImport handling in explicit build #73853
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,6 +20,7 @@ | |
#include "llvm/ADT/IntrusiveRefCntPtr.h" | ||
#include "llvm/ADT/StringMap.h" | ||
#include "llvm/Support/Error.h" | ||
#include "llvm/Support/VersionTuple.h" | ||
#include "llvm/Support/VirtualFileSystem.h" | ||
#include <optional> | ||
|
||
|
@@ -509,6 +510,14 @@ class SearchPathOptions { | |
using CrossImportMap = llvm::StringMap<std::vector<std::string>>; | ||
CrossImportMap CrossImportInfo; | ||
|
||
/// CanImport information passed from scanning. | ||
struct CanImportInfo { | ||
std::string ModuleName; | ||
llvm::VersionTuple Version; | ||
llvm::VersionTuple UnderlyingVersion; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't understand the modeling here: it looks like you actually get a separate CanImportInfo for version vs. underlying version, so why do we store both instead of a flag? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, those are two separate versions, one for swift module and one for clang module, and they can co-exists for one module decl name because swift module can be an overlay for clang module. Alternative is to module clang and swift separately and use different options in swift-frontend. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am guessing this is for some Swift client that may have both:
The names of these version tuples here at least, are confusing (they are confusing in the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, it is the version of the module discovered by scanner if version ever requested. That said, there might be a corner case I need to fix. Looking... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Current variable actually matches the parameter in |
||
}; | ||
std::vector<CanImportInfo> CanImportModuleInfo; | ||
|
||
/// Whether to search for cross import overlay on file system. | ||
bool DisableCrossImportOverlaySearch = false; | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -74,6 +74,7 @@ | |
#include "llvm/Support/Allocator.h" | ||
#include "llvm/Support/Compiler.h" | ||
#include "llvm/Support/FormatVariadic.h" | ||
#include "llvm/Support/VersionTuple.h" | ||
#include "llvm/Support/VirtualOutputBackend.h" | ||
#include "llvm/Support/VirtualOutputBackends.h" | ||
#include <algorithm> | ||
|
@@ -786,6 +787,12 @@ ASTContext::ASTContext( | |
registerAccessRequestFunctions(evaluator); | ||
registerNameLookupRequestFunctions(evaluator); | ||
|
||
// Register canImport module info. | ||
for (auto &info: SearchPathOpts.CanImportModuleInfo) { | ||
addSucceededCanImportModule(info.ModuleName, false, info.Version); | ||
addSucceededCanImportModule(info.ModuleName, true, info.UnderlyingVersion); | ||
} | ||
|
||
// Provide a default OnDiskOutputBackend if user didn't supply one. | ||
if (!OutputBackend) | ||
OutputBackend = llvm::makeIntrusiveRefCnt<llvm::vfs::OnDiskOutputBackend>(); | ||
|
@@ -2385,23 +2392,56 @@ getModuleVersionKindString(const ModuleLoader::ModuleVersionInfo &info) { | |
} | ||
} | ||
|
||
bool ASTContext::canImportModuleImpl(ImportPath::Module ModuleName, | ||
llvm::VersionTuple version, | ||
bool underlyingVersion, | ||
bool updateFailingList) const { | ||
void ASTContext::addSucceededCanImportModule( | ||
StringRef moduleName, bool underlyingVersion, | ||
const llvm::VersionTuple &versionInfo) { | ||
auto &entry = CanImportModuleVersions[moduleName.str()]; | ||
if (!versionInfo.empty()) { | ||
if (underlyingVersion) | ||
entry.UnderlyingVersion = versionInfo; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What if There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The version stored is the version of the module, not the version in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see. I find this naming ("Succeeded") confusing then; these are just the versions for these modules. It's just a nit, though. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I will rename it. |
||
else | ||
entry.Version = versionInfo; | ||
} | ||
} | ||
|
||
bool ASTContext::canImportModuleImpl( | ||
ImportPath::Module ModuleName, llvm::VersionTuple version, | ||
bool underlyingVersion, bool updateFailingList, | ||
llvm::VersionTuple &foundVersion) const { | ||
SmallString<64> FullModuleName; | ||
ModuleName.getString(FullModuleName); | ||
auto ModuleNameStr = FullModuleName.str(); | ||
auto ModuleNameStr = FullModuleName.str().str(); | ||
|
||
// If we've failed loading this module before, don't look for it again. | ||
if (FailedModuleImportNames.count(ModuleNameStr)) | ||
return false; | ||
|
||
if (version.empty()) { | ||
// If this module has already been checked successfully, it is importable. | ||
if (SucceededModuleImportNames.count(ModuleNameStr)) | ||
// If this module has already been checked or there is information for the | ||
// module from commandline, use that information instead of loading the | ||
// module. | ||
auto Found = CanImportModuleVersions.find(ModuleNameStr); | ||
if (Found != CanImportModuleVersions.end()) { | ||
if (version.empty()) | ||
return true; | ||
|
||
if (underlyingVersion) { | ||
if (!Found->second.UnderlyingVersion.empty()) | ||
return version <= Found->second.UnderlyingVersion; | ||
} else { | ||
if (!Found->second.Version.empty()) | ||
return version <= Found->second.Version; | ||
} | ||
|
||
// If the canImport information is coming from the command-line, then no | ||
// need to continue the search, return false. For checking modules that are | ||
// not passed from command-line, allow fallback to the module loading since | ||
// this is not in a canImport request context that has already been resolved | ||
// by scanner. | ||
if (!SearchPathOpts.CanImportModuleInfo.empty()) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this check be outside the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Swift compiler sometimes query canImport module during setup phase (like can I import concurrency?) which doesn't gets mapped out by scanner. For the module name requested that is not in the map, it can fallback to module loading path. Will improve comments. |
||
return false; | ||
} | ||
|
||
if (version.empty()) { | ||
// If this module has already been successfully imported, it is importable. | ||
if (getLoadedModule(ModuleName) != nullptr) | ||
return true; | ||
|
@@ -2453,28 +2493,38 @@ bool ASTContext::canImportModuleImpl(ImportPath::Module ModuleName, | |
return true; | ||
} | ||
|
||
foundVersion = bestVersionInfo.getVersion(); | ||
return version <= bestVersionInfo.getVersion(); | ||
} | ||
|
||
bool ASTContext::canImportModule(ImportPath::Module ModuleName, | ||
void ASTContext::forEachCanImportVersionCheck( | ||
std::function<void(StringRef, const llvm::VersionTuple &, | ||
const llvm::VersionTuple &)> | ||
Callback) const { | ||
for (auto &entry : CanImportModuleVersions) | ||
Callback(entry.first, entry.second.Version, entry.second.UnderlyingVersion); | ||
} | ||
|
||
bool ASTContext::canImportModule(ImportPath::Module moduleName, | ||
llvm::VersionTuple version, | ||
bool underlyingVersion) { | ||
if (!canImportModuleImpl(ModuleName, version, underlyingVersion, true)) | ||
llvm::VersionTuple versionInfo; | ||
if (!canImportModuleImpl(moduleName, version, underlyingVersion, true, | ||
versionInfo)) | ||
return false; | ||
|
||
// If checked successfully, add the top level name to success list as | ||
// dependency to handle clang submodule correctly. Swift does not have | ||
// submodule so the name should be the same. | ||
SmallString<64> TopModuleName; | ||
ModuleName.getTopLevelPath().getString(TopModuleName); | ||
SucceededModuleImportNames.insert(TopModuleName.str()); | ||
SmallString<64> fullModuleName; | ||
moduleName.getString(fullModuleName); | ||
addSucceededCanImportModule(fullModuleName, underlyingVersion, versionInfo); | ||
return true; | ||
} | ||
|
||
bool ASTContext::testImportModule(ImportPath::Module ModuleName, | ||
llvm::VersionTuple version, | ||
bool underlyingVersion) const { | ||
return canImportModuleImpl(ModuleName, version, underlyingVersion, false); | ||
llvm::VersionTuple versionInfo; | ||
return canImportModuleImpl(ModuleName, version, underlyingVersion, false, | ||
versionInfo); | ||
} | ||
|
||
ModuleDecl * | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Doc comment should be on the map; why is it std::map instead of StringMap (or MapVector around StringMap if you need a stable order)? Does it really need to be sorted by key?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is either this needs to be sorted or the scanner that request the module version needs to sort it afterwards before converting to the build args. Since options do not get canonicalized now, the order of the version matters.