Refactor FindPreludeFiles into InstallPaths (#4268)

From the driver's perspective, `FindPreludeFiles` is closely tied to
`compile`. This makes it difficult to refactor commands without
affecting the test dependencies on `FindPreludeFiles`. `InstallPaths`
seems like a decent home since it is responsible for the install
structure.

I'm switching to an `Error` return to allow callers to choose how to
handle it (e.g., in file tests, we typically don't want the direct error
stream).

testing/base/BUILD

@@ -70,6 +70,7 @@ cc_test(
         ":source_gen_lib",
         "//common:set",
         "//toolchain/driver",
+        "//toolchain/install:install_paths_test_helpers",
         "@googletest//:gtest",
         "@llvm-project//llvm:Support",
     ],

testing/base/source_gen_test.cpp

@@ -10,6 +10,7 @@
 #include "common/set.h"
 #include "testing/base/global_exe_path.h"
 #include "toolchain/driver/driver.h"
+#include "toolchain/install/install_paths_test_helpers.h"
 
 namespace Carbon::Testing {
 namespace {
@@ -147,19 +148,7 @@ auto TestCompile(llvm::StringRef source) -> bool {
       InstallPaths::MakeForBazelRunfiles(Testing::GetExePath()));
   Driver driver(fs, &installation, llvm::outs(), llvm::errs());
 
-  // Load the prelude into our VFS.
-  //
-  // TODO: Factor this and analogous code in file_test into a Driver helper.
-  auto prelude =
-      Driver::FindPreludeFiles(installation.core_package(), llvm::errs());
-  CARBON_CHECK(!prelude.empty());
-  for (const auto& path : prelude) {
-    llvm::ErrorOr<std::unique_ptr<llvm::MemoryBuffer>> file =
-        llvm::MemoryBuffer::getFile(path);
-    CARBON_CHECK(file) << file.getError().message();
-    CARBON_CHECK(fs.addFile(path, /*ModificationTime=*/0, std::move(*file)))
-        << "Duplicate file: " << path;
-  }
+  AddPreludeFilesToVfs(installation, &fs);
 
   fs.addFile("test.carbon", /*ModificationTime=*/0,
              llvm::MemoryBuffer::getMemBuffer(source));

toolchain/driver/BUILD

@@ -62,6 +62,7 @@ cc_binary(
         "//testing/base:benchmark_main",
         "//testing/base:global_exe_path",
         "//testing/base:source_gen_lib",
+        "//toolchain/install:install_paths_test_helpers",
         "@google_benchmark//:benchmark",
         "@llvm-project//llvm:Support",
     ],

toolchain/driver/compile_benchmark.cpp

@@ -9,6 +9,7 @@
 #include "testing/base/global_exe_path.h"
 #include "testing/base/source_gen.h"
 #include "toolchain/driver/driver.h"
+#include "toolchain/install/install_paths_test_helpers.h"
 
 namespace Carbon::Testing {
 namespace {
@@ -22,19 +23,7 @@ class CompileBenchmark {
   CompileBenchmark()
       : installation_(InstallPaths::MakeForBazelRunfiles(GetExePath())),
         driver_(fs_, &installation_, llvm::outs(), llvm::errs()) {
-    // Load the prelude into our VFS.
-    //
-    // TODO: Factor this and analogous code in file_test into a Driver helper.
-    auto prelude =
-        Driver::FindPreludeFiles(installation_.core_package(), llvm::errs());
-    CARBON_CHECK(!prelude.empty());
-    for (const auto& path : prelude) {
-      llvm::ErrorOr<std::unique_ptr<llvm::MemoryBuffer>> file =
-          llvm::MemoryBuffer::getFile(path);
-      CARBON_CHECK(file) << file.getError().message();
-      CARBON_CHECK(fs_.addFile(path, /*ModificationTime=*/0, std::move(*file)))
-          << "Duplicate file: " << path;
-    }
+    AddPreludeFilesToVfs(installation_, &fs_);
   }
 
   // Setup a set of source files in the VFS for the driver. Each string input is

toolchain/driver/driver.cpp

@@ -34,44 +34,6 @@
 
 namespace Carbon {
 
-auto Driver::FindPreludeFiles(llvm::StringRef core_package_dir,
-                              llvm::raw_ostream& error_stream)
-    -> llvm::SmallVector<std::string> {
-  llvm::SmallVector<std::string> result;
-
-  // Include <data>/core/prelude.carbon, which is the entry point into the
-  // prelude.
-  {
-    llvm::SmallString<256> prelude_file(core_package_dir);
-    llvm::sys::path::append(prelude_file, llvm::sys::path::Style::posix,
-                            "prelude.carbon");
-    result.push_back(prelude_file.str().str());
-  }
-
-  // Glob for <data>/core/prelude/**/*.carbon and add all the files we find.
-  llvm::SmallString<256> prelude_dir(core_package_dir);
-  llvm::sys::path::append(prelude_dir, llvm::sys::path::Style::posix,
-                          "prelude");
-  std::error_code ec;
-  for (llvm::sys::fs::recursive_directory_iterator prelude_files_it(
-           prelude_dir, ec, /*follow_symlinks=*/false);
-       prelude_files_it != llvm::sys::fs::recursive_directory_iterator();
-       prelude_files_it.increment(ec)) {
-    if (ec) {
-      error_stream << "ERROR: Could not find prelude: " << ec.message() << "\n";
-      result.clear();
-      return result;
-    }
-
-    auto prelude_file = prelude_files_it->path();
-    if (llvm::sys::path::extension(prelude_file) == ".carbon") {
-      result.push_back(prelude_file);
-    }
-  }
-
-  return result;
-}
-
 struct Driver::CodegenOptions {
   void Build(CommandLine::CommandBuilder& b) {
     b.AddStringOption(
@@ -865,13 +827,14 @@ auto Driver::Compile(const CompileOptions& options,
   // Find the files comprising the prelude if we are importing it.
   // TODO: Replace this with a search for library api files in a
   // package-specific search path based on the library name.
-  bool want_prelude =
-      options.prelude_import && options.phase >= CompileOptions::Phase::Check;
-  auto prelude = want_prelude ? FindPreludeFiles(installation_->core_package(),
-                                                 error_stream_)
-                              : llvm::SmallVector<std::string>{};
-  if (want_prelude && prelude.empty()) {
-    return {.success = false};
+  llvm::SmallVector<std::string> prelude;
+  if (options.prelude_import && options.phase >= CompileOptions::Phase::Check) {
+    if (auto find = installation_->FindPreludeFiles(); find.ok()) {
+      prelude = std::move(*find);
+    } else {
+      error_stream_ << "ERROR: " << find.error() << "\n";
+      return {.success = false};
+    }
   }
 
   // Prepare CompilationUnits before building scope exit handlers.

toolchain/driver/driver.h

@@ -49,13 +49,6 @@ class Driver {
   // error stream (stderr by default).
   auto RunCommand(llvm::ArrayRef<llvm::StringRef> args) -> RunResult;
 
-  // Finds the source files that define the prelude and returns a list of their
-  // filenames. On error, writes a message to `error_stream` and returns an
-  // empty list.
-  static auto FindPreludeFiles(llvm::StringRef core_package_dir,
-                               llvm::raw_ostream& error_stream)
-      -> llvm::SmallVector<std::string>;
-
  private:
   struct Options;
   struct CodegenOptions;

toolchain/install/BUILD

@@ -152,6 +152,18 @@ cc_test(
     ],
 )
 
+cc_library(
+    name = "install_paths_test_helpers",
+    testonly = 1,
+    srcs = ["install_paths_test_helpers.cpp"],
+    hdrs = ["install_paths_test_helpers.h"],
+    deps = [
+        ":install_paths",
+        "//testing/base:global_exe_path",
+        "@llvm-project//llvm:Support",
+    ],
+)
+
 # Build rules to construct packaged versions of the toolchain's install.
 
 pkg_files(

toolchain/install/install_paths.cpp

@@ -76,6 +76,46 @@ auto InstallPaths::Make(llvm::StringRef install_prefix) -> InstallPaths {
   return paths;
 }
 
+auto InstallPaths::FindPreludeFiles() const
+    -> ErrorOr<llvm::SmallVector<std::string>> {
+  // This is structured to avoid a vector copy on success.
+  ErrorOr<llvm::SmallVector<std::string>> result =
+      llvm::SmallVector<std::string>();
+
+  std::string dir = core_package();
+
+  // Include <data>/core/prelude.carbon, which is the entry point into the
+  // prelude.
+  {
+    llvm::SmallString<256> prelude_file(dir);
+    llvm::sys::path::append(prelude_file, llvm::sys::path::Style::posix,
+                            "prelude.carbon");
+    result->push_back(prelude_file.str().str());
+  }
+
+  // Glob for <data>/core/prelude/**/*.carbon and add all the files we find.
+  llvm::SmallString<256> prelude_dir(dir);
+  llvm::sys::path::append(prelude_dir, llvm::sys::path::Style::posix,
+                          "prelude");
+  std::error_code ec;
+  for (llvm::sys::fs::recursive_directory_iterator prelude_files_it(
+           prelude_dir, ec, /*follow_symlinks=*/false);
+       prelude_files_it != llvm::sys::fs::recursive_directory_iterator();
+       prelude_files_it.increment(ec)) {
+    if (ec) {
+      result = ErrorBuilder() << "Could not find prelude: " << ec.message();
+      return result;
+    }
+
+    auto prelude_file = prelude_files_it->path();
+    if (llvm::sys::path::extension(prelude_file) == ".carbon") {
+      result->push_back(prelude_file);
+    }
+  }
+
+  return result;
+}
+
 auto InstallPaths::SetError(llvm::Twine message) -> void {
   // Use an empty prefix on error as that should use the working directory which
   // is the least likely problematic.

toolchain/install/install_paths.h

@@ -5,6 +5,7 @@
 #ifndef CARBON_TOOLCHAIN_INSTALL_INSTALL_PATHS_H_
 #define CARBON_TOOLCHAIN_INSTALL_INSTALL_PATHS_H_
 
+#include "common/error.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/Twine.h"
@@ -59,6 +60,10 @@ namespace Carbon {
 // TODO: Need to check the installation structure of LLVM on Windows and figure
 // out what Carbon's should be within a Windows prefix and how much of the
 // structure we can share with the Unix-y layout of the prefix.
+//
+// TODO: InstallPaths is typically called from places using a VFS (both tests
+// and the Driver), but does not use a VFS itself. It currently only supports
+// using the real filesystem, but should probably support a VFS.
 class InstallPaths {
  public:
   // Provide the current executable's path to detect the correct installation
@@ -82,6 +87,10 @@ class InstallPaths {
   // using Carbon in an environment with an unusual path to the installed files.
   static auto Make(llvm::StringRef install_prefix) -> InstallPaths;
 
+  // Finds the source files that define the prelude and returns a list of their
+  // filenames. The list always includes at least one file.
+  auto FindPreludeFiles() const -> ErrorOr<llvm::SmallVector<std::string>>;
+
   // Check for an error detecting the install paths correctly.
   //
   // A nullopt return means no errors encountered and the paths should work

toolchain/install/install_paths_test_helpers.cpp

@@ -0,0 +1,29 @@
+// Part of the Carbon Language project, under the Apache License v2.0 with LLVM
+// Exceptions. See /LICENSE for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+
+#include "toolchain/install/install_paths_test_helpers.h"
+
+#include "testing/base/global_exe_path.h"
+
+namespace Carbon::Testing {
+
+// Prepares the VFS with prelude files from the real filesystem. Primarily for
+// tests.
+auto AddPreludeFilesToVfs(InstallPaths install_paths,
+                          llvm::vfs::InMemoryFileSystem* vfs) -> void {
+  // Load the prelude into the test VFS.
+  auto real_fs = llvm::vfs::getRealFileSystem();
+  auto prelude = install_paths.FindPreludeFiles();
+  CARBON_CHECK(prelude.ok()) << prelude.error();
+
+  for (const auto& path : *prelude) {
+    llvm::ErrorOr<std::unique_ptr<llvm::MemoryBuffer>> file =
+        real_fs->getBufferForFile(path);
+    CARBON_CHECK(file) << "Error getting file: " << file.getError().message();
+    bool added = vfs->addFile(path, /*ModificationTime=*/0, std::move(*file));
+    CARBON_CHECK(added) << "Duplicate file: " << path;
+  }
+}
+
+}  // namespace Carbon::Testing

toolchain/install/install_paths_test_helpers.h

@@ -0,0 +1,19 @@
+// Part of the Carbon Language project, under the Apache License v2.0 with LLVM
+// Exceptions. See /LICENSE for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+
+#ifndef CARBON_TOOLCHAIN_INSTALL_INSTALL_PATHS_TEST_HELPERS_H_
+#define CARBON_TOOLCHAIN_INSTALL_INSTALL_PATHS_TEST_HELPERS_H_
+
+#include "llvm/Support/VirtualFileSystem.h"
+#include "toolchain/install/install_paths.h"
+
+namespace Carbon::Testing {
+
+// Prepares the VFS with prelude files from the real filesystem.
+auto AddPreludeFilesToVfs(InstallPaths install_paths,
+                          llvm::vfs::InMemoryFileSystem* vfs) -> void;
+
+}  // namespace Carbon::Testing
+
+#endif  // CARBON_TOOLCHAIN_INSTALL_INSTALL_PATHS_TEST_HELPERS_H_

toolchain/testing/file_test.cpp

@@ -36,11 +36,7 @@ class ToolchainFileTest : public FileTestBase {
   auto Run(const llvm::SmallVector<llvm::StringRef>& test_args,
            llvm::vfs::InMemoryFileSystem& fs, llvm::raw_pwrite_stream& stdout,
            llvm::raw_pwrite_stream& stderr) -> ErrorOr<RunResult> override {
-    auto prelude =
-        Driver::FindPreludeFiles(installation_.core_package(), stderr);
-    if (prelude.empty()) {
-      return Error("Could not find prelude");
-    }
+    CARBON_ASSIGN_OR_RETURN(auto prelude, installation_.FindPreludeFiles());
     for (const auto& file : prelude) {
       CARBON_RETURN_IF_ERROR(AddFile(fs, file));
     }