explcitly traverse aspects in cquery

This CL adds aspect nodes as explicit targets in cquery deps by making
ConfiguredTargetQueryEnvironment generic over a new interface,
CqueryNode, rather than ConfiguredTarget.

CqueryNode is implemented by AspectKey as well as ConfiguredTarget so
that both can be traversed in a deps query.

PiperOrigin-RevId: 595509373
Change-Id: I8a637cd3ed640907d2b1501bcd2b4a4507d183e7

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

@@ -383,6 +383,7 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/profiler/memory:allocationtracker",
         "//src/main/java/com/google/devtools/build/lib/query2",
         "//src/main/java/com/google/devtools/build/lib/query2:aquery-utils",
+        "//src/main/java/com/google/devtools/build/lib/query2/common:cquery-node",
         "//src/main/java/com/google/devtools/build/lib/query2/engine",
         "//src/main/java/com/google/devtools/build/lib/query2/query/output",
         "//src/main/java/com/google/devtools/build/lib/runtime/events",

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

@@ -407,6 +407,7 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/profiler",
         "//src/main/java/com/google/devtools/build/lib/profiler:google-auto-profiler-utils",
         "//src/main/java/com/google/devtools/build/lib/profiler/memory:current_rule_tracker",
+        "//src/main/java/com/google/devtools/build/lib/query2/common:cquery-node",
         "//src/main/java/com/google/devtools/build/lib/shell",
         "//src/main/java/com/google/devtools/build/lib/skyframe:aspect_key_creator",
         "//src/main/java/com/google/devtools/build/lib/skyframe:build_info_collection_value",
@@ -691,6 +692,7 @@ java_library(
         ":transitive_info_collection",
         "//src/main/java/com/google/devtools/build/lib/actions:action_lookup_key",
         "//src/main/java/com/google/devtools/build/lib/cmdline",
+        "//src/main/java/com/google/devtools/build/lib/query2/common:cquery-node",
         "//src/main/java/com/google/devtools/build/lib/skyframe/config",
         "//src/main/java/net/starlark/java/eval",
         "//third_party:guava",

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

@@ -19,6 +19,7 @@
 import com.google.devtools.build.lib.actions.ActionLookupKey;
 import com.google.devtools.build.lib.analysis.config.ConfigMatchingProvider;
 import com.google.devtools.build.lib.cmdline.Label;
+import com.google.devtools.build.lib.query2.common.CqueryNode;
 import com.google.devtools.build.lib.skyframe.config.BuildConfigurationKey;
 import javax.annotation.Nullable;
 import net.starlark.java.eval.Dict;
@@ -35,7 +36,7 @@
  * their direct dependencies, only the corresponding {@link TransitiveInfoCollection}s. Also, {@link
  * ConfiguredTarget} objects should not be accessible from the action graph.
  */
-public interface ConfiguredTarget extends TransitiveInfoCollection, Structure {
+public interface ConfiguredTarget extends TransitiveInfoCollection, Structure, CqueryNode {
 
   /** All <code>ConfiguredTarget</code>s have a "label" field. */
   String LABEL_FIELD = "label";
@@ -44,13 +45,15 @@ public interface ConfiguredTarget extends TransitiveInfoCollection, Structure {
   String FILES_FIELD = "files";
 
   /** Returns a key that may be used to lookup this {@link ConfiguredTarget}. */
+  @Override
   ActionLookupKey getLookupKey();
 
   @Override
   default Label getLabel() {
     return getLookupKey().getLabel();
   }
 
+  @Override
   @Nullable
   default String getConfigurationChecksum() {
     return getConfigurationKey() == null ? null : getConfigurationKey().getOptions().checksum();
@@ -65,8 +68,9 @@ default String getConfigurationChecksum() {
    * com.google.devtools.build.lib.analysis.configuredtargets.PackageGroupConfiguredTarget} for
    * which it is always <b>null</b>.
    *
-   * <p>If this changes, {@link AspectResolver#aspecMatchesConfiguredTarget} should be updated.
+   * <p>If this changes, {@link AspectResolver#aspectMatchesConfiguredTarget} should be updated.
    */
+  @Override
   @Nullable
   default BuildConfigurationKey getConfigurationKey() {
     return getLookupKey().getConfigurationKey();
@@ -89,6 +93,7 @@ default BuildConfigurationKey getConfigurationKey() {
    * If the configured target is an alias, return the actual target, otherwise return the current
    * target. This follows alias chains.
    */
+  @Override
   default ConfiguredTarget getActual() {
     return this;
   }
@@ -98,6 +103,7 @@ default ConfiguredTarget getActual() {
    * label. This is not the same as {@code getActual().getLabel()}, because it does not follow alias
    * chains.
    */
+  @Override
   default Label getOriginalLabel() {
     return getLabel();
   }
@@ -106,10 +112,12 @@ default Label getOriginalLabel() {
    * The configuration conditions that trigger this configured target's configurable attributes. For
    * targets that do not support configurable attributes, this will be an empty map.
    */
+  @Override
   default ImmutableMap<Label, ConfigMatchingProvider> getConfigConditions() {
     return ImmutableMap.of();
   }
 
+  @Override
   default boolean isRuleConfiguredTarget() {
     return false;
   }
@@ -119,6 +127,7 @@ default boolean isRuleConfiguredTarget() {
    *
    * <p>Unwrapping is recursive if there are multiple layers.
    */
+  @Override
   default ConfiguredTarget unwrapIfMerged() {
     return this;
   }
@@ -128,6 +137,7 @@ default ConfiguredTarget unwrapIfMerged() {
    *
    * @return a map of provider names to their values, or null if there are no providers
    */
+  @Override
   @Nullable
   default Dict<String, Object> getProvidersDictForQuery() {
     return null;

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

@@ -30,6 +30,7 @@
 import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
 import com.google.devtools.build.lib.profiler.AutoProfiler;
 import com.google.devtools.build.lib.profiler.GoogleAutoProfilerUtils;
+import com.google.devtools.build.lib.query2.common.CqueryNode;
 import java.time.Duration;
 import java.util.ArrayList;
 import java.util.HashMap;
@@ -243,7 +244,7 @@ static ArtifactsToBuild getAllArtifactsToBuild(
    *
    * <p>Always returns false for hidden rules and source file targets.
    */
-  public static boolean shouldConsiderForDisplay(ConfiguredTarget configuredTarget) {
+  public static boolean shouldConsiderForDisplay(CqueryNode configuredTarget) {
     // TODO(bazel-team): this is quite ugly. Add a marker provider for this check.
     if (configuredTarget instanceof InputFileConfiguredTarget) {
       // Suppress display of source files (because we do no work to build them).

src/main/java/com/google/devtools/build/lib/buildtool/CqueryProcessor.java

@@ -14,10 +14,10 @@
 package com.google.devtools.build.lib.buildtool;
 
 import com.google.common.collect.ImmutableList;
-import com.google.devtools.build.lib.analysis.ConfiguredTarget;
 import com.google.devtools.build.lib.cmdline.TargetPattern;
 import com.google.devtools.build.lib.packages.semantics.BuildLanguageOptions;
 import com.google.devtools.build.lib.query2.PostAnalysisQueryEnvironment.TopLevelConfigurations;
+import com.google.devtools.build.lib.query2.common.CqueryNode;
 import com.google.devtools.build.lib.query2.cquery.ConfiguredTargetQueryEnvironment;
 import com.google.devtools.build.lib.query2.cquery.CqueryOptions;
 import com.google.devtools.build.lib.query2.engine.QueryEnvironment.QueryFunction;
@@ -29,7 +29,7 @@
 import net.starlark.java.eval.StarlarkSemantics;
 
 /** Performs {@code cquery} processing. */
-public final class CqueryProcessor extends PostAnalysisQueryProcessor<ConfiguredTarget> {
+public final class CqueryProcessor extends PostAnalysisQueryProcessor<CqueryNode> {
 
   public CqueryProcessor(
       QueryExpression queryExpression, TargetPattern.Parser mainRepoTargetParser) {

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

@@ -27,6 +27,7 @@ java_library(
     ),
     deps = [
         "//src/main/java/com/google/devtools/build/lib/actions",
+        "//src/main/java/com/google/devtools/build/lib/actions:action_lookup_key",
         "//src/main/java/com/google/devtools/build/lib/actions:artifacts",
         "//src/main/java/com/google/devtools/build/lib/actions:commandline_item",
         "//src/main/java/com/google/devtools/build/lib/actions:file_metadata",
@@ -74,6 +75,7 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/query2/common:QueryTransitivePackagePreloader",
         "//src/main/java/com/google/devtools/build/lib/query2/common:UniverseSkyKey",
         "//src/main/java/com/google/devtools/build/lib/query2/common:abstract-blaze-query-env",
+        "//src/main/java/com/google/devtools/build/lib/query2/common:cquery-node",
         "//src/main/java/com/google/devtools/build/lib/query2/common:options",
         "//src/main/java/com/google/devtools/build/lib/query2/common:universe-scope",
         "//src/main/java/com/google/devtools/build/lib/query2/compat:fake-load-target",

src/main/java/com/google/devtools/build/lib/query2/PostAnalysisQueryEnvironment.java

@@ -24,7 +24,9 @@
 import com.google.common.collect.Iterables;
 import com.google.common.collect.Maps;
 import com.google.common.collect.Sets;
+import com.google.devtools.build.lib.actions.ActionLookupKey;
 import com.google.devtools.build.lib.analysis.AliasProvider;
+import com.google.devtools.build.lib.analysis.AspectValue;
 import com.google.devtools.build.lib.analysis.ConfiguredTargetValue;
 import com.google.devtools.build.lib.analysis.TargetAndConfiguration;
 import com.google.devtools.build.lib.analysis.config.BuildConfigurationValue;
@@ -74,6 +76,7 @@
 import com.google.devtools.build.lib.supplier.InterruptibleSupplier;
 import com.google.devtools.build.lib.vfs.PathFragment;
 import com.google.devtools.build.skyframe.SkyKey;
+import com.google.devtools.build.skyframe.SkyValue;
 import com.google.devtools.build.skyframe.WalkableGraph;
 import java.io.IOException;
 import java.io.OutputStream;
@@ -144,7 +147,7 @@ public PostAnalysisQueryEnvironment(
 
   public abstract String getOutputFormat();
 
-  protected abstract KeyExtractor<T, ConfiguredTargetKey> getConfiguredTargetKeyExtractor();
+  protected abstract KeyExtractor<T, ActionLookupKey> getConfiguredTargetKeyExtractor();
 
   @Override
   public QueryEvalResult evaluateQuery(
@@ -217,12 +220,18 @@ public T getOrCreate(T target) {
   protected abstract T getNullConfiguredTarget(Label label) throws InterruptedException;
 
   @Nullable
-  public ConfiguredTargetValue getConfiguredTargetValue(SkyKey key) throws InterruptedException {
-    return (ConfiguredTargetValue) walkableGraphSupplier.get().getValue(key);
+  public SkyValue getConfiguredTargetValue(SkyKey key) throws InterruptedException {
+    return walkableGraphSupplier.get().getValue(key);
+  }
+
+  @Nullable
+  public AspectValue getAspectValue(SkyKey key) throws InterruptedException {
+    return (AspectValue) walkableGraphSupplier.get().getValue(key);
   }
 
   private boolean isAliasConfiguredTarget(ConfiguredTargetKey key) throws InterruptedException {
-    return AliasProvider.isAlias(getConfiguredTargetValue(key).getConfiguredTarget());
+    return AliasProvider.isAlias(
+        ((ConfiguredTargetValue) getConfiguredTargetValue(key)).getConfiguredTarget());
   }
 
   public InterruptibleSupplier<ImmutableSet<PathFragment>>
@@ -362,7 +371,7 @@ private Set<SkyKey> unwindReverseDependencyDelegationLayersIfFound(
       if (!rdep.functionName().equals(SkyFunctions.CONFIGURED_TARGET)) {
         continue;
       }
-      ConfiguredTargetKey actualParentKey = getConfiguredTargetKey(getValueFromKey(rdep));
+      var actualParentKey = getConfiguredTargetKey(getValueFromKey(rdep));
       if (actualParentKey.equals(child)) {
         // The parent has the same value as the child because it is delegating.
         foundDelegatingRdep = true;
@@ -386,7 +395,7 @@ private void unwindReverseDependencyDelegationLayers(
         output.add(rdep);
         continue;
       }
-      ConfiguredTargetKey actualParentKey = getConfiguredTargetKey(getValueFromKey(rdep));
+      var actualParentKey = getConfiguredTargetKey(getValueFromKey(rdep));
       if (!actualParentKey.equals(child)) {
         output.add(rdep);
         continue;
@@ -479,19 +488,24 @@ private ImmutableList<ClassifiedDependency<T>> targetifyValues(
       }
     }
 
+    boolean explicitAspects =
+        settings.containsAll(ImmutableSet.of(Setting.INCLUDE_ASPECTS, Setting.EXPLICIT_ASPECTS));
+
     ImmutableList.Builder<ClassifiedDependency<T>> values = ImmutableList.builder();
-    // TODO(bazel-team): An even better approach would be to treat aspects and toolchains as
+    // TODO(bazel-team): The end-goal approach is to treat aspects and toolchains as
     // first-class query nodes just like targets. In other words, let query expressions reference
     // them (they also have identifying labels) and make the graph connections between targets,
     // aspects, and toolchains explicit. That would permit more detailed queries and eliminate the
-    // per-key-type special casing below. The challenge is to generalize all query code that
-    // currently assumes its inputs are configured targets. Toolchains may have additional caveats:
-    // see b/148550864.
+    // per-key-type special casing below.
+    // This is being experimentally implemented in phases. Currently support for aspects has been
+    // implemented behind the --experimental_explicit_aspects flag.
+    // See https://github.com/bazelbuild/bazel/issues/16310 for details.
     for (SkyKey key : dependencies) {
       if (knownCtDeps.contains(key)) {
         continue;
       }
-      if (key.functionName().equals(SkyFunctions.CONFIGURED_TARGET)) {
+      if (key.functionName().equals(SkyFunctions.CONFIGURED_TARGET)
+          || (explicitAspects && key.functionName().equals(SkyFunctions.ASPECT))) {
         T dependency = getValueFromKey(key);
         Preconditions.checkState(
             dependency != null,
@@ -501,17 +515,24 @@ private ImmutableList<ClassifiedDependency<T>> targetifyValues(
                 + " configurability team.",
             key);
 
-        boolean implicit =
+        boolean implicitConfiguredTarget =
             // Check both the original guess key and the second correct key. In the case of the
             // target platform, Util.findImplicitDeps also uses the original guess key.
             implicitDeps == null
                 || implicitDeps.contains(key)
                 || implicitDeps.contains(getConfiguredTargetKey(dependency));
+
+        boolean implicit =
+            !(key.argument() instanceof ConfiguredTargetKey) || implicitConfiguredTarget;
+
         values.add(new ClassifiedDependency<>(dependency, implicit));
         knownCtDeps.add(key);
       } else if (settings.contains(Setting.INCLUDE_ASPECTS)
-          && key.functionName().equals(SkyFunctions.ASPECT)
-          && !resolvedAspectClasses.contains(((AspectKey) key).getAspectClass())) {
+          && key.functionName().equals(SkyFunctions.ASPECT)) {
+        Preconditions.checkState(!settings.contains(Setting.EXPLICIT_ASPECTS));
+        if (resolvedAspectClasses.contains(((AspectKey) key).getAspectClass())) {
+          continue;
+        }
         // When an aspect is attached to an alias configured target, it bypasses standard dependency
         // resolution and just Skyframe-loads the same aspect for the alias' referent. That means
         // the original aspect's attribute deps aren't Skyframe-resolved through AspectFunction's
@@ -550,8 +571,8 @@ private Map<SkyKey, ImmutableList<ClassifiedDependency<T>>> targetifyValues(
           targetifyValues(
               fromTargetsByKey.get(fromKey),
               entry.getValue(),
-              /*knownCtDeps=*/ new HashSet<>(),
-              /*resolvedAspectClasses=*/ new HashSet<>()));
+              /* knownCtDeps= */ new HashSet<>(),
+              /* resolvedAspectClasses= */ new HashSet<>()));
     }
     return result;
   }
@@ -586,7 +607,7 @@ private static <T> ImmutableList<T> getDependencies(
   @Nullable
   protected abstract BuildConfigurationValue getConfiguration(T target);
 
-  protected abstract ConfiguredTargetKey getConfiguredTargetKey(T target);
+  protected abstract ActionLookupKey getConfiguredTargetKey(T target);
 
   @Override
   public ThreadSafeMutableSet<T> getTransitiveClosure(
@@ -649,11 +670,13 @@ public static class TopLevelConfigurations {
 
     /** A map of non-null configured top-level targets sorted by configuration checksum. */
     private final ImmutableMap<Label, BuildConfigurationValue> nonNulls;
+
     /**
      * {@code nonNulls} may often have many duplicate values in its value set so we store a sorted
      * set of all the non-null configurations here.
      */
     private final ImmutableSortedSet<BuildConfigurationValue> nonNullConfigs;
+
     /** A list of null configured top-level targets. */
     private final ImmutableList<Label> nulls;
 

src/main/java/com/google/devtools/build/lib/query2/aquery/ActionGraphQueryEnvironment.java

@@ -19,6 +19,7 @@
 import com.google.common.util.concurrent.AsyncFunction;
 import com.google.common.util.concurrent.Futures;
 import com.google.common.util.concurrent.MoreExecutors;
+import com.google.devtools.build.lib.actions.ActionLookupKey;
 import com.google.devtools.build.lib.analysis.ConfiguredTarget;
 import com.google.devtools.build.lib.analysis.ConfiguredTargetValue;
 import com.google.devtools.build.lib.analysis.config.BuildConfigurationValue;
@@ -70,8 +71,7 @@ public class ActionGraphQueryEnvironment
   private AqueryOptions aqueryOptions;
 
   private AqueryActionFilter actionFilters;
-  private final KeyExtractor<ConfiguredTargetValue, ConfiguredTargetKey>
-      configuredTargetKeyExtractor;
+  private final KeyExtractor<ConfiguredTargetValue, ActionLookupKey> configuredTargetKeyExtractor;
   private final ConfiguredTargetValueAccessor accessor;
 
   public ActionGraphQueryEnvironment(
@@ -187,8 +187,7 @@ public String getOutputFormat() {
   }
 
   @Override
-  protected KeyExtractor<ConfiguredTargetValue, ConfiguredTargetKey>
-      getConfiguredTargetKeyExtractor() {
+  protected KeyExtractor<ConfiguredTargetValue, ActionLookupKey> getConfiguredTargetKeyExtractor() {
     return configuredTargetKeyExtractor;
   }
 
@@ -202,7 +201,7 @@ public Label getCorrectLabel(ConfiguredTargetValue configuredTargetValue) {
   @Nullable
   private ConfiguredTargetValue createConfiguredTargetValueFromKey(ConfiguredTargetKey key)
       throws InterruptedException {
-    ConfiguredTargetValue value = getConfiguredTargetValue(key);
+    ConfiguredTargetValue value = (ConfiguredTargetValue) getConfiguredTargetValue(key);
     if (value == null
         || !Objects.equals(
             value.getConfiguredTarget().getConfigurationKey(), key.getConfigurationKey())) {
@@ -251,7 +250,7 @@ protected ConfiguredTargetValue getNullConfiguredTarget(Label label) throws Inte
   @Override
   protected ConfiguredTargetValue getValueFromKey(SkyKey key) throws InterruptedException {
     Preconditions.checkState(key instanceof ConfiguredTargetKey);
-    return getConfiguredTargetValue(key);
+    return (ConfiguredTargetValue) getConfiguredTargetValue(key);
   }
 
   @Nullable