Simplify CppCompileAction#discoverInputs().

In the process, make it so that running an extra action attached to a
CppCompileAction does not change the state of the CppCompileAction (yes, this
happened: if include scanning was not done, topLevelModules would be changed)

There are two changes in behavior this will: introduce

1. topLevelModules will no longer be set if include scanning is not in effect, but that's okay: it's never read except when shouldPruneModules is true, and if that is true, #discoverInputsStage2() will set it.
2. Extra actions attached to CppCompileAction will not get .pcm files on their inputs that were not used by the compiler

RELNOTES: None.
PiperOrigin-RevId: 199285276

3 files changed, 48 insertions, 67 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java
index eaf79482d3..f19dc727e6 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java
@@ -170,7 +170,7 @@ public class CppCompileAction extends AbstractAction
    * {@link #mandatoryInputs}. Thus, even with include scanning turned off, we pretend that we
    * "discover" these headers.
    */
-  private final NestedSet<Artifact> prunableInputs;
+  private final NestedSet<Artifact> prunableHeaders;
 
   @Nullable private final Artifact grepIncludes;
   private final boolean shouldScanIncludes;
@@ -271,7 +271,7 @@ public class CppCompileAction extends AbstractAction
       NestedSet<Artifact> mandatoryInputs,
       Iterable<Artifact> inputsForInvalidation,
       ImmutableList<Artifact> builtinIncludeFiles,
-      NestedSet<Artifact> prunableInputs,
+      NestedSet<Artifact> prunableHeaders,
       Artifact outputFile,
       DotdFile dotdFile,
       @Nullable Artifact gcnoFile,
@@ -304,7 +304,7 @@ public class CppCompileAction extends AbstractAction
         // artifact and will definitely exist prior to this action execution.
         mandatoryInputs,
         inputsForInvalidation,
-        prunableInputs,
+        prunableHeaders,
         // inputsKnown begins as the logical negation of shouldScanIncludes.
         // When scanning includes, the inputs begin as not known, and become
         // known after inclusion scanning. When *not* scanning includes,
@@ -350,7 +350,7 @@ public class CppCompileAction extends AbstractAction
       Artifact sourceFile,
       NestedSet<Artifact> mandatoryInputs,
       Iterable<Artifact> inputsForInvalidation,
-      NestedSet<Artifact> prunableInputs,
+      NestedSet<Artifact> prunableHeaders,
       boolean shouldScanIncludes,
       boolean shouldPruneModules,
       boolean pruneCppInputDiscovery,
@@ -381,7 +381,7 @@ public class CppCompileAction extends AbstractAction
     this.sourceFile = sourceFile;
     this.mandatoryInputs = mandatoryInputs;
     this.inputsForInvalidation = inputsForInvalidation;
-    this.prunableInputs = prunableInputs;
+    this.prunableHeaders = prunableHeaders;
     this.shouldScanIncludes = shouldScanIncludes;
     this.shouldPruneModules = shouldPruneModules;
     this.pruneCppInputDiscovery = pruneCppInputDiscovery;
@@ -455,8 +455,8 @@ public class CppCompileAction extends AbstractAction
 
   /**
    * Returns the list of additional inputs found by dependency discovery, during action preparation,
-   * and clears the stored list. {@link Action#prepare} must be called before this method is called,
-   * on each action execution.
+   * and clears the stored list. {@link #discoverInputs(ActionExecutionContext)} must be called
+   * before this method is called on each action execution.
    */
   public Iterable<Artifact> getAdditionalInputs() {
     Iterable<Artifact> result = Preconditions.checkNotNull(additionalInputs);
@@ -472,18 +472,17 @@ public class CppCompileAction extends AbstractAction
   @Override
   @VisibleForTesting // productionVisibility = Visibility.PRIVATE
   public Iterable<Artifact> getPossibleInputsForTesting() {
-    return Iterables.concat(getInputs(), prunableInputs);
+    return Iterables.concat(getInputs(), prunableHeaders);
   }
 
   /**
-   * Returns the results of include scanning or, when that is null, all prunable inputs and header
-   * modules.
+   * Returns the results of include scanning or, when that is null, all prunable headers.
    */
-  private Iterable<Artifact> findAdditionalInputs(ActionExecutionContext actionExecutionContext)
+  private Iterable<Artifact> findUsedHeaders(ActionExecutionContext actionExecutionContext)
       throws ActionExecutionException, InterruptedException {
-    Iterable<Artifact> initialResult;
+    Iterable<Artifact> includeScanningResult;
     try {
-      initialResult =
+      includeScanningResult =
           actionExecutionContext
               .getContext(CppIncludeScanningContext.class)
               .findAdditionalInputs(this, actionExecutionContext, includeProcessing);
@@ -494,52 +493,43 @@ public class CppCompileAction extends AbstractAction
           this);
     }
 
-    if (initialResult == null) {
-      NestedSetBuilder<Artifact> result = NestedSetBuilder.stableOrder();
-      if (useHeaderModules) {
-        // Here, we cannot really know what the top-level modules are, so we just mark all
-        // transitive modules as "top level".
-        topLevelModules =
-            Sets.newLinkedHashSet(ccCompilationContext.getTransitiveModules(usePic).toCollection());
-        result.addTransitive(ccCompilationContext.getTransitiveModules(usePic));
-      }
-      result.addTransitive(prunableInputs);
-      return result.build();
-    } else {
-      return initialResult;
-    }
+    return includeScanningResult == null ? prunableHeaders : includeScanningResult;
   }
 
   @Nullable
   @Override
   public Iterable<Artifact> discoverInputs(ActionExecutionContext actionExecutionContext)
       throws ActionExecutionException, InterruptedException {
-    Iterable<Artifact> initialResult = findAdditionalInputs(actionExecutionContext);
+    Iterable<Artifact> foundHeaders = findUsedHeaders(actionExecutionContext);
+    if (!shouldPruneModules) {
+      additionalInputs = foundHeaders;
+      return additionalInputs;
+    }
 
-    if (shouldPruneModules) {
-      Set<Artifact> initialResultSet = Sets.newLinkedHashSet(initialResult);
-      if (sourceFile.isFileType(CppFileTypes.CPP_MODULE)) {
-        usedModules = ImmutableSet.of(sourceFile);
-        initialResultSet.add(sourceFile);
-      } else {
-        usedModules = Sets.newLinkedHashSet();
-        topLevelModules = null;
-        for (CcCompilationContext.TransitiveModuleHeaders usedModule :
-            ccCompilationContext.getUsedModules(usePic, initialResultSet)) {
-          usedModules.add(usedModule.getModule());
-        }
-        initialResultSet.addAll(usedModules);
+    Set<Artifact> usedHeadersAndModules = Sets.newLinkedHashSet(foundHeaders);
+    if (sourceFile.isFileType(CppFileTypes.CPP_MODULE)) {
+      // If we are generating code from a module, the module is all we need.
+      // TODO(djasper): Do we really need the source files?
+      usedModules = ImmutableSet.of(sourceFile);
+      usedHeadersAndModules.add(sourceFile);
+    } else {
+      usedModules = Sets.newLinkedHashSet();
+      // usedHeadersAndModules only contains headers now, so we can pass it to getUsedModules()
+      // (and even if it contained other things, it's only used to check for the presence of headers
+      // so it would not matter)
+      for (CcCompilationContext.TransitiveModuleHeaders usedModule :
+          ccCompilationContext.getUsedModules(usePic, usedHeadersAndModules)) {
+        usedModules.add(usedModule.getModule());
       }
-      initialResult = initialResultSet;
+      usedHeadersAndModules.addAll(usedModules);
     }
-
-    additionalInputs = initialResult;
+    additionalInputs = usedHeadersAndModules;
     return additionalInputs;
   }
 
   @Override
   public Iterable<Artifact> discoverInputsStage2(SkyFunction.Environment env)
-      throws ActionExecutionException, InterruptedException {
+      throws InterruptedException {
     if (this.usedModules == null) {
       return null;
     }
@@ -609,10 +599,6 @@ public class CppCompileAction extends AbstractAction
     return outputFile;
   }
 
-  protected PathFragment getInternalOutputFile() {
-    return outputFile.getExecPath();
-  }
-
   @Override
   public Map<Artifact, Artifact> getLegalGeneratedScannerFileMap() {
     Map<Artifact, Artifact> legalOuts = new HashMap<>();
@@ -854,7 +840,7 @@ public class CppCompileAction extends AbstractAction
     IncludeProblems errors = new IncludeProblems();
     IncludeProblems warnings = new IncludeProblems();
     Set<Artifact> allowedIncludes = new HashSet<>();
-    for (Artifact input : Iterables.concat(mandatoryInputs, prunableInputs)) {
+    for (Artifact input : Iterables.concat(mandatoryInputs, prunableHeaders)) {
       if (input.isMiddlemanArtifact() || input.isTreeArtifact()) {
         actionExecutionContext.getArtifactExpander().expand(input, allowedIncludes);
       }
@@ -1045,7 +1031,7 @@ public class CppCompileAction extends AbstractAction
   public Iterable<Artifact> getAllowedDerivedInputs() {
     HashSet<Artifact> result = new HashSet<>();
     addNonSources(result, mandatoryInputs);
-    addNonSources(result, prunableInputs);
+    addNonSources(result, prunableHeaders);
     addNonSources(result, getDeclaredIncludeSrcs());
     addNonSources(result, ccCompilationContext.getTransitiveCompilationPrerequisites());
     addNonSources(result, ccCompilationContext.getTransitiveModules(usePic));
@@ -1139,7 +1125,7 @@ public class CppCompileAction extends AbstractAction
     fp.addInt(0); // mark the boundary between input types
     actionKeyContext.addNestedSetToFingerprint(fp, getMandatoryInputs());
     fp.addInt(0);
-    actionKeyContext.addNestedSetToFingerprint(fp, prunableInputs);
+    actionKeyContext.addNestedSetToFingerprint(fp, prunableHeaders);
   }
 
   @Override
@@ -1332,7 +1318,7 @@ public class CppCompileAction extends AbstractAction
   public Iterable<Artifact> getInputFilesForExtraAction(
       ActionExecutionContext actionExecutionContext)
       throws ActionExecutionException, InterruptedException {
-    Iterable<Artifact> discoveredInputs = findAdditionalInputs(actionExecutionContext);
+    Iterable<Artifact> discoveredInputs = findUsedHeaders(actionExecutionContext);
     return Sets.<Artifact>difference(
         ImmutableSet.<Artifact>copyOf(discoveredInputs),
         ImmutableSet.<Artifact>copyOf(getInputs()));
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileActionBuilder.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileActionBuilder.java
index 877333bf47..13d8f3eea6 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileActionBuilder.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileActionBuilder.java
@@ -333,9 +333,9 @@ public class CppCompileActionBuilder {
     NestedSet<Artifact> realMandatoryInputs = buildMandatoryInputs();
     NestedSet<Artifact> allInputs = buildAllInputs(realMandatoryInputs);
 
-    NestedSetBuilder<Artifact> prunableInputBuilder = NestedSetBuilder.stableOrder();
-    prunableInputBuilder.addTransitive(ccCompilationContext.getDeclaredIncludeSrcs());
-    prunableInputBuilder.addTransitive(cppSemantics.getAdditionalPrunableIncludes());
+    NestedSetBuilder<Artifact> prunableHeadersBuilder = NestedSetBuilder.stableOrder();
+    prunableHeadersBuilder.addTransitive(ccCompilationContext.getDeclaredIncludeSrcs());
+    prunableHeadersBuilder.addTransitive(cppSemantics.getAdditionalPrunableIncludes());
 
     Iterable<IncludeScannable> lipoScannables = getLipoScannables(realMandatoryInputs);
     // We need to add "legal generated scanner files" coming through LIPO scannables here. These
@@ -346,12 +346,12 @@ public class CppCompileActionBuilder {
     for (IncludeScannable lipoScannable : lipoScannables) {
       for (Artifact value : lipoScannable.getLegalGeneratedScannerFileMap().values()) {
         if (value != null) {
-          prunableInputBuilder.add(value);
+          prunableHeadersBuilder.add(value);
         }
       }
     }
 
-    NestedSet<Artifact> prunableInputs = prunableInputBuilder.build();
+    NestedSet<Artifact> prunableHeaders = prunableHeadersBuilder.build();
 
     // Copying the collections is needed to make the builder reusable.
     CppCompileAction action;
@@ -373,7 +373,7 @@ public class CppCompileActionBuilder {
               realMandatoryInputs,
               inputsForInvalidation,
               getBuiltinIncludeFiles(),
-              prunableInputs,
+              prunableHeaders,
               outputFile,
               tempOutputFile,
               dotdFile,
@@ -402,7 +402,7 @@ public class CppCompileActionBuilder {
               realMandatoryInputs,
               inputsForInvalidation,
               getBuiltinIncludeFiles(),
-              prunableInputs,
+              prunableHeaders,
               outputFile,
               dotdFile,
               gcnoFile,
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/FakeCppCompileAction.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/FakeCppCompileAction.java
index 78fc2130a6..7e6d2783bb 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/FakeCppCompileAction.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/FakeCppCompileAction.java
@@ -71,7 +71,7 @@ public class FakeCppCompileAction extends CppCompileAction {
       NestedSet<Artifact> mandatoryInputs,
       Iterable<Artifact> inputsForInvalidation,
       ImmutableList<Artifact> builtinIncludeFiles,
-      NestedSet<Artifact> prunableInputs,
+      NestedSet<Artifact> prunableHeaders,
       Artifact outputFile,
       PathFragment tempOutputFile,
       DotdFile dotdFile,
@@ -98,7 +98,7 @@ public class FakeCppCompileAction extends CppCompileAction {
         mandatoryInputs,
         inputsForInvalidation,
         builtinIncludeFiles,
-        prunableInputs,
+        prunableHeaders,
         outputFile,
         dotdFile,
         /* gcnoFile=*/ null,
@@ -258,11 +258,6 @@ public class FakeCppCompileAction extends CppCompileAction {
   }
 
   @Override
-  protected PathFragment getInternalOutputFile() {
-    return tempOutputFile;
-  }
-
-  @Override
   public String getMnemonic() {
     return "FakeCppCompile";
   }