[Dependency Scanning] Have the scanner cache answer queries relevant to current search paths only. #38703
Conversation
|
@swift-ci please test |
| /// | ||
| /// \returns the cached result, or \c None if there is no cached entry. | ||
| Optional<ModuleDependencies> findDependencies( | ||
| StringRef moduleName, | ||
| Optional<ModuleDependenciesKind> kind, | ||
| llvm::StringSet<> ¤tSearchPaths) const; |
There was a problem hiding this comment.
Nitpick: can we define a new struct, e.g. ModuleSpecifics, to contain kind and currentSearchPaths for better future extensibility?
lib/AST/ASTContext.cpp
Outdated
| return found; | ||
| } | ||
| if (auto found = cache.findDependencies(moduleName, | ||
| ModuleDependenciesKind::Clang)) | ||
| ModuleDependenciesKind::Clang, | ||
| searchPathSet)) |
There was a problem hiding this comment.
I'm not a big fan of duplicating the same function call for all enum cases. One technique we can use here is to define ModuleDependenciesKind::First_Kind and ModuleDependenciesKind::Last_Kind and use a for loop to iterate them. This applies to other duplications as well.
There was a problem hiding this comment.
Given that this PR does not introduce this behavior, can we pursue this refactoring outside the scope of this change?
Even in this specific example there is additional complexity where simple iteration would not be enough (non-clang dependencies are skipped under a certain condition). And since this PR already re-structures quite a bit of code, I'd prefer to reduce further refactoring of existing patterns here.
There was a problem hiding this comment.
Sure, a following PR would be much appreciated!
| dependencyInfo.getModuleDependencies()); | ||
|
|
||
| // Add the dependency-kind-specific data | ||
| if (auto swiftTextDeps = dependencyInfo.getAsSwiftTextualModule()) { |
There was a problem hiding this comment.
Instead of using chaining if-else here, can we refactor it to a switch statement?
There was a problem hiding this comment.
Done, thanks for pointing it out.
…to current search paths only. The dependency scanner's cache persists across different queries and answering a subsequent query's module lookup with a module not in the query's search path is not correct. For example, suppose we are looking for a Swift module `Foo` with a set of search paths `SP`. And dependency scanner cache already contains a module `Foo`, for which we found an interface file at location `L`. If `L`∉`SP`, then we cannot re-use the cached entry because we’d be resolving the scanning query to a filesystem location that the current scanning context is not aware of. Resolves rdar://81175942
artemcm
force-pushed
the
DepScanCacheSearchPathDisambiguate
branch
from
95754b2 to
6a12dc0
Compare
|
@swift-ci please test |
|
@swift-ci please test Windows platform |
The dependency scanner's cache persists across different queries and answering a subsequent query's module lookup with a module not in the query's search path is not correct.
For example, suppose we are looking for a Swift module
Foowith a set of search pathsSP.And dependency scanner cache already contains a module
Foo, for which we found an interface file at locationL. IfL∉SP, then we cannot re-use the cached entry because we’d be resolving the scanning query to a filesystem location that the current scanning context is not aware of.Resolves rdar://81175942