labels in genquery.scope are no longer configured.

genquery is all about loading phase concepts, so there is no reason why building a `genquery` rule under configuration X should result in the scope being configured. This is done by introducing a new BuildType.GENQUERY_SCOPE_TYPE_LIST, which represents a dependency, which is followed in the loading phase (so incrementality is not affected) but is NOT followed in the analysis phase (so it does not need to be configured). RELNOTES: labels in genquery.scope are no longer configured. PiperOrigin-RevId: 454035034 Change-Id: I08b2c588565cbed84af9fe33aee121e12892f293
bazelbuild · Jun 9, 2022 · e3f319e · e3f319e
1 parent 03e9b7d
commit e3f319e
Show file tree

Hide file tree

Showing 12 changed files with 153 additions and 62 deletions.
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/DependencyResolver.java b/src/main/java/com/google/devtools/build/lib/analysis/DependencyResolver.java
@@ -254,12 +254,7 @@ public final OrderedSetMultimap<DependencyKind, DependencyKey> dependentNodeMap(
 
     OrderedSetMultimap<DependencyKind, PartiallyResolvedDependency> partiallyResolvedDeps =
         partiallyResolveDependencies(
-            config,
-            outgoingLabels,
-            fromRule,
-            attributeMap,
-            toolchainContexts,
-            aspects);
+            config, outgoingLabels, fromRule, attributeMap, toolchainContexts, aspects);
 
     OrderedSetMultimap<DependencyKind, DependencyKey> outgoingEdges =
         fullyResolveDependencies(
@@ -565,7 +560,9 @@ private void resolveAttributes(
       if (type == BuildType.OUTPUT
           || type == BuildType.OUTPUT_LIST
           || type == BuildType.NODEP_LABEL
-          || type == BuildType.NODEP_LABEL_LIST) {
+          || type == BuildType.NODEP_LABEL_LIST
+          || type == BuildType.GENQUERY_SCOPE_TYPE
+          || type == BuildType.GENQUERY_SCOPE_TYPE_LIST) {
         // These types invoke visitLabels() so that they are reported in "bazel query" but do not
         // create a dependency. Maybe it's better to remove that, but then the labels() query
         // function would need to be rethought.

diff --git a/src/main/java/com/google/devtools/build/lib/packages/Attribute.java b/src/main/java/com/google/devtools/build/lib/packages/Attribute.java
@@ -769,7 +769,9 @@ public Builder<TYPE> allowedRuleClasses(String... allowedRuleClasses) {
      */
     public Builder<TYPE> allowedFileTypes(FileTypeSet allowedFileTypes) {
       Preconditions.checkState(
-          type.getLabelClass() == LabelClass.DEPENDENCY, "must be a label-valued type");
+          type.getLabelClass() == LabelClass.DEPENDENCY
+              || type.getLabelClass() == LabelClass.GENQUERY_SCOPE_REFERENCE,
+          "must be a label-valued type");
       propertyFlags.add(PropertyFlag.STRICT_LABEL_CHECKING);
       allowedFileTypesForLabels = Preconditions.checkNotNull(allowedFileTypes);
       return this;

diff --git a/src/main/java/com/google/devtools/build/lib/packages/AttributeFormatter.java b/src/main/java/com/google/devtools/build/lib/packages/AttributeFormatter.java
@@ -14,6 +14,8 @@
 package com.google.devtools.build.lib.packages;
 
 import static com.google.devtools.build.lib.packages.BuildType.DISTRIBUTIONS;
+import static com.google.devtools.build.lib.packages.BuildType.GENQUERY_SCOPE_TYPE;
+import static com.google.devtools.build.lib.packages.BuildType.GENQUERY_SCOPE_TYPE_LIST;
 import static com.google.devtools.build.lib.packages.BuildType.LABEL;
 import static com.google.devtools.build.lib.packages.BuildType.LABEL_DICT_UNARY;
 import static com.google.devtools.build.lib.packages.BuildType.LABEL_KEYED_STRING_DICT;
@@ -43,7 +45,6 @@
 import com.google.devtools.build.lib.query2.proto.proto2api.Build.Attribute.Tristate;
 import com.google.devtools.build.lib.query2.proto.proto2api.Build.LabelDictUnaryEntry;
 import com.google.devtools.build.lib.query2.proto.proto2api.Build.LabelKeyedStringDictEntry;
-import com.google.devtools.build.lib.query2.proto.proto2api.Build.LabelListDictEntry;
 import com.google.devtools.build.lib.query2.proto.proto2api.Build.StringDictEntry;
 import com.google.devtools.build.lib.query2.proto.proto2api.Build.StringListDictEntry;
 import java.util.Collection;
@@ -75,8 +76,8 @@ private AttributeFormatter() {}
   /**
    * Convert attribute value to proto representation.
    *
-   * <p>If {@param value} is null, only the {@code name}, {@code explicitlySpecified}, {@code
-   * nodep} (if applicable), and {@code type} fields will be included in the proto message.
+   * <p>If {@code value} is null, only the {@code name}, {@code explicitlySpecified}, {@code nodep}
+   * (if applicable), and {@code type} fields will be included in the proto message.
    *
    * <p>If {@param encodeBooleanAndTriStateAsIntegerAndString} is true then boolean and tristate
    * values are also encoded as integers and strings.
@@ -129,16 +130,15 @@ private static void maybeSetNoDep(Type<?> type, Build.Attribute.Builder attrPb)
   }
 
   private static void writeSelectorListToBuilder(
-      Build.Attribute.Builder attrPb,
-      Type<?> type,
-      SelectorList<?> selectorList) {
+      Build.Attribute.Builder attrPb, Type<?> type, SelectorList<?> selectorList) {
     Build.Attribute.SelectorList.Builder selectorListBuilder =
         Build.Attribute.SelectorList.newBuilder();
     selectorListBuilder.setType(ProtoUtils.getDiscriminatorFromType(type));
     for (Selector<?> selector : selectorList.getSelectors()) {
-      Build.Attribute.Selector.Builder selectorBuilder = Build.Attribute.Selector.newBuilder()
-          .setNoMatchError(selector.getNoMatchError())
-          .setHasDefaultValue(selector.hasDefault());
+      Build.Attribute.Selector.Builder selectorBuilder =
+          Build.Attribute.Selector.newBuilder()
+              .setNoMatchError(selector.getNoMatchError())
+              .setHasDefaultValue(selector.hasDefault());
 
       // Note that the order of entries returned by selector.getEntries is stable. The map's
       // entries' order is preserved from the fact that Starlark dictionary entry order is stable
@@ -153,9 +153,7 @@ private static void writeSelectorListToBuilder(
         Object conditionValue = entry.getValue();
         if (conditionValue != null) {
           writeAttributeValueToBuilder(
-              new SelectorEntryBuilderAdapter(selectorEntryBuilder),
-              type,
-              conditionValue);
+              new SelectorEntryBuilderAdapter(selectorEntryBuilder), type, conditionValue);
         }
         selectorBuilder.addEntries(selectorEntryBuilder);
       }
@@ -172,10 +170,19 @@ private static void writeAttributeValueToBuilder(
       AttributeValueBuilderAdapter builder, Type<?> type, Object value) {
     if (type == INTEGER) {
       builder.setIntValue(((StarlarkInt) value).toIntUnchecked());
-    } else if (type == STRING || type == LABEL || type == NODEP_LABEL || type == OUTPUT) {
+    } else if (type == STRING
+        || type == LABEL
+        || type == NODEP_LABEL
+        || type == OUTPUT
+        || type == GENQUERY_SCOPE_TYPE) {
+
       builder.setStringValue(value.toString());
-    } else if (type == STRING_LIST || type == LABEL_LIST || type == NODEP_LABEL_LIST
-        || type == OUTPUT_LIST || type == DISTRIBUTIONS) {
+    } else if (type == STRING_LIST
+        || type == LABEL_LIST
+        || type == NODEP_LABEL_LIST
+        || type == OUTPUT_LIST
+        || type == DISTRIBUTIONS
+        || type == GENQUERY_SCOPE_TYPE_LIST) {
       for (Object entry : (Collection<?>) value) {
         builder.addStringListValue(entry.toString());
       }
@@ -264,14 +271,10 @@ private interface AttributeValueBuilderAdapter {
 
     void addStringListValue(String s);
 
-    void addFilesetListValue(Build.FilesetEntry.Builder builder);
-
     void addLabelDictUnaryValue(LabelDictUnaryEntry.Builder builder);
 
     void addLabelKeyedStringDictValue(LabelKeyedStringDictEntry.Builder builder);
 
-    void addLabelListDictValue(LabelListDictEntry.Builder builder);
-
     void addIntListValue(int i);
 
     void addStringDictValue(StringDictEntry.Builder builder);
@@ -292,9 +295,9 @@ private interface AttributeValueBuilderAdapter {
   /**
    * An {@link AttributeValueBuilderAdapter} which writes to a {@link Build.Attribute.Builder}.
    *
-   * <p>If {@param encodeBooleanAndTriStateAsIntegerAndString} is {@code true}, then {@link
-   * Boolean} and {@link TriState} attribute values also write to the integer and string fields.
-   * This offers backwards compatibility to clients that expect attribute values of those types.
+   * <p>If {@param encodeBooleanAndTriStateAsIntegerAndString} is {@code true}, then {@link Boolean}
+   * and {@link TriState} attribute values also write to the integer and string fields. This offers
+   * backwards compatibility to clients that expect attribute values of those types.
    */
   private static class AttributeBuilderAdapter implements AttributeValueBuilderAdapter {
     private final boolean encodeBooleanAndTriStateAsIntegerAndString;
@@ -312,11 +315,6 @@ public void addStringListValue(String s) {
       attributeBuilder.addStringListValue(s);
     }
 
-    @Override
-    public void addFilesetListValue(Build.FilesetEntry.Builder builder) {
-      attributeBuilder.addFilesetListValue(builder);
-    }
-
     @Override
     public void addLabelDictUnaryValue(LabelDictUnaryEntry.Builder builder) {
       attributeBuilder.addLabelDictUnaryValue(builder);
@@ -327,11 +325,6 @@ public void addLabelKeyedStringDictValue(LabelKeyedStringDictEntry.Builder build
       attributeBuilder.addLabelKeyedStringDictValue(builder);
     }
 
-    @Override
-    public void addLabelListDictValue(LabelListDictEntry.Builder builder) {
-      attributeBuilder.addLabelListDictValue(builder);
-    }
-
     @Override
     public void addIntListValue(int i) {
       attributeBuilder.addIntListValue(i);
@@ -431,11 +424,6 @@ public void addStringListValue(String s) {
       selectorEntryBuilder.addStringListValue(s);
     }
 
-    @Override
-    public void addFilesetListValue(Build.FilesetEntry.Builder builder) {
-      selectorEntryBuilder.addFilesetListValue(builder);
-    }
-
     @Override
     public void addLabelDictUnaryValue(LabelDictUnaryEntry.Builder builder) {
       selectorEntryBuilder.addLabelDictUnaryValue(builder);
@@ -446,11 +434,6 @@ public void addLabelKeyedStringDictValue(LabelKeyedStringDictEntry.Builder build
       selectorEntryBuilder.addLabelKeyedStringDictValue(builder);
     }
 
-    @Override
-    public void addLabelListDictValue(LabelListDictEntry.Builder builder) {
-      selectorEntryBuilder.addLabelListDictValue(builder);
-    }
-
     @Override
     public void addIntListValue(int i) {
       selectorEntryBuilder.addIntListValue(i);

diff --git a/src/main/java/com/google/devtools/build/lib/packages/BuildType.java b/src/main/java/com/google/devtools/build/lib/packages/BuildType.java
@@ -75,6 +75,28 @@ public final class BuildType {
   /** The type of a list of {@linkplain #NODEP_LABEL labels} that do not cause dependencies. */
   @SerializationConstant
   public static final ListType<Label> NODEP_LABEL_LIST = ListType.create(NODEP_LABEL);
+
+  /**
+   * This is a label type that causes dependencies, but the dependencies are NOT to be configured.
+   * Does not say anything about whether the attribute of this type is itself configurable.
+   *
+   * <p>Without a special type to handle genquery.scope, configuring a genquery target ends up
+   * configuring the transitive closure of genquery.scope. Since genquery rule implementation loads
+   * the deps through TransitiveTargetFunction, it doesn't need them to be configured. Preventing
+   * the dependencies of scope from being configured, lets us save some resources.
+   */
+  @SerializationConstant
+  public static final Type<Label> GENQUERY_SCOPE_TYPE =
+      new LabelType(LabelClass.GENQUERY_SCOPE_REFERENCE);
+
+  /**
+   * This is a label type that causes dependencies, but the dependencies are NOT to be configured.
+   * Does not say anything about whether the attribute of this type is itself configurable.
+   */
+  @SerializationConstant
+  public static final ListType<Label> GENQUERY_SCOPE_TYPE_LIST =
+      ListType.create(GENQUERY_SCOPE_TYPE);
+
   /**
    * The type of a license. Like Label, licenses aren't first-class, but they're important enough to
    * justify early syntax error detection.

diff --git a/src/main/java/com/google/devtools/build/lib/packages/ProtoUtils.java b/src/main/java/com/google/devtools/build/lib/packages/ProtoUtils.java
@@ -15,6 +15,8 @@
 package com.google.devtools.build.lib.packages;
 
 import static com.google.devtools.build.lib.packages.BuildType.DISTRIBUTIONS;
+import static com.google.devtools.build.lib.packages.BuildType.GENQUERY_SCOPE_TYPE;
+import static com.google.devtools.build.lib.packages.BuildType.GENQUERY_SCOPE_TYPE_LIST;
 import static com.google.devtools.build.lib.packages.BuildType.LABEL;
 import static com.google.devtools.build.lib.packages.BuildType.LABEL_DICT_UNARY;
 import static com.google.devtools.build.lib.packages.BuildType.LABEL_KEYED_STRING_DICT;
@@ -49,6 +51,8 @@ public class ProtoUtils {
           // this way for the sake of backward compatibility.
           .put(NODEP_LABEL, Discriminator.STRING)
           .put(LABEL_LIST, Discriminator.LABEL_LIST)
+          .put(GENQUERY_SCOPE_TYPE, Discriminator.LABEL)
+          .put(GENQUERY_SCOPE_TYPE_LIST, Discriminator.LABEL_LIST)
           .put(NODEP_LABEL_LIST, Discriminator.STRING_LIST)
           .put(STRING, Discriminator.STRING)
           .put(STRING_LIST, Discriminator.STRING_LIST)

diff --git a/src/main/java/com/google/devtools/build/lib/packages/Type.java b/src/main/java/com/google/devtools/build/lib/packages/Type.java
@@ -192,6 +192,11 @@ public enum LabelClass {
      * in cases where doing so would cause a dependency cycle.
      */
     NONDEP_REFERENCE,
+    /**
+     * Used for types which declare a dependency, but the dependency should not be configured. Used
+     * when the label is used only in the loading phase. e.g. genquery.scope
+     */
+    GENQUERY_SCOPE_REFERENCE,
     /** Used for types which use labels to declare an output path. */
     OUTPUT,
     /**

diff --git a/src/main/java/com/google/devtools/build/lib/rules/genquery/GenQuery.java b/src/main/java/com/google/devtools/build/lib/rules/genquery/GenQuery.java
@@ -60,6 +60,7 @@
 import com.google.devtools.build.lib.packages.NoSuchPackageException;
 import com.google.devtools.build.lib.packages.NoSuchTargetException;
 import com.google.devtools.build.lib.packages.Package;
+import com.google.devtools.build.lib.packages.RuleClass.ConfiguredTargetFactory.RuleErrorException;
 import com.google.devtools.build.lib.packages.Target;
 import com.google.devtools.build.lib.packages.Type;
 import com.google.devtools.build.lib.pkgcache.FilteringPolicies;
@@ -191,7 +192,7 @@ public ConfiguredTarget create(RuleContext ruleContext)
           executeQuery(
               ruleContext,
               queryOptions,
-              ruleContext.attributes().get("scope", BuildType.LABEL_LIST),
+              ruleContext.attributes().get("scope", BuildType.GENQUERY_SCOPE_TYPE_LIST),
               query,
               outputArtifact.getPath().getFileSystem().getDigestFunction().getHashFunction());
     }

diff --git a/src/main/java/com/google/devtools/build/lib/rules/genquery/GenQueryRule.java b/src/main/java/com/google/devtools/build/lib/rules/genquery/GenQueryRule.java
@@ -15,7 +15,7 @@
 package com.google.devtools.build.lib.rules.genquery;
 
 import static com.google.devtools.build.lib.packages.Attribute.attr;
-import static com.google.devtools.build.lib.packages.BuildType.LABEL_LIST;
+import static com.google.devtools.build.lib.packages.BuildType.GENQUERY_SCOPE_TYPE_LIST;
 import static com.google.devtools.build.lib.packages.Type.BOOLEAN;
 import static com.google.devtools.build.lib.packages.Type.STRING;
 import static com.google.devtools.build.lib.packages.Type.STRING_LIST;
@@ -45,7 +45,7 @@ public RuleClass build(RuleClass.Builder builder, RuleDefinitionEnvironment env)
         The scope of the query. The query is not allowed to touch targets outside the transitive
         closure of these targets.
         <!-- #END_BLAZE_RULE.ATTRIBUTE --> */
-        .add(attr("scope", LABEL_LIST).mandatory().legacyAllowAnyFileType())
+        .add(attr("scope", GENQUERY_SCOPE_TYPE_LIST).mandatory().legacyAllowAnyFileType())
         /* <!-- #BLAZE_RULE(genquery).ATTRIBUTE(strict) -->
         If true, targets whose queries escape the transitive closure of their scopes will fail to
         build. If false, Bazel will print a warning and skip whatever query path led it outside of

diff --git a/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java b/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java
@@ -1120,6 +1120,23 @@ protected FileConfiguredTarget getHostFileConfiguredTarget(String label)
     return (FileConfiguredTarget) getHostConfiguredTarget(label);
   }
 
+  /** Returns the configurations in which the given label has already been configured. */
+  protected Set<BuildConfigurationKey> getKnownConfigurations(String label) throws Exception {
+    Label parsed = Label.parseAbsoluteUnchecked(label);
+    Set<BuildConfigurationKey> cts = new HashSet<>();
+    for (Map.Entry<SkyKey, SkyValue> e :
+        skyframeExecutor.getEvaluator().getDoneValues().entrySet()) {
+      if (!(e.getKey() instanceof ConfiguredTargetKey)) {
+        continue;
+      }
+      ConfiguredTargetKey ctKey = (ConfiguredTargetKey) e.getKey();
+      if (parsed.equals(ctKey.getLabel())) {
+        cts.add(ctKey.getConfigurationKey());
+      }
+    }
+    return cts;
+  }
+
   /**
    * Returns the {@link ConfiguredAspect} with the given label. For example: {@code
    * //my:base_target%my_aspect}.
@@ -2028,6 +2045,17 @@ protected void useLoadingOptions(String... options) throws OptionsParsingExcepti
     customLoadingOptions = Options.parse(LoadingOptions.class, options).getOptions();
   }
 
+  protected AnalysisResult update(String target, int loadingPhaseThreads, boolean doAnalysis)
+      throws Exception {
+    return update(
+        ImmutableList.of(target),
+        ImmutableList.of(),
+        /*keepGoing=*/ true, // value doesn't matter since we have only one target.
+        loadingPhaseThreads,
+        doAnalysis,
+        new EventBus());
+  }
+
   protected AnalysisResult update(
       List<String> targets,
       boolean keepGoing,

diff --git a/src/test/java/com/google/devtools/build/lib/query2/cquery/ConfiguredTargetQueryTest.java b/src/test/java/com/google/devtools/build/lib/query2/cquery/ConfiguredTargetQueryTest.java
@@ -112,6 +112,12 @@ public void testMultipleTopLevelConfigurations_nullConfigs() throws Exception {
     }
   }
 
+  @Override
+  @Test
+  public void testGenqueryScope() throws Exception {
+    runGenqueryScopeTest(true);
+  }
+
   @Override
   public void testMultipleTopLevelConfigurations_multipleConfigsPrefersTopLevel() {
     // When the same target exists in multiple configurations, cquery doesn't guarantee which

diff --git a/src/test/java/com/google/devtools/build/lib/query2/testutil/AbstractQueryTest.java b/src/test/java/com/google/devtools/build/lib/query2/testutil/AbstractQueryTest.java
@@ -454,6 +454,34 @@ public void testAttrOperatorOnBooleans() throws Exception {
     assertThat(evalToString("attr(testonly, 1, t:*)")).isEqualTo("//t:t_test");
   }
 
+  protected void runGenqueryScopeTest(boolean isCquery) throws Exception {
+    // Tests the relationship between deps(genquery_rule) and that of its scope.
+    // For "query", deps(genquery_rule) should include transitive deps of its scope
+    // For "cquery", deps(genquery_rule) should include its scope, but not its transitive deps.
+
+    writeFile("a/BUILD", "sh_library(name='a')");
+    writeFile("b/BUILD", "sh_library(name='b', deps=['//a:a'])");
+    writeFile("q/BUILD", "genquery(name='q', scope=['//b'], expression='deps(//b)')");
+
+    // Assure that deps of a genquery rule includes the transitive closure of its scope.
+    // This is required for correctness of incremental "blaze build genqueryrule"
+    ImmutableList<String> evalResult = evalToListOfStrings("deps(//q:q)");
+    if (isCquery) {
+      // Not checking for equality, since when run as a cquery test, there will be other
+      // dependencies.
+      assertThat(evalResult).contains("//q:q");
+      // assert that transitive closure of scope is NOT present.
+      assertThat(evalResult).containsNoneOf("//a:a", "//b:b");
+    } else {
+      assertThat(evalResult).containsExactly("//q:q", "//a:a", "//b:b");
+    }
+  }
+
+  @Test
+  public void testGenqueryScope() throws Exception {
+    runGenqueryScopeTest(false);
+  }
+
   @Test
   public void testAttrOnPackageDefaultVisibility() throws Exception {
     writeFile(