Properly handle package-default visibility in AggregatingAttributeMapper

Have `#visitAttribute` recognize that visibility is special, which lets it play
nice with the `attr` query function and lets us delete
`#getPossibleAttributeValues`. We check for the visibility special case after
checking for selects and computed defaults - visibility shouldn't be either of
these, but stranger things have happened.

I think the history here is that package-default visibility was always broken
for the `attr` query function, which uses `#visitAttribute`, while query
output was correct because it used #getPossibleAttributeValues (or other
scattered special-casing which existed at various times).

rule_test_test needed to be updated because it was expecting the broken
behavior - package-default visibility being ignored by `attr(...)`.

PiperOrigin-RevId: 309477218

src/main/java/com/google/devtools/build/lib/packages/AggregatingAttributeMapper.java

@@ -18,7 +18,6 @@
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Iterables;
-import com.google.common.collect.Lists;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.collect.CollectionUtils;
 import com.google.devtools.build.lib.packages.Attribute.ComputationLimiter;
@@ -186,40 +185,6 @@ public Set<Label> checkForDuplicateLabels(Attribute attribute) {
     return duplicates == null ? ImmutableSet.of() : duplicates.build();
   }
 
-  /**
-   * Returns a list of the possible values of the specified attribute in the specified rule.
-   *
-   * <p>If the attribute's value is a simple value, then this returns a singleton list of that
-   * value.
-   *
-   * <p>If the attribute's value is an expression containing one or many {@code select(...)}
-   * expressions, then this returns a list of all values that expression may evaluate to.
-   *
-   * <p>If the attribute does not have an explicit value for this rule, and the rule provides a
-   * computed default, the computed default function is evaluated given the rule's other attribute
-   * values as inputs and the output is returned in a singleton list.
-   *
-   * <p>If the attribute does not have an explicit value for this rule, and the rule provides a
-   * computed default, and the computed default function depends on other attributes whose values
-   * contain {@code select(...)} expressions, then the computed default function is evaluated for
-   * every possible combination of input values, and the list of outputs is returned.
-   *
-   * <p><b>EFFICIENCY WARNING:</b> Do not use this method unless you really need every single value
-   * the attribute might take. See {@link #visitAttribute}'s documentation for details.
-   */
-  public Iterable<Object> getPossibleAttributeValues(Rule rule, Attribute attr) {
-    // Values may be null, so use normal collections rather than immutable collections.
-    // This special case for the visibility attribute is needed because its value is replaced
-    // with an empty list during package loading if it is public or private in order not to visit
-    // the package called 'visibility'.
-    if (attr.getName().equals("visibility")) {
-      List<Object> result = new ArrayList<>(1);
-      result.add(rule.getVisibility().getDeclaredLabels());
-      return result;
-    }
-    return Lists.<Object>newArrayList(visitAttribute(attr.getName(), attr.getType()));
-  }
-
   /**
    * If the attribute is a selector list of list type, then this method returns a list with number
    * of elements equal to the number of select statements in the selector list. Each element of this
@@ -250,20 +215,33 @@ public <T> Iterable<T> getConcatenatedSelectorListsOfListType(
   /**
    * Returns a list of all possible values an attribute can take for this rule.
    *
-   * <p><b>EFFICIENCY WARNING:</b> Do not use this method unless you really need every single value
-   * the attribute might take.
+   * <p>If the attribute's value is a simple value, then this returns a singleton list of that
+   * value.
    *
-   * <p>This is dangerous because it's easy to write attributes with an exponential number of
-   * possible values:
+   * <p>If the attribute's value is an expression containing one or many {@code select(...)}
+   * expressions, then this returns a list of all values that expression may evaluate to. This is
+   * dangerous because it's easy to write attributes with an exponential number of possible values:
    *
    * <pre>
    *   foo = select({a: 1, b: 2} + select({c: 3, d: 4}) + select({e: 5, f: 6})
    * </pre>
    *
    * <p>Possible values: <code>[135, 136, 145, 146, 235, 236, 245, 246]</code> (i.e. 2^3).
    *
-   * <p>This is true not just for attributes with multiple selects, but also
-   * {@link Attribute.ComputedDefault}s depending on such attributes.
+   * <p>This is true not just for attributes with multiple selects, but also {@link
+   * Attribute.ComputedDefault}s depending on such attributes.
+   *
+   * <p>If the attribute does not have an explicit value for this rule, and the rule provides a
+   * computed default, the computed default function is evaluated given the rule's other attribute
+   * values as inputs and the output is returned in a singleton list.
+   *
+   * <p>If the attribute does not have an explicit value for this rule, and the rule provides a
+   * computed default, and the computed default function depends on other attributes whose values
+   * contain {@code select(...)} expressions, then the computed default function is evaluated for
+   * every possible combination of input values, and the list of outputs is returned.
+   *
+   * <p><b>EFFICIENCY WARNING:</b> Do not use this method unless you really need every single value
+   * the attribute might take.
    *
    * <p>More often than not, calling code doesn't really need every value, but really just wants to
    * know, e.g., which labels might appear in a dependency list. For such cases, merging methods
@@ -289,9 +267,16 @@ public <T> Iterable<T> visitAttribute(String attributeName, Type<T> type) {
       return computedDefault.getPossibleValues(type, rule);
     }
 
+    if ("visibility".equals(attributeName) && type.equals(BuildType.NODEP_LABEL_LIST)) {
+      // This special case for the visibility attribute is needed because its value is replaced
+      // with an empty list during package loading if it is public or private in order not to visit
+      // the package called 'visibility'.
+      return ImmutableList.of(type.cast(rule.getVisibility().getDeclaredLabels()));
+    }
+
     // For any other attribute, just return its direct value.
     T value = get(attributeName, type);
-    return value == null ? ImmutableList.<T>of() : ImmutableList.of(value);
+    return value == null ? ImmutableList.of() : ImmutableList.of(value);
   }
 
   /**

src/main/java/com/google/devtools/build/lib/packages/AttributeFormatter.java

@@ -450,13 +450,11 @@ public void setTristateValue(Tristate tristate) {
    * An {@link AttributeValueBuilderAdapter} which writes to a {@link SelectorEntry.Builder}.
    *
    * <p>Note that there is no {@code encodeBooleanAndTriStateAsIntegerAndString} parameter needed
-   * here. This is because the clients that expect those alternate encodings of boolean and
-   * tristate attribute values do not support {@link SelectorList} values. When providing output to
-   * those clients, we compute the set of possible attribute values (expanding {@link SelectorList}
+   * here. This is because the clients that expect those alternate encodings of boolean and tristate
+   * attribute values do not support {@link SelectorList} values. When providing output to those
+   * clients, we compute the set of possible attribute values (expanding {@link SelectorList}
    * values, evaluating computed defaults, and flattening collections of collections; see {@link
-   * com.google.devtools.build.lib.packages.AggregatingAttributeMapper#getPossibleAttributeValues}
-   * and {@link
-   * com.google.devtools.build.lib.packages.AggregatingAttributeMapper#flattenAttributeValues}).
+   * com.google.devtools.build.lib.packages.AggregatingAttributeMapper#visitAttribute}).
    */
   private static class SelectorEntryBuilderAdapter implements AttributeValueBuilderAdapter {
     private final SelectorEntry.Builder selectorEntryBuilder;

src/main/java/com/google/devtools/build/lib/query2/query/output/PossibleAttributeValues.java

@@ -44,8 +44,8 @@ public Iterator<Object> iterator() {
   /**
    * Returns the possible values of the specified attribute in the specified rule. For simple
    * attributes, this is a single value. For configurable and computed attributes, this may be a
-   * list of values. See {@link AggregatingAttributeMapper#getPossibleAttributeValues} for how the
-   * values are determined.
+   * list of values. See {@link AggregatingAttributeMapper#visitAttribute} for how the values are
+   * determined.
    *
    * <p>This applies an important optimization for label lists: instead of returning all possible
    * values, it only returns possible <i>labels</i>. For example, given:
@@ -87,9 +87,11 @@ static PossibleAttributeValues forRuleAndAttribute(Rule rule, Attribute attr) {
         != null) {
       return new PossibleAttributeValues(Lists.newArrayList(list), source);
     } else {
-      // The call to getPossibleAttributeValues below is especially slow with selector lists.
-      return new PossibleAttributeValues(attributeMap.getPossibleAttributeValues(rule, attr),
-          source);
+      // The call to visitAttributes below is especially slow with selector lists.
+      @SuppressWarnings("unchecked") // Casting Iterable<T> -> Iterable<Object>
+      Iterable<Object> possibleValues =
+          (Iterable<Object>) attributeMap.visitAttribute(attr.getName(), attr.getType());
+      return new PossibleAttributeValues(possibleValues, source);
     }
   }
 }

src/test/java/com/google/devtools/build/lib/query2/testutil/AbstractQueryTest.java

@@ -348,21 +348,28 @@ public void testNullAttrOperator() throws Exception {
     assertThat(evalToString("attr(deprecation, ' ', c:*)")).isEmpty();
   }
 
-  private void writeBooleanBuildFiles() throws Exception {
+  @Test
+  public void testAttrOperatorOnBooleans() throws Exception {
     writeFile(
         "t/BUILD",
         "cc_library(name='t', srcs=['t.cc'], data=['r'], testonly=0)",
         "cc_library(name='t_test', srcs=['t.cc'], data=['r'], testonly=1)");
-  }
 
-  @Test
-  public void testAttrOperatorOnBooleans() throws Exception {
-    writeBooleanBuildFiles();
     // Assure that integers query correctly for BOOLEAN values.
     assertThat(evalToString("attr(testonly, 0, t:*)")).isEqualTo("//t:t");
     assertThat(evalToString("attr(testonly, 1, t:*)")).isEqualTo("//t:t_test");
   }
 
+  @Test
+  public void testAttrOnPackageDefaultVisibility() throws Exception {
+    writeFile(
+        "t/BUILD",
+        "package(default_visibility=['//visibility:public'])",
+        "cc_library(name='t', srcs=['t.cc'])");
+
+    assertThat(evalToString("attr(visibility, public, t:*)")).isEqualTo("//t:t");
+  }
+
   @Test
   public void testSomeOperator() throws Exception {
     writeBuildFiles2();

src/test/shell/bazel/rule_test_test.sh

@@ -221,8 +221,8 @@ EOF
 
   bazel query --output=label 'attr(visibility, private, //:all)' >& "$TEST_log" || fail "query failed"
   expect_log '//:x_test_impl'
+  expect_log '//:x\b'
   expect_not_log '//:x_test\b'
-  expect_not_log '//:x\b'
 
   bazel query --output=label 'attr(visibility, foo, //:all)' >& "$TEST_log" || fail "query failed"
   expect_log '//:x_test\b'