[NFC] Improvements suggested in code review

beccadax · beccadax · commit 12286197c97e · 2020-02-18T11:08:36.000-08:00
Thank you, @hamishknight and @varungandhi-apple.
diff --git a/docs/Lexicon.rst b/docs/Lexicon.rst
@@ -264,10 +264,32 @@ source code, tests, and commit messages. See also the `LLVM lexicon`_.
     compiler can do something with it.
 
   overlay
-    A library that is imported whenever a C library or framework by the same
-    name is imported. The purpose of an overlay is to augment and extend a
-    library on the system when the library on the system cannot be modified.
-    Apple has a number of overlays for its own SDKs in stdlib/public/SDK/.
+    A wrapper library that is implicitly imported "on top of" another library.
+    It contains an @_exported import of the underlying library, but it augments
+    it with additional APIs which, for one reason or another, are not included
+    in the underlying library directly.
+
+    There are two kinds of overlays:
+
+    A "clang overlay" (the older kind, so it's often just called an "overlay")
+    is a Swift library that adds Swift-specific functionality to a C-family
+    library or framework. Clang overlays are used with system libraries that
+    cannot be modified to add Swift features. A clang overlay has the same
+    module name as the underlying library and can do a few special things that
+    normal modules can't, like adding required initializers to classes. If a
+    module has a clang overlay, the Clang Importer will always load it unless it
+    is actually compiling the overlay itself. Apple has a number of clang
+    overlays for its own SDKs in stdlib/public/Darwin/.
+
+    A "separately-imported overlay" is a separate module with its own name.
+    Unlike a clang overlay, it can be imported in some SourceFiles and not
+    others. When the compiler processes import declarations, it determines which
+    separately-imported overlays need to be imported and then adds them to the
+    list of imports for that file; name lookup also knows to look through the
+    overlay when it looks for declarations in the underlying module.
+    Separately-imported overlays are used to implement the "cross-import
+    overlays" feature, which is used to augment a module with additional
+    functionality when it is imported alongside another module.
 
   PCH
     Precompiled header, a type of file ending in .pch. A precompiled header is
diff --git a/include/swift/AST/Module.h b/include/swift/AST/Module.h
@@ -205,7 +205,6 @@ class ModuleDecl : public DeclContext, public TypeDecl {
 
   SmallVector<FileUnit *, 2> Files;
 
-  friend OverlayFile;
   llvm::SmallDenseMap<Identifier, SmallVector<OverlayFile *, 1>>
     declaredCrossImports;
 
@@ -268,7 +267,7 @@ class ModuleDecl : public DeclContext, public TypeDecl {
   /// reverse the positions of the two modules involved in the cross-import.
   void findDeclaredCrossImportOverlays(
       Identifier bystanderName, SmallVectorImpl<Identifier> &overlayNames,
-      SourceLoc diagLoc);
+      SourceLoc diagLoc) const;
 
   /// Get the list of all modules this module declares a cross-import with.
   void getDeclaredCrossImportBystanders(
diff --git a/lib/AST/Module.cpp b/lib/AST/Module.cpp
@@ -1464,19 +1464,18 @@ const clang::Module *ModuleDecl::findUnderlyingClangModule() const {
 namespace swift {
 /// Represents a file containing information about cross-module overlays.
 class OverlayFile {
-  /// If not empty, contains the name of the cross-import file that must be
-  /// loaded; \c overlayModuleNames is not valid and must be empty.
-  ///
-  /// If empty, \c overlayModuleNames is valid and will contain the list of
-  /// module names unless loading the file failed.
+  /// The file that data should be loaded from.
   StringRef filePath;
 
   /// The list of module names; empty if loading failed.
   llvm::TinyPtrVector<Identifier> overlayModuleNames;
 
+  enum class State { pending, loaded, failed };
+  State state = State::pending;
+
   /// Actually loads the overlay module name list. This should mutate
   /// \c overlayModuleNames, but not \c filePath.
-  void loadOverlayModuleNames(ModuleDecl *M, SourceLoc diagLoc,
+  bool loadOverlayModuleNames(const ModuleDecl *M, SourceLoc diagLoc,
                               Identifier bystandingModule);
 
 public:
@@ -1487,8 +1486,7 @@ class OverlayFile {
     return ctx.Allocate(bytes, alignment);
   }
 
-  OverlayFile(StringRef filePath)
-    : filePath(filePath), overlayModuleNames() { }
+  OverlayFile(StringRef filePath) : filePath(filePath) { }
 
   /// Returns the list of additional modules that should be imported if both
   /// the primary and secondary modules have been imported. This may load a
@@ -1497,13 +1495,12 @@ class OverlayFile {
   ///
   /// The result can be empty, either because of an error or because the file
   /// didn't contain any overlay module names.
-  ArrayRef<Identifier> getOverlayModuleNames(ModuleDecl *M, SourceLoc diagLoc,
+  ArrayRef<Identifier> getOverlayModuleNames(const ModuleDecl *M,
+                                             SourceLoc diagLoc,
                                              Identifier bystandingModule) {
-    if (!filePath.empty()) {
-      assert(overlayModuleNames.empty()
-             && "cross-import list can't be full before loading");
-      loadOverlayModuleNames(M, diagLoc, bystandingModule);
-      filePath = "";
+    if (state == State::pending) {
+      state = loadOverlayModuleNames(M, diagLoc, bystandingModule)
+            ? State::loaded : State::failed;
     }
     return overlayModuleNames;
   }
@@ -1521,12 +1518,12 @@ void ModuleDecl::addCrossImportOverlayFile(StringRef file) {
 void ModuleDecl::
 findDeclaredCrossImportOverlays(Identifier bystanderName,
                                 SmallVectorImpl<Identifier> &overlayNames,
-                                SourceLoc diagLoc) {
+                                SourceLoc diagLoc) const {
   if (getName() == bystanderName)
     // We don't currently support self-cross-imports.
     return;
 
-  for (auto &crossImportFile : declaredCrossImports[bystanderName])
+  for (auto &crossImportFile : declaredCrossImports.lookup(bystanderName))
     llvm::copy(crossImportFile->getOverlayModuleNames(this, diagLoc,
                                                       bystanderName),
                std::back_inserter(overlayNames));
@@ -1602,20 +1599,20 @@ OverlayFileContents::load(std::unique_ptr<llvm::MemoryBuffer> input,
   return contents;
 }
 
-void OverlayFile::loadOverlayModuleNames(ModuleDecl *M, SourceLoc diagLoc,
-                                         Identifier bystanderName) {
+bool
+OverlayFile::loadOverlayModuleNames(const ModuleDecl *M, SourceLoc diagLoc,
+                                    Identifier bystanderName) {
   assert(!filePath.empty());
 
   auto &ctx = M->getASTContext();
   llvm::vfs::FileSystem &fs = *ctx.SourceMgr.getFileSystem();
 
   auto bufOrError = fs.getBufferForFile(filePath);
   if (!bufOrError) {
-    M->getASTContext().Diags
-        .diagnose(diagLoc, diag::cannot_load_swiftoverlay_file,
-                  M->getName(), bystanderName, bufOrError.getError().message(),
-                  filePath);
-    return;
+    ctx.Diags.diagnose(diagLoc, diag::cannot_load_swiftoverlay_file,
+                       M->getName(), bystanderName,
+                       bufOrError.getError().message(), filePath);
+    return false;
   }
 
   SmallVector<std::string, 4> errorMessages;
@@ -1626,10 +1623,9 @@ void OverlayFile::loadOverlayModuleNames(ModuleDecl *M, SourceLoc diagLoc,
       errorMessages.push_back(contentsOrErr.getError().message());
 
     for (auto message : errorMessages)
-      M->getASTContext().Diags
-          .diagnose(diagLoc, diag::cannot_load_swiftoverlay_file,
-                    M->getName(), bystanderName, message, filePath);
-    return;
+      ctx.Diags.diagnose(diagLoc, diag::cannot_load_swiftoverlay_file,
+                         M->getName(), bystanderName, message, filePath);
+    return false;
   }
 
   auto contents = std::move(*contentsOrErr);
@@ -1638,6 +1634,8 @@ void OverlayFile::loadOverlayModuleNames(ModuleDecl *M, SourceLoc diagLoc,
     auto moduleIdent = ctx.getIdentifier(module.name);
     overlayModuleNames.push_back(moduleIdent);
   }
+
+  return true;
 }
 
 //===----------------------------------------------------------------------===//
diff --git a/lib/AST/ModuleLoader.cpp b/lib/AST/ModuleLoader.cpp
@@ -97,34 +97,51 @@ void ModuleLoader::findOverlayFiles(SourceLoc diagLoc, ModuleDecl *module,
 
   auto &langOpts = module->getASTContext().LangOpts;
 
+  // This method constructs several paths to directories near the module and
+  // scans them for .swiftoverlay files. These paths can be in various
+  // directories and have a few different filenames at the end, but I'll
+  // illustrate the path transformations by showing examples for a module
+  // defined by a swiftinterface at:
+  //
+  // /usr/lib/swift/FooKit.swiftmodule/x86_64-apple-macos.swiftinterface
+
+  // dirPath = /usr/lib/swift/FooKit.swiftmodule
   SmallString<64> dirPath{file->getModuleDefiningPath()};
-
   if (dirPath.empty())
     return;
+
+  // dirPath = /usr/lib/swift/
   path::remove_filename(dirPath);
 
-  // <parent-dir>/<module-name>.swiftcrossimport
+  // dirPath = /usr/lib/swift/FooKit.swiftcrossimport
   path::append(dirPath, file->getExportedModuleName());
   path::replace_extension(dirPath, getExtension(TY_SwiftCrossImportDir));
+
+  // Search for swiftoverlays that apply to all platforms.
   if (!findOverlayFilesInDirectory(diagLoc, dirPath, module, dependencyTracker))
     // If we diagnosed an error, or we didn't find the directory at all, don't
     // bother trying the target-specific directories.
     return;
 
-  // <parent-dir>/<module-name>.swiftcrossimport/<target-module-triple>
+  // dirPath = /usr/lib/swift/FooKit.swiftcrossimport/x86_64-apple-macos
   auto moduleTriple = getTargetSpecificModuleTriple(langOpts.Target);
   path::append(dirPath, moduleTriple.str());
+
+  // Search for swiftoverlays specific to the target triple's platform.
   findOverlayFilesInDirectory(diagLoc, dirPath, module, dependencyTracker);
 
+  // The rest of this handles target variant triples, which are only used for
+  // certain MacCatalyst builds.
   if (!langOpts.TargetVariant)
     return;
 
+  // dirPath = /usr/lib/swift/FooKit.swiftcrossimport/x86_64-apple-ios-macabi
   path::remove_filename(dirPath);
-
-  // <parent-dir>/<module-name>.swiftcrossimport/<targetvariant-module-triple>
   auto moduleVariantTriple =
       getTargetSpecificModuleTriple(*langOpts.TargetVariant);
   path::append(dirPath, moduleVariantTriple.str());
+
+  // Search for swiftoverlays specific to the target variant's platform.
   findOverlayFilesInDirectory(diagLoc, dirPath, module, dependencyTracker);
 }
 
diff --git a/lib/Sema/NameBinding.cpp b/lib/Sema/NameBinding.cpp
@@ -594,8 +594,9 @@ void UnboundImport::diagnoseInvalidAttr(DeclAttrKind attrKind,
 BoundImport::BoundImport(UnboundImport unbound, ImportedModuleDesc desc,
                          ModuleDecl *module, bool needsScopeValidation)
   : unbound(unbound), desc(desc), module(module),
-    needsScopeValidation(needsScopeValidation)
-{ }
+    needsScopeValidation(needsScopeValidation) {
+  assert(module && "Can't have an import bound to nothing");
+}
 
 void NameBinder::finishImports(SmallVectorImpl<ImportedModuleDesc> &imports) {
   for (auto &unvalidated : unvalidatedImports) {
@@ -674,11 +675,6 @@ void BoundImport::validateScope(SourceFile &SF) {
   if (unbound.importKind.Item == ImportKind::Module || !needsScopeValidation)
     return;
 
-  // Per getTopLevelModule(), topLevelModule should only be nullptr if we are
-  // importing a submodule, and we should never do a scoped import of a
-  // submodule.
-//  assert(topLevelModule && "scoped import of a submodule import?");
-
   ASTContext &ctx = SF.getASTContext();
 
   // If we're importing a specific decl, validate the import kind.
@@ -797,6 +793,21 @@ UnboundImport::UnboundImport(ASTContext &ctx,
 }
 
 void NameBinder::crossImport(ModuleDecl *M, UnboundImport &I) {
+  // FIXME: There is a fundamental problem with this find-as-we-go approach:
+  // The '@_exported import'-ed modules in this module's other files should be
+  // taken into account, but they haven't been bound yet, and binding them would
+  // require cross-importing. Chicken, meet egg.
+  //
+  // The way to fix this is probably to restructure name binding so we first
+  // bind all exported imports in all files, then bind all other imports in each
+  // file. This may become simpler if we bind all ImportDecls before we start
+  // computing cross-imports, but I haven't figured that part out yet.
+  //
+  // Fixing this is tracked within Apple by rdar://problem/59527118. I haven't
+  // filed an SR because I plan to address it myself, but if this comment is
+  // still here in April 2020 without an SR number, please file a Swift bug and
+  // harass @brentdax to fill in the details.
+
   if (!SF.shouldCrossImport())
     return;
 
diff --git a/test/CrossImport/horrible.swift b/test/CrossImport/horrible.swift
@@ -16,7 +16,7 @@ import UnitaryGoose
 // expected-error@-1 {{cannot list cross-import overlays for 'UnitaryGoose': Not a directory}}
 
 // FIXME: It might be better to diagnose these errors on HorribleGoose's import
-// decl, since they actually belong to HorribleGoose.
+// decl, since they actually belong to HorribleGoose. (SR-12223)
 
 import FlockOfGoose
 // expected-error@-1 {{cannot load cross-import overlay for 'HorribleGoose' and 'FlockOfGoose': Is a directory}}
