Merge pull request swiftlang#80414 from artemcm/RestorePriorCanImportBehaviour

[Explicit Module Builds] Switch versioned `canImport` to return `true` when encountering unversioned candidate

Author: artemcm

include/swift/AST/ASTContext.h

@@ -1192,12 +1192,14 @@ class ASTContext final {
   /// module is loaded in full.
   bool canImportModuleImpl(ImportPath::Module ModulePath, SourceLoc loc,
                            llvm::VersionTuple version, bool underlyingVersion,
-                           bool updateFailingList,
-                           llvm::VersionTuple &foundVersion) const;
+                           bool isSourceCanImport,
+                           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

@@ -849,18 +849,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 {
@@ -2655,34 +2653,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 updateFailingList,
-                                     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();
@@ -2691,6 +2680,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.
@@ -2699,35 +2702,95 @@ bool ASTContext::canImportModuleImpl(ImportPath::Module ModuleName,
     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;
+    const auto &foundComparisonVersion = underlyingVersion
+                                      ? Found->second.UnderlyingVersion
+                                      : Found->second.Version;
+    if (!foundComparisonVersion.empty())
+      return version <= foundComparisonVersion;
+    else
+      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 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())
+    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;
+    
+    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;
 
-    // Otherwise, ask whether any module loader can load the module.
-    for (auto &importer : getImpl().ModuleLoaders) {
-      if (importer->canImportModule(ModuleName, loc, nullptr))
-        return true;
-    }
-
-    if (updateFailingList)
+    if (isSourceCanImport)
       FailedModuleImportNames.insert(ModuleNameStr);
 
     return false;
@@ -2736,43 +2799,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(
@@ -2787,22 +2822,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,9 +73,10 @@ 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)
-  // Bar is a Swift module with no underlying clang module.
-  let underlyingMinorSmaller = 1
+#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
 
 #if canImport(Bar)