Automated rollback of commit f309ad3be36363070e87eef0ee04b12f4956d601.

*** Reason for rollback ***

Fixed duplicate derived inputs bug. Test is in diffbase.

RELNOTES[INC]: If the same artifact is generated by two distinct but identical actions, and a downstream action has both those actions' outputs in its inputs, the artifact will now appear twice in the downstream action's inputs. If this causes problems in Skylark actions, you can use the uniquify=True argument in Args.add_args.

PiperOrigin-RevId: 205863806

17 files changed, 275 insertions, 218 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/actions/Actions.java b/src/main/java/com/google/devtools/build/lib/actions/Actions.java
index 5364316f0c..a3a6ba6b58 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/Actions.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/Actions.java
@@ -17,7 +17,6 @@ package com.google.devtools.build.lib.actions;
 import com.google.common.base.Preconditions;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
-import com.google.common.collect.Iterables;
 import com.google.common.escape.Escaper;
 import com.google.common.escape.Escapers;
 import com.google.devtools.build.lib.actions.MutableActionGraph.ActionConflictException;
@@ -25,6 +24,7 @@ import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe;
 import com.google.devtools.build.lib.vfs.OsPathPolicy;
 import com.google.devtools.build.lib.vfs.PathFragment;
+import java.util.Collection;
 import java.util.Comparator;
 import java.util.HashMap;
 import java.util.Iterator;
@@ -74,15 +74,39 @@ public final class Actions {
     }
     // Don't bother to check input and output counts first; the expected result for these tests is
     // to always be true (i.e., that this method returns true).
-    if (!Iterables.elementsEqual(actionA.getMandatoryInputs(), actionB.getMandatoryInputs())) {
+    if (!artifactsEqualWithoutOwner(actionA.getMandatoryInputs(), actionB.getMandatoryInputs())) {
       return false;
     }
-    if (!Iterables.elementsEqual(actionA.getOutputs(), actionB.getOutputs())) {
+    if (!artifactsEqualWithoutOwner(actionA.getOutputs(), actionB.getOutputs())) {
       return false;
     }
     return true;
   }
 
+  private static boolean artifactsEqualWithoutOwner(
+      Iterable<Artifact> iterable1, Iterable<Artifact> iterable2) {
+    if (iterable1 instanceof Collection && iterable2 instanceof Collection) {
+      Collection<?> collection1 = (Collection<?>) iterable1;
+      Collection<?> collection2 = (Collection<?>) iterable2;
+      if (collection1.size() != collection2.size()) {
+        return false;
+      }
+    }
+    Iterator<Artifact> iterator1 = iterable1.iterator();
+    Iterator<Artifact> iterator2 = iterable2.iterator();
+    while (iterator1.hasNext()) {
+      if (!iterator2.hasNext()) {
+        return false;
+      }
+      Artifact artifact1 = iterator1.next();
+      Artifact artifact2 = iterator2.next();
+      if (!artifact1.equalsWithoutOwner(artifact2)) {
+        return false;
+      }
+    }
+    return !iterator2.hasNext();
+  }
+
   /**
    * Finds action conflicts. An action conflict happens if two actions generate the same output
    * artifact. Shared actions are tolerated. See {@link #canBeShared} for details.
diff --git a/src/main/java/com/google/devtools/build/lib/actions/Artifact.java b/src/main/java/com/google/devtools/build/lib/actions/Artifact.java
index f21d579aed..49dad3ff38 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/Artifact.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/Artifact.java
@@ -22,14 +22,14 @@ import com.google.common.base.Functions;
 import com.google.common.base.Joiner;
 import com.google.common.base.Preconditions;
 import com.google.common.base.Predicate;
-import com.google.common.cache.Cache;
-import com.google.common.cache.CacheBuilder;
 import com.google.common.collect.ImmutableList;
+import com.google.common.collect.Interner;
 import com.google.common.collect.Iterables;
 import com.google.common.collect.Streams;
 import com.google.devtools.build.lib.actions.ActionAnalysisMetadata.MiddlemanType;
 import com.google.devtools.build.lib.actions.ArtifactResolver.ArtifactResolverSupplier;
 import com.google.devtools.build.lib.cmdline.Label;
+import com.google.devtools.build.lib.concurrent.BlazeInterners;
 import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
 import com.google.devtools.build.lib.shell.ShellUtils;
 import com.google.devtools.build.lib.skyframe.serialization.DeserializationContext;
@@ -47,6 +47,7 @@ import com.google.devtools.build.lib.util.FileTypeSet;
 import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.lib.vfs.PathFragment;
 import com.google.devtools.build.skyframe.SkyFunctionName;
+import com.google.devtools.build.skyframe.SkyKey;
 import com.google.protobuf.CodedInputStream;
 import com.google.protobuf.CodedOutputStream;
 import java.io.IOException;
@@ -55,7 +56,6 @@ import java.util.Collection;
 import java.util.Comparator;
 import java.util.List;
 import java.util.Objects;
-import java.util.concurrent.ExecutionException;
 import javax.annotation.Nullable;
 
 /**
@@ -110,7 +110,8 @@ public class Artifact
         ActionInput,
         FileApi,
         Comparable<Object>,
-        CommandLineItem {
+        CommandLineItem,
+        SkyKey {
 
   /** Compares artifact according to their exec paths. Sorts null values first. */
   @SuppressWarnings("ReferenceEquality")  // "a == b" is an optimization
@@ -167,8 +168,7 @@ public class Artifact
   /** A Predicate that evaluates to true if the Artifact is not a middleman artifact. */
   public static final Predicate<Artifact> MIDDLEMAN_FILTER = input -> !input.isMiddlemanArtifact();
 
-  private static final Cache<InternedArtifact, Artifact> ARTIFACT_INTERNER =
-      CacheBuilder.newBuilder().weakValues().build();
+  private static final Interner<Artifact> ARTIFACT_INTERNER = BlazeInterners.newWeakInterner();
 
   private final int hashCode;
   private final ArtifactRoot root;
@@ -203,15 +203,7 @@ public class Artifact
     if (artifact.isSourceArtifact()) {
       return artifact;
     } else {
-      return intern(artifact);
-    }
-  }
-
-  private static Artifact intern(Artifact artifact) {
-    try {
-      return ARTIFACT_INTERNER.get(new InternedArtifact(artifact), () -> artifact);
-    } catch (ExecutionException e) {
-      throw new IllegalStateException(e);
+      return ARTIFACT_INTERNER.intern(artifact);
     }
   }
 
@@ -257,6 +249,9 @@ public class Artifact
       throw new IllegalArgumentException(
           "it is illegal to create an artifact with an empty execPath");
     }
+    // The ArtifactOwner is not part of this computation because it is very rare that two Artifacts
+    // have the same execPath and different owners, so a collision is fine there. If this is
+    // changed, OwnerlessArtifactWrapper must also be changed.
     this.hashCode = execPath.hashCode() + this.getClass().hashCode() * 13;
     this.root = root;
     this.execPath = execPath;
@@ -484,6 +479,15 @@ public class Artifact
     public SourceArtifact asSourceArtifact() {
       return this;
     }
+
+    /**
+     * SourceArtifacts are compared without their owners, since owners do not affect behavior,
+     * unlike with derived artifacts, whose owners determine their generating actions.
+     */
+    @Override
+    public boolean equals(Object other) {
+      return other instanceof SourceArtifact && equalsWithoutOwner((SourceArtifact) other);
+    }
   }
 
   /**
@@ -614,34 +618,6 @@ public class Artifact
   }
 
   /**
-   * Wraps an artifact for interning because we need to check the artifact owner when doing equals.
-   */
-  private static final class InternedArtifact {
-    private final Artifact wrappedArtifact;
-
-    InternedArtifact(Artifact artifact) {
-      this.wrappedArtifact = artifact;
-    }
-
-    @Override
-    public boolean equals(Object other) {
-      if (!(other instanceof InternedArtifact)) {
-        return false;
-      }
-      if (!getClass().equals(other.getClass())) {
-        return false;
-      }
-      InternedArtifact that = (InternedArtifact) other;
-      return Artifact.equalWithOwner(wrappedArtifact, that.wrappedArtifact);
-    }
-
-    @Override
-    public final int hashCode() {
-      return wrappedArtifact.hashCode();
-    }
-  }
-
-  /**
    * Returns the relative path to this artifact relative to its root.  (Useful
    * when deriving output filenames from input files, etc.)
    */
@@ -693,37 +669,28 @@ public class Artifact
     return rootRelativePath.toString();
   }
 
+  @SuppressWarnings("EqualsGetClass") // Distinct classes of Artifact are never equal.
   @Override
-  public final boolean equals(Object other) {
+  public boolean equals(Object other) {
     if (!(other instanceof Artifact)) {
       return false;
     }
     if (!getClass().equals(other.getClass())) {
       return false;
     }
-    // We don't bother to check root in the equivalence relation, because we
-    // assume that no root is an ancestor of another one.
     Artifact that = (Artifact) other;
-    return Objects.equals(this.execPath, that.execPath) && Objects.equals(this.root, that.root);
+    return equalsWithoutOwner(that) && owner.equals(that.getArtifactOwner());
   }
 
-  /**
-   * Compare equality including Artifact owner equality, a notable difference compared to the
-   * {@link #equals(Object)} method of {@link Artifact}.
-   */
-  public static boolean equalWithOwner(@Nullable Artifact first, @Nullable Artifact second) {
-    if (first != null) {
-      return first.equals(second) && first.getArtifactOwner().equals(second.getArtifactOwner());
-    } else {
-      return second == null;
-    }
+  public boolean equalsWithoutOwner(Artifact other) {
+    return Objects.equals(this.execPath, other.execPath) && Objects.equals(this.root, other.root);
   }
 
   @Override
   public final int hashCode() {
-    // This is just path.hashCode().  We cache a copy in the Artifact object to reduce LLC misses
-    // during operations which build a HashSet out of many Artifacts.  This is a slight loss for
-    // memory but saves ~1% overall CPU in some real builds.
+    // This is just execPath.hashCode() (along with the class). We cache a copy in the Artifact
+    // object to reduce LLC misses during operations which build a HashSet out of many Artifacts.
+    // This is a slight loss for memory but saves ~1% overall CPU in some real builds.
     return hashCode;
   }
 
@@ -1031,4 +998,33 @@ public class Artifact
       printer.append("<generated file " + rootRelativePath + ">");
     }
   }
+
+  /**
+   * A utility class that compares {@link Artifact}s without taking their owners into account.
+   * Should only be used for detecting action conflicts and merging shared action data.
+   */
+  public static class OwnerlessArtifactWrapper {
+    private final Artifact artifact;
+
+    public OwnerlessArtifactWrapper(Artifact artifact) {
+      this.artifact = artifact;
+    }
+
+    @Override
+    public int hashCode() {
+      // Depends on the fact that Artifact#hashCode does not use ArtifactOwner.
+      return artifact.hashCode();
+    }
+
+    @Override
+    public boolean equals(Object obj) {
+      return obj instanceof OwnerlessArtifactWrapper
+          && this.artifact.equalsWithoutOwner(((OwnerlessArtifactWrapper) obj).artifact);
+    }
+  }
+
+  @Override
+  public SkyFunctionName functionName() {
+    return ARTIFACT;
+  }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/actions/ArtifactSkyKey.java b/src/main/java/com/google/devtools/build/lib/actions/ArtifactSkyKey.java
index 277b26933b..701aacaec4 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/ArtifactSkyKey.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/ArtifactSkyKey.java
@@ -13,99 +13,52 @@
 // limitations under the License.
 package com.google.devtools.build.lib.actions;
 
-import com.google.common.base.Function;
 import com.google.common.base.Preconditions;
-import com.google.common.collect.Collections2;
 import com.google.common.collect.Interner;
-import com.google.common.collect.Iterables;
 import com.google.devtools.build.lib.concurrent.BlazeInterners;
 import com.google.devtools.build.lib.concurrent.ThreadSafety;
 import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec;
 import com.google.devtools.build.skyframe.SkyFunctionName;
 import com.google.devtools.build.skyframe.SkyKey;
-import java.util.Collection;
 
 /**
- * A {@link SkyKey} coming from an {@link Artifact}. Source artifacts are checked for existence,
- * while output artifacts imply creation of the output file.
+ * A {@link SkyKey} coming from an {@link Artifact} that is not mandatory: absence of the Artifact
+ * does not imply any error.
  *
- * <p>There are effectively three kinds of output artifact values corresponding to these keys. The
- * first corresponds to an ordinary artifact {@link FileArtifactValue}. It stores the relevant data
- * for the artifact -- digest/mtime and size. The second corresponds to either an "aggregating"
- * artifact -- the output of an aggregating middleman action -- or a TreeArtifact. It stores the
- * relevant data of all its inputs, as well as a combined digest for itself.
- *
- * <p>Artifacts are compared using just their paths, but in Skyframe, the configured target that
- * owns an artifact must also be part of the comparison. For example, suppose we build //foo:foo in
- * configurationA, yielding artifact foo.out. If we change the configuration to configurationB in
- * such a way that the path to the artifact does not change, requesting foo.out from the graph will
- * result in the value entry for foo.out under configurationA being returned. This would prevent
- * caching the graph in different configurations, and also causes big problems with change pruning,
- * which assumes the invariant that a value's first dependency will always be the same. In this
- * case, the value entry's old dependency on //foo:foo in configurationA would cause it to request
- * (//foo:foo, configurationA) from the graph, causing an undesired re-analysis of (//foo:foo,
- * configurationA).
- *
- * <p>In order to prevent that, instead of just comparing Artifacts, we compare for equality using
- * both the Artifact, and the owner. The effect is functionally that of making Artifact.equals()
- * check the owner, but only within these keys, since outside of Skyframe it is quite crucial that
- * Artifacts with different owners be able to compare equal.
+ * <p>Since {@link Artifact} is already a {@link SkyKey}, this wrapper is only needed when the
+ * {@link Artifact} is not mandatory (discovered during include scanning).
  */
+// TODO(janakr): pull mandatory/non-mandatory handling up to consumers and get rid of this wrapper.
 @AutoCodec
 public class ArtifactSkyKey implements SkyKey {
   private static final Interner<ArtifactSkyKey> INTERNER = BlazeInterners.newWeakInterner();
-  private static final Function<Artifact, ArtifactSkyKey> TO_MANDATORY_KEY =
-      artifact -> key(artifact, true);
-  private static final Function<ArtifactSkyKey, Artifact> TO_ARTIFACT = ArtifactSkyKey::getArtifact;
 
-  private final Artifact artifact;
-  // Always true for derived artifacts.
-  private final boolean isMandatory;
-  // TODO(janakr): we may want to remove this field in the future. The expensive hash computation
-  // is already cached one level down (in the Artifact), so the CPU overhead here may not be
-  // worth the memory. However, when running with +CompressedOops, this field is free, so we leave
-  // it. When running with -CompressedOops, we might be able to save memory by using polymorphism
-  // for isMandatory and dropping this field.
-  private int hashCode = 0;
+  private final Artifact.SourceArtifact artifact;
 
-  private ArtifactSkyKey(Artifact sourceArtifact, boolean mandatory) {
-    Preconditions.checkArgument(sourceArtifact.isSourceArtifact());
+  private ArtifactSkyKey(Artifact.SourceArtifact sourceArtifact) {
     this.artifact = Preconditions.checkNotNull(sourceArtifact);
-    this.isMandatory = mandatory;
-  }
-
-  /**
-   * Constructs an ArtifactSkyKey for a derived artifact. The mandatory attribute is not needed
-   * because a derived artifact must be a mandatory input for some action in order to ensure that it
-   * is built in the first place. If it fails to build, then that fact is cached in the node, so any
-   * action that has it as a non-mandatory input can retrieve that information from the node.
-   */
-  private ArtifactSkyKey(Artifact derivedArtifact) {
-    this.artifact = Preconditions.checkNotNull(derivedArtifact);
-    Preconditions.checkArgument(!derivedArtifact.isSourceArtifact(), derivedArtifact);
-    this.isMandatory = true; // Unused.
   }
 
   @ThreadSafety.ThreadSafe
-  @AutoCodec.Instantiator
-  public static ArtifactSkyKey key(Artifact artifact, boolean isMandatory) {
-    return INTERNER.intern(
-        artifact.isSourceArtifact()
-            ? new ArtifactSkyKey(artifact, isMandatory)
-            : new ArtifactSkyKey(artifact));
+  public static SkyKey key(Artifact artifact, boolean isMandatory) {
+    if (isMandatory || !artifact.isSourceArtifact()) {
+      return artifact;
+    }
+    return create(((Artifact.SourceArtifact) artifact));
   }
 
-  @ThreadSafety.ThreadSafe
-  public static Iterable<ArtifactSkyKey> mandatoryKeys(Iterable<Artifact> artifacts) {
-    return Iterables.transform(artifacts, TO_MANDATORY_KEY);
+  @AutoCodec.VisibleForSerialization
+  @AutoCodec.Instantiator
+  static ArtifactSkyKey create(Artifact.SourceArtifact artifact) {
+    return INTERNER.intern(new ArtifactSkyKey(artifact));
   }
 
-  public static Collection<Artifact> artifacts(Collection<? extends ArtifactSkyKey> keys) {
-    return Collections2.transform(keys, TO_ARTIFACT);
+  public static Artifact artifact(SkyKey key) {
+    return key instanceof Artifact ? (Artifact) key : ((ArtifactSkyKey) key).artifact;
   }
 
-  public static Artifact artifact(SkyKey key) {
-    return TO_ARTIFACT.apply((ArtifactSkyKey) key.argument());
+  public static boolean isMandatory(SkyKey key) {
+    return key instanceof Artifact;
   }
 
   @Override
@@ -115,32 +68,7 @@ public class ArtifactSkyKey implements SkyKey {
 
   @Override
   public int hashCode() {
-    // We use the hash code caching strategy employed by java.lang.String. There are three subtle
-    // things going on here:
-    //
-    // (1) We use a value of 0 to indicate that the hash code hasn't been computed and cached yet.
-    // Yes, this means that if the hash code is really 0 then we will "recompute" it each time.
-    // But this isn't a problem in practice since a hash code of 0 should be rare.
-    //
-    // (2) Since we have no synchronization, multiple threads can race here thinking there are the
-    // first one to compute and cache the hash code.
-    //
-    // (3) Moreover, since 'hashCode' is non-volatile, the cached hash code value written from one
-    // thread may not be visible by another. Note that we probably don't need to worry about
-    // multiple inefficient reads of 'hashCode' on the same thread since it's non-volatile.
-    //
-    // All three of these issues are benign from a correctness perspective; in the end we have no
-    // overhead from synchronization, at the cost of potentially computing the hash code more than
-    // once.
-    if (hashCode == 0) {
-      hashCode = computeHashCode();
-    }
-    return hashCode;
-  }
-
-  private int computeHashCode() {
-    int initialHash = artifact.hashCode() + artifact.getArtifactOwner().hashCode();
-    return isMandatory ? initialHash : 47 * initialHash + 1;
+    return artifact.hashCode();
   }
 
   @Override
@@ -152,24 +80,13 @@ public class ArtifactSkyKey implements SkyKey {
       return false;
     }
     ArtifactSkyKey thatArtifactSkyKey = ((ArtifactSkyKey) that);
-    return Artifact.equalWithOwner(artifact, thatArtifactSkyKey.artifact)
-        && isMandatory == thatArtifactSkyKey.isMandatory;
+    return artifact.equals(thatArtifactSkyKey.artifact);
   }
 
   public Artifact getArtifact() {
     return artifact;
   }
 
-  /**
-   * Returns whether the artifact is a mandatory input of its requesting action. May only be called
-   * for source artifacts, since a derived artifact must be a mandatory input of some action in
-   * order to have been built in the first place.
-   */
-  public boolean isMandatory() {
-    Preconditions.checkState(artifact.isSourceArtifact(), artifact);
-    return isMandatory;
-  }
-
   @Override
   public String toString() {
     return artifact.prettyPrint() + " " + artifact.getArtifactOwner();
diff --git a/src/main/java/com/google/devtools/build/lib/actions/FilesetTraversalParams.java b/src/main/java/com/google/devtools/build/lib/actions/FilesetTraversalParams.java
index 7740a05b51..75907d4f24 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/FilesetTraversalParams.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/FilesetTraversalParams.java
@@ -26,6 +26,7 @@ import com.google.devtools.build.lib.util.Fingerprint;
 import com.google.devtools.build.lib.vfs.PathFragment;
 import com.google.devtools.build.lib.vfs.Root;
 import com.google.devtools.build.lib.vfs.RootedPath;
+import java.util.Objects;
 import javax.annotation.Nullable;
 
 /**
@@ -111,9 +112,7 @@ public interface FilesetTraversalParams {
       if (o instanceof FilesetTraversalParams.DirectTraversalRoot) {
         FilesetTraversalParams.DirectTraversalRoot that =
             (FilesetTraversalParams.DirectTraversalRoot) o;
-        // Careful! We must compare the artifact owners, which the default {@link Artifact#equals()}
-        // method does not do. See the comments on {@link ArtifactSkyKey} and http://b/73738481.
-        return Artifact.equalWithOwner(this.getOutputArtifact(), that.getOutputArtifact())
+        return Objects.equals(this.getOutputArtifact(), that.getOutputArtifact())
             && (this.getRootPart().equals(that.getRootPart()))
             && (this.getRelativePart().equals(that.getRelativePart()));
       }
diff --git a/src/main/java/com/google/devtools/build/lib/actions/MapBasedActionGraph.java b/src/main/java/com/google/devtools/build/lib/actions/MapBasedActionGraph.java
index f35f963bf7..dadad155e6 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/MapBasedActionGraph.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/MapBasedActionGraph.java
@@ -15,6 +15,7 @@
 package com.google.devtools.build.lib.actions;
 
 import com.google.common.base.Preconditions;
+import com.google.devtools.build.lib.actions.Artifact.OwnerlessArtifactWrapper;
 import javax.annotation.Nullable;
 import javax.annotation.concurrent.ThreadSafe;
 
@@ -24,7 +25,7 @@ import javax.annotation.concurrent.ThreadSafe;
 @ThreadSafe
 public final class MapBasedActionGraph implements MutableActionGraph {
   private final ActionKeyContext actionKeyContext;
-  private final ConcurrentMultimapWithHeadElement<Artifact, ActionAnalysisMetadata>
+  private final ConcurrentMultimapWithHeadElement<OwnerlessArtifactWrapper, ActionAnalysisMetadata>
       generatingActionMap = new ConcurrentMultimapWithHeadElement<>();
 
   public MapBasedActionGraph(ActionKeyContext actionKeyContext) {
@@ -34,17 +35,18 @@ public final class MapBasedActionGraph implements MutableActionGraph {
   @Override
   @Nullable
   public ActionAnalysisMetadata getGeneratingAction(Artifact artifact) {
-    return generatingActionMap.get(artifact);
+    return generatingActionMap.get(new OwnerlessArtifactWrapper(artifact));
   }
 
   @Override
   public void registerAction(ActionAnalysisMetadata action) throws ActionConflictException {
     for (Artifact artifact : action.getOutputs()) {
-      ActionAnalysisMetadata previousAction = generatingActionMap.putAndGet(artifact, action);
+      OwnerlessArtifactWrapper wrapper = new OwnerlessArtifactWrapper(artifact);
+      ActionAnalysisMetadata previousAction = generatingActionMap.putAndGet(wrapper, action);
       if (previousAction != null
           && previousAction != action
           && !Actions.canBeShared(actionKeyContext, action, previousAction)) {
-        generatingActionMap.remove(artifact, action);
+        generatingActionMap.remove(wrapper, action);
         throw new ActionConflictException(actionKeyContext, artifact, previousAction, action);
       }
     }
@@ -53,8 +55,9 @@ public final class MapBasedActionGraph implements MutableActionGraph {
   @Override
   public void unregisterAction(ActionAnalysisMetadata action) {
     for (Artifact artifact : action.getOutputs()) {
-      generatingActionMap.remove(artifact, action);
-      ActionAnalysisMetadata otherAction = generatingActionMap.get(artifact);
+      OwnerlessArtifactWrapper wrapper = new OwnerlessArtifactWrapper(artifact);
+      generatingActionMap.remove(wrapper, action);
+      ActionAnalysisMetadata otherAction = generatingActionMap.get(wrapper);
       Preconditions.checkState(
           otherAction == null
               || (otherAction != action
diff --git a/src/main/java/com/google/devtools/build/lib/collect/nestedset/NestedSetCodecWithStore.java b/src/main/java/com/google/devtools/build/lib/collect/nestedset/NestedSetCodecWithStore.java
index fa835618ec..1496bc288e 100644
--- a/src/main/java/com/google/devtools/build/lib/collect/nestedset/NestedSetCodecWithStore.java
+++ b/src/main/java/com/google/devtools/build/lib/collect/nestedset/NestedSetCodecWithStore.java
@@ -126,8 +126,7 @@ public class NestedSetCodecWithStore implements ObjectCodec<NestedSet<?>> {
    * NestedSet#getChildren()}. When that field is an Object[], this is just identity hash code and
    * reference equality, but when it is something else (like an Artifact), we will do an actual
    * equality comparison. This may make some singleton NestedSets reference-equal where they were
-   * not before. This should be ok, but isn't because Artifact does not properly implement equality
-   * (it ignores the ArtifactOwner).
+   * not before. This should be ok as long as the contained object properly implements equality.
    */
   @SuppressWarnings("unchecked")
   private NestedSet<?> intern(Order order, Object contents) {
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/HeaderDiscovery.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/HeaderDiscovery.java
index 503e8ce2fe..505d75a3c4 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/HeaderDiscovery.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/HeaderDiscovery.java
@@ -33,6 +33,7 @@ import com.google.devtools.build.lib.vfs.PathFragment;
 import java.util.Collection;
 import java.util.HashMap;
 import java.util.List;
+import java.util.Map;
 
 /**
  * HeaderDiscovery checks whether all header files that a compile action uses are actually declared
@@ -242,13 +243,23 @@ public class HeaderDiscovery {
 
     /** Creates a CppHeaderDiscovery instance. */
     public HeaderDiscovery build() {
-      ImmutableMap.Builder<PathFragment, Artifact> allowedDerivedInputsMap = ImmutableMap.builder();
+      Map<PathFragment, Artifact> allowedDerivedInputsMap = new HashMap<>();
       ImmutableSet.Builder<PathFragment> treeArtifactPrefixes = ImmutableSet.builder();
       for (Artifact a : allowedDerivedInputs) {
+        PathFragment execPath = a.getExecPath();
         if (a.isTreeArtifact()) {
-          treeArtifactPrefixes.add(a.getExecPath());
+          treeArtifactPrefixes.add(execPath);
         }
-        allowedDerivedInputsMap.put(a.getExecPath(), a);
+        // We may encounter duplicate keys in the derived inputs if two artifacts have different
+        // owners. Just use the first one. The two artifacts must be generated by equivalent
+        // (shareable) actions in order to have not generated a conflict in Bazel. If on an
+        // incremental build one changes without the other one changing, then if their paths remain
+        // the same, that will trigger an action conflict and fail the build. If one path changes,
+        // then this action will be re-analyzed, and will execute in Skyframe. It can legitimately
+        // get an action cache hit in that case, since even if it previously depended on the
+        // artifact whose path changed, that is not taken into account by the action cache, and it
+        // will get an action cache hit using the remaining un-renamed artifact.
+        allowedDerivedInputsMap.putIfAbsent(execPath, a);
       }
 
       return new HeaderDiscovery(
@@ -257,7 +268,7 @@ public class HeaderDiscovery {
           shouldValidateInclusions,
           dependencies,
           permittedSystemIncludePrefixes,
-          allowedDerivedInputsMap.build(),
+          ImmutableMap.copyOf(allowedDerivedInputsMap),
           treeArtifactPrefixes.build());
     }
   }
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionValue.java
index 956e6500e4..d6c258f8de 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionValue.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionValue.java
@@ -19,10 +19,14 @@ import com.google.common.base.Objects;
 import com.google.common.base.Preconditions;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Maps;
+import com.google.devtools.build.lib.actions.ActionInputHelper;
 import com.google.devtools.build.lib.actions.ActionLookupData;
 import com.google.devtools.build.lib.actions.ActionLookupValue;
 import com.google.devtools.build.lib.actions.Artifact;
+import com.google.devtools.build.lib.actions.Artifact.OwnerlessArtifactWrapper;
+import com.google.devtools.build.lib.actions.ArtifactOwner;
 import com.google.devtools.build.lib.actions.FileArtifactValue;
 import com.google.devtools.build.lib.actions.FileStateValue;
 import com.google.devtools.build.lib.actions.FileValue;
@@ -34,6 +38,8 @@ import com.google.devtools.build.lib.vfs.RootedPath;
 import com.google.devtools.build.skyframe.SkyKey;
 import com.google.devtools.build.skyframe.SkyValue;
 import java.util.Map;
+import java.util.function.Function;
+import java.util.stream.Collectors;
 import javax.annotation.Nullable;
 
 /**
@@ -245,4 +251,75 @@ public class ActionExecutionValue implements SkyValue {
           artifactData, treeArtifactData, additionalOutputData, outputSymlinks, discoveredModules);
     }
   }
+
+  private static <V> ImmutableMap<Artifact, V> transformKeys(
+      ImmutableMap<Artifact, V> data, Map<OwnerlessArtifactWrapper, Artifact> newArtifactMap) {
+    if (data.isEmpty()) {
+      return data;
+    }
+    ImmutableMap.Builder<Artifact, V> result = ImmutableMap.builderWithExpectedSize(data.size());
+    for (Map.Entry<Artifact, V> entry : data.entrySet()) {
+      Artifact artifact = entry.getKey();
+      Artifact transformedArtifact =
+          newArtifactMap.get(new OwnerlessArtifactWrapper(entry.getKey()));
+      if (transformedArtifact == null) {
+        // If this action generated a tree artifact, then the declared outputs of the action will
+        // not include the contents of the directory corresponding to that artifact, but the
+        // contents are present in this ActionExecutionValue as TreeFileArtifacts. We must create
+        // corresponding artifacts in the shared action's ActionExecutionValue. We can do that since
+        // a TreeFileArtifact is uniquely described by its parent, its owner, and its parent-
+        // relative path. Since the child was not a declared output, the child and parent must be
+        // generated by the same action, hence they have the same owner, and the parent was a
+        // declared output, so it is present in the shared action. Then we can create the new
+        // TreeFileArtifact to have the shared action's version of the parent artifact (instead of
+        // the original parent artifact); the same parent-relative path; and the new parent's
+        // ArtifactOwner.
+        Preconditions.checkState(
+            artifact.hasParent(),
+            "Output artifact %s from one shared action not present in another's outputs (%s)",
+            artifact,
+            newArtifactMap);
+        ArtifactOwner childOwner = artifact.getArtifactOwner();
+        Artifact parent = Preconditions.checkNotNull(artifact.getParent(), artifact);
+        ArtifactOwner parentOwner = parent.getArtifactOwner();
+        Preconditions.checkState(
+            parentOwner.equals(childOwner),
+            "A parent tree artifact %s has a different ArtifactOwner (%s) than its child %s (owned "
+                + "by %s), but both artifacts were generated by the same action",
+            parent,
+            parentOwner,
+            artifact,
+            childOwner);
+        Artifact newParent =
+            Preconditions.checkNotNull(
+                newArtifactMap.get(new OwnerlessArtifactWrapper(parent)),
+                "parent %s of %s was not present in shared action's data (%s)",
+                parent,
+                artifact,
+                newArtifactMap);
+        transformedArtifact =
+            ActionInputHelper.treeFileArtifact(
+                (Artifact.SpecialArtifact) newParent, artifact.getParentRelativePath());
+      }
+      result.put(transformedArtifact, entry.getValue());
+    }
+    return result.build();
+  }
+
+  ActionExecutionValue transformForSharedAction(ImmutableSet<Artifact> outputs) {
+    Map<OwnerlessArtifactWrapper, Artifact> newArtifactMap =
+        outputs
+            .stream()
+            .collect(Collectors.toMap(OwnerlessArtifactWrapper::new, Function.identity()));
+    // This is only called for shared actions, so we'll almost certainly have to transform all keys
+    // in all sets.
+    // Discovered modules come from the action's inputs, and so don't need to be transformed.
+    return create(
+        transformKeys(artifactData, newArtifactMap),
+        transformKeys(treeArtifactData, newArtifactMap),
+        transformKeys(additionalOutputData, newArtifactMap),
+        outputSymlinks,
+        discoveredModules,
+        this instanceof CrossServerUnshareableActionExecutionValue);
+  }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ArtifactFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/ArtifactFunction.java
index 3587f089b7..467391af18 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ArtifactFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ArtifactFunction.java
@@ -54,11 +54,11 @@ class ArtifactFunction implements SkyFunction {
   @Override
   public SkyValue compute(SkyKey skyKey, Environment env)
       throws ArtifactFunctionException, InterruptedException {
-    ArtifactSkyKey artifactSkyKey = (ArtifactSkyKey) skyKey.argument();
-    Artifact artifact = artifactSkyKey.getArtifact();
+    Artifact artifact = ArtifactSkyKey.artifact(skyKey);
+    boolean isMandatory = ArtifactSkyKey.isMandatory(skyKey);
     if (artifact.isSourceArtifact()) {
       try {
-        return createSourceValue(artifact, artifactSkyKey.isMandatory(), env);
+        return createSourceValue(artifact, isMandatory, env);
       } catch (MissingInputFileException e) {
         // The error is not necessarily truly transient, but we mark it as such because we have
         // the above side effect of posting an event to the EventBus. Importantly, that event
@@ -281,8 +281,7 @@ class ArtifactFunction implements SkyFunction {
       throws InterruptedException {
     // This artifact aggregates other artifacts. Keep track of them so callers can find them.
     ImmutableList.Builder<Pair<Artifact, FileArtifactValue>> inputs = ImmutableList.builder();
-    for (Map.Entry<SkyKey, SkyValue> entry :
-        env.getValues(ArtifactSkyKey.mandatoryKeys(action.getInputs())).entrySet()) {
+    for (Map.Entry<SkyKey, SkyValue> entry : env.getValues(action.getInputs()).entrySet()) {
       Artifact input = ArtifactSkyKey.artifact(entry.getKey());
       SkyValue inputValue = entry.getValue();
       if (inputValue == null) {
@@ -323,7 +322,7 @@ class ArtifactFunction implements SkyFunction {
 
   @Override
   public String extractTag(SkyKey skyKey) {
-    return Label.print(((ArtifactSkyKey) skyKey.argument()).getArtifact().getOwner());
+    return Label.print(ArtifactSkyKey.artifact(skyKey).getOwner());
   }
 
   static ActionLookupKey getActionLookupKey(Artifact artifact) {
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/CompletionFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/CompletionFunction.java
index 76179b3f14..0f07d7ee79 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/CompletionFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/CompletionFunction.java
@@ -303,8 +303,7 @@ public final class CompletionFunction<TValue extends SkyValue, TResult extends S
 
     Map<SkyKey, ValueOrException2<MissingInputFileException, ActionExecutionException>> inputDeps =
         env.getValuesOrThrow(
-            ArtifactSkyKey.mandatoryKeys(
-                completor.getAllArtifactsToBuild(value, topLevelContext).getAllArtifacts()),
+            completor.getAllArtifactsToBuild(value, topLevelContext).getAllArtifacts(),
             MissingInputFileException.class,
             ActionExecutionException.class);
 
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 8cca0c45d4..e2cc9df529 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
@@ -51,6 +51,7 @@ import com.google.devtools.build.lib.actions.Actions;
 import com.google.devtools.build.lib.actions.AlreadyReportedActionExecutionException;
 import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.actions.Artifact.ArtifactExpander;
+import com.google.devtools.build.lib.actions.Artifact.OwnerlessArtifactWrapper;
 import com.google.devtools.build.lib.actions.ArtifactPathResolver;
 import com.google.devtools.build.lib.actions.ArtifactPrefixConflictException;
 import com.google.devtools.build.lib.actions.CachedActionEvent;
@@ -146,7 +147,8 @@ public final class SkyframeActionExecutor {
   // We don't want to execute the action again on the second entry to the SkyFunction.
   // In both cases, we store the already-computed ActionExecutionValue to avoid having to compute it
   // again.
-  private ConcurrentMap<Artifact, Pair<ActionLookupData, FutureTask<ActionExecutionValue>>>
+  private ConcurrentMap<
+          OwnerlessArtifactWrapper, Pair<ActionLookupData, FutureTask<ActionExecutionValue>>>
       buildActionMap;
 
   // Errors found when examining all actions in the graph are stored here, so that they can be
@@ -403,13 +405,16 @@ public final class SkyframeActionExecutor {
   }
 
   boolean probeActionExecution(Action action) {
-    return buildActionMap.containsKey(action.getPrimaryOutput());
+    return buildActionMap.containsKey(new OwnerlessArtifactWrapper(action.getPrimaryOutput()));
   }
 
   private boolean actionReallyExecuted(Action action, ActionLookupData actionLookupData) {
     Pair<ActionLookupData, ?> cachedRun =
         Preconditions.checkNotNull(
-            buildActionMap.get(action.getPrimaryOutput()), "%s %s", action, actionLookupData);
+            buildActionMap.get(new OwnerlessArtifactWrapper(action.getPrimaryOutput())),
+            "%s %s",
+            action,
+            actionLookupData);
     return actionLookupData.equals(cachedRun.getFirst());
   }
 
@@ -449,7 +454,8 @@ public final class SkyframeActionExecutor {
                 actionLookupData));
     // Check to see if another action is already executing/has executed this value.
     Pair<ActionLookupData, FutureTask<ActionExecutionValue>> oldAction =
-        buildActionMap.putIfAbsent(primaryOutput, Pair.of(actionLookupData, actionTask));
+        buildActionMap.putIfAbsent(
+            new OwnerlessArtifactWrapper(primaryOutput), Pair.of(actionLookupData, actionTask));
     // true if this is a non-shared action or it's shared and to be executed.
     boolean isPrimaryActionForTheValue = oldAction == null;
 
@@ -460,7 +466,10 @@ public final class SkyframeActionExecutor {
       actionTask = oldAction.second;
     }
     try {
-      return actionTask.get();
+      ActionExecutionValue value = actionTask.get();
+      return isPrimaryActionForTheValue
+          ? value
+          : value.transformForSharedAction(action.getOutputs());
     } catch (ExecutionException e) {
       Throwables.propagateIfPossible(e.getCause(),
           ActionExecutionException.class, InterruptedException.class);
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
index 378d4a980b..95537d629e 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
@@ -48,7 +48,6 @@ import com.google.devtools.build.lib.actions.ArtifactFactory;
 import com.google.devtools.build.lib.actions.ArtifactOwner;
 import com.google.devtools.build.lib.actions.ArtifactResolver.ArtifactResolverSupplier;
 import com.google.devtools.build.lib.actions.ArtifactRoot;
-import com.google.devtools.build.lib.actions.ArtifactSkyKey;
 import com.google.devtools.build.lib.actions.CommandLineExpansionException;
 import com.google.devtools.build.lib.actions.EnvironmentalExecException;
 import com.google.devtools.build.lib.actions.Executor;
@@ -1309,14 +1308,13 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory {
     resourceManager.resetResourceUsage();
     try {
       progressReceiver.executionProgressReceiver = executionProgressReceiver;
-      Iterable<ArtifactSkyKey> artifactKeys = ArtifactSkyKey.mandatoryKeys(artifactsToBuild);
       Iterable<TargetCompletionValue.TargetCompletionKey> targetKeys =
           TargetCompletionValue.keys(targetsToBuild, topLevelArtifactContext, targetsToTest);
       Iterable<SkyKey> aspectKeys = AspectCompletionValue.keys(aspects, topLevelArtifactContext);
       Iterable<SkyKey> testKeys =
           TestCompletionValue.keys(targetsToTest, topLevelArtifactContext, exclusiveTesting);
       return buildDriver.evaluate(
-          Iterables.concat(artifactKeys, targetKeys, aspectKeys, testKeys),
+          Iterables.concat(artifactsToBuild, targetKeys, aspectKeys, testKeys),
           keepGoing,
           numJobs,
           reporter);
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/TestCompletionFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/TestCompletionFunction.java
index a1358c53fe..9a7d7a6aac 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/TestCompletionFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/TestCompletionFunction.java
@@ -19,7 +19,6 @@ import com.google.common.collect.Multimaps;
 import com.google.devtools.build.lib.actions.ActionLookupData;
 import com.google.devtools.build.lib.actions.ActionLookupValue;
 import com.google.devtools.build.lib.actions.Artifact;
-import com.google.devtools.build.lib.actions.ArtifactSkyKey;
 import com.google.devtools.build.lib.analysis.ConfiguredTarget;
 import com.google.devtools.build.lib.analysis.TopLevelArtifactContext;
 import com.google.devtools.build.lib.analysis.test.TestProvider;
@@ -67,10 +66,9 @@ public final class TestCompletionFunction implements SkyFunction {
         }
       }
     } else {
-      Multimap<ActionLookupValue.ActionLookupKey, ArtifactSkyKey> keyToArtifactMap =
+      Multimap<ActionLookupValue.ActionLookupKey, Artifact> keyToArtifactMap =
           Multimaps.index(
-              ArtifactSkyKey.mandatoryKeys(TestProvider.getTestStatusArtifacts(ct)),
-              (val) -> ArtifactFunction.getActionLookupKey(val.getArtifact()));
+              TestProvider.getTestStatusArtifacts(ct), ArtifactFunction::getActionLookupKey);
       Map<SkyKey, SkyValue> actionLookupValues = env.getValues(keyToArtifactMap.keySet());
       if (env.valuesMissing()) {
         return null;
@@ -82,7 +80,7 @@ public final class TestCompletionFunction implements SkyFunction {
               .map(
                   entry ->
                       getActionLookupData(
-                          entry.getValue().getArtifact(),
+                          entry.getValue(),
                           entry.getKey(),
                           (ActionLookupValue) actionLookupValues.get(entry.getKey())))
               .distinct()
diff --git a/src/test/java/com/google/devtools/build/lib/actions/ArtifactTest.java b/src/test/java/com/google/devtools/build/lib/actions/ArtifactTest.java
index abbd3472c8..e4bd5b9a46 100644
--- a/src/test/java/com/google/devtools/build/lib/actions/ArtifactTest.java
+++ b/src/test/java/com/google/devtools/build/lib/actions/ArtifactTest.java
@@ -20,6 +20,7 @@ import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.Iterables;
 import com.google.common.collect.Lists;
+import com.google.common.testing.EqualsTester;
 import com.google.devtools.build.lib.actions.ActionAnalysisMetadata.MiddlemanType;
 import com.google.devtools.build.lib.actions.Artifact.SourceArtifact;
 import com.google.devtools.build.lib.actions.ArtifactResolver.ArtifactResolverSupplier;
@@ -411,6 +412,40 @@ public class ArtifactTest {
     assertThat(new Artifact(scratch.file("/newRoot/foo"), root).getRoot()).isEqualTo(root);
   }
 
+  @Test
+  public void hashCodeAndEquals() throws IOException {
+    Path execRoot = scratch.getFileSystem().getPath("/");
+    ArtifactRoot root = ArtifactRoot.asDerivedRoot(execRoot, scratch.dir("/newRoot"));
+    ArtifactOwner firstOwner = () -> Label.parseAbsoluteUnchecked("//bar:bar");
+    ArtifactOwner secondOwner = () -> Label.parseAbsoluteUnchecked("//foo:foo");
+    Artifact derived1 = new Artifact(root, PathFragment.create("newRoot/shared"), firstOwner);
+    Artifact derived2 = new Artifact(root, PathFragment.create("newRoot/shared"), secondOwner);
+    ArtifactRoot sourceRoot = ArtifactRoot.asSourceRoot(Root.fromPath(root.getRoot().asPath()));
+    Artifact source1 = new SourceArtifact(sourceRoot, PathFragment.create("shared"), firstOwner);
+    Artifact source2 = new SourceArtifact(sourceRoot, PathFragment.create("shared"), secondOwner);
+    new EqualsTester()
+        .addEqualityGroup(derived1)
+        .addEqualityGroup(derived2)
+        .addEqualityGroup(source1, source2)
+        .testEquals();
+    assertThat(derived1.hashCode()).isEqualTo(derived2.hashCode());
+    assertThat(derived1.hashCode()).isNotEqualTo(source1.hashCode());
+    assertThat(source1.hashCode()).isEqualTo(source2.hashCode());
+    Artifact.OwnerlessArtifactWrapper wrapper1 = new Artifact.OwnerlessArtifactWrapper(derived1);
+    Artifact.OwnerlessArtifactWrapper wrapper2 = new Artifact.OwnerlessArtifactWrapper(derived2);
+    Artifact.OwnerlessArtifactWrapper wrapper3 = new Artifact.OwnerlessArtifactWrapper(source1);
+    Artifact.OwnerlessArtifactWrapper wrapper4 = new Artifact.OwnerlessArtifactWrapper(source2);
+    new EqualsTester()
+        .addEqualityGroup(wrapper1, wrapper2)
+        .addEqualityGroup(wrapper3, wrapper4)
+        .testEquals();
+    Path path1 = derived1.getPath();
+    Path path2 = derived2.getPath();
+    Path path3 = source1.getPath();
+    Path path4 = source2.getPath();
+    new EqualsTester().addEqualityGroup(path1, path2, path3, path4).testEquals();
+  }
+
   private Artifact createDirNameArtifact() throws Exception {
     return new Artifact(
         scratch.file("/aaa/bbb/ccc/ddd"),
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/ActionTemplateExpansionFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/ActionTemplateExpansionFunctionTest.java
index 786fe20a10..3a21b58b36 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/ActionTemplateExpansionFunctionTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/ActionTemplateExpansionFunctionTest.java
@@ -33,7 +33,6 @@ import com.google.devtools.build.lib.actions.Artifact.TreeFileArtifact;
 import com.google.devtools.build.lib.actions.ArtifactOwner;
 import com.google.devtools.build.lib.actions.ArtifactPrefixConflictException;
 import com.google.devtools.build.lib.actions.ArtifactRoot;
-import com.google.devtools.build.lib.actions.ArtifactSkyKey;
 import com.google.devtools.build.lib.actions.FileArtifactValue;
 import com.google.devtools.build.lib.actions.MutableActionGraph.ActionConflictException;
 import com.google.devtools.build.lib.actions.util.ActionsTestUtil;
@@ -262,9 +261,7 @@ public final class ActionTemplateExpansionFunctionTest extends FoundationTestCas
     }
     @Override
     public SkyValue compute(SkyKey skyKey, Environment env) {
-      ArtifactSkyKey artifactSkyKey = (ArtifactSkyKey) skyKey.argument();
-      Artifact artifact = artifactSkyKey.getArtifact();
-      return Preconditions.checkNotNull(artifactValueMap.get(artifact));
+      return Preconditions.checkNotNull(artifactValueMap.get(skyKey));
     }
 
     @Override
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/ArtifactFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/ArtifactFunctionTest.java
index 19443a536c..914ff46bff 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/ArtifactFunctionTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/ArtifactFunctionTest.java
@@ -150,10 +150,7 @@ public class ArtifactFunctionTest extends ArtifactFunctionTestCase {
     actions.add(action);
     file(input2.getPath(), "contents");
     file(input1.getPath(), "source contents");
-    evaluate(
-        Iterables.toArray(
-            ArtifactSkyKey.mandatoryKeys(ImmutableSet.of(input2, input1, input2, tree)),
-            SkyKey.class));
+    evaluate(Iterables.toArray(ImmutableSet.of(input2, input1, input2, tree), SkyKey.class));
     SkyValue value = evaluateArtifactValue(output);
     assertThat(((AggregatingArtifactValue) value).getInputs())
         .containsExactly(
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/RecursiveFilesystemTraversalFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/RecursiveFilesystemTraversalFunctionTest.java
index da9b01b98a..9268ccee79 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/RecursiveFilesystemTraversalFunctionTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/RecursiveFilesystemTraversalFunctionTest.java
@@ -950,10 +950,9 @@ public final class RecursiveFilesystemTraversalFunctionTest extends FoundationTe
     @Override
     public SkyValue compute(SkyKey skyKey, Environment env)
         throws SkyFunctionException, InterruptedException {
-      ArtifactSkyKey artifactKey = (ArtifactSkyKey) skyKey.argument();
-      Artifact artifact = artifactKey.getArtifact();
       try {
-        return FileArtifactValue.create(artifact.getPath());
+        return FileArtifactValue.create(
+            ArtifactSkyKey.artifact((SkyKey) skyKey.argument()).getPath());
       } catch (IOException e) {
         throw new SkyFunctionException(e, Transience.PERSISTENT){};
       }
@@ -971,7 +970,7 @@ public final class RecursiveFilesystemTraversalFunctionTest extends FoundationTe
     @Override
     public SkyValue compute(SkyKey skyKey, Environment env)
         throws SkyFunctionException, InterruptedException {
-      return env.getValue(new NonHermeticArtifactSkyKey((ArtifactSkyKey) skyKey));
+      return env.getValue(new NonHermeticArtifactSkyKey(skyKey));
     }
 
     @Nullable
@@ -981,8 +980,8 @@ public final class RecursiveFilesystemTraversalFunctionTest extends FoundationTe
     }
   }
 
-  private static class NonHermeticArtifactSkyKey extends AbstractSkyKey<ArtifactSkyKey> {
-    private NonHermeticArtifactSkyKey(ArtifactSkyKey arg) {
+  private static class NonHermeticArtifactSkyKey extends AbstractSkyKey<SkyKey> {
+    private NonHermeticArtifactSkyKey(SkyKey arg) {
       super(arg);
     }