commit f15d08d4e48336c5f16b1d74400a7f95b7986fa5
author janakr <janakr@google.com> Wed Apr 22 12:53:03 2020 -0700
committer Copybara-Service <copybara-worker@google.com> Wed Apr 22 12:54:36 2020 -0700
tree d511132d7f7e0e65d391f46f4a4f5d99a770c394
parent 1d7db16c095d976d54570f3f28a4c1c9a30bb40c

Try to clean up some of the excess baggage Aspects are carting around, mainly from a code cleanliness perspective, but with some minor memory benefits:

1. Use AspectKey instead of AspectValue wherever possible at the top-level, since keys are what callers should be using, not values.
2. Remove the freestanding label field from AspectKey. It is always equal to its ConfiguredTargetKey field's label. Having two fields with redundant data is confusing.
3. Remove the freestanding label field from AspectValue. Same reason: it was always the AspectKey's label.
4. Remove AspectDescriptor from ConfiguredAspect: it comes from the AspectKey, so just use the key.

This is also good because I think maximizing correspondence between ConfiguredAspect and ConfiguredTarget is a goal. Having an AspectValue muddies the waters. Note that, for historical reasons, ConfiguredTarget has much more data in it than we'd want a ConfiguredAspect to have (ideally, both would just have actions and providers).

In a follow-up, I may wrap AspectValue together with an AspectKey in TopLevelSkylarkAspectFunction#compute's return value. I think that, plus some tolerable casting, would remove the only remaining real need for AspectValue to have an AspectKey inside it.

PiperOrigin-RevId: 307877866

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

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/AnalysisResult.java b/src/main/java/com/google/devtools/build/lib/analysis/AnalysisResult.java
index 60edd8f..ae03826 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/AnalysisResult.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/AnalysisResult.java
@@ -15,12 +15,14 @@
 package com.google.devtools.build.lib.analysis;
 
 import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.ImmutableSortedSet;
 import com.google.common.collect.Sets;
 import com.google.devtools.build.lib.actions.ActionGraph;
 import com.google.devtools.build.lib.actions.PackageRoots;
 import com.google.devtools.build.lib.analysis.config.BuildConfigurationCollection;
+import com.google.devtools.build.lib.skyframe.AspectValueKey.AspectKey;
 import java.util.Collection;
 import javax.annotation.Nullable;
 
@@ -38,7 +40,7 @@
   private final ImmutableSet<ConfiguredTarget> parallelTests;
   private final ImmutableSet<ConfiguredTarget> exclusiveTests;
   @Nullable private final TopLevelArtifactContext topLevelContext;
-  private final ImmutableSet<AspectValue> aspects;
+  private final ImmutableMap<AspectKey, ConfiguredAspect> aspects;
   private final PackageRoots packageRoots;
   private final String workspaceName;
   private final Collection<TargetAndConfiguration> topLevelTargetsWithConfigs;
@@ -47,7 +49,7 @@
   AnalysisResult(
       BuildConfigurationCollection configurations,
       ImmutableSet<ConfiguredTarget> targetsToBuild,
-      ImmutableSet<AspectValue> aspects,
+      ImmutableMap<AspectKey, ConfiguredAspect> aspects,
       @Nullable ImmutableList<ConfiguredTarget> targetsToTest,
       ImmutableSet<ConfiguredTarget> targetsToSkip,
       @Nullable String error,
@@ -93,13 +95,8 @@
     return packageRoots;
   }
 
-  /**
-   * Returns aspects of configured targets to build.
-   *
-   * <p>If this list is empty, build the targets returned by {@code getTargetsToBuild()}.
-   * Otherwise, only build these aspects of the targets returned by {@code getTargetsToBuild()}.
-   */
-  public ImmutableSet<AspectValue> getAspects() {
+  /** Returns aspects to build. */
+  public ImmutableMap<AspectKey, ConfiguredAspect> getAspectsMap() {
     return aspects;
   }