Refactor link and compile into subcommand objects. (#4303)

Note the purpose here is to make it simpler to add more subcommands,
without adding a lot of things to Driver.

This creates a copy of CodegenOptions, but it was double-registered at
present which felt odd. It's also fairly small right now. If this
becomes an issue, maybe we can look into using optional for delayed
initialization, or just go back to straight sharing.

---------

Co-authored-by: josh11b <15258583+josh11b@users.noreply.github.com>

toolchain/driver/BUILD

@@ -86,7 +86,10 @@ cc_library(
         "link_subcommand.cpp",
         "link_subcommand.h",
     ],
-    hdrs = ["driver.h"],
+    hdrs = [
+        "driver.h",
+        "driver_subcommand.h",
+    ],
     data = [
         "//toolchain/install:install_lib_data",
     ],

toolchain/driver/compile_subcommand.cpp

@@ -17,9 +17,6 @@
 #include "toolchain/sem_ir/inst_namer.h"
 #include "toolchain/source/source_buffer.h"
 
-// TODO: Remove this include.
-#include "toolchain/driver/driver.h"
-
 namespace Carbon {
 
 auto operator<<(llvm::raw_ostream& out, CompileOptions::Phase phase)
@@ -59,8 +56,7 @@ can be written to standard output as these phases progress.
 )""",
 };
 
-auto CompileOptions::Build(CommandLine::CommandBuilder& b,
-                           CodegenOptions& codegen_options) -> void {
+auto CompileOptions::Build(CommandLine::CommandBuilder& b) -> void {
   b.AddStringPositionalArg(
       {
           .name = "FILE",
@@ -268,34 +264,33 @@ Emit DWARF debug information.
       });
 }
 
-auto Driver::ValidateCompileOptions(const CompileOptions& options) const
-    -> bool {
+auto CompileSubcommand::ValidateOptions(DriverEnv& driver_env) const -> bool {
   using Phase = CompileOptions::Phase;
-  switch (options.phase) {
+  switch (options_.phase) {
     case Phase::Lex:
-      if (options.dump_parse_tree) {
-        driver_env_.error_stream
+      if (options_.dump_parse_tree) {
+        driver_env.error_stream
             << "ERROR: Requested dumping the parse tree but compile "
                "phase is limited to '"
-            << options.phase << "'.\n";
+            << options_.phase << "'.\n";
         return false;
       }
       [[fallthrough]];
     case Phase::Parse:
-      if (options.dump_sem_ir) {
-        driver_env_.error_stream
+      if (options_.dump_sem_ir) {
+        driver_env.error_stream
             << "ERROR: Requested dumping the SemIR but compile phase "
                "is limited to '"
-            << options.phase << "'.\n";
+            << options_.phase << "'.\n";
         return false;
       }
       [[fallthrough]];
     case Phase::Check:
-      if (options.dump_llvm_ir) {
-        driver_env_.error_stream
+      if (options_.dump_llvm_ir) {
+        driver_env.error_stream
             << "ERROR: Requested dumping the LLVM IR but compile "
                "phase is limited to '"
-            << options.phase << "'.\n";
+            << options_.phase << "'.\n";
         return false;
       }
       [[fallthrough]];
@@ -307,16 +302,17 @@ auto Driver::ValidateCompileOptions(const CompileOptions& options) const
   return true;
 }
 
+namespace {
 // Ties together information for a file being compiled.
-class Driver::CompilationUnit {
+// TODO: Refactor this because it's a long class to have function definitions
+// inline.
+class CompilationUnit {
  public:
   explicit CompilationUnit(DriverEnv& driver_env, const CompileOptions& options,
-                           const CodegenOptions& codegen_options,
                            DiagnosticConsumer* consumer,
                            llvm::StringRef input_filename)
       : driver_env_(&driver_env),
         options_(options),
-        codegen_options_(codegen_options),
         input_filename_(input_filename),
         vlog_stream_(driver_env_->vlog_stream) {
     if (vlog_stream_ != nullptr || options_.stream_errors) {
@@ -497,7 +493,7 @@ class Driver::CompilationUnit {
   // Do codegen. Returns true on success.
   auto RunCodeGenHelper() -> bool {
     std::optional<CodeGen> codegen = CodeGen::Make(
-        *module_, codegen_options_.target, driver_env_->error_stream);
+        *module_, options_.codegen_options.target, driver_env_->error_stream);
     if (!codegen) {
       return false;
     }
@@ -593,7 +589,6 @@ class Driver::CompilationUnit {
   DriverEnv* driver_env_;
   SharedValueStores value_stores_;
   const CompileOptions& options_;
-  const CodegenOptions& codegen_options_;
   std::string input_filename_;
 
   // Copied from driver_ for CARBON_VLOG.
@@ -617,10 +612,10 @@ class Driver::CompilationUnit {
   std::unique_ptr<llvm::LLVMContext> llvm_context_;
   std::unique_ptr<llvm::Module> module_;
 };
+}  // namespace
 
-auto Driver::Compile(const CompileOptions& options,
-                     const CodegenOptions& codegen_options) -> RunResult {
-  if (!ValidateCompileOptions(options)) {
+auto CompileSubcommand::Run(DriverEnv& driver_env) -> DriverResult {
+  if (!ValidateOptions(driver_env)) {
     return {.success = false};
   }
 
@@ -628,33 +623,31 @@ auto Driver::Compile(const CompileOptions& options,
   // TODO: Replace this with a search for library api files in a
   // package-specific search path based on the library name.
   llvm::SmallVector<std::string> prelude;
-  if (options.prelude_import && options.phase >= CompileOptions::Phase::Check) {
-    if (auto find = driver_env_.installation->ReadPreludeManifest();
-        find.ok()) {
+  if (options_.prelude_import &&
+      options_.phase >= CompileOptions::Phase::Check) {
+    if (auto find = driver_env.installation->ReadPreludeManifest(); find.ok()) {
       prelude = std::move(*find);
     } else {
-      driver_env_.error_stream << "ERROR: " << find.error() << "\n";
+      driver_env.error_stream << "ERROR: " << find.error() << "\n";
       return {.success = false};
     }
   }
 
   // Prepare CompilationUnits before building scope exit handlers.
-  StreamDiagnosticConsumer stream_consumer(driver_env_.error_stream);
+  StreamDiagnosticConsumer stream_consumer(driver_env.error_stream);
   llvm::SmallVector<std::unique_ptr<CompilationUnit>> units;
-  units.reserve(prelude.size() + options.input_filenames.size());
+  units.reserve(prelude.size() + options_.input_filenames.size());
 
   // Add the prelude files.
   for (const auto& input_filename : prelude) {
-    units.push_back(
-        std::make_unique<CompilationUnit>(driver_env_, options, codegen_options,
-                                          &stream_consumer, input_filename));
+    units.push_back(std::make_unique<CompilationUnit>(
+        driver_env, options_, &stream_consumer, input_filename));
   }
 
   // Add the input source files.
-  for (const auto& input_filename : options.input_filenames) {
-    units.push_back(
-        std::make_unique<CompilationUnit>(driver_env_, options, codegen_options,
-                                          &stream_consumer, input_filename));
+  for (const auto& input_filename : options_.input_filenames) {
+    units.push_back(std::make_unique<CompilationUnit>(
+        driver_env, options_, &stream_consumer, input_filename));
   }
 
   auto on_exit = llvm::make_scope_exit([&]() {
@@ -667,9 +660,9 @@ auto Driver::Compile(const CompileOptions& options,
     stream_consumer.Flush();
   });
 
-  // Returns a RunResult object. Called whenever Compile returns.
+  // Returns a DriverResult object. Called whenever Compile returns.
   auto make_result = [&]() {
-    RunResult result = {.success = true};
+    DriverResult result = {.success = true};
     for (const auto& unit : units) {
       result.success &= unit->success();
       result.per_file_success.push_back(
@@ -682,7 +675,7 @@ auto Driver::Compile(const CompileOptions& options,
   for (auto& unit : units) {
     unit->RunLex();
   }
-  if (options.phase == CompileOptions::Phase::Lex) {
+  if (options_.phase == CompileOptions::Phase::Lex) {
     return make_result();
   }
   // Parse and check phases examine `has_source` because they want to proceed if
@@ -695,7 +688,7 @@ auto Driver::Compile(const CompileOptions& options,
       unit->RunParse();
     }
   }
-  if (options.phase == CompileOptions::Phase::Parse) {
+  if (options_.phase == CompileOptions::Phase::Parse) {
     return make_result();
   }
 
@@ -713,24 +706,24 @@ auto Driver::Compile(const CompileOptions& options,
     node_converters.emplace_back(unit.tokens, unit.tokens->source().filename(),
                                  unit.get_parse_tree_and_subtrees);
   }
-  CARBON_VLOG_TO(driver_env_.vlog_stream, "*** Check::CheckParseTrees ***\n");
-  Check::CheckParseTrees(check_units, node_converters, options.prelude_import,
-                         driver_env_.vlog_stream);
-  CARBON_VLOG_TO(driver_env_.vlog_stream,
+  CARBON_VLOG_TO(driver_env.vlog_stream, "*** Check::CheckParseTrees ***\n");
+  Check::CheckParseTrees(check_units, node_converters, options_.prelude_import,
+                         driver_env.vlog_stream);
+  CARBON_VLOG_TO(driver_env.vlog_stream,
                  "*** Check::CheckParseTrees done ***\n");
   for (auto& unit : units) {
     if (unit->has_source()) {
       unit->PostCheck();
     }
   }
-  if (options.phase == CompileOptions::Phase::Check) {
+  if (options_.phase == CompileOptions::Phase::Check) {
     return make_result();
   }
 
   // Unlike previous steps, errors block further progress.
   if (std::any_of(units.begin(), units.end(),
                   [&](const auto& unit) { return !unit->success(); })) {
-    CARBON_VLOG_TO(driver_env_.vlog_stream,
+    CARBON_VLOG_TO(driver_env.vlog_stream,
                    "*** Stopping before lowering due to errors ***");
     return make_result();
   }
@@ -741,10 +734,10 @@ auto Driver::Compile(const CompileOptions& options,
                                               &**unit->GetCheckUnit().sem_ir);
     unit->RunLower(converter);
   }
-  if (options.phase == CompileOptions::Phase::Lower) {
+  if (options_.phase == CompileOptions::Phase::Lower) {
     return make_result();
   }
-  CARBON_CHECK(options.phase == CompileOptions::Phase::CodeGen,
+  CARBON_CHECK(options_.phase == CompileOptions::Phase::CodeGen,
                "CodeGen should be the last stage");
 
   // Codegen.

toolchain/driver/compile_subcommand.h

@@ -10,6 +10,8 @@
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringRef.h"
 #include "toolchain/driver/codegen_options.h"
+#include "toolchain/driver/driver_env.h"
+#include "toolchain/driver/driver_subcommand.h"
 
 namespace Carbon {
 
@@ -30,8 +32,9 @@ struct CompileOptions {
   friend auto operator<<(llvm::raw_ostream& out, Phase phase)
       -> llvm::raw_ostream&;
 
-  auto Build(CommandLine::CommandBuilder& b, CodegenOptions& codegen_options)
-      -> void;
+  auto Build(CommandLine::CommandBuilder& b) -> void;
+
+  CodegenOptions codegen_options;
 
   Phase phase;
 
@@ -57,6 +60,21 @@ struct CompileOptions {
   llvm::StringRef exclude_dump_file_prefix;
 };
 
+// Implements the compile subcommand of the driver.
+class CompileSubcommand : public DriverSubcommand {
+ public:
+  auto BuildOptions(CommandLine::CommandBuilder& b) { options_.Build(b); }
+
+  auto Run(DriverEnv& driver_env) -> DriverResult override;
+
+ private:
+  // Does custom validation of the compile-subcommand options structure beyond
+  // what the command line parsing library supports.
+  auto ValidateOptions(DriverEnv& driver_env) const -> bool;
+
+  CompileOptions options_;
+};
+
 }  // namespace Carbon
 
 #endif  // CARBON_TOOLCHAIN_DRIVER_COMPILE_SUBCOMMAND_H_

toolchain/driver/driver.cpp

@@ -19,49 +19,26 @@
 
 namespace Carbon {
 
-struct Driver::Options {
+namespace {
+struct Options {
   static const CommandLine::CommandInfo Info;
 
-  enum class Subcommand : int8_t {
-    Compile,
-    Link,
-  };
-
-  void Build(CommandLine::CommandBuilder& b) {
-    b.AddFlag(
-        {
-            .name = "verbose",
-            .short_name = "v",
-            .help = "Enable verbose logging to the stderr stream.",
-        },
-        [&](CommandLine::FlagBuilder& arg_b) { arg_b.Set(&verbose); });
-
-    b.AddSubcommand(CompileOptions::Info,
-                    [&](CommandLine::CommandBuilder& sub_b) {
-                      compile_options.Build(sub_b, codegen_options);
-                      sub_b.Do([&] { subcommand = Subcommand::Compile; });
-                    });
-
-    b.AddSubcommand(LinkOptions::Info, [&](CommandLine::CommandBuilder& sub_b) {
-      link_options.Build(sub_b, codegen_options);
-      sub_b.Do([&] { subcommand = Subcommand::Link; });
-    });
-
-    b.RequiresSubcommand();
-  }
+  auto Build(CommandLine::CommandBuilder& b) -> void;
 
   bool verbose;
-  Subcommand subcommand;
 
-  CodegenOptions codegen_options;
-  CompileOptions compile_options;
-  LinkOptions link_options;
+  CompileSubcommand compile;
+  LinkSubcommand link;
+
+  // On success, this is set to the subcommand to run.
+  DriverSubcommand* subcommand = nullptr;
 };
+}  // namespace
 
 // Note that this is not constexpr so that it can include information generated
 // in separate translation units and potentially overridden at link time in the
 // version string.
-const CommandLine::CommandInfo Driver::Options::Info = {
+const CommandLine::CommandInfo Options::Info = {
     .name = "carbon",
     .version = Version::ToolchainInfo,
     .help = R"""(
@@ -77,16 +54,36 @@ For questions, issues, or bug reports, please use our GitHub project:
 )""",
 };
 
-auto Driver::ParseArgs(llvm::ArrayRef<llvm::StringRef> args, Options& options)
-    -> CommandLine::ParseResult {
-  return CommandLine::Parse(
-      args, driver_env_.output_stream, driver_env_.error_stream, Options::Info,
-      [&](CommandLine::CommandBuilder& b) { options.Build(b); });
+auto Options::Build(CommandLine::CommandBuilder& b) -> void {
+  b.AddFlag(
+      {
+          .name = "verbose",
+          .short_name = "v",
+          .help = "Enable verbose logging to the stderr stream.",
+      },
+      [&](CommandLine::FlagBuilder& arg_b) { arg_b.Set(&verbose); });
+
+  b.AddSubcommand(CompileOptions::Info,
+                  [&](CommandLine::CommandBuilder& sub_b) {
+                    compile.BuildOptions(sub_b);
+                    sub_b.Do([&] { subcommand = &compile; });
+                  });
+
+  b.AddSubcommand(LinkOptions::Info, [&](CommandLine::CommandBuilder& sub_b) {
+    link.BuildOptions(sub_b);
+    sub_b.Do([&] { subcommand = &link; });
+  });
+
+  b.RequiresSubcommand();
 }
 
-auto Driver::RunCommand(llvm::ArrayRef<llvm::StringRef> args) -> RunResult {
+auto Driver::RunCommand(llvm::ArrayRef<llvm::StringRef> args) -> DriverResult {
   Options options;
-  CommandLine::ParseResult result = ParseArgs(args, options);
+
+  CommandLine::ParseResult result = CommandLine::Parse(
+      args, driver_env_.output_stream, driver_env_.error_stream, Options::Info,
+      [&](CommandLine::CommandBuilder& b) { options.Build(b); });
+
   if (result == CommandLine::ParseResult::Error) {
     return {.success = false};
   } else if (result == CommandLine::ParseResult::MetaSuccess) {
@@ -97,14 +94,8 @@ auto Driver::RunCommand(llvm::ArrayRef<llvm::StringRef> args) -> RunResult {
     // Note this implies streamed output in order to interleave.
     driver_env_.vlog_stream = &driver_env_.error_stream;
   }
-
-  switch (options.subcommand) {
-    case Options::Subcommand::Compile:
-      return Compile(options.compile_options, options.codegen_options);
-    case Options::Subcommand::Link:
-      return Link(options.link_options, options.codegen_options);
-  }
-  llvm_unreachable("All subcommands handled!");
+  CARBON_CHECK(options.subcommand != nullptr);
+  return options.subcommand->Run(driver_env_);
 }
 
 }  // namespace Carbon

toolchain/driver/driver.h

@@ -11,6 +11,7 @@
 #include "toolchain/driver/codegen_options.h"
 #include "toolchain/driver/compile_subcommand.h"
 #include "toolchain/driver/driver_env.h"
+#include "toolchain/driver/driver_subcommand.h"
 #include "toolchain/driver/link_subcommand.h"
 
 namespace Carbon {
@@ -22,16 +23,6 @@ namespace Carbon {
 // with the language.
 class Driver {
  public:
-  // The result of RunCommand().
-  struct RunResult {
-    // Overall success result.
-    bool success;
-
-    // Per-file success results. May be empty if files aren't individually
-    // processed.
-    llvm::SmallVector<std::pair<std::string, bool>> per_file_success;
-  };
-
   // Constructs a driver with any error or informational output directed to a
   // specified stream.
   Driver(llvm::vfs::FileSystem& fs, const InstallPaths* installation,
@@ -48,38 +39,9 @@ class Driver {
   // Returns true if the operation succeeds. If the operation fails, returns
   // false and any information about the failure is printed to the registered
   // error stream (stderr by default).
-  auto RunCommand(llvm::ArrayRef<llvm::StringRef> args) -> RunResult;
+  auto RunCommand(llvm::ArrayRef<llvm::StringRef> args) -> DriverResult;
 
  private:
-  struct Options;
-
-  // Implementation is in compile_subcommand.cpp.
-  // TODO: Remove from Driver.
-  class CompilationUnit;
-
-  // Delegates to the command line library to parse the arguments and store the
-  // results in a custom `Options` structure that the rest of the driver uses.
-  auto ParseArgs(llvm::ArrayRef<llvm::StringRef> args, Options& options)
-      -> CommandLine::ParseResult;
-
-  // Does custom validation of the compile-subcommand options structure beyond
-  // what the command line parsing library supports.
-  // Implementation is in compile_subcommand.cpp.
-  // TODO: Remove from Driver.
-  auto ValidateCompileOptions(const CompileOptions& options) const -> bool;
-
-  // Implements the compile subcommand of the driver.
-  // Implementation is in compile_subcommand.cpp.
-  // TODO: Remove from Driver.
-  auto Compile(const CompileOptions& options,
-               const CodegenOptions& codegen_options) -> RunResult;
-
-  // Implements the link subcommand of the driver.
-  // Implementation is in link_subcommand.cpp.
-  // TODO: Remove from Driver.
-  auto Link(const LinkOptions& options, const CodegenOptions& codegen_options)
-      -> RunResult;
-
   DriverEnv driver_env_;
 };
 

toolchain/driver/driver_subcommand.h

@@ -0,0 +1,33 @@
+// 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_DRIVER_DRIVER_SUBCOMMAND_H_
+#define CARBON_TOOLCHAIN_DRIVER_DRIVER_SUBCOMMAND_H_
+
+#include "common/ostream.h"
+#include "llvm/Support/VirtualFileSystem.h"
+#include "toolchain/driver/driver_env.h"
+#include "toolchain/install/install_paths.h"
+
+namespace Carbon {
+
+// The result of a driver run.
+struct DriverResult {
+  // Overall success result.
+  bool success;
+
+  // Per-file success results. May be empty if files aren't individually
+  // processed.
+  llvm::SmallVector<std::pair<std::string, bool>> per_file_success;
+};
+
+// A subcommand for the driver.
+class DriverSubcommand {
+ public:
+  virtual auto Run(DriverEnv& driver_env) -> DriverResult = 0;
+};
+
+}  // namespace Carbon
+
+#endif  // CARBON_TOOLCHAIN_DRIVER_DRIVER_SUBCOMMAND_H_

toolchain/driver/link_subcommand.cpp

@@ -7,9 +7,6 @@
 #include "llvm/TargetParser/Triple.h"
 #include "toolchain/driver/clang_runner.h"
 
-// TODO: Remove this include.
-#include "toolchain/driver/driver.h"
-
 namespace Carbon {
 
 constexpr CommandLine::CommandInfo LinkOptions::Info = {
@@ -24,8 +21,7 @@ TODO: Support linking against binary libraries.
 )""",
 };
 
-auto LinkOptions::Build(CommandLine::CommandBuilder& b,
-                        CodegenOptions& codegen_options) -> void {
+auto LinkOptions::Build(CommandLine::CommandBuilder& b) -> void {
   b.AddStringPositionalArg(
       {
           .name = "OBJECT_FILE",
@@ -84,8 +80,7 @@ static void AddOSFlags(llvm::StringRef target,
   }
 }
 
-auto Driver::Link(const LinkOptions& options,
-                  const CodegenOptions& codegen_options) -> RunResult {
+auto LinkSubcommand::Run(DriverEnv& driver_env) -> DriverResult {
   // TODO: Currently we use the Clang driver to link. This works well on Unix
   // OSes but we likely need to directly build logic to invoke `link.exe` on
   // Windows where `cl.exe` doesn't typically cover that logic.
@@ -109,15 +104,15 @@ auto Driver::Link(const LinkOptions& options,
   clang_args.push_back("-nostdlib++");
 
   // Add OS-specific flags based on the target.
-  AddOSFlags(codegen_options.target, clang_args);
+  AddOSFlags(options_.codegen_options.target, clang_args);
 
   clang_args.push_back("-o");
-  clang_args.push_back(options.output_filename);
-  clang_args.append(options.object_filenames.begin(),
-                    options.object_filenames.end());
+  clang_args.push_back(options_.output_filename);
+  clang_args.append(options_.object_filenames.begin(),
+                    options_.object_filenames.end());
 
-  ClangRunner runner(driver_env_.installation, codegen_options.target,
-                     driver_env_.vlog_stream);
+  ClangRunner runner(driver_env.installation, options_.codegen_options.target,
+                     driver_env.vlog_stream);
   return {.success = runner.Run(clang_args)};
 }
 

toolchain/driver/link_subcommand.h

@@ -9,6 +9,8 @@
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringRef.h"
 #include "toolchain/driver/codegen_options.h"
+#include "toolchain/driver/driver_env.h"
+#include "toolchain/driver/driver_subcommand.h"
 
 namespace Carbon {
 
@@ -18,13 +20,24 @@ namespace Carbon {
 struct LinkOptions {
   static const CommandLine::CommandInfo Info;
 
-  auto Build(CommandLine::CommandBuilder& b, CodegenOptions& codegen_options)
-      -> void;
+  auto Build(CommandLine::CommandBuilder& b) -> void;
 
+  CodegenOptions codegen_options;
   llvm::StringRef output_filename;
   llvm::SmallVector<llvm::StringRef> object_filenames;
 };
 
+// Implements the link subcommand of the driver.
+class LinkSubcommand : public DriverSubcommand {
+ public:
+  auto BuildOptions(CommandLine::CommandBuilder& b) { options_.Build(b); }
+
+  auto Run(DriverEnv& driver_env) -> DriverResult override;
+
+ private:
+  LinkOptions options_;
+};
+
 }  // namespace Carbon
 
 #endif  // CARBON_TOOLCHAIN_DRIVER_LINK_SUBCOMMAND_H_