Skylark: Replaced late bound attributes with computed default attributes

Motivation:

Compared to computed default attributes, late bound attributes are
evaluated in a later phase (analysis instead of loading phase). While
both mechanisms provide access to other attributes of a rule, only
late bound attributes additionally provide access to the build
configuration.

However, late bound attributes could be used to return new labels that
have not been loaded before. Since this happens in the analysis phase,
it can break one of Blaze's underlying principles, thus introducing a
serious correctness bug.

We decided to replace late bound attributes in Skylark with computed
default attributes since this moves the evaluation of values into the
loading phase, thus fixing this bug. Moreover, none of the existing
users of this mechanism required access to the build configuration,
which means that the user impact of this change is minimal.

Implementation details:

Unlike attributes of non-Skylark rules, however, Skylark computed
defaults need to be able to depend on more than two configurable
attributes since all attributes of Skylark rules are configurable by
default. This has two implications:

1. Unlike "normal" Skylark attributes, Skylark computed defaults need
to know about the existence of other attributes in order to declare a
dependency on them. This CL takes advantage of work previously done to
require parameter names to be the names of attributes used by the
computed default function.

2. Since Bazel computes the combinations of all possible attribute
values, a Skylark rule with a computed default that depends on a lot
of configurable attributes could crash Bazel. Consequently, this CL
also introduces an upper bound (64) for the number of valid
combinations.

Caveats:

1. Getting the depended-on attributes' names from function paramters is
mildly surprising.

Alternatives:

The best solution would be to keep SkylarkLateBound, but restrict it in
a way that it can only return already loaded labels. This is not
possible right now.

--
MOS_MIGRATED_REVID=131967238

2 files changed, 12 insertions, 14 deletions

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
index 9f4514cd94..1426ea58fc 100644
--- 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
@@ -858,12 +858,11 @@ public abstract class BuildViewTestCase extends FoundationTestCase {
       getTarget(target);
       fail(
           String.format(
-              "checkLoadingPhaseError(): expected an error with '%s' when loading target '%s'.",
-              expectedErrorMessage,
-              target));
-    } catch (Exception e) {
-      assertContainsEvent(expectedErrorMessage);
+              "checkLoadingPhaseError(): expected an exception with '%s' when loading target '%s'.",
+              expectedErrorMessage, target));
+    } catch (Exception expected) {
     }
+    assertContainsEvent(expectedErrorMessage);
   }
 
   /**
diff --git a/src/test/java/com/google/devtools/build/lib/packages/RuleClassTest.java b/src/test/java/com/google/devtools/build/lib/packages/RuleClassTest.java
index 24965818f0..03fbd31bda 100644
--- a/src/test/java/com/google/devtools/build/lib/packages/RuleClassTest.java
+++ b/src/test/java/com/google/devtools/build/lib/packages/RuleClassTest.java
@@ -48,6 +48,7 @@ import com.google.devtools.build.lib.events.EventCollector;
 import com.google.devtools.build.lib.events.EventKind;
 import com.google.devtools.build.lib.events.Location;
 import com.google.devtools.build.lib.events.Location.LineAndColumn;
+import com.google.devtools.build.lib.packages.Attribute.SkylarkComputedDefaultTemplate.CannotPrecomputeDefaultsException;
 import com.google.devtools.build.lib.packages.Attribute.ValidityPredicate;
 import com.google.devtools.build.lib.packages.ConfigurationFragmentPolicy.MissingFragmentPolicy;
 import com.google.devtools.build.lib.packages.RuleClass.Builder.RuleClassType;
@@ -59,12 +60,6 @@ import com.google.devtools.build.lib.syntax.BaseFunction;
 import com.google.devtools.build.lib.syntax.Environment;
 import com.google.devtools.build.lib.syntax.Type;
 import com.google.devtools.build.lib.vfs.Path;
-
-import org.junit.Before;
-import org.junit.Test;
-import org.junit.runner.RunWith;
-import org.junit.runners.JUnit4;
-
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collections;
@@ -75,8 +70,11 @@ import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
-
 import javax.annotation.Nullable;
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.JUnit4;
 
 /**
  * Tests for {@link RuleClass}.
@@ -772,8 +770,9 @@ public class RuleClassTest extends PackageLoadingTestCase {
                  attributes.get("my-sorted-stringlist-attr", Type.STRING_LIST));
   }
 
-  private Rule createRule(RuleClass ruleClass, String name, Map<String, Object> attributeValues,
-      Location location) throws LabelSyntaxException, InterruptedException {
+  private Rule createRule(
+      RuleClass ruleClass, String name, Map<String, Object> attributeValues, Location location)
+      throws LabelSyntaxException, InterruptedException, CannotPrecomputeDefaultsException {
     Package.Builder pkgBuilder = createDummyPackageBuilder();
     Label ruleLabel;
     try {