Refactor ModuleType from an enum to a bitset. This will significantly reduce the tech debt and reduce the difficulty in adding support for autogenerated nontextual modules. This also removes unneccesary modulemap, as it no longer serves a purpose now that we have double modulemaps. Bug: b:500845363 Change-Id: I192a62ccbdc7937328b84de6af5e9f186a6a6964 Reviewed-on: https://gn-review.googlesource.com/c/gn/+/21840 Reviewed-by: Takuto Ikuta <tikuta@google.com> Commit-Queue: Matt Stark <msta@google.com>

diff --git a/src/gn/binary_target_generator.cc b/src/gn/binary_target_generator.cc
index 39eb47c..3fa160d 100644
--- a/src/gn/binary_target_generator.cc
+++ b/src/gn/binary_target_generator.cc

@@ -295,15 +295,10 @@
   const Value* generate_modulemap_val =
       scope_->GetValue(variables::kGenerateModulemap, true);
 
+  Target::ModuleType type;
+  type.set(Target::HAS_MODULEMAP);
   if (target_->source_types_used().Get(SourceFile::SOURCE_MODULEMAP)) {
-    target_->set_module_type(Target::EXPLICIT_MODULEMAP);
-    return true;
-  }
-
-  if (target_->all_headers_public()
-          ? !target_->source_types_used().Get(SourceFile::SOURCE_H)
-          : target_->public_headers().empty()) {
-    target_->set_module_type(Target::UNNECESSARY_MODULEMAP);
+    target_->set_module_type(type);
     return true;
   }
 
@@ -316,13 +311,26 @@
     return false;
   }
   auto value = generate_modulemap_val->string_value();
+  type.set(Target::MODULEMAP_IS_GENERATED);
   if (value == "textual") {
-    target_->set_module_type(Target::GENERATED_TEXTUAL_MODULEMAP);
-  } else if (value != "none") {
+    type.set(Target::MODULEMAP_IS_TEXTUAL);
+  } else if (value == "none" || value == "") {
+    return true;
+  } else {
     *err_ = Err(*generate_modulemap_val,
                 "Invalid value for generate_modulemap. Expected \"textual\" or "
                 "\"none\"");
     return false;
   }
+
+  // Even if generate_modulemap was explicitly set, if we're compiling non-c++
+  // code, we shouldn't mark it as a module.
+  if (target_->source_types_used().Get(SourceFile::SOURCE_CPP) ||
+      (target_->all_headers_public()
+           ? target_->source_types_used().Get(SourceFile::SOURCE_H)
+           : !target_->public_headers().empty())) {
+    target_->set_module_type(type);
+  }
+
   return true;
 }

diff --git a/src/gn/binary_target_generator_unittest.cc b/src/gn/binary_target_generator_unittest.cc
index c1e4215..ba1ae5a 100644
--- a/src/gn/binary_target_generator_unittest.cc
+++ b/src/gn/binary_target_generator_unittest.cc

@@ -12,7 +12,7 @@
 
 using BinaryTargetGeneratorTest = TestWithScheduler;
 
-TEST_F(BinaryTargetGeneratorTest, UnnecessaryModuleMapAllPublic) {
+TEST_F(BinaryTargetGeneratorTest, NonModuleTarget) {
   TestWithScope setup;
   Scope::ItemVector items_;
   setup.scope()->set_item_collector(&items_);
@@ -21,7 +21,7 @@
   TestParseInput input(
       R"(static_library("foo") {
            generate_modulemap = "textual"
-           sources = [ "//foo.cc" ]
+           sources = [ "//foo.c" ]
          })");
   ASSERT_FALSE(input.has_error());
 
@@ -33,7 +33,7 @@
   Target* target = items_[0]->AsTarget();
   ASSERT_TRUE(target);
 
-  EXPECT_EQ(Target::UNNECESSARY_MODULEMAP, target->module_type());
+  EXPECT_TRUE(target->module_type().none());
 }
 
 TEST_F(BinaryTargetGeneratorTest, GeneratedModuleMapAllPublic) {
@@ -57,7 +57,9 @@
   Target* target = items_[0]->AsTarget();
   ASSERT_TRUE(target);
 
-  EXPECT_EQ(Target::GENERATED_TEXTUAL_MODULEMAP, target->module_type());
+  EXPECT_TRUE(target->module_type().test(Target::HAS_MODULEMAP));
+  EXPECT_TRUE(target->module_type().test(Target::MODULEMAP_IS_GENERATED));
+  EXPECT_TRUE(target->module_type().test(Target::MODULEMAP_IS_TEXTUAL));
 }
 
 TEST_F(BinaryTargetGeneratorTest, GeneratedModuleMap) {
@@ -82,5 +84,7 @@
   Target* target = items_[0]->AsTarget();
   ASSERT_TRUE(target);
 
-  EXPECT_EQ(Target::GENERATED_TEXTUAL_MODULEMAP, target->module_type());
+  EXPECT_TRUE(target->module_type().test(Target::HAS_MODULEMAP));
+  EXPECT_TRUE(target->module_type().test(Target::MODULEMAP_IS_GENERATED));
+  EXPECT_TRUE(target->module_type().test(Target::MODULEMAP_IS_TEXTUAL));
 }

diff --git a/src/gn/compile_commands_writer_unittest.cc b/src/gn/compile_commands_writer_unittest.cc
index 8069d0f..81ec2e2 100644
--- a/src/gn/compile_commands_writer_unittest.cc
+++ b/src/gn/compile_commands_writer_unittest.cc

@@ -643,7 +643,9 @@
   module_target.visibility().SetPublic();
   module_target.sources().push_back(SourceFile("//foo/foo.modulemap"));
   module_target.source_types_used().Set(SourceFile::SOURCE_MODULEMAP);
-  module_target.set_module_type(Target::EXPLICIT_MODULEMAP);
+  Target::ModuleType module_type;
+  module_type.set(Target::HAS_MODULEMAP);
+  module_target.set_module_type(module_type);
   module_target.SetToolchain(&module_toolchain);
   ASSERT_TRUE(module_target.OnResolved(&err));
 

diff --git a/src/gn/ninja_binary_target_writer.cc b/src/gn/ninja_binary_target_writer.cc
index 7355920..b93bd28 100644
--- a/src/gn/ninja_binary_target_writer.cc
+++ b/src/gn/ninja_binary_target_writer.cc

@@ -47,7 +47,7 @@
     for (const auto& pair : *current) {
       const Target* target = pair.ptr;
       if (visited.insert(target).second) {
-        if (target->module_type() == Target::NO_MODULEMAP) {
+        if (target->module_type().none()) {
           stack.push_back(&target->public_deps());
           // If you declare `public_deps = ...` on a group, it shows up as a
           // private dep. Probably because groups don't distinguish between

diff --git a/src/gn/ninja_c_binary_target_writer_unittest.cc b/src/gn/ninja_c_binary_target_writer_unittest.cc
index dce8dfd..41dd35d 100644
--- a/src/gn/ninja_c_binary_target_writer_unittest.cc
+++ b/src/gn/ninja_c_binary_target_writer_unittest.cc

@@ -20,6 +20,11 @@
 
 using NinjaCBinaryTargetWriterTest = TestWithScheduler;
 
+constexpr Target::ModuleType kExplicitModulemap = 1 << Target::HAS_MODULEMAP;
+constexpr Target::ModuleType kGeneratedTextualModulemap =
+    1 << Target::HAS_MODULEMAP | 1 << Target::MODULEMAP_IS_GENERATED |
+    1 << Target::MODULEMAP_IS_TEXTUAL;
+
 TEST_F(NinjaCBinaryTargetWriterTest, SourceSet) {
   Err err;
   TestWithScope setup;
@@ -2271,7 +2276,7 @@
   target.sources().push_back(SourceFile("//foo/bar.modulemap"));
   target.source_types_used().Set(SourceFile::SOURCE_CPP);
   target.source_types_used().Set(SourceFile::SOURCE_MODULEMAP);
-  target.set_module_type(Target::EXPLICIT_MODULEMAP);
+  target.set_module_type(kExplicitModulemap);
   ASSERT_TRUE(target.OnResolved(&err));
 
   std::ostringstream out;
@@ -2313,7 +2318,7 @@
   target.sources().push_back(SourceFile("//foo/bar.modulemap"));
   target.source_types_used().Set(SourceFile::SOURCE_CPP);
   target.source_types_used().Set(SourceFile::SOURCE_MODULEMAP);
-  target.set_module_type(Target::EXPLICIT_MODULEMAP);
+  target.set_module_type(kExplicitModulemap);
   ASSERT_TRUE(target.OnResolved(&err));
 
   std::ostringstream out;
@@ -2574,7 +2579,7 @@
   target.sources().push_back(SourceFile("//blah/a.h"));
   target.source_types_used().Set(SourceFile::SOURCE_CPP);
   target.source_types_used().Set(SourceFile::SOURCE_MODULEMAP);
-  target.set_module_type(Target::EXPLICIT_MODULEMAP);
+  target.set_module_type(kExplicitModulemap);
   target.SetToolchain(&module_toolchain);
   ASSERT_TRUE(target.OnResolved(&err));
 
@@ -2622,7 +2627,7 @@
   target2.sources().push_back(SourceFile("//stuff/b.h"));
   target2.source_types_used().Set(SourceFile::SOURCE_CPP);
   target2.source_types_used().Set(SourceFile::SOURCE_MODULEMAP);
-  target2.set_module_type(Target::EXPLICIT_MODULEMAP);
+  target2.set_module_type(kExplicitModulemap);
   target2.public_deps().push_back(LabelTargetPair(&target));
   target2.SetToolchain(&module_toolchain);
   ASSERT_TRUE(target2.OnResolved(&err));
@@ -2668,7 +2673,7 @@
   target3.visibility().SetPublic();
   target3.sources().push_back(SourceFile("//stuff/c.modulemap"));
   target3.source_types_used().Set(SourceFile::SOURCE_MODULEMAP);
-  target3.set_module_type(Target::EXPLICIT_MODULEMAP);
+  target3.set_module_type(kExplicitModulemap);
   target3.public_deps().push_back(LabelTargetPair(&target2));
   target3.SetToolchain(&module_toolchain);
   ASSERT_TRUE(target3.OnResolved(&err));
@@ -2938,7 +2943,7 @@
   target.public_headers().push_back(SourceFile("//foo/public_header.h"));
   target.set_all_headers_public(false);
   target.source_types_used().Set(SourceFile::SOURCE_CPP);
-  target.set_module_type(Target::GENERATED_TEXTUAL_MODULEMAP);
+  target.set_module_type(kGeneratedTextualModulemap);
   target.SetToolchain(setup.toolchain());
   ASSERT_TRUE(target.OnResolved(&err));
 
@@ -2990,7 +2995,7 @@
   target_no_public.sources().push_back(SourceFile("//foo/header1.h"));
   target_no_public.source_types_used().Set(SourceFile::SOURCE_CPP);
   target_no_public.source_types_used().Set(SourceFile::SOURCE_H);
-  target_no_public.set_module_type(Target::GENERATED_TEXTUAL_MODULEMAP);
+  target_no_public.set_module_type(kGeneratedTextualModulemap);
   target_no_public.SetToolchain(setup.toolchain());
   ASSERT_TRUE(target_no_public.OnResolved(&err));
 
@@ -3020,7 +3025,7 @@
   transitive.sources().push_back(SourceFile("//foo/invisible.h"));
   transitive.source_types_used().Set(SourceFile::SOURCE_H);
   transitive.set_output_type(Target::SOURCE_SET);
-  transitive.set_module_type(Target::GENERATED_TEXTUAL_MODULEMAP);
+  transitive.set_module_type(kGeneratedTextualModulemap);
   transitive.SetToolchain(setup.toolchain());
   ASSERT_TRUE(transitive.OnResolved(&err));
 
@@ -3033,7 +3038,7 @@
   private_dep.sources().push_back(SourceFile("//foo/private_dep.h"));
   private_dep.source_types_used().Set(SourceFile::SOURCE_H);
   private_dep.public_deps().push_back(LabelTargetPair(&transitive));
-  private_dep.set_module_type(Target::GENERATED_TEXTUAL_MODULEMAP);
+  private_dep.set_module_type(kGeneratedTextualModulemap);
   private_dep.SetToolchain(setup.toolchain());
   ASSERT_TRUE(private_dep.OnResolved(&err));
 
@@ -3045,7 +3050,7 @@
   public_dep.sources().push_back(SourceFile("//foo/public_dep.h"));
   public_dep.private_deps().push_back(LabelTargetPair(&transitive));
   public_dep.public_deps().push_back(LabelTargetPair(&transitive));
-  public_dep.set_module_type(Target::GENERATED_TEXTUAL_MODULEMAP);
+  public_dep.set_module_type(kGeneratedTextualModulemap);
   public_dep.SetToolchain(setup.toolchain());
   ASSERT_TRUE(public_dep.OnResolved(&err));
 
@@ -3070,7 +3075,7 @@
   root.source_types_used().Set(SourceFile::SOURCE_H);
   root.public_deps().push_back(LabelTargetPair(&group_dep));
   root.private_deps().push_back(LabelTargetPair(&private_dep));
-  root.set_module_type(Target::GENERATED_TEXTUAL_MODULEMAP);
+  root.set_module_type(kGeneratedTextualModulemap);
   root.SetToolchain(setup.toolchain());
   ASSERT_TRUE(root.OnResolved(&err));
 

diff --git a/src/gn/ninja_module_writer_util.cc b/src/gn/ninja_module_writer_util.cc
index eab5554..621b4b2 100644
--- a/src/gn/ninja_module_writer_util.cc
+++ b/src/gn/ninja_module_writer_util.cc

@@ -8,6 +8,7 @@
 #include <set>
 #include <utility>
 
+#include "base/strings/stringprintf.h"
 #include "gn/resolved_target_data.h"
 #include "gn/substitution_writer.h"
 #include "gn/target.h"
@@ -47,31 +48,27 @@
   std::set<ClangModuleDep> ret;
 
   auto add_if_new = [&ret](const Target* t, bool is_self) {
+    if (!t->module_type().test(Target::HAS_MODULEMAP))
+      return;
+
     std::optional<OutputFile> pcm = std::nullopt;
-    switch (t->module_type()) {
-      case Target::GENERATED_TEXTUAL_MODULEMAP:
-        ret.emplace(t->modulemap_file(), t->module_name(), std::nullopt,
-                    is_self);
-        break;
-      case Target::EXPLICIT_MODULEMAP: {
-        auto modulemap = t->modulemap_file();
-        CHECK(modulemap != nullptr);
-        const char* tool_type;
-        std::vector<OutputFile> modulemap_outputs;
-        CHECK(t->GetOutputFilesForSource(*modulemap, &tool_type,
-                                         &modulemap_outputs));
-        CHECK(modulemap_outputs.size() == 1u);
-        ret.emplace(modulemap, t->module_name(),
-                    std::move(modulemap_outputs[0]), is_self);
-        break;
-      }
-      default:
-        break;
+    auto modulemap = t->modulemap_file();
+    CHECK(modulemap != nullptr);
+    if (!t->module_type().test(Target::MODULEMAP_IS_TEXTUAL)) {
+      const char* tool_type;
+      std::vector<OutputFile> modulemap_outputs;
+      CHECK(t->GetOutputFilesForSource(*modulemap, &tool_type,
+                                       &modulemap_outputs));
+      CHECK(modulemap_outputs.size() == 1u);
+      pcm = std::move(modulemap_outputs[0]);
     }
+
+    ret.emplace(modulemap, t->module_name(), pcm, is_self);
   };
 
-  if (target->source_types_used().Get(SourceFile::SOURCE_MODULEMAP))
+  if (target->module_type().test(Target::HAS_MODULEMAP)) {
     add_if_new(target, true);
+  }
 
   for (const auto& pair : resolved.GetModuleDepsInformation(target))
     add_if_new(pair.target(), false);

diff --git a/src/gn/ninja_target_writer.cc b/src/gn/ninja_target_writer.cc
index e9c4bf4..9b45628 100644
--- a/src/gn/ninja_target_writer.cc
+++ b/src/gn/ninja_target_writer.cc

@@ -166,7 +166,7 @@
     NinjaBinaryTargetWriter writer(target, rules);
     writer.SetResolvedTargetData(resolved);
     writer.SetNinjaOutputs(ninja_outputs);
-    if (target->module_type() == Target::GENERATED_TEXTUAL_MODULEMAP) {
+    if (target->module_type().test(Target::MODULEMAP_IS_GENERATED)) {
       const SourceFile* modulemap = target->modulemap_file();
       CHECK(modulemap);
 

diff --git a/src/gn/target.cc b/src/gn/target.cc
index 956d66c..fbb440c 100644
--- a/src/gn/target.cc
+++ b/src/gn/target.cc

@@ -1399,40 +1399,32 @@
 
 void Target::set_module_type(ModuleType type) {
   module_type_ = type;
-  switch (type) {
-    case GENERATED_TEXTUAL_MODULEMAP: {
-      auto source_dir =
-          GetBuildDirForTargetAsOutputFile(this, BuildDirType::GEN)
-              .AsSourceDir(settings()->build_settings());
+  if (module_type_.test(MODULEMAP_IS_GENERATED)) {
+    auto source_dir = GetBuildDirForTargetAsOutputFile(this, BuildDirType::GEN)
+                          .AsSourceDir(settings()->build_settings());
 
-      generated_modulemap_file_ = SourceFile(
-          base::StringPrintf("%s%s.modulemap", source_dir.value().c_str(),
-                             label().name().c_str()));
-      break;
-    }
-    default:
-      break;
+    generated_modulemap_file_ = SourceFile(base::StringPrintf(
+        "%s%s.modulemap", source_dir.value().c_str(), label().name().c_str()));
   }
 }
 
 const SourceFile* Target::modulemap_file() const {
-  switch (module_type_) {
-    case GENERATED_TEXTUAL_MODULEMAP:
-      return &generated_modulemap_file_;
-    case EXPLICIT_MODULEMAP:
-      for (const SourceFile& sf : sources_) {
-        if (sf.IsModuleMapType()) {
-          return &sf;
-        }
-      }
-    default:
-      return nullptr;
+  if (module_type_.test(MODULEMAP_IS_GENERATED)) {
+    return &generated_modulemap_file_;
   }
+  if (module_type_.any()) {
+    for (const SourceFile& sf : sources_) {
+      if (sf.IsModuleMapType()) {
+        return &sf;
+      }
+    }
+  }
+  return nullptr;
 }
 
 const SourceFile* Target::private_modulemap_file() const {
   if (private_modulemap_file_.is_null() &&
-      module_type_ == GENERATED_TEXTUAL_MODULEMAP) {
+      module_type_.test(MODULEMAP_IS_GENERATED)) {
     auto public_modulemap = modulemap_file();
     std::string private_name = public_modulemap->GetName();
     // foo.modulemap -> foo.private.modulemap

diff --git a/src/gn/target.h b/src/gn/target.h
index 210df0c..c06f5fc 100644
--- a/src/gn/target.h
+++ b/src/gn/target.h

@@ -5,6 +5,7 @@
 #ifndef TOOLS_GN_TARGET_H_
 #define TOOLS_GN_TARGET_H_
 
+#include <bitset>
 #include <set>
 #include <string>
 #include <utility>
@@ -60,13 +61,14 @@
     DEPS_LINKED,  // Iterates through all non-data dependencies.
   };
 
-  enum ModuleType {
-    NO_MODULEMAP,
-    EXPLICIT_MODULEMAP,
-    // The target didn't have any public headers, so no modulemap is needed.
-    UNNECESSARY_MODULEMAP,
-    GENERATED_TEXTUAL_MODULEMAP,
+  enum ModuleTypeBits {
+    HAS_MODULEMAP,
+    MODULEMAP_IS_GENERATED,
+    MODULEMAP_IS_TEXTUAL,
+
+    N_MODULE_TYPE_BITS,
   };
+  using ModuleType = std::bitset<N_MODULE_TYPE_BITS>;
 
   using FileList = std::vector<SourceFile>;
   using StringVector = std::vector<std::string>;
@@ -538,7 +540,7 @@
   bool output_extension_set_ = false;
 
   std::string module_name_;
-  ModuleType module_type_ = NO_MODULEMAP;
+  ModuleType module_type_;
   // Only filled if the module type is GENERATED_*
   SourceFile generated_modulemap_file_;
   // For performance reasons we cache private_modulemap_file.