From 19db71413329da3f5d22b5fc7681471f3d971d88 Mon Sep 17 00:00:00 2001 From: Mark Schaller Date: Thu, 1 Sep 2016 17:58:02 +0000 Subject: 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 --- .../build/lib/analysis/util/BuildViewTestCase.java | 9 ++++----- .../devtools/build/lib/packages/RuleClassTest.java | 17 ++++++++--------- 2 files changed, 12 insertions(+), 14 deletions(-) (limited to 'src/test/java/com/google') 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 attributeValues, - Location location) throws LabelSyntaxException, InterruptedException { + private Rule createRule( + RuleClass ruleClass, String name, Map attributeValues, Location location) + throws LabelSyntaxException, InterruptedException, CannotPrecomputeDefaultsException { Package.Builder pkgBuilder = createDummyPackageBuilder(); Label ruleLabel; try { -- cgit v1.2.3