Clean up the semantics of input discovering actions a bit by making updateInputs() and inputsKnown() non-overridable and removing setInputs().

This comes at the cost of adding a flag to every action instance that's not used for non-input-discovering actions, but I think that's a deal. Simpler APIs are good, mmmmkay?

Also fixed a few pre-existing issues in TestAction and ObjcCompileAction.

--
PiperOrigin-RevId: 148749734
MOS_MIGRATED_REVID=148749734

Author: lberki

src/main/java/com/google/devtools/build/lib/actions/AbstractAction.java

@@ -45,6 +45,7 @@
 import java.util.Collection;
 import java.util.Map;
 import java.util.Map.Entry;
+import javax.annotation.concurrent.GuardedBy;
 
 /**
  * Abstract implementation of Action which implements basic functionality: the inputs, outputs, and
@@ -94,8 +95,13 @@ public abstract class AbstractAction implements Action, SkylarkValue {
    */
   private final Iterable<Artifact> tools;
 
+  @GuardedBy("this")
+  private boolean inputsDiscovered = false;  // Only used when discoversInputs() returns true
+
   // The variable inputs is non-final only so that actions that discover their inputs can modify it.
+  @GuardedBy("this")
   private Iterable<Artifact> inputs;
+
   private final Iterable<String> clientEnvironmentVariables;
   private final RunfilesSupplier runfilesSupplier;
   private final ImmutableSet<Artifact> outputs;
@@ -164,15 +170,36 @@ public final ActionOwner getOwner() {
   }
 
   @Override
-  public boolean inputsKnown() {
-    return true;
+  public final synchronized boolean inputsDiscovered() {
+    return discoversInputs() ? inputsDiscovered : true;
   }
 
+  /**
+   * Should be overridden by actions that do input discovery.
+   *
+   * <p>The value returned by each instance should be constant over the lifetime of that instance.
+   *
+   * <p>If this returns true, {@link #discoverInputs(ActionExecutionContext)} must also be
+   * implemented.
+   */
   @Override
   public boolean discoversInputs() {
     return false;
   }
 
+  /**
+   * Run input discovery on the action.
+   *
+   * <p>Called by Blaze if {@link #discoversInputs()} returns true. It must return the set of
+   * input artifacts that were not known at analysis time. May also call
+   * {@link #updateInputs(Iterable<Artifact>)}; if it doesn't, the action itself must arrange for
+   * the newly discovered artifacts to be available during action execution, probably by keeping
+   * state in the action instance and using a custom action execution context and for
+   * {@code #updateInputs()} to be called during the execution of the action.
+   *
+   * <p>Since keeping state within an action bad, don't do that unless there is a very good reason
+   * to do so.
+   */
   @Override
   public Iterable<Artifact> discoverInputs(ActionExecutionContext actionExecutionContext)
       throws ActionExecutionException, InterruptedException {
@@ -192,10 +219,21 @@ public Iterable<Artifact> getAllowedDerivedInputs() {
         "Method must be overridden for actions that may have unknown inputs.");
   }
 
+  /**
+   * Should be called when the inputs of the action become known, that is, either during
+   * {@link #discoverInputs(ActionExecutionContext)} or during
+   * {@link #execute(ActionExecutionContext)}.
+   *
+   * <p>When an action discovers inputs, it must have been called by the time {@code #execute()}
+   * returns. It can be called both during {@code discoverInputs} and during {@code execute()}.
+   *
+   * <p>In addition to being called from action implementations, it will also be called by Bazel
+   * itself when an action is loaded from the on-disk action cache.
+   */
   @Override
-  public void updateInputs(Iterable<Artifact> inputs) {
-    throw new IllegalStateException(
-        "Method must be overridden for actions that may have unknown inputs.");
+  public final synchronized void updateInputs(Iterable<Artifact> inputs) {
+    this.inputs = CollectionUtils.makeImmutable(inputs);
+    inputsDiscovered = true;
   }
 
   @Override
@@ -204,12 +242,10 @@ public Iterable<Artifact> getTools() {
   }
 
   /**
-   * Should only be overridden by actions that need to optionally insert inputs. Actions that
-   * discover their inputs should use {@link #setInputs} to set the new iterable of inputs when they
-   * know it.
+   * Should not be overridden (it's non-final only for tests)
    */
   @Override
-  public Iterable<Artifact> getInputs() {
+  public synchronized Iterable<Artifact> getInputs() {
     return inputs;
   }
 
@@ -223,15 +259,6 @@ public RunfilesSupplier getRunfilesSupplier() {
     return runfilesSupplier;
   }
 
-  /**
-   * Set the inputs of the action. May only be used by an action that {@link #discoversInputs()}.
-   * The iterable passed in is automatically made immutable.
-   */
-  protected void setInputs(Iterable<Artifact> inputs) {
-    Preconditions.checkState(discoversInputs(), this);
-    this.inputs = CollectionUtils.makeImmutable(inputs);
-  }
-
   @Override
   public ImmutableSet<Artifact> getOutputs() {
     return outputs;
@@ -259,7 +286,7 @@ public Iterable<Artifact> getMandatoryInputs() {
   @Override
   public String toString() {
     return prettyPrint() + " (" + getMnemonic() + "[" + ImmutableList.copyOf(getInputs())
-        + (inputsKnown() ? " -> " : ", unknown inputs -> ")
+        + (inputsDiscovered() ? " -> " : ", unknown inputs -> ")
         + getOutputs() + "]" + ")";
   }
 

src/main/java/com/google/devtools/build/lib/actions/Action.java

@@ -176,7 +176,7 @@ Iterable<Artifact> discoverInputsStage2(SkyFunction.Environment env)
   /**
    * Informs the action that its inputs are {@code inputs}, and that its inputs are now known. Can
    * only be called for actions that discover inputs. After this method is called,
-   * {@link ActionExecutionMetadata#inputsKnown} should return true.
+   * {@link ActionExecutionMetadata#inputsDiscovered} should return true.
    */
   void updateInputs(Iterable<Artifact> inputs);
 

src/main/java/com/google/devtools/build/lib/actions/ActionCacheChecker.java

@@ -196,8 +196,8 @@ public Token getTokenIfNeedToExecute(
     }
     Iterable<Artifact> actionInputs = action.getInputs();
     // Resolve action inputs from cache, if necessary.
-    boolean inputsKnown = action.inputsKnown();
-    if (!inputsKnown && resolvedCacheArtifacts != null) {
+    boolean inputsDiscovered = action.inputsDiscovered();
+    if (!inputsDiscovered && resolvedCacheArtifacts != null) {
       // The action doesn't know its inputs, but the caller has a good idea of what they are.
       Preconditions.checkState(action.discoversInputs(),
           "Actions that don't know their inputs must discover them: %s", action);
@@ -211,7 +211,7 @@ public Token getTokenIfNeedToExecute(
       return new Token(getKeyString(action));
     }
 
-    if (!inputsKnown) {
+    if (!inputsDiscovered) {
       action.updateInputs(actionInputs);
     }
     return null;

src/main/java/com/google/devtools/build/lib/actions/ActionExecutionMetadata.java

@@ -106,7 +106,7 @@ public interface ActionExecutionMetadata extends ActionAnalysisMetadata {
    * built first, as usual.
    */
   @ThreadSafe
-  boolean inputsKnown();
+  boolean inputsDiscovered();
 
   /**
    * Returns true iff inputsKnown() may ever return false.

src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java

@@ -78,7 +78,6 @@
 import java.util.Set;
 import java.util.UUID;
 import javax.annotation.Nullable;
-import javax.annotation.concurrent.GuardedBy;
 
 /** Action that represents some kind of C++ compilation step. */
 @ThreadCompatible
@@ -215,9 +214,8 @@ public Collection<Artifact> getInputsForIncludedFile(
    */
   private final UUID actionClassId;
 
-  // This can be read/written from multiple threads, and so accesses should be synchronized.
-  @GuardedBy("this")
-  private boolean inputsKnown = false;
+  /** Whether this action needs to discover inputs. */
+  private final boolean discoversInputs;
 
   /**
    * Set when the action prepares for execution. Used to preserve state between preparation and
@@ -267,8 +265,6 @@ public Collection<Artifact> getInputsForIncludedFile(
    * @param lipoScannables List of artifacts to include-scan when this action is a lipo action
    * @param additionalIncludeScannables list of additional artifacts to include-scan
    * @param actionClassId TODO(bazel-team): Add parameter description
-   * @param executionRequirements out-of-band hints to be passed to the execution backend to signal
-   *     platform requirements
    * @param environment TODO(bazel-team): Add parameter description
    * @param builtinIncludeFiles List of include files that may be included even if they are not
    *     mentioned in the source file or any of the headers included by it
@@ -338,7 +334,7 @@ protected CppCompileAction(
     Preconditions.checkArgument(!shouldPruneModules || shouldScanIncludes, this);
     this.usePic = usePic;
     this.useHeaderModules = useHeaderModules;
-    this.inputsKnown = !shouldScanIncludes && !cppSemantics.needsDotdInputPruning();
+    this.discoversInputs = shouldScanIncludes || cppSemantics.needsDotdInputPruning();
     this.cppCompileCommandLine =
         new CppCompileCommandLine(
             sourceFile, dotdFile, copts, coptsFilter, features, variables, actionName);
@@ -405,11 +401,6 @@ public ImmutableSet<Artifact> getMandatoryOutputs() {
     return super.getMandatoryOutputs();
   }
 
-  @Override
-  public synchronized boolean inputsKnown() {
-    return inputsKnown;
-  }
-
   /**
    * Returns the list of additional inputs found by dependency discovery, during action preparation,
    * and clears the stored list. {@link #prepare} must be called before this method is called, on
@@ -428,7 +419,7 @@ public void setResolvedInputsForTesting(ImmutableList<Artifact> resolvedInputs)
 
   @Override
   public boolean discoversInputs() {
-    return true;
+    return discoversInputs;
   }
 
   @VisibleForTesting  // productionVisibility = Visibility.PRIVATE
@@ -761,7 +752,7 @@ public ExtraActionInfo.Builder getExtraActionInfo() {
     }
     info.setOutputFile(outputFile.getExecPathString());
     info.setSourceFile(getSourceFile().getExecPathString());
-    if (inputsKnown()) {
+    if (inputsDiscovered()) {
       info.addAllSourcesAndHeaders(Artifact.toExecPaths(getInputs()));
     } else {
       info.addSourcesAndHeaders(getSourceFile().getExecPathString());
@@ -958,11 +949,10 @@ private static boolean isDeclaredIn(
    *
    * @throws ActionExecutionException iff any errors happen during update.
    */
-  @VisibleForTesting
+  @VisibleForTesting  // productionVisibility = Visibility.PRIVATE
   @ThreadCompatible
-  public final synchronized void updateActionInputs(NestedSet<Artifact> discoveredInputs)
+  public final void updateActionInputs(NestedSet<Artifact> discoveredInputs)
       throws ActionExecutionException {
-    inputsKnown = false;
     NestedSetBuilder<Artifact> inputs = NestedSetBuilder.stableOrder();
     Profiler.instance().startTask(ProfilerTask.ACTION_UPDATE, this);
     try {
@@ -972,12 +962,9 @@ public final synchronized void updateActionInputs(NestedSet<Artifact> discovered
       }
       inputs.addAll(context.getTransitiveCompilationPrerequisites());
       inputs.addTransitive(discoveredInputs);
-      inputsKnown = true;
+      updateInputs(inputs.build());
     } finally {
       Profiler.instance().completeTask(ProfilerTask.ACTION_UPDATE);
-      synchronized (this) {
-        setInputs(inputs.build());
-      }
     }
   }
 
@@ -1013,18 +1000,6 @@ private static CcToolchainFeatures.Variables getOverwrittenVariables(
     return variableBuilder.build();
   }
 
-  @Override protected void setInputs(Iterable<Artifact> inputs) {
-    super.setInputs(inputs);
-  }
-
-  @Override
-  public synchronized void updateInputs(Iterable<Artifact> inputs) {
-    inputsKnown = true;
-    synchronized (this) {
-      setInputs(inputs);
-    }
-  }
-
   @Override
   public Iterable<Artifact> getAllowedDerivedInputs() {
     return getAllowedDerivedInputsMap().values();

src/main/java/com/google/devtools/build/lib/rules/cpp/LTOBackendAction.java

@@ -37,7 +37,6 @@
 import java.util.Map;
 import java.util.Set;
 import javax.annotation.Nullable;
-import javax.annotation.concurrent.GuardedBy;
 
 /**
  * Action used by LTOBackendArtifacts to create an LTOBackendAction. Similar to {@link SpawnAction},
@@ -54,10 +53,6 @@
  * http://blog.llvm.org/2016/06/thinlto-scalable-and-incremental-lto.html.
  */
 public final class LTOBackendAction extends SpawnAction {
-  // This can be read/written from multiple threads, and so accesses should be synchronized.
-  @GuardedBy("this")
-  private boolean inputsKnown;
-
   private Collection<Artifact> mandatoryInputs;
   private Map<PathFragment, Artifact> bitcodeFiles;
   private Artifact imports;
@@ -92,8 +87,6 @@ public LTOBackendAction(
         mnemonic,
         false,
         null);
-
-    inputsKnown = false;
     mandatoryInputs = inputs;
     bitcodeFiles = allBitcodeFiles;
     imports = importsFile;
@@ -149,11 +142,6 @@ public Iterable<Artifact> discoverInputs(ActionExecutionContext actionExecutionC
     return bitcodeInputSet;
   }
 
-  @Override
-  public synchronized boolean inputsKnown() {
-    return inputsKnown;
-  }
-
   @Override
   public Collection<Artifact> getMandatoryInputs() {
     return mandatoryInputs;
@@ -166,12 +154,6 @@ private static Iterable<Artifact> createInputs(
     return result;
   }
 
-  @Override
-  public synchronized void updateInputs(Iterable<Artifact> discoveredInputs) {
-    setInputs(discoveredInputs);
-    inputsKnown = true;
-  }
-
   @Override
   public Iterable<Artifact> getAllowedDerivedInputs() {
     return bitcodeFiles.values();
@@ -181,10 +163,6 @@ public Iterable<Artifact> getAllowedDerivedInputs() {
   public void execute(ActionExecutionContext actionExecutionContext)
       throws ActionExecutionException, InterruptedException {
     super.execute(actionExecutionContext);
-
-    synchronized (this) {
-      inputsKnown = true;
-    }
   }
 
   @Override