Add a feature to put each header into it's own submodule. This will be required

to keep the existing behavior when turning on C++ modules. The problem is that
a #include of a header gets internally translated into an import of the
header's (sub)module. Now, if all headers of a cc_library are in the same
module, this means, a single import of one of them makes the symbols of all of
them accessible. While this isn't inherently problematic, it breaks the
similarity between a modules and non-modules build.

This behavior is hidden behind a feature for now as I have just committed
necessary changes to Clang, which will need to be release to stable, before we
can flip everything.

Also, don't put a header as both normal and private header if it is present in both. (Or don't output two submodules with the new implementation).

--
MOS_MIGRATED_REVID=88723759

3 files changed, 49 insertions, 19 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcLibraryHelper.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcLibraryHelper.java
index a03fd9cd85..b530e9a04e 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcLibraryHelper.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcLibraryHelper.java
@@ -760,7 +760,8 @@ public final class CcLibraryHelper {
             collectModuleMaps(),
             additionalExportedHeaders,
             featureConfiguration.isEnabled(CppRuleClasses.HEADER_MODULES),
-            featureConfiguration.isEnabled(CppRuleClasses.MODULE_MAP_HOME_CWD));
+            featureConfiguration.isEnabled(CppRuleClasses.MODULE_MAP_HOME_CWD),
+            featureConfiguration.isEnabled(CppRuleClasses.GENERATE_SUBMODULES));
         ruleContext.registerAction(action);
       }
       if (model.getGeneratesPicHeaderModule()) {
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppModuleMapAction.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppModuleMapAction.java
index 23005c37d1..a5b5a7ec0a 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppModuleMapAction.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppModuleMapAction.java
@@ -30,6 +30,7 @@ import java.io.OutputStream;
 import java.nio.charset.StandardCharsets;
 import java.util.ArrayList;
 import java.util.Collection;
+import java.util.HashSet;
 import java.util.List;
 
 /**
@@ -55,11 +56,12 @@ public class CppModuleMapAction extends AbstractFileWriteAction {
   private final ImmutableList<CppModuleMap> dependencies;
   private final ImmutableList<PathFragment> additionalExportedHeaders;
   private final boolean compiledModule;
+  private final boolean generateSubmodules;
 
   public CppModuleMapAction(ActionOwner owner, CppModuleMap cppModuleMap,
       Iterable<Artifact> privateHeaders, Iterable<Artifact> publicHeaders,
       Iterable<CppModuleMap> dependencies, Iterable<PathFragment> additionalExportedHeaders,
-      boolean compiledModule, boolean moduleMapHomeIsCwd) {
+      boolean compiledModule, boolean moduleMapHomeIsCwd, boolean generateSubmodules) {
     super(owner, ImmutableList.<Artifact>of(), cppModuleMap.getArtifact(),
         /*makeExecutable=*/false);
     this.cppModuleMap = cppModuleMap;
@@ -69,6 +71,7 @@ public class CppModuleMapAction extends AbstractFileWriteAction {
     this.dependencies = ImmutableList.copyOf(dependencies);
     this.additionalExportedHeaders = ImmutableList.copyOf(additionalExportedHeaders);
     this.compiledModule = compiledModule;
+    this.generateSubmodules = generateSubmodules;
   }
 
   @Override
@@ -85,15 +88,19 @@ public class CppModuleMapAction extends AbstractFileWriteAction {
         String leadingPeriods = moduleMapHomeIsCwd ? "" : Strings.repeat("../", segmentsToExecPath);
         content.append("module \"").append(cppModuleMap.getName()).append("\" {\n");
         content.append("  export *\n");
+
+        HashSet<PathFragment> deduper = new HashSet<>();
+        for (Artifact artifact : publicHeaders) {
+          appendHeader(
+              content, "", artifact.getExecPath(), leadingPeriods, /*canCompile=*/true, deduper);
+        }
         for (Artifact artifact : privateHeaders) {
           appendHeader(content, "private", artifact.getExecPath(), leadingPeriods,
-              /*canCompile=*/true);
-        }
-        for (Artifact artifact : publicHeaders) {
-          appendHeader(content, "", artifact.getExecPath(), leadingPeriods, /*canCompile=*/true);
+              /*canCompile=*/true, deduper);
         }
         for (PathFragment additionalExportedHeader : additionalExportedHeaders) {
-          appendHeader(content, "", additionalExportedHeader, leadingPeriods, /*canCompile*/false);
+          appendHeader(
+              content, "", additionalExportedHeader, leadingPeriods, /*canCompile*/ false, deduper);
         }
         for (CppModuleMap dep : dependencies) {
           content.append("  use \"").append(dep.getName()).append("\"\n");
@@ -113,7 +120,15 @@ public class CppModuleMapAction extends AbstractFileWriteAction {
   }
   
   private void appendHeader(StringBuilder content, String visibilitySpecifier, PathFragment path,
-      String leadingPeriods, boolean canCompile) {
+      String leadingPeriods, boolean canCompile, HashSet<PathFragment> deduper) {
+    if (deduper.contains(path)) {
+      return;
+    }
+    deduper.add(path);
+    if (generateSubmodules) {
+      content.append("  module \"").append(path).append("\" {\n");
+      content.append("    export *\n  ");
+    }
     content.append("  ");
     if (!visibilitySpecifier.isEmpty()) {
       content.append(visibilitySpecifier).append(" ");
@@ -121,7 +136,11 @@ public class CppModuleMapAction extends AbstractFileWriteAction {
     if (!canCompile || !shouldCompileHeader(path)) {
       content.append("textual ");
     }
-    content.append("header \"").append(leadingPeriods).append(path).append("\"\n");
+    content.append("header \"").append(leadingPeriods).append(path).append("\"");
+    if (generateSubmodules) {
+      content.append("\n  }");
+    }
+    content.append("\n");
   }
   
   private boolean shouldCompileHeader(PathFragment path) {
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppRuleClasses.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppRuleClasses.java
index 34b4f1840e..9fa675d3dc 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppRuleClasses.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppRuleClasses.java
@@ -80,7 +80,7 @@ public class CppRuleClasses {
    * A string constant for the parse_headers feature.
    */
   public static final String PARSE_HEADERS = "parse_headers";
-  
+
   /**
    * A string constant for the preprocess_headers feature.
    */
@@ -91,43 +91,53 @@ public class CppRuleClasses {
    * header_modules features.
    */
   public static final String MODULE_MAPS = "module_maps";
-  
+
   /**
    * A string constant for the module_map_home_cwd feature.
    */
   public static final String MODULE_MAP_HOME_CWD = "module_map_home_cwd";
-  
+
   /**
    * A string constant for the layering_check feature.
    */
   public static final String LAYERING_CHECK = "layering_check";
-  
+
   /**
    * A string constant for the header_modules feature.
    */
   public static final String HEADER_MODULES = "header_modules";
-  
+
   /**
    * A string constant for the use_header_modules feature.
-   * 
+   *
    * <p>This feature is only used during rollout; we expect to default enable this once we
    * have verified that module-enabled compilation is stable enough.
    */
   public static final String USE_HEADER_MODULES = "use_header_modules";
 
   /**
+   * A string constant for the generate_submodules feature.
+   *
+   * <p>This feature is only used temporarily to make the switch to using submodules easier. With
+   * submodules, each header of a cc_library is placed into a submodule of the module generated for
+   * the appropriate target. As this influences the layering_check semantics and needs to be synced
+   * with a clang release, we want to be able to switch back and forth easily.
+   */
+  public static final String GENERATE_SUBMODULES = "generate_submodules";
+
+  /**
    * A string constant for switching on that a header module file includes information about
    * all its dependencies, so we do not need to pass all transitive dependent header modules on
    * the command line.
    */
   public static final String HEADER_MODULE_INCLUDES_DEPENDENCIES =
       "header_module_includes_dependencies";
-  
+
   /**
    * A string constant for the no_legacy_features feature.
-   * 
-   * </p>If this feature is enabled, Bazel will not extend the crosstool configuration with the
-   * default legacy feature set. 
+   *
+   * <p>If this feature is enabled, Bazel will not extend the crosstool configuration with the
+   * default legacy feature set.
    */
   public static final String NO_LEGACY_FEATURES = "no_legacy_features";
 }