[Dependency Scanning] Always record best version of discovered 'canImport'-ed modules

Suppose module 'Foo' exists in the search paths and specifies user module version '1.0'.

If the first encountered 'canImport' query is unversioned:
...

Followed by a versioned one:
...

The success of the first check will record an unversioned successful canImport, which will cause the second check to evaluate to 'true', which is incorrect.

This change causes even unversioned 'canImport' checks to track and record the discovered user module version.

Author: artemcm

include/swift/AST/ASTContext.h

@@ -1185,11 +1185,13 @@ class ASTContext final {
   bool canImportModuleImpl(ImportPath::Module ModulePath, SourceLoc loc,
                            llvm::VersionTuple version, bool underlyingVersion,
                            bool isSourceCanImport,
-                           llvm::VersionTuple &foundVersion) const;
+                           llvm::VersionTuple &foundVersion,
+                           llvm::VersionTuple &foundUnderlyingClangVersion) const;
 
   /// Add successful canImport modules.
-  void addSucceededCanImportModule(StringRef moduleName, bool underlyingVersion,
-                                   const llvm::VersionTuple &versionInfo);
+  void addSucceededCanImportModule(StringRef moduleName,
+                                   const llvm::VersionTuple &versionInfo,
+                                   const llvm::VersionTuple &underlyingVersionInfo);
 
 public:
   namelookup::ImportCache &getImportCache() const;

include/swift/AST/DiagnosticsSema.def

@@ -1272,7 +1272,8 @@ REMARK(explicit_interface_build_skipped,none,
        (StringRef))
 
 WARNING(cannot_find_project_version,none,
-        "cannot find user version number for %0 module '%1'; version number ignored", (StringRef, StringRef))
+        "cannot find user version number for%select{| Clang}1 module '%0';"
+        " version number ignored", (StringRef, bool))
 
 // Operator decls
 ERROR(ambiguous_operator_decls,none,

lib/AST/ASTContext.cpp

@@ -847,18 +847,16 @@ ASTContext::ASTContext(
   registerNameLookupRequestFunctions(evaluator);
 
   // Register canImport module info.
-  for (auto &info: SearchPathOpts.CanImportModuleInfo) {
-    addSucceededCanImportModule(info.ModuleName, false, info.Version);
-    addSucceededCanImportModule(info.ModuleName, true, info.UnderlyingVersion);
-  }
+  for (auto &info: SearchPathOpts.CanImportModuleInfo)
+    addSucceededCanImportModule(info.ModuleName, info.Version, info.UnderlyingVersion);
 
   // Provide a default OnDiskOutputBackend if user didn't supply one.
   if (!OutputBackend)
     OutputBackend = llvm::makeIntrusiveRefCnt<llvm::vfs::OnDiskOutputBackend>();
+
   // Insert all block list config paths.
-  for (auto path: langOpts.BlocklistConfigFilePaths) {
+  for (auto path: langOpts.BlocklistConfigFilePaths)
     blockListConfig.addConfigureFilePath(path);
-  }
 }
 
 void ASTContext::Implementation::dump(llvm::raw_ostream &os) const {
@@ -2648,34 +2646,25 @@ static bool isClangModuleVersion(const ModuleLoader::ModuleVersionInfo &info) {
   }
 }
 
-static StringRef
-getModuleVersionKindString(const ModuleLoader::ModuleVersionInfo &info) {
-  switch (info.getSourceKind()) {
-  case ModuleLoader::ModuleVersionSourceKind::ClangModuleTBD:
-    return "Clang";
-  case ModuleLoader::ModuleVersionSourceKind::SwiftBinaryModule:
-  case ModuleLoader::ModuleVersionSourceKind::SwiftInterface:
-    return "Swift";
-  }
-}
-
 void ASTContext::addSucceededCanImportModule(
-    StringRef moduleName, bool underlyingVersion,
-    const llvm::VersionTuple &versionInfo) {
+    StringRef moduleName,
+    const llvm::VersionTuple &versionInfo,
+    const llvm::VersionTuple &underlyingVersionInfo) {
+  // We have previously recorded a successful canImport
+  // information for this module.
+  if (CanImportModuleVersions.count(moduleName.str()))
+    return;
+
   auto &entry = CanImportModuleVersions[moduleName.str()];
-  if (!versionInfo.empty()) {
-    if (underlyingVersion)
-      entry.UnderlyingVersion = versionInfo;
-    else
-      entry.Version = versionInfo;
-  }
+  entry.Version = versionInfo;
+  entry.UnderlyingVersion = underlyingVersionInfo;
 }
 
-bool ASTContext::canImportModuleImpl(ImportPath::Module ModuleName,
-                                     SourceLoc loc, llvm::VersionTuple version,
-                                     bool underlyingVersion,
-                                     bool isSourceCanImport,
-                                     llvm::VersionTuple &foundVersion) const {
+bool ASTContext::canImportModuleImpl(
+    ImportPath::Module ModuleName, SourceLoc loc, llvm::VersionTuple version,
+    bool underlyingVersion, bool isSourceCanImport,
+    llvm::VersionTuple &foundVersion,
+    llvm::VersionTuple &foundUnderlyingClangVersion) const {
   SmallString<64> FullModuleName;
   ModuleName.getString(FullModuleName);
   auto ModuleNameStr = FullModuleName.str().str();
@@ -2684,6 +2673,20 @@ bool ASTContext::canImportModuleImpl(ImportPath::Module ModuleName,
   if (FailedModuleImportNames.count(ModuleNameStr))
     return false;
 
+  auto missingVersion = [this, &loc, &ModuleName,
+                                 &underlyingVersion]() -> bool {
+    // The module version could not be parsed from the preferred source for
+    // this query. Diagnose and return `true` to indicate that the unversioned module
+    // will satisfy the query.
+    auto mID = ModuleName[0];
+    auto diagLoc = mID.Loc;
+    if (mID.Loc.isInvalid())
+      diagLoc = loc;
+    Diags.diagnose(diagLoc, diag::cannot_find_project_version, mID.Item.str(),
+                   underlyingVersion);
+    return true;
+  };
+
   // If this module has already been checked or there is information for the
   // module from commandline, use that information instead of loading the
   // module.
@@ -2692,26 +2695,93 @@ bool ASTContext::canImportModuleImpl(ImportPath::Module ModuleName,
     if (version.empty())
       return true;
 
-    if (underlyingVersion)
-      return Found->second.UnderlyingVersion.empty()
-                 ? true
-                 : version <= Found->second.UnderlyingVersion;
+    const auto &foundComparisonVersion = underlyingVersion
+                                      ? Found->second.UnderlyingVersion
+                                      : Found->second.Version;
+    if (!foundComparisonVersion.empty())
+      return version <= foundComparisonVersion;
     else
-      return Found->second.Version.empty()
-                 ? true
-                 : version <= Found->second.Version;
+      return missingVersion();
   }
 
+  // When looking up a module, each module importer will report back
+  // if it finds a module with a specified version. This routine verifies
+  // whether said version is valid and if it superceeds the best
+  // previously-discovered version of this module found.
+  auto validateVersion =
+      [](const ModuleLoader::ModuleVersionInfo &bestVersionInfo,
+         const ModuleLoader::ModuleVersionInfo &versionInfo,
+         bool underlyingVersion) {
+        if (!versionInfo.isValid())
+          return false; // The loader didn't attempt to parse a version.
+
+        if (underlyingVersion && !isClangModuleVersion(versionInfo))
+          return false; // We're only matching Clang module versions.
+
+        if (bestVersionInfo.isValid() &&
+            versionInfo.getSourceKind() <= bestVersionInfo.getSourceKind())
+          return false; // This module version's source is lower priority.
+
+        return true;
+      };
+
+  // For each module loader, attempt to discover queried module,
+  // along the way record the discovered module's version as well as
+  // the discovered module's underlying Clang module's version.
+  auto lookupVersionedModule =
+      [&](ModuleLoader::ModuleVersionInfo &bestVersionInfo,
+          ModuleLoader::ModuleVersionInfo &bestUnderlyingVersionInfo) -> bool {
+    for (auto &importer : getImpl().ModuleLoaders) {
+      ModuleLoader::ModuleVersionInfo versionInfo;
+      if (!importer->canImportModule(ModuleName, loc, &versionInfo))
+        continue; // The loader can't find the module.
+
+      if (validateVersion(bestVersionInfo, versionInfo,
+                          /* underlyingVersion */ false))
+        bestVersionInfo = versionInfo;
+      if (validateVersion(bestUnderlyingVersionInfo, versionInfo,
+                          /* underlyingVersion */ true))
+        bestUnderlyingVersionInfo = versionInfo;
+    }
+
+    if (!underlyingVersion && !bestVersionInfo.isValid())
+      return false;
+
+    if (underlyingVersion && !bestUnderlyingVersionInfo.isValid())
+      return false;
+
+    foundVersion = bestVersionInfo.getVersion();
+    foundUnderlyingClangVersion = bestUnderlyingVersionInfo.getVersion();
+    return true;
+  };
+
+  // For queries which do not care about any kind of module information
+  // such as e.g. `testImportModule`, simply return `true` as soon
+  // as *any* loader can find the queried module.
+  auto lookupModule = [&]() -> bool {
+    for (auto &importer : getImpl().ModuleLoaders) {
+      ModuleLoader::ModuleVersionInfo versionInfo;
+      if (!importer->canImportModule(ModuleName, loc, &versionInfo))
+        continue; // The loader can't find the module.
+      return true;
+    }
+    return false;
+  };
+
   if (version.empty()) {
     // If this module has already been successfully imported, it is importable.
     if (getLoadedModule(ModuleName) != nullptr)
       return true;
-
-    // Otherwise, ask whether any module loader can load the module.
-    for (auto &importer : getImpl().ModuleLoaders) {
-      if (importer->canImportModule(ModuleName, loc, nullptr))
-        return true;
-    }
+    
+    if (!isSourceCanImport)
+      return lookupModule();
+
+    // Otherwise, ask whether any module loader can load the module,
+    // and record the module version that the succeeding loader
+    // observed.
+    ModuleLoader::ModuleVersionInfo versionInfo, underlyingVersionInfo;
+    if (lookupVersionedModule(versionInfo, underlyingVersionInfo))
+      return true;
 
     if (isSourceCanImport)
       FailedModuleImportNames.insert(ModuleNameStr);
@@ -2722,43 +2792,15 @@ bool ASTContext::canImportModuleImpl(ImportPath::Module ModuleName,
   // We need to check whether the version of the module is high enough.
   // Retrieve a module version from each module loader that can find the module
   // and use the best source available for the query.
-  ModuleLoader::ModuleVersionInfo bestVersionInfo;
-  for (auto &importer : getImpl().ModuleLoaders) {
-    ModuleLoader::ModuleVersionInfo versionInfo;
-
-    if (!importer->canImportModule(ModuleName, loc, &versionInfo))
-      continue; // The loader can't find the module.
-
-    if (!versionInfo.isValid())
-      continue; // The loader didn't attempt to parse a version.
-
-    if (underlyingVersion && !isClangModuleVersion(versionInfo))
-      continue; // We're only matching Clang module versions.
-
-    if (bestVersionInfo.isValid() &&
-        versionInfo.getSourceKind() <= bestVersionInfo.getSourceKind())
-      continue; // This module version's source is lower priority.
-
-    bestVersionInfo = versionInfo;
-  }
-
-  if (!bestVersionInfo.isValid())
+  ModuleLoader::ModuleVersionInfo versionInfo, underlyingVersionInfo;
+  if (!lookupVersionedModule(versionInfo, underlyingVersionInfo))
     return false;
 
-  if (bestVersionInfo.getVersion().empty()) {
-    // The module version could not be parsed from the preferred source for
-    // this query. Diagnose and treat the query as if it succeeded.
-    auto mID = ModuleName[0];
-    auto diagLoc = mID.Loc;
-    if (mID.Loc.isInvalid())
-      diagLoc = loc;
-    Diags.diagnose(diagLoc, diag::cannot_find_project_version,
-                   getModuleVersionKindString(bestVersionInfo), mID.Item.str());
-    return true;
-  }
+  const auto &queryVersion = underlyingVersion ? underlyingVersionInfo : versionInfo;
+  if (queryVersion.getVersion().empty())
+    return missingVersion();
 
-  foundVersion = bestVersionInfo.getVersion();
-  return version <= bestVersionInfo.getVersion();
+  return version <= queryVersion.getVersion();
 }
 
 void ASTContext::forEachCanImportVersionCheck(
@@ -2773,22 +2815,26 @@ bool ASTContext::canImportModule(ImportPath::Module moduleName, SourceLoc loc,
                                  llvm::VersionTuple version,
                                  bool underlyingVersion) {
   llvm::VersionTuple versionInfo;
+  llvm::VersionTuple underlyingVersionInfo;
   if (!canImportModuleImpl(moduleName, loc, version, underlyingVersion, true,
-                           versionInfo))
+                           versionInfo, underlyingVersionInfo))
     return false;
 
   SmallString<64> fullModuleName;
   moduleName.getString(fullModuleName);
-  addSucceededCanImportModule(fullModuleName, underlyingVersion, versionInfo);
+  
+  addSucceededCanImportModule(fullModuleName, versionInfo, underlyingVersionInfo);
   return true;
 }
 
 bool ASTContext::testImportModule(ImportPath::Module ModuleName,
                                   llvm::VersionTuple version,
                                   bool underlyingVersion) const {
   llvm::VersionTuple versionInfo;
+  llvm::VersionTuple underlyingVersionInfo;
   return canImportModuleImpl(ModuleName, SourceLoc(), version,
-                             underlyingVersion, false, versionInfo);
+                             underlyingVersion, false, versionInfo,
+                             underlyingVersionInfo);
 }
 
 ModuleDecl *

test/CAS/can-import.swift

@@ -21,8 +21,10 @@
 // CMD-NEXT: "C"
 // CMD-NEXT: "1.0"
 // CMD-NEXT: "0"
-// CMD-NEXT: "-module-can-import"
+// CMD-NEXT: "-module-can-import-version"
 // CMD-NEXT: "D"
+// CMD-NEXT: "1.0"
+// CMD-NEXT: "0"
 // CMD-NEXT: "-module-can-import-version"
 // CMD-NEXT: "Simple"
 // CMD-NEXT: "0"
@@ -111,6 +113,11 @@ public func c() { }
 // swift-module-flags: -module-name D -O -disable-implicit-string-processing-module-import -disable-implicit-concurrency-module-import -parse-stdlib -user-module-version 1.0
 public func d() { }
 
+//--- include/Simple.swiftinterface
+// swift-interface-format-version: 1.0
+// swift-module-flags: -module-name Simple -O -disable-implicit-string-processing-module-import -disable-implicit-concurrency-module-import -parse-stdlib
+public func simple() { }
+
 //--- frameworks/Simple.framework/Modules/module.modulemap
 framework module Simple {
   umbrella header "Simple.h"

test/ClangImporter/can_import_underlying_version_tbd_missing_version.swift

@@ -16,8 +16,8 @@ func canImportUnderlyingVersion() {
 }
 
 func canImportVersion() {
-#if canImport(Simple, _version: 3.3) // expected-warning {{cannot find user version number for Clang module 'Simple'; version number ignored}}
-  // TODO(ParserValidation): expected-warning@-1 *{{cannot find user version number for Clang module 'Simple'; version number ignored}}
+#if canImport(Simple, _version: 3.3) // expected-warning {{cannot find user version number for module 'Simple'; version number ignored}}
+  // TODO(ParserValidation): expected-warning@-1 *{{cannot find user version number for module 'Simple'; version number ignored}}
   let a = 1  // expected-warning {{initialization of immutable value 'a' was never used; consider replacing with assignment to '_' or removing it}}
 #endif
 }

test/Parse/ConditionalCompilation/can_import_options.swift

@@ -9,9 +9,10 @@ func canImport() {
   let basicCheck = 1 // expected-warning {{initialization of immutable value 'basicCheck' was never used; consider replacing with assignment to '_' or removing it}}
 #endif
 
-#if canImport(Foo, _version: 1)
-  // No actual Foo to be imported since it is not versioned.
-  let versionCheck = 1
+#if canImport(Foo, _version: 1) // expected-warning {{cannot find user version number for module 'Foo'; version number ignored}}
+  // TODO(ParserValidation): expected-warning@-1 *{{cannot find user version number for module 'Foo'; version number ignored}}
+  // An unversioned 'Foo' causes versioned queries to evaluate to 'true'
+  let versionCheck = 1 // expected-warning {{initialization of immutable value 'versionCheck' was never used; consider replacing with assignment to '_' or removing it}}
 #endif
 }
 
@@ -72,7 +73,8 @@ func canImportVersioned() {
   let extraComponent = 1 // expected-warning {{initialization of immutable value 'extraComponent' was never used; consider replacing with assignment to '_' or removing it}}
 #endif
 
-#if canImport(Bar, _underlyingVersion: 113.33)
+#if canImport(Bar, _underlyingVersion: 113.33) // expected-warning{{cannot find user version number for Clang module 'Bar'; version number ignored}}
+  // TODO(ParserValidation): expected-warning@-1 *{{cannot find user version number for Clang module 'Bar'; version number ignored}}
   // Bar is an unversioned Swift module with no underlying clang module.
   let underlyingMinorSmaller = 1 // expected-warning {{initialization of immutable value 'underlyingMinorSmaller' was never used; consider replacing with assignment to '_' or removing it}}
 #endif

test/Parse/ConditionalCompilation/can_import_version.swift

@@ -67,8 +67,9 @@ func canImportVersioned() {
   let extraComponent = 1 // expected-warning {{initialization of immutable value 'extraComponent' was never used; consider replacing with assignment to '_' or removing it}}
 #endif
 
-#if canImport(Foo, _underlyingVersion: 113.33)
-  // Foo is an unversioned Swift module with no underlying clang module.
+#if canImport(Foo, _underlyingVersion: 113.33) // expected-warning {{cannot find user version number for Clang module 'Foo'; version number ignored}}
+  // TODO(ParserValidation): expected-warning@-1 *{{cannot find user version number for Clang module 'Foo'; version number ignored}}
+  // Foo is a Swift module with no underlying clang module.
   let underlyingMinorSmaller = 1 // expected-warning {{initialization of immutable value 'underlyingMinorSmaller' was never used; consider replacing with assignment to '_' or removing it}}
 #endif
 
@@ -135,8 +136,9 @@ func canImportVersionedString() {
   let extraComponent = 1 // expected-warning {{initialization of immutable value 'extraComponent' was never used; consider replacing with assignment to '_' or removing it}}
 #endif
 
-#if canImport(Foo, _underlyingVersion: "113.33")
-  // Foo is an unversioned Swift module with no underlying clang module.
+#if canImport(Foo, _underlyingVersion: "113.33") // expected-warning {{cannot find user version number for Clang module 'Foo'; version number ignored}}
+  // TODO(ParserValidation): expected-warning@-1 *{{cannot find user version number for Clang module 'Foo'; version number ignored}}
+  // Foo is a Swift module with no underlying clang module.
   let underlyingMinorSmaller = 1 // expected-warning {{initialization of immutable value 'underlyingMinorSmaller' was never used; consider replacing with assignment to '_' or removing it}}
 #endif
 }