Minimize calls to AggregatingAttributeMapper.visitAttribute. Attributes with

multiple selects run the risk of exponential value growth, so we shouldn't
request a full iteration of possible values unless that's really what the
caller needs.

--
MOS_MIGRATED_REVID=91118257

6 files changed, 67 insertions, 52 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredAttributeMapper.java b/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredAttributeMapper.java
index bd0fd68d57..a773cd0dde 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredAttributeMapper.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredAttributeMapper.java
@@ -15,7 +15,6 @@ package com.google.devtools.build.lib.analysis;
 
 import com.google.common.base.Preconditions;
 import com.google.common.base.Verify;
-import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.devtools.build.lib.analysis.config.ConfigMatchingProvider;
 import com.google.devtools.build.lib.packages.AbstractAttributeMapper;
@@ -169,10 +168,4 @@ public class ConfiguredAttributeMapper extends AbstractAttributeMapper {
           "lookup failed on attribute " + attributeName + ": " + e.getMessage());
     }
   }
-
-  @Override
-  protected <T> Iterable<T> visitAttribute(String attributeName, Type<T> type) {
-    T value = get(attributeName, type);
-    return value == null ? ImmutableList.<T>of() : ImmutableList.of(value);
-  }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/RedirectChaser.java b/src/main/java/com/google/devtools/build/lib/analysis/RedirectChaser.java
index 2c0b98a254..459868460d 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/RedirectChaser.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/RedirectChaser.java
@@ -59,11 +59,6 @@ public final class RedirectChaser {
       }
       return super.get(attributeName, type);
     }
-
-    @Override
-    protected <T> Iterable<T> visitAttribute(String attributeName, Type<T> type) {
-      throw new IllegalStateException("Attribute visitation not supported redirect resolution");
-    }
   }
 
   /**
diff --git a/src/main/java/com/google/devtools/build/lib/packages/AbstractAttributeMapper.java b/src/main/java/com/google/devtools/build/lib/packages/AbstractAttributeMapper.java
index 544435afd4..2742b1e297 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/AbstractAttributeMapper.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/AbstractAttributeMapper.java
@@ -135,30 +135,27 @@ public abstract class AbstractAttributeMapper implements AttributeMap {
   public void visitLabels(AcceptsLabelAttribute observer) {
     for (Attribute attribute : ruleClass.getAttributes()) {
       Type<?> type = attribute.getType();
-      // TODO(bazel-team): This is incoherent: we shouldn't have to special-case these types
-      // for our visitation policy. But this is the semantics the calling code requires. Audit
-      // exactly which calling code expects what and clean up this interface.
-      if (type == Type.OUTPUT || type == Type.OUTPUT_LIST
-              || type == Type.NODEP_LABEL || type == Type.NODEP_LABEL_LIST) {
-        continue;
-      }
-      for (Object value : visitAttribute(attribute.getName(), type)) {
-        if (value == null) {
-          // This is particularly possible for computed defaults.
-          continue;
-        }
-        for (Label label : type.getLabels(value)) {
-          observer.acceptLabelAttribute(label, attribute);
-        }
+      // TODO(bazel-team): clean up the typing / visitation interface so we don't have to
+      // special-case these types.
+      if (type != Type.OUTPUT && type != Type.OUTPUT_LIST
+          && type != Type.NODEP_LABEL && type != Type.NODEP_LABEL_LIST) {
+        visitLabels(attribute, observer);
       }
     }
   }
 
   /**
-   * Implementations should provide policy-appropriate mappings when an attribute is requested in
-   * the context of a rule visitation.
+   * Visits all labels reachable from the given attribute.
    */
-  protected abstract <T> Iterable<T> visitAttribute(String attributeName, Type<T> type);
+  protected void visitLabels(Attribute attribute, AcceptsLabelAttribute observer) {
+    Type<?> type = attribute.getType();
+    Object value = get(attribute.getName(), type);
+    if (value != null) { // null values are particularly possible for computed defaults.
+      for (Label label : type.getLabels(value)) {
+        observer.acceptLabelAttribute(label, attribute);
+      }
+    }
+  }
 
   /**
    * Returns a {@link Type.SelectorList} for the given attribute if the attribute is configurable
diff --git a/src/main/java/com/google/devtools/build/lib/packages/AggregatingAttributeMapper.java b/src/main/java/com/google/devtools/build/lib/packages/AggregatingAttributeMapper.java
index b51b195ebc..afc4b7d769 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/AggregatingAttributeMapper.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/AggregatingAttributeMapper.java
@@ -59,25 +59,68 @@ public class AggregatingAttributeMapper extends AbstractAttributeMapper {
   /**
    * Override that also visits the rule's configurable attribute keys (which are
    * themselves labels).
+   *
+   * <p>Note that we directly parse the selectors rather than just calling {@link #visitAttribute}
+   * to iterate over all possible values. That's because {@link #visitAttribute} can grow
+   * exponentially with respect to the number of selects (e.g. if an attribute uses three selects
+   * with three conditions each, it can take nine possible values). So we want to avoid that code
+   * path whenever actual value iteration isn't specifically needed.
    */
   @Override
-  public void visitLabels(AcceptsLabelAttribute observer) {
-    super.visitLabels(observer);
-    for (String attrName : getAttributeNames()) {
-      Attribute attribute = getAttributeDefinition(attrName);
-      Type.SelectorList<?> selectorList = getSelectorList(attrName, attribute.getType());
-      if (selectorList != null) {
-        for (Label configLabel : selectorList.getKeyLabels()) {
-          observer.acceptLabelAttribute(configLabel, attribute);
+  protected void visitLabels(Attribute attribute, AcceptsLabelAttribute observer) {
+    Type<?> type = attribute.getType();
+    Type.SelectorList<?> selectorList = getSelectorList(attribute.getName(), type);
+    if (selectorList == null) {
+      if (getComputedDefault(attribute.getName(), attribute.getType()) != null) {
+        // Computed defaults are a special pain: we have no choice but to iterate through their
+        // (computed) values and look for labels.
+        for (Object value : visitAttribute(attribute.getName(), attribute.getType())) {
+          if (value != null) {
+            for (Label label : type.getLabels(value)) {
+              observer.acceptLabelAttribute(label, attribute);
+            }
+          }
+        }
+      } else {
+        super.visitLabels(attribute, observer);
+      }
+    } else {
+      for (Type.Selector<?> selector : selectorList.getSelectors()) {
+        for (Map.Entry<Label, ?> selectorEntry : selector.getEntries().entrySet()) {
+          if (!Type.Selector.isReservedLabel(selectorEntry.getKey())) {
+            observer.acceptLabelAttribute(selectorEntry.getKey(), attribute);
+          }
+          for (Label value : type.getLabels(selectorEntry.getValue())) {
+            observer.acceptLabelAttribute(value, attribute);
+          }
         }
       }
     }
   }
 
   /**
+   * Returns all labels reachable via the given attribute.
+   *
+   * <p>Use this over {@link #visitAttribute} when this is sufficient, since the latter grows
+   * exponentially with respect to the number of selects in the attribute.
+   */
+  public Iterable<Label> getReachableLabels(String attributeName) {
+    final ImmutableList.Builder<Label> builder = ImmutableList.builder();
+    visitLabels(getAttributeDefinition(attributeName), new AcceptsLabelAttribute() {
+      @Override
+      public void acceptLabelAttribute(Label label, Attribute attribute) {
+        builder.add(label);
+      }
+    });
+    return builder.build();
+  }
+
+  /**
    * Returns a list of all possible values an attribute can take for this rule.
+   *
+   * <p>Note that when an attribute uses multiple selects, it can potentially take on many
+   * values. So be cautious about unnecessarily relying on this method.
    */
-  @Override
   public <T> Iterable<T> visitAttribute(String attributeName, Type<T> type) {
     // If this attribute value is configurable, visit all possible values.
     Type.SelectorList<T> selectorList = getSelectorList(attributeName, type);
diff --git a/src/main/java/com/google/devtools/build/lib/packages/NonconfigurableAttributeMapper.java b/src/main/java/com/google/devtools/build/lib/packages/NonconfigurableAttributeMapper.java
index 03d778bafd..ff515f3be6 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/NonconfigurableAttributeMapper.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/NonconfigurableAttributeMapper.java
@@ -14,7 +14,6 @@
 package com.google.devtools.build.lib.packages;
 
 import com.google.common.base.Preconditions;
-import com.google.common.collect.ImmutableList;
 
 /**
  * {@link AttributeMap} implementation that triggers an {@link IllegalStateException} if called
@@ -47,10 +46,4 @@ public class NonconfigurableAttributeMapper extends AbstractAttributeMapper {
         "Attribute '%s' is potentially configurable - not allowed here", attributeName);
     return super.get(attributeName, type);
   }
-
-  @Override
-  protected <T> Iterable<T> visitAttribute(String attributeName, Type<T> type) {
-    T value = get(attributeName, type);
-    return value == null ? ImmutableList.<T>of() : ImmutableList.of(value);
-  }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/packages/RawAttributeMapper.java b/src/main/java/com/google/devtools/build/lib/packages/RawAttributeMapper.java
index 64c22bf35d..84b8fac051 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/RawAttributeMapper.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/RawAttributeMapper.java
@@ -40,12 +40,6 @@ public class RawAttributeMapper extends AbstractAttributeMapper {
         rule.getAttributeContainer());
   }
 
-  @Override
-  protected <T> Iterable<T> visitAttribute(String attributeName, Type<T> type) {
-    T value = get(attributeName, type);
-    return value == null ? ImmutableList.<T>of() : ImmutableList.of(value);
-  }
-
   /**
    * Variation of {@link #get} that merges the values of configurable lists together (with
    * duplicates removed).