C++: Separates adding sources and private headers to CcCompilationHelper

This is part of a chain of CLs that will pull initialization of
CcCompilationContext from CcCompilationHelper.

RELNOTES:none
PiperOrigin-RevId: 198060027

5 files changed, 76 insertions, 27 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcBinary.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcBinary.java
index 334035c29d..eb3fb4747d 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcBinary.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcBinary.java
@@ -235,7 +235,8 @@ public abstract class CcBinary implements RuleConfiguredTargetFactory {
     CcCompilationHelper compilationHelper =
         new CcCompilationHelper(
                 ruleContext, semantics, featureConfiguration, ccToolchain, fdoSupport)
-            .fromCommon(common, /* additionalCopts= */ImmutableList.of())
+            .fromCommon(common, /* additionalCopts= */ ImmutableList.of())
+            .addPrivateHeaders(common.getPrivateHeaders())
             .addSources(common.getSources())
             .addDeps(ImmutableList.of(CppHelper.mallocForTarget(ruleContext)))
             .setFake(fake)
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCommon.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCommon.java
index 954671a4ca..8786a4b679 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCommon.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCommon.java
@@ -323,11 +323,11 @@ public final class CcCommon {
   }
 
   /**
-   * Returns a list of ({@link Artifact}, {@link Label}) pairs. Each pair represents an input
-   * source file and the label of the rule that generates it (or the label of the source file
-   * itself if it is an input file).
+   * Returns a list of ({@link Artifact}, {@link Label}) pairs. Each pair represents an input source
+   * file and the label of the rule that generates it (or the label of the source file itself if it
+   * is an input file).
    */
-  List<Pair<Artifact, Label>> getSources() {
+  List<Pair<Artifact, Label>> getPrivateHeaders() {
     Map<Artifact, Label> map = Maps.newLinkedHashMap();
     Iterable<? extends TransitiveInfoCollection> providers =
         ruleContext.getPrerequisitesIf("srcs", Mode.TARGET, FileProvider.class);
@@ -337,19 +337,43 @@ public final class CcCommon {
         // non-source artifacts with different labels, as that would require cleaning up the code
         // base without significant benefit; we should eventually make this consistent one way or
         // the other.
-        Label oldLabel = map.put(artifact, provider.getLabel());
-        boolean isHeader = CppFileTypes.CPP_HEADER.matches(artifact.getExecPath());
-        if (!isHeader
-            && SourceCategory.CC_AND_OBJC.getSourceTypes().matches(artifact.getExecPathString())
-            && oldLabel != null
-            && !oldLabel.equals(provider.getLabel())) {
-          ruleContext.attributeError("srcs", String.format(
-              "Artifact '%s' is duplicated (through '%s' and '%s')",
-              artifact.getExecPathString(), oldLabel, provider.getLabel()));
+        if (CppFileTypes.CPP_HEADER.matches(artifact.getExecPath())) {
+          map.put(artifact, provider.getLabel());
         }
       }
     }
+    return mapToListOfPairs(map);
+  }
+
+  /**
+   * Returns a list of ({@link Artifact}, {@link Label}) pairs. Each pair represents an input source
+   * file and the label of the rule that generates it (or the label of the source file itself if it
+   * is an input file).
+   */
+  List<Pair<Artifact, Label>> getSources() {
+    Map<Artifact, Label> map = Maps.newLinkedHashMap();
+    Iterable<? extends TransitiveInfoCollection> providers =
+        ruleContext.getPrerequisitesIf("srcs", Mode.TARGET, FileProvider.class);
+    for (TransitiveInfoCollection provider : providers) {
+      for (Artifact artifact : provider.getProvider(FileProvider.class).getFilesToBuild()) {
+        if (!CppFileTypes.CPP_HEADER.matches(artifact.getExecPath())) {
+          Label oldLabel = map.put(artifact, provider.getLabel());
+          if (SourceCategory.CC_AND_OBJC.getSourceTypes().matches(artifact.getExecPathString())
+              && oldLabel != null
+              && !oldLabel.equals(provider.getLabel())) {
+            ruleContext.attributeError(
+                "srcs",
+                String.format(
+                    "Artifact '%s' is duplicated (through '%s' and '%s')",
+                    artifact.getExecPathString(), oldLabel, provider.getLabel()));
+          }
+        }
+      }
+    }
+    return mapToListOfPairs(map);
+  }
 
+  private List<Pair<Artifact, Label>> mapToListOfPairs(Map<Artifact, Label> map) {
     ImmutableList.Builder<Pair<Artifact, Label>> result = ImmutableList.builder();
     for (Map.Entry<Artifact, Label> entry : map.entrySet()) {
       result.add(Pair.of(entry.getKey(), entry.getValue()));
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCompilationHelper.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCompilationHelper.java
index 5b782ef492..cec53199e4 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCompilationHelper.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCompilationHelper.java
@@ -387,6 +387,33 @@ public final class CcCompilationHelper {
     return this;
   }
 
+  public CcCompilationHelper addPrivateHeaders(Collection<Artifact> privateHeaders) {
+    for (Artifact privateHeader : privateHeaders) {
+      addPrivateHeader(privateHeader, ruleContext.getLabel());
+    }
+    return this;
+  }
+
+  public CcCompilationHelper addPrivateHeaders(Iterable<Pair<Artifact, Label>> privateHeaders) {
+    for (Pair<Artifact, Label> headerLabelPair : privateHeaders) {
+      addPrivateHeader(headerLabelPair.first, headerLabelPair.second);
+    }
+    return this;
+  }
+
+  private CcCompilationHelper addPrivateHeader(Artifact privateHeader, Label label) {
+    boolean isHeader = CppFileTypes.CPP_HEADER.matches(privateHeader.getExecPath());
+    boolean isTextualInclude =
+        CppFileTypes.CPP_TEXTUAL_INCLUDE.matches(privateHeader.getExecPath());
+    Preconditions.checkState(isHeader || isTextualInclude);
+
+    if (shouldProcessHeaders() && !isTextualInclude) {
+      compilationUnitSources.add(CppSource.create(privateHeader, label, CppSource.Type.HEADER));
+    }
+    this.privateHeaders.add(privateHeader);
+    return this;
+  }
+
   /**
    * Add the corresponding files as source files. These may also be header files, in which case they
    * will not be compiled, but also not made visible as includes to dependent rules. The given build
@@ -453,23 +480,19 @@ public final class CcCompilationHelper {
    */
   private void addSource(Artifact source, Label label) {
     Preconditions.checkNotNull(featureConfiguration);
-    boolean isHeader = CppFileTypes.CPP_HEADER.matches(source.getExecPath());
-    boolean isTextualInclude = CppFileTypes.CPP_TEXTUAL_INCLUDE.matches(source.getExecPath());
+    Preconditions.checkState(!CppFileTypes.CPP_HEADER.matches(source.getExecPath()));
     // We assume TreeArtifacts passed in are directories containing proper sources for compilation.
-    boolean isCompiledSource =
-        sourceCategory.getSourceTypes().matches(source.getExecPathString())
-            || source.isTreeArtifact();
-    if (isHeader || isTextualInclude) {
-      privateHeaders.add(source);
-    }
-    if (isTextualInclude || !isCompiledSource || (isHeader && !shouldProcessHeaders())) {
+    if (!sourceCategory.getSourceTypes().matches(source.getExecPathString())
+        && !source.isTreeArtifact()) {
+      // TODO(plf): If it's a non-source file we ignore it. This is only the case for precompiled
+      // files which should be forbidden in srcs of cc_library|binary and instead be migrated to
+      // cc_import rules.
       return;
     }
+
     boolean isClifInputProto = CppFileTypes.CLIF_INPUT_PROTO.matches(source.getExecPathString());
     CppSource.Type type;
-    if (isHeader) {
-      type = CppSource.Type.HEADER;
-    } else if (isClifInputProto) {
+    if (isClifInputProto) {
       type = CppSource.Type.CLIF_INPUT_PROTO;
     } else {
       type = CppSource.Type.SOURCE;
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcLibrary.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcLibrary.java
index f5d8e50eb7..e8e84dfc48 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcLibrary.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcLibrary.java
@@ -145,6 +145,7 @@ public abstract class CcLibrary implements RuleConfiguredTargetFactory {
                 ruleContext, semantics, featureConfiguration, ccToolchain, fdoSupport)
             .fromCommon(common, additionalCopts)
             .addSources(common.getSources())
+            .addPrivateHeaders(common.getPrivateHeaders())
             .addPublicHeaders(common.getHeaders())
             .enableCompileProviders()
             .addPrecompiledFiles(precompiledFiles);
diff --git a/src/main/java/com/google/devtools/build/lib/rules/objc/CompilationSupport.java b/src/main/java/com/google/devtools/build/lib/rules/objc/CompilationSupport.java
index 7ebb7aaea8..f6b6052895 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/objc/CompilationSupport.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/objc/CompilationSupport.java
@@ -318,7 +318,7 @@ public class CompilationSupport {
                 fdoSupport,
                 buildConfiguration)
             .addSources(sources)
-            .addSources(privateHdrs)
+            .addPrivateHeaders(privateHdrs)
             .addDefines(objcProvider.get(DEFINE))
             .enableCompileProviders()
             .addPublicHeaders(publicHdrs)