[ParseableInterfaces] Handle swiftdoc files correctly

jrose-apple · jrose-apple · commit 5417ca0aa296 · 2018-12-21T15:37:32.000-08:00
The previous 'openModuleFiles' interface in SerializedModuleLoaderBase
still assumed that swiftmodule files and swiftdoc files would be found
next to each other, but that's not true anymore with
swiftinterfaces-built-to-modules. Give up on this assumption (and on
the minor optimization of passing down a scratch buffer) and split out
the interface into the customization point
'findModuleFilesInDirectory' and the implementation 'openModuleFiles'.
The latter now takes two full paths: one for the swiftmodule, one for
the swiftdoc.
diff --git a/include/swift/Frontend/ParseableInterfaceSupport.h b/include/swift/Frontend/ParseableInterfaceSupport.h
@@ -79,12 +79,11 @@ class ParseableInterfaceModuleLoader : public SerializedModuleLoaderBase {
   static void configureSubInvocationInputsAndOutputs(
     CompilerInvocation &SubInvocation, StringRef InPath, StringRef OutPath);
 
-  std::error_code
-  openModuleFiles(AccessPathElem ModuleID, StringRef DirName,
-                  StringRef ModuleFilename, StringRef ModuleDocFilename,
-                  std::unique_ptr<llvm::MemoryBuffer> *ModuleBuffer,
-                  std::unique_ptr<llvm::MemoryBuffer> *ModuleDocBuffer,
-                  llvm::SmallVectorImpl<char> &Scratch) override;
+  std::error_code findModuleFilesInDirectory(
+      AccessPathElem ModuleID, StringRef DirPath, StringRef ModuleFilename,
+      StringRef ModuleDocFilename,
+      std::unique_ptr<llvm::MemoryBuffer> *ModuleBuffer,
+      std::unique_ptr<llvm::MemoryBuffer> *ModuleDocBuffer) override;
 
 public:
   static std::unique_ptr<ParseableInterfaceModuleLoader>
diff --git a/include/swift/Serialization/SerializedModuleLoader.h b/include/swift/Serialization/SerializedModuleLoader.h
@@ -51,12 +51,17 @@ class SerializedModuleLoaderBase : public ModuleLoader {
                   std::unique_ptr<llvm::MemoryBuffer> *moduleDocBuffer,
                   bool &isFramework);
 
-  virtual std::error_code
-  openModuleFiles(AccessPathElem ModuleID, StringRef DirName,
-                  StringRef ModuleFilename, StringRef ModuleDocFilename,
+  virtual std::error_code findModuleFilesInDirectory(
+      AccessPathElem ModuleID, StringRef DirPath, StringRef ModuleFilename,
+      StringRef ModuleDocFilename,
+      std::unique_ptr<llvm::MemoryBuffer> *ModuleBuffer,
+      std::unique_ptr<llvm::MemoryBuffer> *ModuleDocBuffer) = 0;
+
+  std::error_code
+  openModuleFiles(AccessPathElem ModuleID,
+                  StringRef ModulePath, StringRef ModuleDocPath,
                   std::unique_ptr<llvm::MemoryBuffer> *ModuleBuffer,
-                  std::unique_ptr<llvm::MemoryBuffer> *ModuleDocBuffer,
-                  llvm::SmallVectorImpl<char> &Scratch);
+                  std::unique_ptr<llvm::MemoryBuffer> *ModuleDocBuffer);
 
   /// If the module loader subclass knows that all options have been tried for
   /// loading an architecture-specific file out of a swiftmodule bundle, try
@@ -128,12 +133,11 @@ class SerializedModuleLoader : public SerializedModuleLoaderBase {
     : SerializedModuleLoaderBase(ctx, tracker, loadMode)
   {}
 
-  std::error_code
-  openModuleFiles(AccessPathElem ModuleID, StringRef DirName,
-                  StringRef ModuleFilename, StringRef ModuleDocFilename,
-                  std::unique_ptr<llvm::MemoryBuffer> *ModuleBuffer,
-                  std::unique_ptr<llvm::MemoryBuffer> *ModuleDocBuffer,
-                  llvm::SmallVectorImpl<char> &Scratch) override;
+  std::error_code findModuleFilesInDirectory(
+      AccessPathElem ModuleID, StringRef DirPath, StringRef ModuleFilename,
+      StringRef ModuleDocFilename,
+      std::unique_ptr<llvm::MemoryBuffer> *ModuleBuffer,
+      std::unique_ptr<llvm::MemoryBuffer> *ModuleDocBuffer) override;
 
   bool maybeDiagnoseArchitectureMismatch(SourceLoc sourceLocation,
                                          StringRef moduleName,
diff --git a/lib/Frontend/ParseableInterfaceSupport.cpp b/lib/Frontend/ParseableInterfaceSupport.cpp
@@ -168,7 +168,7 @@ createInvocationForBuildingFromInterface(ASTContext &Ctx, StringRef ModuleName,
 static void computeCachedOutputPath(ASTContext &Ctx,
                                     const CompilerInvocation &SubInvocation,
                                     StringRef InPath,
-                                    llvm::SmallString<128> &OutPath) {
+                                    llvm::SmallString<256> &OutPath) {
   OutPath = SubInvocation.getClangModuleCachePath();
   llvm::sys::path::append(OutPath, SubInvocation.getModuleName());
   OutPath.append("-");
@@ -437,12 +437,11 @@ static bool serializedASTLooksValidOrCannotBeRead(clang::vfs::FileSystem &FS,
 /// Load a .swiftmodule associated with a .swiftinterface either from a
 /// cache or by converting it in a subordinate \c CompilerInstance, caching
 /// the results.
-std::error_code ParseableInterfaceModuleLoader::openModuleFiles(
-    AccessPathElem ModuleID, StringRef DirName, StringRef ModuleFilename,
+std::error_code ParseableInterfaceModuleLoader::findModuleFilesInDirectory(
+    AccessPathElem ModuleID, StringRef DirPath, StringRef ModuleFilename,
     StringRef ModuleDocFilename,
     std::unique_ptr<llvm::MemoryBuffer> *ModuleBuffer,
-    std::unique_ptr<llvm::MemoryBuffer> *ModuleDocBuffer,
-    llvm::SmallVectorImpl<char> &Scratch) {
+    std::unique_ptr<llvm::MemoryBuffer> *ModuleDocBuffer) {
 
   namespace path = llvm::sys::path;
 
@@ -452,10 +451,10 @@ std::error_code ParseableInterfaceModuleLoader::openModuleFiles(
 
   auto &FS = *Ctx.SourceMgr.getFileSystem();
   auto &Diags = Ctx.Diags;
-  llvm::SmallString<128> ModPath, InPath, OutPath;
+  llvm::SmallString<256> ModPath, InPath, OutPath;
 
   // First check to see if the .swiftinterface exists at all. Bail if not.
-  ModPath = DirName;
+  ModPath = DirPath;
   path::append(ModPath, ModuleFilename);
 
   auto Ext = file_types::getExtension(file_types::TY_SwiftParseableInterfaceFile);
@@ -531,12 +530,10 @@ std::error_code ParseableInterfaceModuleLoader::openModuleFiles(
   // routine that can load the recently-manufactured serialized module.
   LLVM_DEBUG(llvm::dbgs() << "Loading " << OutPath
              << " via normal module loader\n");
-  // FIXME: This will never find the swiftdoc file, because that's in the
-  // original directory. We should probably just not try to match the signature
-  // of the overridable entry point.
+  llvm::SmallString<256> DocPath{DirPath};
+  path::append(DocPath, ModuleDocFilename);
   auto ErrorCode = SerializedModuleLoaderBase::openModuleFiles(
-      ModuleID, path::parent_path(OutPath), path::filename(OutPath),
-      ModuleDocFilename, ModuleBuffer, ModuleDocBuffer, Scratch);
+      ModuleID, OutPath, DocPath, ModuleBuffer, ModuleDocBuffer);
   LLVM_DEBUG(llvm::dbgs() << "Loaded " << OutPath
              << " via normal module loader");
   if (ErrorCode) {
diff --git a/lib/Serialization/SerializedModuleLoader.cpp b/lib/Serialization/SerializedModuleLoader.cpp
@@ -43,11 +43,9 @@ SerializedModuleLoaderBase::~SerializedModuleLoaderBase() = default;
 SerializedModuleLoader::~SerializedModuleLoader() = default;
 
 std::error_code SerializedModuleLoaderBase::openModuleFiles(
-    AccessPathElem ModuleID, StringRef DirName, StringRef ModuleFilename,
-    StringRef ModuleDocFilename,
+    AccessPathElem ModuleID, StringRef ModulePath, StringRef ModuleDocPath,
     std::unique_ptr<llvm::MemoryBuffer> *ModuleBuffer,
-    std::unique_ptr<llvm::MemoryBuffer> *ModuleDocBuffer,
-    llvm::SmallVectorImpl<char> &Scratch) {
+    std::unique_ptr<llvm::MemoryBuffer> *ModuleDocBuffer) {
   assert(((ModuleBuffer && ModuleDocBuffer) ||
           (!ModuleBuffer && !ModuleDocBuffer)) &&
          "Module and Module Doc buffer must both be initialized or NULL");
@@ -56,12 +54,11 @@ std::error_code SerializedModuleLoaderBase::openModuleFiles(
 
   // Try to open the module file first.  If we fail, don't even look for the
   // module documentation file.
-  Scratch.clear();
-  llvm::sys::path::append(Scratch, DirName, ModuleFilename);
+
   // If there are no buffers to load into, simply check for the existence of
   // the module file.
   if (!(ModuleBuffer || ModuleDocBuffer)) {
-    llvm::ErrorOr<clang::vfs::Status> statResult = FS.status(Scratch);
+    llvm::ErrorOr<clang::vfs::Status> statResult = FS.status(ModulePath);
     if (!statResult)
       return statResult.getError();
     if (!statResult->exists())
@@ -72,16 +69,14 @@ std::error_code SerializedModuleLoaderBase::openModuleFiles(
   }
 
   llvm::ErrorOr<std::unique_ptr<llvm::MemoryBuffer>> ModuleOrErr =
-      FS.getBufferForFile(StringRef(Scratch.data(), Scratch.size()));
+      FS.getBufferForFile(ModulePath);
   if (!ModuleOrErr)
     return ModuleOrErr.getError();
 
   // Try to open the module documentation file.  If it does not exist, ignore
   // the error.  However, pass though all other errors.
-  Scratch.clear();
-  llvm::sys::path::append(Scratch, DirName, ModuleDocFilename);
   llvm::ErrorOr<std::unique_ptr<llvm::MemoryBuffer>> ModuleDocOrErr =
-      FS.getBufferForFile(StringRef(Scratch.data(), Scratch.size()));
+      FS.getBufferForFile(ModuleDocPath);
   if (!ModuleDocOrErr &&
       ModuleDocOrErr.getError() != std::errc::no_such_file_or_directory) {
     return ModuleDocOrErr.getError();
@@ -94,20 +89,23 @@ std::error_code SerializedModuleLoaderBase::openModuleFiles(
   return std::error_code();
 }
 
-std::error_code SerializedModuleLoader::openModuleFiles(
-    AccessPathElem ModuleID, StringRef DirName, StringRef ModuleFilename,
+std::error_code SerializedModuleLoader::findModuleFilesInDirectory(
+    AccessPathElem ModuleID, StringRef DirPath, StringRef ModuleFilename,
     StringRef ModuleDocFilename,
     std::unique_ptr<llvm::MemoryBuffer> *ModuleBuffer,
-    std::unique_ptr<llvm::MemoryBuffer> *ModuleDocBuffer,
-    llvm::SmallVectorImpl<char> &Scratch) {
+    std::unique_ptr<llvm::MemoryBuffer> *ModuleDocBuffer) {
   if (LoadMode == ModuleLoadingMode::OnlyParseable)
     return std::make_error_code(std::errc::not_supported);
 
-  return SerializedModuleLoaderBase::openModuleFiles(ModuleID, DirName,
-                                                     ModuleFilename,
-                                                     ModuleDocFilename,
+  llvm::SmallString<256> ModulePath{DirPath};
+  llvm::sys::path::append(ModulePath, ModuleFilename);
+  llvm::SmallString<256> ModuleDocPath{DirPath};
+  llvm::sys::path::append(ModuleDocPath, ModuleDocFilename);
+  return SerializedModuleLoaderBase::openModuleFiles(ModuleID,
+                                                     ModulePath,
+                                                     ModuleDocPath,
                                                      ModuleBuffer,
-                                                     ModuleDocBuffer, Scratch);
+                                                     ModuleDocBuffer);
 }
 
 bool SerializedModuleLoader::maybeDiagnoseArchitectureMismatch(
@@ -194,10 +192,7 @@ SerializedModuleLoaderBase::findModule(AccessPathElem moduleID,
   auto &fs = *Ctx.SourceMgr.getFileSystem();
   isFramework = false;
 
-  // Declare some reusable buffers up front; if we end up spilling onto the
-  // heap once, we're likely to do so again.
-  llvm::SmallString<128> scratch;
-  llvm::SmallString<128> currPath;
+  llvm::SmallString<256> currPath;
   for (auto path : Ctx.SearchPathOpts.ImportSearchPaths) {
     std::error_code result;
 
@@ -207,18 +202,17 @@ SerializedModuleLoaderBase::findModule(AccessPathElem moduleID,
 
     if (statResult && statResult->isDirectory()) {
       // A .swiftmodule directory contains architecture-specific files.
-      result = openModuleFiles(moduleID, currPath,
-                               archFileNames.first, archFileNames.second,
-                               moduleBuffer, moduleDocBuffer,
-                               scratch);
+      result = findModuleFilesInDirectory(moduleID, currPath,
+                                          archFileNames.first,
+                                          archFileNames.second,
+                                          moduleBuffer, moduleDocBuffer);
 
       if (result == std::errc::no_such_file_or_directory &&
           !alternateArchName.empty()) {
-        result = openModuleFiles(moduleID, currPath,
-                                 alternateArchFileNames.first,
-                                 alternateArchFileNames.second,
-                                 moduleBuffer, moduleDocBuffer,
-                                 scratch);
+        result = findModuleFilesInDirectory(moduleID, currPath,
+                                            alternateArchFileNames.first,
+                                            alternateArchFileNames.second,
+                                            moduleBuffer, moduleDocBuffer);
       }
 
       if (result == std::errc::no_such_file_or_directory) {
@@ -231,10 +225,10 @@ SerializedModuleLoaderBase::findModule(AccessPathElem moduleID,
     } else {
       // We can't just return the error; the path we're looking for might not
       // be "Foo.swiftmodule".
-      result = openModuleFiles(moduleID, path,
-                               moduleFilename.str(), moduleDocFilename.str(),
-                               moduleBuffer, moduleDocBuffer,
-                               scratch);
+      result = findModuleFilesInDirectory(moduleID, path,
+                                          moduleFilename.str(),
+                                          moduleDocFilename.str(),
+                                          moduleBuffer, moduleDocBuffer);
     }
     if (!result)
       return true;
@@ -257,16 +251,17 @@ SerializedModuleLoaderBase::findModule(AccessPathElem moduleID,
       // Frameworks always use architecture-specific files within a .swiftmodule
       // directory.
       llvm::sys::path::append(currPath, "Modules", moduleFilename.str());
-      auto err = openModuleFiles(moduleID, currPath,
-                                 archFileNames.first, archFileNames.second,
-                                 moduleBuffer, moduleDocBuffer, scratch);
+      auto err = findModuleFilesInDirectory(moduleID, currPath,
+                                            archFileNames.first,
+                                            archFileNames.second,
+                                            moduleBuffer, moduleDocBuffer);
 
       if (err == std::errc::no_such_file_or_directory &&
           !alternateArchName.empty()) {
-        err = openModuleFiles(moduleID, currPath,
-                              alternateArchFileNames.first,
-                              alternateArchFileNames.second,
-                              moduleBuffer, moduleDocBuffer, scratch);
+        err = findModuleFilesInDirectory(moduleID, currPath,
+                                         alternateArchFileNames.first,
+                                         alternateArchFileNames.second,
+                                         moduleBuffer, moduleDocBuffer);
       }
 
       if (err == std::errc::no_such_file_or_directory) {
@@ -287,6 +282,7 @@ SerializedModuleLoaderBase::findModule(AccessPathElem moduleID,
     if (Ctx.LangOpts.Target.isOSDarwin()) {
       // Apple platforms have extra implicit framework search paths:
       // $SDKROOT/System/Library/Frameworks/ and $SDKROOT/Library/Frameworks/
+      SmallString<256> scratch;
       scratch = Ctx.SearchPathOpts.SDKPath;
       llvm::sys::path::append(scratch, "System", "Library", "Frameworks");
       if (tryFrameworkImport(scratch))
@@ -305,9 +301,10 @@ SerializedModuleLoaderBase::findModule(AccessPathElem moduleID,
 
   // Search the runtime import path.
   isFramework = false;
-  return !openModuleFiles(moduleID, Ctx.SearchPathOpts.RuntimeLibraryImportPath,
-                          moduleFilename.str(), moduleDocFilename.str(),
-                          moduleBuffer, moduleDocBuffer, scratch);
+  return !findModuleFilesInDirectory(
+      moduleID, Ctx.SearchPathOpts.RuntimeLibraryImportPath,
+      moduleFilename.str(), moduleDocFilename.str(), moduleBuffer,
+      moduleDocBuffer);
 }
 
 static std::pair<StringRef, clang::VersionTuple>
diff --git a/test/ParseableInterface/ModuleCache/swiftdoc-next-to-swiftinterface.swift b/test/ParseableInterface/ModuleCache/swiftdoc-next-to-swiftinterface.swift
@@ -0,0 +1,25 @@
+// RUN: %empty-directory(%t)
+// RUN: %target-swift-frontend -emit-module -emit-parseable-module-interface-path %t/Lib.swiftinterface -emit-module-doc -parse-stdlib -o %t/Lib.swiftmodule %s
+// RUN: %target-swift-ide-test -print-module -module-to-print=Lib -access-filter-public -I %t -source-filename=x > %t/from-module.txt
+// RUN: %FileCheck %s < %t/from-module.txt
+
+// RUN: rm %t/Lib.swiftmodule
+// RUN: env SWIFT_FORCE_MODULE_LOADING=prefer-serialized %target-swift-ide-test -print-module -module-to-print=Lib -access-filter-public -I %t -source-filename=x > %t/from-interface.txt
+// RUN: diff %t/from-module.txt %t/from-interface.txt
+
+// Try again with architecture-specific subdirectories.
+// RUN: %empty-directory(%t)
+// RUN: %empty-directory(%t/Lib.swiftmodule)
+// RUN: %target-swift-frontend -emit-module -emit-parseable-module-interface-path %t/Lib.swiftmodule/$(basename %target-swiftmodule-name .swiftmodule).swiftinterface -emit-module-doc -parse-stdlib -o %t/Lib.swiftmodule/%target-swiftmodule-name -module-name Lib %s
+// RUN: %target-swift-ide-test -print-module -module-to-print=Lib -access-filter-public -I %t -source-filename=x > %t/from-module.txt
+// RUN: %FileCheck %s < %t/from-module.txt
+
+// RUN: rm %t/Lib.swiftmodule/%target-swiftmodule-name
+// RUN: env SWIFT_FORCE_MODULE_LOADING=prefer-serialized %target-swift-ide-test -print-module -module-to-print=Lib -access-filter-public -I %t -source-filename=x > %t/from-interface.txt
+// RUN: diff %t/from-module.txt %t/from-interface.txt
+
+/// Very important documentation!
+public struct SomeStructWithDocumentation {}
+
+// CHECK: Very important documentation!
+// CHECK-NEXT: struct SomeStructWithDocumentation {
