From c82168eea66609bd0fc3acbc02e837f6a65c3b1b Mon Sep 17 00:00:00 2001
From: Googler <jhorvitz@google.com>
Date: Wed, 22 Mar 2023 17:50:33 -0700
Subject: [PATCH] Avoid storing `LateBoundDefault` attribute values in `Rule`.

The change that produces the desired effect is to compare the attribute value with `Attribute#getDefaultValueUnchecked`. The unchecked method returns `LateBoundDefault`, not its default value. This allows the optimization added in https://github.com/bazelbuild/bazel/commit/1d03d53b56f90d26818186158fd31d41f67c2508 to also work for `LateBoundDefault`.

Rename `getRawAttrValue` to `getAttrIfStored` to better reflect that it does not fall back to the default value.

PiperOrigin-RevId: 518720504
Change-Id: I3141a1d731aaede8ff81b46ebc819cdb6af0fa0a
---
 .../packages/AggregatingAttributeMapper.java  |   4 +-
 .../devtools/build/lib/packages/Rule.java     |  20 ++--
 .../packages/RuleAttributeStorageTest.java    | 102 ++++++++++++------
 3 files changed, 86 insertions(+), 40 deletions(-)

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 96d5ab52919b62..39cf61b8c88790 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
@@ -115,7 +115,7 @@ private <T> void visitLabels(
     if (type.getLabelClass() == LabelClass.NONE) {
       // The only way for LabelClass.NONE to contain labels is in select keys.
       if (includeSelectKeys && attr.isConfigurable()) {
-        rawVal = rule.getRawAttrValue(i);
+        rawVal = rule.getAttrIfStored(i);
         if (rawVal instanceof SelectorList) {
           visitLabelsInSelect(
               (SelectorList<T>) rawVal,
@@ -128,7 +128,7 @@ private <T> void visitLabels(
       }
       return;
     }
-    rawVal = rule.getRawAttrValue(i);
+    rawVal = rule.getAttrIfStored(i);
     if (rawVal == null) {
       // Frozen rules don't store computed defaults.
       if (!attr.hasComputedDefault() || rule.isFrozen()) {
diff --git a/src/main/java/com/google/devtools/build/lib/packages/Rule.java b/src/main/java/com/google/devtools/build/lib/packages/Rule.java
index a931906337f734..ea4a8e307f9021 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/Rule.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/Rule.java
@@ -505,7 +505,7 @@ public <T> Object getAttr(String attrName, Type<T> type) {
    */
   @Nullable
   private Object getAttrWithIndex(int attrIndex) {
-    Object value = getRawAttrValue(attrIndex);
+    Object value = getAttrIfStored(attrIndex);
     if (value != null) {
       return value;
     }
@@ -519,6 +519,11 @@ private Object getAttrWithIndex(int attrIndex) {
       // compute the value.
       return isFrozen() ? attr.getDefaultValue() : null;
     }
+    if (attr.isLateBound()) {
+      // Frozen rules don't store late bound defaults.
+      checkState(isFrozen(), "Mutable rule missing LateBoundDefault");
+      return attr.getLateBoundDefault();
+    }
     switch (attr.getName()) {
       case GENERATOR_FUNCTION:
         return callstack.size() > 1 ? callstack.getFrame(1).name : "";
@@ -536,7 +541,7 @@ private Object getAttrWithIndex(int attrIndex) {
    * <p>Unlike {@link #getAttr}, does not fall back to the default value.
    */
   @Nullable
-  Object getRawAttrValue(int attrIndex) {
+  Object getAttrIfStored(int attrIndex) {
     checkPositionIndex(attrIndex, attrCount() - 1);
     switch (getAttrState()) {
       case MUTABLE:
@@ -642,8 +647,9 @@ private void checkAttrType(String attrName, Type<?> requestedType, Attribute att
    * Returns {@code true} if this rule's attributes are immutable.
    *
    * <p>Frozen rules optimize for space by omitting storage for non-explicit attribute values that
-   * match the {@link Attribute} default. If {@link #getRawAttrValue} returns {@code null}, the
-   * value should be taken from {@link Attribute#getDefaultValue}, even for computed defaults.
+   * match the {@link Attribute} default. If {@link #getAttrIfStored} returns {@code null}, the
+   * value should be taken from either {@link Attribute#getLateBoundDefault} for late-bound defaults
+   * or {@link Attribute#getDefaultValue} for all other attributes (including computed defaults).
    *
    * <p>Mutable rules have no such optimization. During rule creation, this allows for
    * distinguishing whether a computed default (which may depend on other unset attributes) is
@@ -667,7 +673,7 @@ void freeze() {
       }
       if (!getExplicitBit(i)) {
         Attribute attr = ruleClass.getAttribute(i);
-        if (value.equals(attr.getDefaultValue())) {
+        if (value.equals(attr.getDefaultValueUnchecked())) {
           // Non-explicit value matches the attribute's default. Save space by omitting storage.
           continue;
         }
@@ -774,7 +780,7 @@ public <T> BuildType.SelectorList<T> getSelectorList(String attributeName, Type<
     if (index == null) {
       return null;
     }
-    Object attrValue = getRawAttrValue(index);
+    Object attrValue = getAttrIfStored(index);
     if (!(attrValue instanceof BuildType.SelectorList)) {
       return null;
     }
@@ -1108,7 +1114,7 @@ private List<Label> getRawVisibilityLabels() {
     if (visibilityIndex == null) {
       return null;
     }
-    return (List<Label>) getRawAttrValue(visibilityIndex);
+    return (List<Label>) getAttrIfStored(visibilityIndex);
   }
 
   private RuleVisibility getDefaultVisibility() {
diff --git a/src/test/java/com/google/devtools/build/lib/packages/RuleAttributeStorageTest.java b/src/test/java/com/google/devtools/build/lib/packages/RuleAttributeStorageTest.java
index 8dbf0ec064025e..b0eae0302fdab1 100644
--- a/src/test/java/com/google/devtools/build/lib/packages/RuleAttributeStorageTest.java
+++ b/src/test/java/com/google/devtools/build/lib/packages/RuleAttributeStorageTest.java
@@ -23,6 +23,7 @@
 import com.google.devtools.build.lib.analysis.util.MockRule;
 import com.google.devtools.build.lib.analysis.util.MockRuleDefaults;
 import com.google.devtools.build.lib.packages.Attribute.ComputedDefault;
+import com.google.devtools.build.lib.packages.Attribute.LateBoundDefault;
 import com.google.devtools.build.lib.testutil.TestRuleClassProvider;
 import com.google.testing.junit.testparameterinjector.TestParameter;
 import com.google.testing.junit.testparameterinjector.TestParameterInjector;
@@ -39,6 +40,7 @@ public final class RuleAttributeStorageTest extends BuildViewTestCase {
 
   private static final String STRING_DEFAULT = Type.STRING.getDefaultValue();
   private static final int COMPUTED_DEFAULT_OFFSET = 1;
+  private static final int LATE_BOUND_DEFAULT_OFFSET = 2;
 
   private enum ContainerSize {
     SMALL(16),
@@ -60,6 +62,8 @@ private enum ContainerSize {
   private Attribute lastCustomAttr;
   private int computedDefaultIndex;
   private Attribute computedDefaultAttr;
+  private int lateBoundDefaultIndex;
+  private Attribute lateBoundDefaultAttr;
 
   @Override
   protected ConfiguredRuleClassProvider createRuleClassProvider() {
@@ -72,18 +76,29 @@ protected ConfiguredRuleClassProvider createRuleClassProvider() {
                 IntStream.range(0, numCustomAttrs)
                     .mapToObj(
                         i -> {
-                          var attr = attr("attr" + i, Type.STRING);
-                          // Make one of the attributes a computed default.
+                          // Make one attribute a computed default and one a late bound default.
                           if (i == COMPUTED_DEFAULT_OFFSET) {
-                            attr.value(
-                                new ComputedDefault() {
-                                  @Override
-                                  public Object getDefault(AttributeMap rule) {
-                                    return "computed";
-                                  }
-                                });
+                            return attr("attr" + i + "_computed_default", Type.STRING)
+                                .value(
+                                    new ComputedDefault() {
+                                      @Override
+                                      public Object getDefault(AttributeMap rule) {
+                                        return "computed";
+                                      }
+                                    });
                           }
-                          return attr;
+                          if (i == LATE_BOUND_DEFAULT_OFFSET) {
+                            return attr(":attr" + i + "_late_bound_default", Type.STRING)
+                                .value(
+                                    new LateBoundDefault<>(Void.class, "late_bound") {
+                                      @Override
+                                      public String resolve(
+                                          Rule rule, AttributeMap attributes, Void input) {
+                                        return "late_bound";
+                                      }
+                                    });
+                          }
+                          return attr("attr" + i, Type.STRING);
                         })
                     .toArray(Attribute.Builder[]::new));
     var builder = new ConfiguredRuleClassProvider.Builder().addRuleDefinition(exampleRule);
@@ -111,6 +126,8 @@ public void setUpForRule() throws Exception {
     lastCustomAttr = attrAt(lastCustomAttrIndex);
     computedDefaultIndex = firstCustomAttrIndex + COMPUTED_DEFAULT_OFFSET;
     computedDefaultAttr = attrAt(computedDefaultIndex);
+    lateBoundDefaultIndex = firstCustomAttrIndex + LATE_BOUND_DEFAULT_OFFSET;
+    lateBoundDefaultAttr = attrAt(lateBoundDefaultIndex);
   }
 
   @Test
@@ -122,9 +139,9 @@ public void attributeSettingAndRetrieval(@TestParameter boolean frozen) {
       rule.freeze();
     }
 
-    assertThat(rule.getRawAttrValue(firstCustomAttrIndex)).isEqualTo("val1");
+    assertThat(rule.getAttrIfStored(firstCustomAttrIndex)).isEqualTo("val1");
     assertThat(rule.isAttributeValueExplicitlySpecified(firstCustomAttr)).isTrue();
-    assertThat(rule.getRawAttrValue(lastCustomAttrIndex)).isEqualTo("val2");
+    assertThat(rule.getAttrIfStored(lastCustomAttrIndex)).isEqualTo("val2");
     assertThat(rule.isAttributeValueExplicitlySpecified(lastCustomAttr)).isTrue();
   }
 
@@ -134,7 +151,7 @@ public void indexOutOfBounds_throws(@TestParameter boolean frozen) {
       rule.freeze();
     }
     assertThrows(
-        IndexOutOfBoundsException.class, () -> rule.getRawAttrValue(lastCustomAttrIndex + 1));
+        IndexOutOfBoundsException.class, () -> rule.getAttrIfStored(lastCustomAttrIndex + 1));
   }
 
   @Test
@@ -146,10 +163,10 @@ public void testForOffByOneError(@TestParameter boolean frozen) {
       rule.freeze();
     }
 
-    assertThat(rule.getRawAttrValue(firstCustomAttrIndex - 1)).isNull();
+    assertThat(rule.getAttrIfStored(firstCustomAttrIndex - 1)).isNull();
     assertThat(rule.isAttributeValueExplicitlySpecified(attrAt(firstCustomAttrIndex - 1)))
         .isFalse();
-    assertThat(rule.getRawAttrValue(firstCustomAttrIndex + 1)).isNull();
+    assertThat(rule.getAttrIfStored(firstCustomAttrIndex + 1)).isNull();
     assertThat(rule.isAttributeValueExplicitlySpecified(attrAt(firstCustomAttrIndex + 1)))
         .isFalse();
   }
@@ -166,9 +183,9 @@ public void testFreezeWorks() {
     // Double freezing is a no-op
     rule.freeze();
     // reads/explicit bits work as expected
-    assertThat(rule.getRawAttrValue(firstCustomAttrIndex)).isEqualTo("val1");
+    assertThat(rule.getAttrIfStored(firstCustomAttrIndex)).isEqualTo("val1");
     assertThat(rule.isAttributeValueExplicitlySpecified(firstCustomAttr)).isTrue();
-    assertThat(rule.getRawAttrValue(lastCustomAttrIndex)).isEqualTo("val2");
+    assertThat(rule.getAttrIfStored(lastCustomAttrIndex)).isEqualTo("val2");
     assertThat(rule.isAttributeValueExplicitlySpecified(lastCustomAttr)).isFalse();
     // writes no longer work.
     assertThrows(
@@ -189,7 +206,7 @@ public void allAttributesSet(@TestParameter boolean frozen) {
     }
 
     for (int i = 1; i < size; i++) { // Skip attribute 0 (name) which is never stored.
-      assertThat(rule.getRawAttrValue(i)).isEqualTo("value " + i);
+      assertThat(rule.getAttrIfStored(i)).isEqualTo("value " + i);
       assertWithMessage("attribute " + i)
           .that(rule.isAttributeValueExplicitlySpecified(attrAt(i)))
           .isEqualTo(i % 2 == 0);
@@ -235,7 +252,7 @@ public void boundaryOfFrozenContainer() {
 
     assertThat(rule.getAttr("name")).isEqualTo(ruleName);
     assertThat(rule.isAttributeValueExplicitlySpecified("name")).isTrue();
-    assertThat(rule.getRawAttrValue(lastCustomAttrIndex)).isEqualTo("last");
+    assertThat(rule.getAttrIfStored(lastCustomAttrIndex)).isEqualTo("last");
     assertThat(rule.isAttributeValueExplicitlySpecified(lastCustomAttr)).isTrue();
   }
 
@@ -248,7 +265,7 @@ public void nameNotStoredAsRawAttr(@TestParameter boolean frozen) {
       rule.freeze();
     }
 
-    assertThat(rule.getRawAttrValue(0)).isNull();
+    assertThat(rule.getAttrIfStored(0)).isNull();
     assertThat(rule.getRawAttrValues()).doesNotContain(ruleName);
     assertThat(rule.getAttr("name")).isEqualTo(ruleName);
     assertThat(rule.isAttributeValueExplicitlySpecified("name")).isTrue();
@@ -262,7 +279,7 @@ public void explicitDefaultValue_stored(@TestParameter boolean frozen) {
       rule.freeze();
     }
 
-    assertThat(rule.getRawAttrValue(firstCustomAttrIndex)).isNotNull();
+    assertThat(rule.getAttrIfStored(firstCustomAttrIndex)).isNotNull();
     assertThat(rule.isAttributeValueExplicitlySpecified(firstCustomAttr)).isTrue();
   }
 
@@ -270,7 +287,7 @@ public void explicitDefaultValue_stored(@TestParameter boolean frozen) {
   public void nonExplicitDefaultValue_mutable_stored() {
     rule.setAttributeValue(firstCustomAttr, STRING_DEFAULT, /* explicit= */ false);
 
-    assertThat(rule.getRawAttrValue(firstCustomAttrIndex)).isNotNull();
+    assertThat(rule.getAttrIfStored(firstCustomAttrIndex)).isNotNull();
     assertThat(rule.isAttributeValueExplicitlySpecified(firstCustomAttr)).isFalse();
   }
 
@@ -280,37 +297,60 @@ public void nonExplicitDefaultValue_frozen_notStored() {
 
     rule.freeze();
 
-    assertThat(rule.getRawAttrValue(firstCustomAttrIndex)).isNull();
+    assertThat(rule.getAttrIfStored(firstCustomAttrIndex)).isNull();
     assertThat(rule.isAttributeValueExplicitlySpecified(firstCustomAttr)).isFalse();
   }
 
   @Test
   public void computedDefault_mutable_stored() {
-    Attribute attr = rule.getRuleClassObject().getAttribute(computedDefaultIndex);
-    var computedDefault = attr.getDefaultValue();
-    assertThat(attr.hasComputedDefault()).isTrue();
+    var computedDefault = computedDefaultAttr.getDefaultValue();
+    assertThat(computedDefaultAttr.hasComputedDefault()).isTrue();
     assertThat(computedDefault).isInstanceOf(ComputedDefault.class);
 
     rule.setAttributeValue(computedDefaultAttr, computedDefault, /* explicit= */ false);
 
-    assertThat(rule.getRawAttrValue(computedDefaultIndex)).isEqualTo(computedDefault);
+    assertThat(rule.getAttrIfStored(computedDefaultIndex)).isEqualTo(computedDefault);
+    assertThat(rule.getAttr(computedDefaultAttr.getName())).isEqualTo(computedDefault);
     assertThat(rule.isAttributeValueExplicitlySpecified(computedDefaultAttr)).isFalse();
   }
 
   @Test
   public void computedDefault_frozen_notStored() {
-    Attribute attr = rule.getRuleClassObject().getAttribute(computedDefaultIndex);
-    var computedDefault = attr.getDefaultValue();
-    assertThat(attr.hasComputedDefault()).isTrue();
+    var computedDefault = computedDefaultAttr.getDefaultValue();
+    assertThat(computedDefaultAttr.hasComputedDefault()).isTrue();
     assertThat(computedDefault).isInstanceOf(ComputedDefault.class);
 
     rule.setAttributeValue(computedDefaultAttr, computedDefault, /* explicit= */ false);
     rule.freeze();
 
-    assertThat(rule.getRawAttrValue(computedDefaultIndex)).isNull();
+    assertThat(rule.getAttrIfStored(computedDefaultIndex)).isNull();
+    assertThat(rule.getAttr(computedDefaultAttr.getName())).isEqualTo(computedDefault);
     assertThat(rule.isAttributeValueExplicitlySpecified(computedDefaultAttr)).isFalse();
   }
 
+  @Test
+  public void lateBoundDefault_mutable_stored() {
+    var lateBoundDefault = lateBoundDefaultAttr.getLateBoundDefault();
+
+    rule.setAttributeValue(lateBoundDefaultAttr, lateBoundDefault, /* explicit= */ false);
+
+    assertThat(rule.getAttrIfStored(lateBoundDefaultIndex)).isEqualTo(lateBoundDefault);
+    assertThat(rule.getAttr(lateBoundDefaultAttr.getName())).isEqualTo(lateBoundDefault);
+    assertThat(rule.isAttributeValueExplicitlySpecified(lateBoundDefaultAttr)).isFalse();
+  }
+
+  @Test
+  public void lateBoundDefault_frozen_notStored() {
+    var lateBoundDefault = lateBoundDefaultAttr.getLateBoundDefault();
+
+    rule.setAttributeValue(lateBoundDefaultAttr, lateBoundDefault, /* explicit= */ false);
+    rule.freeze();
+
+    assertThat(rule.getAttrIfStored(lateBoundDefaultIndex)).isNull();
+    assertThat(rule.getAttr(lateBoundDefaultAttr.getName())).isEqualTo(lateBoundDefault);
+    assertThat(rule.isAttributeValueExplicitlySpecified(lateBoundDefaultAttr)).isFalse();
+  }
+
   private Attribute attrAt(int attrIndex) {
     return rule.getRuleClassObject().getAttribute(attrIndex);
   }