Embed ConfiguredTargetKey in ConfiguredTarget.

This is for safety of ConfiguredTargetKey.fromConfiguredTarget,
especially in the presence of ProxyConfiguredTargetKey. There's
a number of places in the codebase that may assume that if a
ConfiguredTargetValue is available, then it can be looked up using
ConfiguredTargetKey.fromConfiguredTarget. Without this change, it is
possible for a ProxyConfiguredTargetKey to fall out of the interner when
the ConfiguredTarget does not have any Actions. The ensuing lookup would
possibly result in null instead of the correct delegate value.

* Since the key is present in the ConfiguredTarget already,
  KeyedConfiguredTargetValue no longer needs to exist and is deleted.
* Minor improvement in fromConfiguredTarget soundness: not including
  executionPlatformLabel in fromConfiguredTarget makes it unsound for
  ConfiguredTargets that have one.

PiperOrigin-RevId: 535850072
Change-Id: Idffcd0ed7809a3eaccb95ee666e9860cb856d21c

src/main/java/com/google/devtools/build/lib/analysis/BUILD

@@ -712,6 +712,7 @@ java_library(
     deps = [
         ":config/config_matching_provider",
         ":transitive_info_collection",
+        "//src/main/java/com/google/devtools/build/lib/actions",
         "//src/main/java/com/google/devtools/build/lib/cmdline",
         "//src/main/java/com/google/devtools/build/lib/skyframe:build_configuration",
         "//src/main/java/net/starlark/java/eval",

src/main/java/com/google/devtools/build/lib/analysis/ConfiguredTarget.java

@@ -16,6 +16,7 @@
 
 import com.google.common.collect.ImmutableCollection;
 import com.google.common.collect.ImmutableMap;
+import com.google.devtools.build.lib.actions.ActionLookupKeyOrProxy;
 import com.google.devtools.build.lib.analysis.config.ConfigMatchingProvider;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.skyframe.BuildConfigurationKey;
@@ -42,6 +43,14 @@ public interface ConfiguredTarget extends TransitiveInfoCollection, Structure {
   /** All <code>ConfiguredTarget</code>s have a "files" field. */
   String FILES_FIELD = "files";
 
+  /** Returns a key provider that may be used to lookup this {@link ConfiguredTarget}. */
+  ActionLookupKeyOrProxy getKeyOrProxy();
+
+  @Override
+  default Label getLabel() {
+    return getKeyOrProxy().getLabel();
+  }
+
   @Nullable
   default String getConfigurationChecksum() {
     return getConfigurationKey() == null ? null : getConfigurationKey().getOptions().checksum();
@@ -59,7 +68,9 @@ default String getConfigurationChecksum() {
    * <p>If this changes, {@link AspectResolver#aspecMatchesConfiguredTarget} should be updated.
    */
   @Nullable
-  BuildConfigurationKey getConfigurationKey();
+  default BuildConfigurationKey getConfigurationKey() {
+    return getKeyOrProxy().getConfigurationKey();
+  }
 
   /** Returns keys for a legacy Starlark provider. */
   @Override

src/main/java/com/google/devtools/build/lib/analysis/ConfiguredTargetFactory.java

@@ -22,7 +22,6 @@
 import com.google.common.base.Verify;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
-import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Iterables;
 import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.actions.Artifact.SourceArtifact;
@@ -271,11 +270,9 @@ public ConfiguredTarget createConfiguredTarget(
               config,
               prerequisiteMap.get(DependencyKind.VISIBILITY_DEPENDENCY),
               visibility);
-      return new PackageGroupConfiguredTarget(targetContext, packageGroup);
+      return new PackageGroupConfiguredTarget(configuredTargetKey, targetContext, packageGroup);
     } else if (target instanceof EnvironmentGroup) {
-      TargetContext targetContext =
-          new TargetContext(analysisEnvironment, target, config, ImmutableSet.of(), visibility);
-      return new EnvironmentGroupConfiguredTarget(targetContext);
+      return new EnvironmentGroupConfiguredTarget(configuredTargetKey);
     } else {
       throw new AssertionError("Unexpected target class: " + target.getClass().getName());
     }

src/main/java/com/google/devtools/build/lib/analysis/EmptyConfiguredTarget.java

@@ -14,21 +14,20 @@
 
 package com.google.devtools.build.lib.analysis;
 
+import com.google.devtools.build.lib.actions.ActionLookupKeyOrProxy;
 import com.google.devtools.build.lib.analysis.configuredtargets.AbstractConfiguredTarget;
-import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
 import com.google.devtools.build.lib.collect.nestedset.Order;
 import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
 import com.google.devtools.build.lib.packages.Info;
 import com.google.devtools.build.lib.packages.Provider;
-import com.google.devtools.build.lib.skyframe.BuildConfigurationKey;
 import javax.annotation.Nullable;
 
 /** A configured target that is empty. */
 @Immutable
 public class EmptyConfiguredTarget extends AbstractConfiguredTarget {
-  public EmptyConfiguredTarget(Label label, BuildConfigurationKey configurationKey) {
-    super(label, configurationKey, NestedSetBuilder.emptySet(Order.STABLE_ORDER));
+  public EmptyConfiguredTarget(ActionLookupKeyOrProxy actionLookupKey) {
+    super(actionLookupKey, NestedSetBuilder.emptySet(Order.STABLE_ORDER));
   }
 
   @Nullable

src/main/java/com/google/devtools/build/lib/analysis/configuredtargets/AbstractConfiguredTarget.java

@@ -17,22 +17,21 @@
 import com.google.common.collect.ImmutableCollection;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableSet;
+import com.google.devtools.build.lib.actions.ActionLookupKeyOrProxy;
 import com.google.devtools.build.lib.analysis.AnalysisUtils;
 import com.google.devtools.build.lib.analysis.ConfiguredTarget;
 import com.google.devtools.build.lib.analysis.DefaultInfo;
 import com.google.devtools.build.lib.analysis.FilesToRunProvider;
 import com.google.devtools.build.lib.analysis.OutputGroupInfo;
 import com.google.devtools.build.lib.analysis.TransitiveInfoProvider;
 import com.google.devtools.build.lib.analysis.VisibilityProvider;
-import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.collect.nestedset.NestedSet;
 import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
 import com.google.devtools.build.lib.collect.nestedset.Order;
 import com.google.devtools.build.lib.packages.Info;
 import com.google.devtools.build.lib.packages.PackageSpecification.PackageGroupContents;
 import com.google.devtools.build.lib.packages.Provider;
 import com.google.devtools.build.lib.packages.semantics.BuildLanguageOptions;
-import com.google.devtools.build.lib.skyframe.BuildConfigurationKey;
 import java.util.function.Consumer;
 import javax.annotation.Nullable;
 import net.starlark.java.eval.EvalException;
@@ -45,8 +44,13 @@
  * default values.
  */
 public abstract class AbstractConfiguredTarget implements ConfiguredTarget, VisibilityProvider {
-  private final Label label;
-  private final BuildConfigurationKey configurationKey;
+  // This should really never be null, but is null in two cases.
+  // 1. MergedConfiguredTarget: these are ephemeral and never added to the Skyframe graph.
+  // 2. TestActionBuilder.EmptyPackageProvider: it is used here only to inject an empty
+  //    PackageSpecificationProvider.
+  // TODO(b/281522692): The existence of these cases suggest that there should be some additional
+  // abstraction that does not have a key.
+  private final ActionLookupKeyOrProxy actionLookupKey;
 
   private final NestedSet<PackageGroupContents> visibility;
 
@@ -74,19 +78,21 @@ public abstract class AbstractConfiguredTarget implements ConfiguredTarget, Visi
           OutputGroupInfo.STARLARK_NAME,
           ACTIONS_FIELD_NAME);
 
-  public AbstractConfiguredTarget(Label label, BuildConfigurationKey configurationKey) {
-    this(label, configurationKey, NestedSetBuilder.emptySet(Order.STABLE_ORDER));
+  public AbstractConfiguredTarget(ActionLookupKeyOrProxy actionLookupKey) {
+    this(actionLookupKey, NestedSetBuilder.emptySet(Order.STABLE_ORDER));
   }
 
   protected AbstractConfiguredTarget(
-      Label label,
-      BuildConfigurationKey configurationKey,
-      NestedSet<PackageGroupContents> visibility) {
-    this.label = label;
-    this.configurationKey = configurationKey;
+      ActionLookupKeyOrProxy actionLookupKey, NestedSet<PackageGroupContents> visibility) {
+    this.actionLookupKey = actionLookupKey;
     this.visibility = visibility;
   }
 
+  @Override
+  public ActionLookupKeyOrProxy getKeyOrProxy() {
+    return actionLookupKey;
+  }
+
   @Override
   public boolean isImmutable() {
     return true; // all Targets are immutable and Starlark-hashable
@@ -97,16 +103,6 @@ public final NestedSet<PackageGroupContents> getVisibility() {
     return visibility;
   }
 
-  @Override
-  public BuildConfigurationKey getConfigurationKey() {
-    return configurationKey;
-  }
-
-  @Override
-  public Label getLabel() {
-    return label;
-  }
-
   @Override
   public String toString() {
     return "ConfiguredTarget(" + getLabel() + ", " + getConfigurationChecksum() + ")";

src/main/java/com/google/devtools/build/lib/analysis/configuredtargets/EnvironmentGroupConfiguredTarget.java

@@ -15,8 +15,7 @@
 package com.google.devtools.build.lib.analysis.configuredtargets;
 
 import com.google.common.base.Preconditions;
-import com.google.devtools.build.lib.analysis.TargetContext;
-import com.google.devtools.build.lib.cmdline.Label;
+import com.google.devtools.build.lib.actions.ActionLookupKeyOrProxy;
 import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
 import com.google.devtools.build.lib.packages.Info;
 import com.google.devtools.build.lib.packages.Provider;
@@ -29,13 +28,9 @@
 @Immutable
 public final class EnvironmentGroupConfiguredTarget extends AbstractConfiguredTarget {
 
-  private EnvironmentGroupConfiguredTarget(Label label) {
-    super(label, null);
-  }
-
-  public EnvironmentGroupConfiguredTarget(TargetContext targetContext) {
-    this(targetContext.getLabel());
-    Preconditions.checkState(targetContext.getConfiguration() == null, targetContext);
+  public EnvironmentGroupConfiguredTarget(ActionLookupKeyOrProxy actionLookupKey) {
+    super(actionLookupKey);
+    Preconditions.checkState(actionLookupKey.getConfigurationKey() == null, actionLookupKey);
   }
 
   @Override

src/main/java/com/google/devtools/build/lib/analysis/configuredtargets/FileConfiguredTarget.java

@@ -15,6 +15,7 @@
 package com.google.devtools.build.lib.analysis.configuredtargets;
 
 import com.google.common.collect.ImmutableMap;
+import com.google.devtools.build.lib.actions.ActionLookupKeyOrProxy;
 import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.analysis.AnalysisUtils;
 import com.google.devtools.build.lib.analysis.FileProvider;
@@ -27,15 +28,13 @@
 import com.google.devtools.build.lib.analysis.TransitiveInfoProviderMapBuilder;
 import com.google.devtools.build.lib.analysis.VisibilityProvider;
 import com.google.devtools.build.lib.analysis.test.InstrumentedFilesInfo;
-import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.collect.nestedset.NestedSet;
 import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
 import com.google.devtools.build.lib.collect.nestedset.Order;
 import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
 import com.google.devtools.build.lib.packages.Info;
 import com.google.devtools.build.lib.packages.PackageSpecification.PackageGroupContents;
 import com.google.devtools.build.lib.packages.Provider;
-import com.google.devtools.build.lib.skyframe.BuildConfigurationKey;
 import com.google.devtools.build.lib.util.FileType;
 import javax.annotation.Nullable;
 import net.starlark.java.eval.Dict;
@@ -51,15 +50,14 @@ public abstract class FileConfiguredTarget extends AbstractConfiguredTarget
   private final TransitiveInfoProviderMap providers;
 
   FileConfiguredTarget(
-      Label label,
-      BuildConfigurationKey configurationKey,
+      ActionLookupKeyOrProxy actionLookupKey,
       NestedSet<PackageGroupContents> visibility,
       Artifact artifact,
       @Nullable InstrumentedFilesInfo instrumentedFilesInfo,
       @Nullable RequiredConfigFragmentsProvider configFragmentsProvider,
       @Nullable OutputGroupInfo generatingRuleOutputGroupInfo) {
 
-    super(label, configurationKey, visibility);
+    super(actionLookupKey, visibility);
 
     NestedSet<Artifact> filesToBuild = NestedSetBuilder.create(Order.STABLE_ORDER, artifact);
     FileProvider fileProvider = new FileProvider(filesToBuild);

src/main/java/com/google/devtools/build/lib/analysis/configuredtargets/InputFileConfiguredTarget.java

@@ -18,14 +18,12 @@
 import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.actions.Artifact.SourceArtifact;
 import com.google.devtools.build.lib.analysis.TargetContext;
-import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.collect.nestedset.NestedSet;
 import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
 import com.google.devtools.build.lib.collect.nestedset.Order;
 import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
 import com.google.devtools.build.lib.packages.InputFile;
 import com.google.devtools.build.lib.packages.License;
-import com.google.devtools.build.lib.packages.PackageSpecification.PackageGroupContents;
 import java.util.Objects;
 import javax.annotation.Nullable;
 import net.starlark.java.eval.Printer;
@@ -41,21 +39,19 @@ public final class InputFileConfiguredTarget extends FileConfiguredTarget {
   private final SourceArtifact artifact;
   private final NestedSet<TargetLicense> licenses;
 
-  private InputFileConfiguredTarget(
-      Label label,
-      NestedSet<PackageGroupContents> visibility,
-      SourceArtifact artifact,
-      NestedSet<TargetLicense> licenses) {
-    super(label, null, visibility, artifact, null, null, null);
-    this.artifact = artifact;
-    this.licenses = licenses;
-  }
-
   public InputFileConfiguredTarget(
       TargetContext targetContext, InputFile inputFile, SourceArtifact artifact) {
-    this(inputFile.getLabel(), targetContext.getVisibility(), artifact, makeLicenses(inputFile));
+    super(
+        targetContext.getAnalysisEnvironment().getOwner(),
+        targetContext.getVisibility(),
+        artifact,
+        /* instrumentedFilesInfo= */ null,
+        /* configFragmentsProvider= */ null,
+        /* generatingRuleOutputGroupInfo= */ null);
     Preconditions.checkArgument(getConfigurationKey() == null, getLabel());
     Preconditions.checkArgument(targetContext.getTarget() == inputFile, getLabel());
+    this.artifact = artifact;
+    this.licenses = makeLicenses(inputFile);
   }
 
   private static NestedSet<TargetLicense> makeLicenses(InputFile inputFile) {

src/main/java/com/google/devtools/build/lib/analysis/configuredtargets/MergedConfiguredTarget.java

@@ -15,6 +15,7 @@
 
 import com.google.common.collect.ImmutableList;
 import com.google.devtools.build.lib.actions.ActionAnalysisMetadata;
+import com.google.devtools.build.lib.actions.ActionLookupKeyOrProxy;
 import com.google.devtools.build.lib.analysis.AnalysisUtils;
 import com.google.devtools.build.lib.analysis.ConfiguredAspect;
 import com.google.devtools.build.lib.analysis.ConfiguredTarget;
@@ -28,15 +29,18 @@
 import com.google.devtools.build.lib.analysis.test.AnalysisFailure;
 import com.google.devtools.build.lib.analysis.test.AnalysisFailureInfo;
 import com.google.devtools.build.lib.analysis.test.InstrumentedFilesInfo;
+import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.collect.nestedset.NestedSet;
 import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
 import com.google.devtools.build.lib.packages.Info;
 import com.google.devtools.build.lib.packages.Provider;
+import com.google.devtools.build.lib.skyframe.BuildConfigurationKey;
 import com.google.devtools.build.lib.starlarkbuildapi.ActionApi;
 import java.util.ArrayList;
 import java.util.Collection;
 import java.util.List;
 import java.util.function.Consumer;
+import javax.annotation.Nullable;
 import net.starlark.java.eval.Dict;
 import net.starlark.java.eval.Printer;
 
@@ -62,12 +66,32 @@ private MergedConfiguredTarget(
       ConfiguredTarget base,
       Iterable<ConfiguredAspect> aspects,
       TransitiveInfoProviderMap nonBaseProviders) {
-    super(base.getLabel(), base.getConfigurationKey());
+    // TODO(b/281522692): it's unsound to pass a null key here, but the type system doesn't
+    // currently provide a better way to do this.
+    super(/* actionLookupKey= */ null);
     this.base = base;
     this.aspects = ImmutableList.copyOf(aspects);
     this.nonBaseProviders = nonBaseProviders;
   }
 
+  @Override
+  public ActionLookupKeyOrProxy getKeyOrProxy() {
+    throw new IllegalStateException(
+        "MergedConfiguredTarget is ephemeral. It does not exist in the Skyframe graph and it does"
+            + " not have a key.");
+  }
+
+  @Override
+  public Label getLabel() {
+    return base.getLabel();
+  }
+
+  @Override
+  @Nullable
+  public BuildConfigurationKey getConfigurationKey() {
+    return base.getConfigurationKey();
+  }
+
   @Override
   public <P extends TransitiveInfoProvider> P getProvider(Class<P> providerClass) {
     AnalysisUtils.checkProvider(providerClass);