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

14 files changed, 82 insertions, 214 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/actions/AbstractAction.java b/src/main/java/com/google/devtools/build/lib/actions/AbstractAction.java
index 80f6d7df6b..78a1d5e15a 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/AbstractAction.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/AbstractAction.java
@@ -45,6 +45,7 @@ import java.io.IOException;
 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 abstract class AbstractAction implements Action, SkylarkValue {
   }
 
   @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 abstract class AbstractAction implements Action, SkylarkValue {
         "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 abstract class AbstractAction implements Action, SkylarkValue {
   }
 
   /**
-   * 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 abstract class AbstractAction implements Action, SkylarkValue {
     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 abstract class AbstractAction implements Action, SkylarkValue {
   @Override
   public String toString() {
     return prettyPrint() + " (" + getMnemonic() + "[" + ImmutableList.copyOf(getInputs())
-        + (inputsKnown() ? " -> " : ", unknown inputs -> ")
+        + (inputsDiscovered() ? " -> " : ", unknown inputs -> ")
         + getOutputs() + "]" + ")";
   }
 
diff --git a/src/main/java/com/google/devtools/build/lib/actions/Action.java b/src/main/java/com/google/devtools/build/lib/actions/Action.java
index bee4774a66..ee193ba943 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/Action.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/Action.java
@@ -176,7 +176,7 @@ public interface Action extends ActionExecutionMetadata, Describable {
   /**
    * 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);
 
diff --git a/src/main/java/com/google/devtools/build/lib/actions/ActionCacheChecker.java b/src/main/java/com/google/devtools/build/lib/actions/ActionCacheChecker.java
index 717083685f..eac03d982b 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/ActionCacheChecker.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/ActionCacheChecker.java
@@ -196,8 +196,8 @@ public class ActionCacheChecker {
     }
     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 class ActionCacheChecker {
       return new Token(getKeyString(action));
     }
 
-    if (!inputsKnown) {
+    if (!inputsDiscovered) {
       action.updateInputs(actionInputs);
     }
     return null;
diff --git a/src/main/java/com/google/devtools/build/lib/actions/ActionExecutionMetadata.java b/src/main/java/com/google/devtools/build/lib/actions/ActionExecutionMetadata.java
index 63748aff99..6184954699 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/ActionExecutionMetadata.java
+++ b/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.
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 afc77d23c3..d1f0e4b8f6 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
@@ -78,7 +78,6 @@ import java.util.Map;
 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 class CppCompileAction extends AbstractAction
    */
   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 class CppCompileAction extends AbstractAction
    * @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 @@ public class CppCompileAction extends AbstractAction
     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 class CppCompileAction extends AbstractAction
     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 class CppCompileAction extends AbstractAction
 
   @Override
   public boolean discoversInputs() {
-    return true;
+    return discoversInputs;
   }
 
   @VisibleForTesting  // productionVisibility = Visibility.PRIVATE
@@ -761,7 +752,7 @@ public class CppCompileAction extends AbstractAction
     }
     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 @@ public class CppCompileAction extends AbstractAction
    *
    * @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 class CppCompileAction extends AbstractAction
       }
       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 @@ public class CppCompileAction extends AbstractAction
     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();
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/LTOBackendAction.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/LTOBackendAction.java
index 0b71041ceb..9af100ffd8 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/LTOBackendAction.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/LTOBackendAction.java
@@ -37,7 +37,6 @@ import java.util.LinkedHashSet;
 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 @@ import javax.annotation.concurrent.GuardedBy;
  * 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 final class LTOBackendAction extends SpawnAction {
         mnemonic,
         false,
         null);
-
-    inputsKnown = false;
     mandatoryInputs = inputs;
     bitcodeFiles = allBitcodeFiles;
     imports = importsFile;
@@ -150,11 +143,6 @@ public final class LTOBackendAction extends SpawnAction {
   }
 
   @Override
-  public synchronized boolean inputsKnown() {
-    return inputsKnown;
-  }
-
-  @Override
   public Collection<Artifact> getMandatoryInputs() {
     return mandatoryInputs;
   }
@@ -167,12 +155,6 @@ public final class LTOBackendAction extends SpawnAction {
   }
 
   @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 final class LTOBackendAction extends SpawnAction {
   public void execute(ActionExecutionContext actionExecutionContext)
       throws ActionExecutionException, InterruptedException {
     super.execute(actionExecutionContext);
-
-    synchronized (this) {
-      inputsKnown = true;
-    }
   }
 
   @Override
diff --git a/src/main/java/com/google/devtools/build/lib/rules/extra/ExtraAction.java b/src/main/java/com/google/devtools/build/lib/rules/extra/ExtraAction.java
index a293b00497..b2f7fdbbac 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/extra/ExtraAction.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/extra/ExtraAction.java
@@ -41,7 +41,6 @@ import java.util.Collection;
 import java.util.Map;
 import java.util.Set;
 import javax.annotation.Nullable;
-import javax.annotation.concurrent.GuardedBy;
 
 /**
  * Action used by extra_action rules to create an action that shadows an existing action. Runs a
@@ -54,10 +53,6 @@ public final class ExtraAction extends SpawnAction {
   private final ImmutableSet<Artifact> extraActionInputs;
   private final Iterable<Artifact> originalShadowedActionInputs;
 
-  // This can be read/written from multiple threads, and so accesses should be synchronized.
-  @GuardedBy("this")
-  private boolean inputsKnown;
-
   /**
    * A long way to say (ExtraAction xa) -> xa.getShadowedAction().
    */
@@ -104,7 +99,6 @@ public final class ExtraAction extends SpawnAction {
     this.createDummyOutput = createDummyOutput;
 
     this.extraActionInputs = extraActionInputs;
-    inputsKnown = shadowedAction.inputsKnown();
     if (createDummyOutput) {
       // Expecting just a single dummy file in the outputs.
       Preconditions.checkArgument(outputs.size() == 1, outputs);
@@ -123,7 +117,7 @@ public final class ExtraAction extends SpawnAction {
     Preconditions.checkState(discoversInputs(), this);
     // We depend on the outputs of actions doing input discovery and they should know their inputs
     // after having been executed
-    Preconditions.checkState(shadowedAction.inputsKnown());
+    Preconditions.checkState(shadowedAction.inputsDiscovered());
 
     // We need to update our inputs to take account of any additional
     // inputs the shadowed action may need to do its work.
@@ -132,11 +126,6 @@ public final class ExtraAction extends SpawnAction {
         ImmutableSet.copyOf(originalShadowedActionInputs));
   }
 
-  @Override
-  public synchronized boolean inputsKnown() {
-    return inputsKnown;
-  }
-
   private static NestedSet<Artifact> createInputs(
       Iterable<Artifact> shadowedActionInputs,
       ImmutableSet<Artifact> extraActionInputs,
@@ -152,12 +141,6 @@ public final class ExtraAction extends SpawnAction {
   }
 
   @Override
-  public synchronized void updateInputs(Iterable<Artifact> discoveredInputs) {
-    setInputs(discoveredInputs);
-    inputsKnown = true;
-  }
-
-  @Override
   public Iterable<Artifact> getAllowedDerivedInputs() {
     return shadowedAction.getAllowedDerivedInputs();
   }
@@ -189,9 +172,6 @@ public final class ExtraAction extends SpawnAction {
         }
       }
     }
-    synchronized (this) {
-      inputsKnown = true;
-    }
   }
 
   /**
diff --git a/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcCompileAction.java b/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcCompileAction.java
index 8164cc3c85..8087928219 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcCompileAction.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcCompileAction.java
@@ -46,7 +46,6 @@ import com.google.devtools.build.lib.rules.apple.Platform;
 import com.google.devtools.build.lib.rules.cpp.CppCompileAction.DotdFile;
 import com.google.devtools.build.lib.rules.cpp.CppFileTypes;
 import com.google.devtools.build.lib.rules.cpp.HeaderDiscovery;
-import com.google.devtools.build.lib.rules.cpp.HeaderDiscovery.DotdPruningMode;
 import com.google.devtools.build.lib.rules.cpp.IncludeScanningContext;
 import com.google.devtools.build.lib.util.DependencySet;
 import com.google.devtools.build.lib.util.Fingerprint;
@@ -56,13 +55,11 @@ import com.google.devtools.build.lib.vfs.PathFragment;
 import java.io.IOException;
 import java.nio.charset.StandardCharsets;
 import java.util.ArrayList;
-import java.util.Collections;
 import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
 import java.util.Map.Entry;
 import javax.annotation.Nullable;
-import javax.annotation.concurrent.GuardedBy;
 
 /**
  * An action that compiles objc or objc++ source.
@@ -111,10 +108,6 @@ public class ObjcCompileAction extends SpawnAction {
 
   private Iterable<Artifact> discoveredInputs;
 
-  // This can be read/written from multiple threads, so accesses must be synchronized.
-  @GuardedBy("this")
-  private boolean inputsKnown = false;
-
   private static final String GUID = "a00d5bac-a72c-4f0f-99a7-d5fdc6072137";
 
   private ObjcCompileAction(
@@ -157,7 +150,6 @@ public class ObjcCompileAction extends SpawnAction {
     this.sourceFile = sourceFile;
     this.mandatoryInputs = mandatoryInputs;
     this.dotdPruningPlan = dotdPruningPlan;
-    this.inputsKnown = (dotdPruningPlan == DotdPruningMode.DO_NOT_USE && headersListFile == null);
     this.headers = headers;
     this.headersListFile = headersListFile;
   }
@@ -182,11 +174,6 @@ public class ObjcCompileAction extends SpawnAction {
   }
 
   @Override
-  public synchronized boolean inputsKnown() {
-    return inputsKnown;
-  }
-
-  @Override
   public final Spawn getSpawn(Map<String, String> clientEnv) {
     return new ObjcCompileActionSpawn(clientEnv);
   }
@@ -287,14 +274,6 @@ public class ObjcCompileAction extends SpawnAction {
   }
 
   @Override
-  public synchronized void updateInputs(Iterable<Artifact> inputs) {
-    inputsKnown = true;
-    synchronized (this) {
-      setInputs(inputs);
-    }
-  }
-
-  @Override
   public ImmutableSet<Artifact> getMandatoryOutputs() {
     return ImmutableSet.of(dotdFile.artifact());
   }
@@ -312,6 +291,12 @@ public class ObjcCompileAction extends SpawnAction {
               executor.getExecRoot(), scanningContext.getArtifactResolver());
 
       updateActionInputs(discoveredInputs);
+    } else {
+      // TODO(lberki): This is awkward, but necessary since updateInputs() must be called when
+      // input discovery is in effect. I *think* it's possible to avoid setting discoversInputs()
+      // to true if the header list file is null and then we'd not need to have this here, but I
+      // haven't quite managed to get that right yet.
+      updateActionInputs(getInputs());
     }
   }
 
@@ -370,26 +355,22 @@ public class ObjcCompileAction extends SpawnAction {
    *
    * @throws ActionExecutionException iff any errors happen during update.
    */
-  @VisibleForTesting
+  @VisibleForTesting  // productionVisibility = Visibility.PRIVATE
   @ThreadCompatible
-  public final synchronized void updateActionInputs(Iterable<Artifact> discoveredInputs)
+  final void updateActionInputs(Iterable<Artifact> discoveredInputs)
       throws ActionExecutionException {
-    inputsKnown = false;
-    Iterable<Artifact> inputs = Collections.emptyList();
     Profiler.instance().startTask(ProfilerTask.ACTION_UPDATE, this);
     try {
-      inputs = Iterables.concat(mandatoryInputs, discoveredInputs);
-      inputsKnown = true;
+      updateInputs(Iterables.concat(mandatoryInputs, discoveredInputs));
     } finally {
       Profiler.instance().completeTask(ProfilerTask.ACTION_UPDATE);
-      setInputs(inputs);
     }
   }
 
   @Override
   protected SpawnInfo getExtraActionSpawnInfo() {
     SpawnInfo.Builder info = SpawnInfo.newBuilder(super.getExtraActionSpawnInfo());
-    if (!inputsKnown()) {
+    if (!inputsDiscovered()) {
       for (Artifact headerArtifact : filterHeaderFiles()) {
         // As in SpawnAction#getExtraActionSpawnInfo explicitly ignore middleman artifacts here.
         if (!headerArtifact.isMiddlemanArtifact()) {
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java
index 538ef90008..905c3a93cf 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java
@@ -227,7 +227,7 @@ public class ActionExecutionFunction implements SkyFunction, CompletionReceiver
       throws ActionExecutionFunctionException, InterruptedException {
     Iterable<Artifact> allKnownInputs = Iterables.concat(
         action.getInputs(), action.getRunfilesSupplier().getArtifacts());
-    if (action.inputsKnown()) {
+    if (action.inputsDiscovered()) {
       return new AllInputs(allKnownInputs);
     }
 
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java
index c122f3b1ee..1d23376469 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java
@@ -798,7 +798,7 @@ public final class SkyframeActionExecutor implements ActionExecutionContextFacto
   private void completeAction(Action action, MetadataHandler metadataHandler, FileOutErr fileOutErr,
       boolean outputAlreadyDumped) throws ActionExecutionException {
     try {
-      Preconditions.checkState(action.inputsKnown(),
+      Preconditions.checkState(action.inputsDiscovered(),
           "Action %s successfully executed, but inputs still not known", action);
 
       profiler.startTask(ProfilerTask.ACTION_COMPLETE, action);
diff --git a/src/test/java/com/google/devtools/build/lib/actions/ResourceManagerTest.java b/src/test/java/com/google/devtools/build/lib/actions/ResourceManagerTest.java
index d05123e70c..9605c31bc4 100644
--- a/src/test/java/com/google/devtools/build/lib/actions/ResourceManagerTest.java
+++ b/src/test/java/com/google/devtools/build/lib/actions/ResourceManagerTest.java
@@ -492,7 +492,7 @@ public class ResourceManagerTest {
     }
 
     @Override
-    public boolean inputsKnown() {
+    public boolean inputsDiscovered() {
       throw new IllegalStateException();
     }
 
diff --git a/src/test/java/com/google/devtools/build/lib/actions/util/TestAction.java b/src/test/java/com/google/devtools/build/lib/actions/util/TestAction.java
index c0a32308af..2d74dd8426 100644
--- a/src/test/java/com/google/devtools/build/lib/actions/util/TestAction.java
+++ b/src/test/java/com/google/devtools/build/lib/actions/util/TestAction.java
@@ -81,7 +81,7 @@ public class TestAction extends AbstractAction {
   @Override
   public boolean discoversInputs() {
     for (Artifact input : getInputs()) {
-      if (!input.getExecPath().getBaseName().endsWith(".optional")) {
+      if (input.getExecPath().getBaseName().endsWith(".optional")) {
         return true;
       }
     }
@@ -91,6 +91,7 @@ public class TestAction extends AbstractAction {
   @Override
   public Iterable<Artifact> discoverInputs(ActionExecutionContext actionExecutionContext) {
     Preconditions.checkState(discoversInputs(), this);
+    updateInputs(getInputs());
     return ImmutableList.of();
   }
 
diff --git a/src/test/java/com/google/devtools/build/lib/rules/cpp/LTOBackendActionTest.java b/src/test/java/com/google/devtools/build/lib/rules/cpp/LTOBackendActionTest.java
index bef984f453..eb196ec627 100644
--- a/src/test/java/com/google/devtools/build/lib/rules/cpp/LTOBackendActionTest.java
+++ b/src/test/java/com/google/devtools/build/lib/rules/cpp/LTOBackendActionTest.java
@@ -98,11 +98,11 @@ public class LTOBackendActionTest extends BuildViewTestCase {
     assertEquals(AbstractAction.DEFAULT_RESOURCE_SET, action.getSpawn().getLocalResources());
     assertThat(action.getArguments()).containsExactly("/bin/clang");
     assertEquals("Test", action.getProgressMessage());
-    assertThat(action.inputsKnown()).isFalse();
+    assertThat(action.inputsDiscovered()).isFalse();
 
     // Discover inputs, which should not add any inputs since bitcode1.imports is empty.
     action.discoverInputs(context);
-    assertThat(action.inputsKnown()).isTrue();
+    assertThat(action.inputsDiscovered()).isTrue();
     assertThat(action.getInputs()).containsExactly(bitcode1Artifact, index1Artifact);
   }
 
@@ -125,11 +125,11 @@ public class LTOBackendActionTest extends BuildViewTestCase {
     assertEquals(AbstractAction.DEFAULT_RESOURCE_SET, action.getSpawn().getLocalResources());
     assertThat(action.getArguments()).containsExactly("/bin/clang");
     assertEquals("Test", action.getProgressMessage());
-    assertThat(action.inputsKnown()).isFalse();
+    assertThat(action.inputsDiscovered()).isFalse();
 
     // Discover inputs, which should add bitcode1.o which is listed in bitcode2.imports.
     action.discoverInputs(context);
-    assertThat(action.inputsKnown()).isTrue();
+    assertThat(action.inputsDiscovered()).isTrue();
     assertThat(action.getInputs())
         .containsExactly(bitcode1Artifact, bitcode2Artifact, index2Artifact);
   }
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/ActionDataTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/ActionDataTest.java
index 342784cbd4..05442e6114 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/ActionDataTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/ActionDataTest.java
@@ -14,30 +14,24 @@
 package com.google.devtools.build.lib.skyframe;
 
 import static org.junit.Assert.assertSame;
-import static org.junit.Assert.assertTrue;
 
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.Sets;
 import com.google.devtools.build.lib.actions.AbstractAction;
-import com.google.devtools.build.lib.actions.Action;
 import com.google.devtools.build.lib.actions.ActionExecutionContext;
 import com.google.devtools.build.lib.actions.ActionExecutionException;
-import com.google.devtools.build.lib.actions.Actions;
 import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.actions.Executor;
 import com.google.devtools.build.lib.actions.ResourceSet;
-import com.google.devtools.build.lib.actions.Root;
 import com.google.devtools.build.lib.actions.util.ActionsTestUtil;
 import com.google.devtools.build.lib.actions.util.DummyExecutor;
 import com.google.devtools.build.lib.vfs.FileSystemUtils;
-
-import org.junit.Test;
-import org.junit.runner.RunWith;
-import org.junit.runners.JUnit4;
-
 import java.io.IOException;
 import java.util.Collection;
 import java.util.Set;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.JUnit4;
 
 /**
  * Tests that the data passed from the application to the Builder is passed
@@ -104,72 +98,4 @@ public class ActionDataTest extends TimestampBuilderTestCase {
             null);
     assertSame(executor, action.executor);
   }
-
-  private static class InputDiscoveringAction extends AbstractAction {
-    private final Collection<Artifact> discoveredInputs;
-
-    public InputDiscoveringAction(Artifact output, Collection<Artifact> discoveredInputs) {
-      super(
-          ActionsTestUtil.NULL_ACTION_OWNER,
-          ImmutableList.<Artifact>of(),
-          ImmutableList.of(output));
-      this.discoveredInputs = discoveredInputs;
-    }
-
-    @Override
-    public boolean discoversInputs() {
-      return true;
-    }
-
-    @Override
-    public boolean inputsKnown() {
-      return true;
-    }
-
-    @Override
-    public Iterable<Artifact> getMandatoryInputs() {
-      return ImmutableList.of();
-    }
-
-    @Override
-    public Iterable<Artifact> getInputs() {
-      return discoveredInputs;
-    }
-
-    @Override
-    public void execute(ActionExecutionContext actionExecutionContext) {
-      throw new IllegalStateException();
-    }
-
-    @Override
-    public String getMnemonic() {
-      return "InputDiscovering";
-    }
-
-    @Override
-    protected String computeKey() {
-      return "";
-    }
-
-    @Override
-    public ResourceSet estimateResourceConsumption(Executor executor) {
-      return ResourceSet.ZERO;
-    }
-  }
-
-  @Test
-  public void testActionSharabilityAndDiscoveredInputs() throws Exception {
-    Artifact output =
-        new Artifact(
-            scratch.file("/out/output"), Root.asDerivedRoot(scratch.dir("/"), scratch.dir("/out")));
-    Artifact discovered =
-        new Artifact(
-            scratch.file("/bin/discovered"),
-            Root.asDerivedRoot(scratch.dir("/"), scratch.dir("/bin")));
-
-    Action a = new InputDiscoveringAction(output, ImmutableList.of(discovered));
-    Action b = new InputDiscoveringAction(output, ImmutableList.<Artifact>of());
-
-    assertTrue(Actions.canBeShared(a, b));
-  }
 }