[ScanDependency][canImport] Improve canImport handling in explicit build #73853

cachemeifyoucan · 2024-05-23T18:38:36Z

Teach dependency scanner to report all the module canImport check result to swift-frontend, so swift-frontend doesn't need to parse swiftmodule or parse TBD file to determine the versions. This ensures dependency scanner and swift-frontend will have the same resolution for all canImport checks.

This also fixes two related issues:

Previously, in order to get consistant results between scanner and frontend, scanner will request building the module in canImport check even it is not imported later. This slightly alters the definition of the canImport to only succeed when the module can be found AND be built. This also can affect the auto-link in such cases.
For caching build, the location of the clang module is abstracted away so swift-frontend cannot locate the TBD file to resolve underlyingVersion.

rdar://128067152

cachemeifyoucan · 2024-05-23T18:38:55Z

@swift-ci please smoke test

benlangmuir · 2024-05-24T22:18:51Z

include/swift/AST/ASTContext.h

@@ -411,7 +412,11 @@ class ASTContext final {

  /// Cache of module names that passed the 'canImport' test. This cannot be
  /// mutable since it needs to be queried for dependency discovery.
-  llvm::StringSet<> SucceededModuleImportNames;
+  struct ImportedModuleVersionInfo {


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?

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.

benlangmuir · 2024-05-24T22:21:55Z

include/swift/AST/SearchPathOptions.h

+  struct CanImportInfo {
+    std::string ModuleName;
+    llvm::VersionTuple Version;
+    llvm::VersionTuple UnderlyingVersion;


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?

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.

I am guessing this is for some Swift client that may have both:

#if canImport(Foo, _version: 10.0) { ... } #if canImport(Foo, _underlyingVersion: 10.0) { ... }

The names of these version tuples here at least, are confusing (they are confusing in the canImport directive itself, but that's a separate point). Maybe UnderlyingClangModuleVersion. Also, IIUC these tuples represent the lowest version for this module for which the versioned canImport evaluates to true, right? That's worth capturing in the comments here as well.

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...

Maybe UnderlyingClangModuleVersion

Current variable actually matches the parameter in canImport :)

lib/AST/ASTContext.cpp

benlangmuir · 2024-05-24T22:24:55Z

lib/AST/ASTContext.cpp

+
+    // If the canImport information is coming from the command-line, then no
+    // need to continue the search, return false.
+    if (!SearchPathOpts.CanImportModuleInfo.empty())


Should this check be outside the if (Found != SucceededModuleImportNames.end())?

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.

cachemeifyoucan · 2024-05-24T23:07:04Z

@swift-ci please smoke test

cachemeifyoucan · 2024-05-24T23:46:41Z

@swift-ci please smoke test

cachemeifyoucan · 2024-05-30T22:49:02Z

ping

tshortli · 2024-05-31T22:09:03Z

lib/AST/ASTContext.cpp

+  auto &entry = SucceededModuleImportNames[moduleName.str()];
+  if (!versionInfo.empty()) {
+    if (underlyingVersion)
+      entry.UnderlyingVersion = versionInfo;


What if canImport() is evaluated multiple times with different version numbers? Shouldn't we only store the new version if it is higher than the previously stored version?

The version stored is the version of the module, not the version in canImport statement. The version is either requested or not requested, and cannot change during the build. And we can use that one version to check for every version requested through canImport.

I see. I find this naming ("Succeeded") confusing then; these are just the versions for these modules. It's just a nit, though.

I will rename it.

Teach dependency scanner to report all the module canImport check result to swift-frontend, so swift-frontend doesn't need to parse swiftmodule or parse TBD file to determine the versions. This ensures dependency scanner and swift-frontend will have the same resolution for all canImport checks. This also fixes two related issues: * Previously, in order to get consistant results between scanner and frontend, scanner will request building the module in canImport check even it is not imported later. This slightly alters the definition of the canImport to only succeed when the module can be found AND be built. This also can affect the auto-link in such cases. * For caching build, the location of the clang module is abstracted away so swift-frontend cannot locate the TBD file to resolve underlyingVersion. rdar://128067152

cachemeifyoucan · 2024-05-31T22:38:24Z

@swift-ci please smoke test

cachemeifyoucan requested review from akyrtzi and benlangmuir May 23, 2024 18:38

cachemeifyoucan requested review from artemcm, ahoppen, bnbarham, CodaFi, DougGregor, hamishknight, rintaro, zoecarver, hyp, egorzhdan, beccadax, ian-twilightcoder, hborla, slavapestov, xedin and tshortli as code owners May 23, 2024 18:38

ahoppen removed their request for review May 23, 2024 20:05

benlangmuir reviewed May 24, 2024

View reviewed changes

cachemeifyoucan force-pushed the eng/PR-128067152 branch from 15119e8 to 4ffd4fe Compare May 24, 2024 23:06

cachemeifyoucan force-pushed the eng/PR-128067152 branch from 4ffd4fe to f66d916 Compare May 24, 2024 23:46

tshortli reviewed May 31, 2024

View reviewed changes

tshortli approved these changes May 31, 2024

View reviewed changes

cachemeifyoucan force-pushed the eng/PR-128067152 branch from f66d916 to 026fcd2 Compare May 31, 2024 22:37

cachemeifyoucan merged commit 0aa0687 into swiftlang:main Jun 3, 2024
3 checks passed

cachemeifyoucan mentioned this pull request Jun 3, 2024

[6.0][ScanDependency][canImport] Improve canImport handling in explicit build #74105

Merged

[ScanDependency][canImport] Improve canImport handling in explicit build #73853

[ScanDependency][canImport] Improve canImport handling in explicit build #73853

Uh oh!

Conversation

cachemeifyoucan commented May 23, 2024

Uh oh!

cachemeifyoucan commented May 23, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cachemeifyoucan commented May 24, 2024

Uh oh!

cachemeifyoucan commented May 24, 2024

Uh oh!

cachemeifyoucan commented May 30, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cachemeifyoucan commented May 31, 2024

Uh oh!

Uh oh!

Uh oh!