bazel packages: prep for AttributeContainer optimization
Details:
- RuleFactory:
- don't plumb AttributeContainer through create{RuleUnchecked,AndAddRule}.
- avoid overloading of createAndAddRule.
- inline and simplify setRuleAttributeValue
(only one of two callers wants 'visibility' logic)
- inline and simplify getAttributeNoncomputedDefaultValue
(check both name and type like we do for licenses)
- AttributeContainer:
- inline setAttributeValueByName into sole use (for $implicit_test).
- add TODO comments.
- make isAttributeValueExplicitlySpecified private
PiperOrigin-RevId: 318841494
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleClassFunctions.java b/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleClassFunctions.java
index 3a68926..5baf3db 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleClassFunctions.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleClassFunctions.java
@@ -48,7 +48,6 @@
import com.google.devtools.build.lib.cmdline.RepositoryName;
import com.google.devtools.build.lib.packages.Attribute;
import com.google.devtools.build.lib.packages.Attribute.StarlarkComputedDefaultTemplate;
-import com.google.devtools.build.lib.packages.AttributeContainer;
import com.google.devtools.build.lib.packages.AttributeMap;
import com.google.devtools.build.lib.packages.AttributeValueSource;
import com.google.devtools.build.lib.packages.BazelModuleContext;
@@ -679,12 +678,7 @@
+ "Rules may be instantiated only in a BUILD thread.");
}
RuleFactory.createAndAddRule(
- pkgContext,
- ruleClass,
- attributeValues,
- thread.getSemantics(),
- thread.getCallStack(),
- new AttributeContainer(ruleClass));
+ pkgContext, ruleClass, attributeValues, thread.getSemantics(), thread.getCallStack());
} catch (InvalidRuleException | NameConflictException e) {
throw new EvalException(null, e.getMessage());
}
diff --git a/src/main/java/com/google/devtools/build/lib/packages/AttributeContainer.java b/src/main/java/com/google/devtools/build/lib/packages/AttributeContainer.java
index 9aeddc8..f11a540 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/AttributeContainer.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/AttributeContainer.java
@@ -27,23 +27,27 @@
* be a robust public interface, but rather just an input to {@link AttributeMap} instances. Use
* those instances for all domain-level attribute access.
*/
+// TODO(adonovan): eliminate this class. 99% of all external calls to its methods are of the form
+// rule.getAttributeContainer().foo(), so it's just needless indirection for the caller, and
+// needless indirection in the representation. Instead, make Rule implement an interface with the
+// two methods getAttr and isAttributeValueExplicitlySpecified; it already has those methods.
+// The only time the AttributeContainer needs to be distinct from the Rule itself is in the
+// WorkspaceFactory.setParent hack. Perhaps we can eliminate that?
public final class AttributeContainer {
private final RuleClass ruleClass;
- // Attribute values, keyed by attribute index:
+ // Attribute values, keyed by attribute index.
private final Object[] attributeValues;
- // Holds a list of attribute indices.
+ // Holds a list of attribute indices (+1, to make them nonzero).
// The first byte gives the length of the list.
// The list records which attributes were set explicitly in the BUILD file.
// The list may be padded with zeros at the end.
private byte[] state;
- /**
- * Create a container for a rule of the given rule class.
- */
- public AttributeContainer(RuleClass ruleClass) {
+ /** Creates a container for a rule of the given rule class. */
+ AttributeContainer(RuleClass ruleClass) {
int n = ruleClass.getAttributeCount();
if (n > 254) {
// We reserve the zero byte as a hole/sentinel inside state[].
@@ -66,10 +70,8 @@
return idx != null ? attributeValues[idx] : null;
}
- /**
- * See {@link #isAttributeValueExplicitlySpecified(String)}.
- */
- public boolean isAttributeValueExplicitlySpecified(Attribute attribute) {
+ /** See {@link #isAttributeValueExplicitlySpecified(String)}. */
+ boolean isAttributeValueExplicitlySpecified(Attribute attribute) {
return isAttributeValueExplicitlySpecified(attribute.getName());
}
@@ -144,12 +146,4 @@
setExplicit(index);
}
}
-
- // This sets the attribute "explicitly" as if it came from the BUILD file.
- // At present, the sole use of this is for the test_suite.$implicit_tests
- // attribute, which is synthesized during package loading. We do want to
- // consider that "explicitly set" so that it appears in query output.
- void setAttributeValueByName(String attrName, Object value) {
- setAttributeValue(ruleClass.getAttributeByName(attrName), value, true);
- }
}
diff --git a/src/main/java/com/google/devtools/build/lib/packages/Package.java b/src/main/java/com/google/devtools/build/lib/packages/Package.java
index c52beb8..646d068 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/Package.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/Package.java
@@ -1232,15 +1232,15 @@
* Creates a new {@link Rule} {@code r} where {@code r.getPackage()} is the {@link Package}
* associated with this {@link Builder}.
*
- * <p>The created {@link Rule} will have no attribute values, no output files, and therefore
- * will be in an invalid state.
+ * <p>The created {@link Rule} will have no output files and therefore will be in an invalid
+ * state.
*/
Rule createRule(
Label label,
RuleClass ruleClass,
Location location,
List<StarlarkThread.CallStackEntry> callstack,
- AttributeContainer attributeContainer) {
+ AttributeContainer attributeContainer) { // required by WorkspaceFactory.setParent hack
return new Rule(
pkg, label, ruleClass, location, callStackBuilder.of(callstack), attributeContainer);
}
@@ -1255,7 +1255,6 @@
RuleClass ruleClass,
Location location,
List<StarlarkThread.CallStackEntry> callstack,
- AttributeContainer attributeContainer,
ImplicitOutputsFunction implicitOutputsFunction) {
return new Rule(
pkg,
@@ -1263,7 +1262,7 @@
ruleClass,
location,
callStackBuilder.of(callstack),
- attributeContainer,
+ new AttributeContainer(ruleClass),
implicitOutputsFunction);
}
@@ -1514,7 +1513,8 @@
}
// "test_suite" rules have the idiosyncratic semantics of implicitly
- // depending on all tests in the package, iff tests=[] and suites=[].
+ // depending on all tests in the package, iff tests=[] and suites=[],
+ // which is about 20% of >1M test_suite instances in Google's corpus.
// Note, we implement this here when the Package is fully constructed,
// since clearly this information isn't available at Rule construction
// time, as forward references are permitted.
@@ -1536,7 +1536,13 @@
if (!implicitTestSuiteRuleInstances.isEmpty()) {
Collections.sort(labelsOfTestTargets);
for (Rule rule : implicitTestSuiteRuleInstances) {
- rule.setAttributeValueByName("$implicit_tests", labelsOfTestTargets);
+ // Pretend the test_suite.$implicit_tests attribute
+ // (which is synthesized during package loading)
+ // is explicitly set so that it appears in query output.
+ rule.setAttributeValue(
+ rule.getRuleClassObject().getAttributeByName("$implicit_tests"),
+ labelsOfTestTargets,
+ /*explicit=*/ true);
}
}
return this;
diff --git a/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java b/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java
index 6d4df0f..2024fa8 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java
@@ -386,8 +386,7 @@
ruleClass,
new BuildLangTypedAttributeValuesMap(kwargs),
thread.getSemantics(),
- thread.getCallStack(),
- new AttributeContainer(ruleClass));
+ thread.getCallStack());
} catch (RuleFactory.InvalidRuleException | Package.NameConflictException e) {
throw new EvalException(null, e.getMessage());
}
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 406e28f..b89ffcf 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
@@ -127,10 +127,6 @@
attributes.setAttributeValue(attribute, value, explicit);
}
- void setAttributeValueByName(String attrName, Object value) {
- attributes.setAttributeValueByName(attrName, value);
- }
-
void setContainsErrors() {
this.containsErrors = true;
}
diff --git a/src/main/java/com/google/devtools/build/lib/packages/RuleClass.java b/src/main/java/com/google/devtools/build/lib/packages/RuleClass.java
index e83d4c4..aad0239 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/RuleClass.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/RuleClass.java
@@ -1784,11 +1784,11 @@
/**
* Returns the default function for determining the set of implicit outputs generated by a given
- * rule. If not otherwise specified, this will be the implementation used by {@link Rule}s
- * created with this {@link RuleClass}.
+ * rule. If not otherwise specified, this will be the implementation used by {@link Rule}s created
+ * with this {@link RuleClass}.
*
- * <p>Do not use this value to calculate implicit outputs for a rule, instead use
- * {@link Rule#getImplicitOutputsFunction()}.
+ * <p>Do not use this value to calculate implicit outputs for a rule, instead use {@link
+ * Rule#getImplicitOutputsFunction()}.
*
* <p>An implicit output is an OutputFile that automatically comes into existence when a rule of
* this class is declared, and whose name is derived from the name of the rule.
@@ -1796,7 +1796,7 @@
* <p>Implicit outputs are a widely-relied upon. All ".so", and "_deploy.jar" targets referenced
* in BUILD files are examples.
*/
- @VisibleForTesting
+ // (public for serialization)
public ImplicitOutputsFunction getDefaultImplicitOutputsFunction() {
return implicitOutputsFunction;
}
@@ -1984,10 +1984,10 @@
EventHandler eventHandler,
Location location,
List<StarlarkThread.CallStackEntry> callstack,
- AttributeContainer attributeContainer,
boolean checkThirdPartyRulesHaveLicenses)
throws LabelSyntaxException, InterruptedException, CannotPrecomputeDefaultsException {
- Rule rule = pkgBuilder.createRule(ruleLabel, this, location, callstack, attributeContainer);
+ Rule rule =
+ pkgBuilder.createRule(ruleLabel, this, location, callstack, new AttributeContainer(this));
populateRuleAttributeValues(rule, pkgBuilder, attributeValues, eventHandler);
checkAspectAllowedValues(rule, eventHandler);
rule.populateOutputFiles(eventHandler, pkgBuilder);
@@ -2022,12 +2022,10 @@
AttributeValues<T> attributeValues,
Location location,
List<StarlarkThread.CallStackEntry> callstack,
- AttributeContainer attributeContainer,
ImplicitOutputsFunction implicitOutputsFunction)
throws InterruptedException, CannotPrecomputeDefaultsException {
Rule rule =
- pkgBuilder.createRule(
- ruleLabel, this, location, callstack, attributeContainer, implicitOutputsFunction);
+ pkgBuilder.createRule(ruleLabel, this, location, callstack, implicitOutputsFunction);
populateRuleAttributeValues(rule, pkgBuilder, attributeValues, NullEventHandler.INSTANCE);
rule.populateOutputFilesUnchecked(NullEventHandler.INSTANCE, pkgBuilder);
return rule;
@@ -2046,6 +2044,37 @@
AttributeValues<T> attributeValues,
EventHandler eventHandler)
throws InterruptedException, CannotPrecomputeDefaultsException {
+
+ // TODO(b/157751486): opt: optimize attribute representation.
+ //
+ // Conceptually, the attribute values of a rule instance are a mapping
+ // from each attribute name to its value plus a boolean indicating
+ // whether it was set explicitly, equivalent to a table of triples:
+ //
+ // (Attribute attr, Object value, bool explicit)
+ //
+ // (Attributes may be represented by name, Attribute reference, or by
+ // a small integer, because the RuleClass provides constant-time
+ // conversions between all three.)
+ //
+ // Empirically, about half of all rule attribute values are equal to
+ // the default value trivially provided by the RuleClass.
+ // To save space, don't record these values; instead, record only the
+ // explicitly provided ones, plus those whose defaults were non-trivially
+ // computed. (Because of the latter category, we must record the
+ // 'explicit' boolean.)
+ //
+ // One possible representation would be an Object[] array of length
+ // n, for the values sorted by ascending index, and a byte[] of length
+ // n bytes + n bits, the bytes being the indices and the bits being
+ // the 'explicit' flags. Lookup would require binary search over
+ // the bytes.
+ //
+ // Currently, the attributes are queried even as they are under
+ // construction (see checkAllowedValues), but that check could be moved after table
+ // construction, like
+ // checkForDuplicateLabels.
+
BitSet definedAttrIndices =
populateDefinedRuleAttributeValues(
rule,
@@ -2096,7 +2125,7 @@
Attribute attr = getAttribute(attrIndex);
if (attributeName.equals("licenses") && ignoreLicenses) {
- setRuleAttributeValue(rule, eventHandler, attr, License.NO_LICENSE, /*explicit=*/ false);
+ rule.setAttributeValue(attr, License.NO_LICENSE, /*explicit=*/ false);
definedAttrIndices.set(attrIndex);
continue;
}
@@ -2115,8 +2144,25 @@
nativeAttributeValue = attributeValue;
}
+ // visibility is additionally recorded by rule.setVisibility.
+ if (attr.getName().equals("visibility")) {
+ @SuppressWarnings("unchecked")
+ List<Label> vis = (List<Label>) nativeAttributeValue;
+ if (!vis.isEmpty() && vis.get(0).equals(ConstantRuleVisibility.LEGACY_PUBLIC_LABEL)) {
+ rule.reportError(
+ rule.getLabel() + ": //visibility:legacy_public only allowed in package declaration",
+ eventHandler);
+ }
+ try {
+ rule.setVisibility(PackageUtils.getVisibility(rule.getLabel(), vis));
+ } catch (EvalException e) {
+ rule.reportError(rule.getLabel() + " " + e.getMessage(), eventHandler);
+ }
+ }
+
boolean explicit = attributeValues.isExplicitlySpecified(attributeAccessor);
- setRuleAttributeValue(rule, eventHandler, attr, nativeAttributeValue, explicit);
+ rule.setAttributeValue(attr, nativeAttributeValue, explicit);
+ checkAllowedValues(rule, attr, eventHandler);
definedAttrIndices.set(attrIndex);
}
return definedAttrIndices;
@@ -2152,19 +2198,38 @@
eventHandler);
}
- if (attr.getName().equals("licenses") && ignoreLicenses) {
- rule.setAttributeValue(attr, License.NO_LICENSE, /*explicit=*/ false);
- } else if (attr.hasComputedDefault()) {
+ // We must check both the name and the type of each attribute below in case a Starlark rule
+ // defines a licenses or distributions attribute of another type.
+
+ if (attr.hasComputedDefault()) {
// Note that it is necessary to set all non-computed default values before calling
// Attribute#getDefaultValue for computed default attributes. Computed default attributes
// may have a condition predicate (i.e. the predicate returned by Attribute#getCondition)
// that depends on non-computed default attribute values, and that condition predicate is
// evaluated by the call to Attribute#getDefaultValue.
attrsWithComputedDefaults.add(attr);
+
} else if (attr.isLateBound()) {
rule.setAttributeValue(attr, attr.getLateBoundDefault(), /*explicit=*/ false);
+
+ } else if (attr.getName().equals("applicable_licenses")
+ && attr.getType() == BuildType.LICENSE) {
+ // TODO(b/149505729): Determine the right semantics for someone trying to define their own
+ // attribute named applicable_licenses.
+ rule.setAttributeValue(
+ attr, pkgBuilder.getDefaultApplicableLicenses(), /*explicit=*/ false);
+
+ } else if (attr.getName().equals("licenses") && attr.getType() == BuildType.LICENSE) {
+ rule.setAttributeValue(
+ attr,
+ ignoreLicenses ? License.NO_LICENSE : pkgBuilder.getDefaultLicense(),
+ /*explicit=*/ false);
+
+ } else if (attr.getName().equals("distribs") && attr.getType() == BuildType.DISTRIBUTIONS) {
+ rule.setAttributeValue(attr, pkgBuilder.getDefaultDistribs(), /*explicit=*/ false);
+
} else {
- Object defaultValue = getAttributeNoncomputedDefaultValue(attr, pkgBuilder);
+ Object defaultValue = attr.getDefaultValue(/*rule=*/ null);
rule.setAttributeValue(attr, defaultValue, /*explicit=*/ false);
checkAllowedValues(rule, attr, eventHandler);
}
@@ -2339,69 +2404,6 @@
}
/**
- * Returns the default value for the specified rule attribute.
- *
- * <p>For most rule attributes, the default value is either explicitly specified
- * in the attribute, or implicitly based on the type of the attribute, except
- * for some special cases (e.g. "licenses", "distribs") where it comes from
- * some other source, such as state in the package.
- *
- * <p>Precondition: {@code !attr.hasComputedDefault()}. (Computed defaults are
- * evaluated in second pass.)
- */
- private static Object getAttributeNoncomputedDefaultValue(Attribute attr,
- Package.Builder pkgBuilder) {
- // TODO(b/149505729): Determine the right semantics for someone trying to define their own
- // attribute named applicable_licenses.
- if (attr.getName().equals("applicable_licenses")) {
- return pkgBuilder.getDefaultApplicableLicenses();
- }
- // Starlark rules may define their own "licenses" attributes with different types -
- // we shouldn't trigger the special "licenses" on those cases.
- if (attr.getName().equals("licenses") && attr.getType() == BuildType.LICENSE) {
- return pkgBuilder.getDefaultLicense();
- }
- if (attr.getName().equals("distribs")) {
- return pkgBuilder.getDefaultDistribs();
- }
- return attr.getDefaultValue(null);
- }
-
- /**
- * Sets the value of attribute {@code attr} in {@code rule} to the native value {@code
- * nativeAttrVal}, and sets the value's explicitness to {@code explicit}.
- *
- * <p>Handles the special case of the "visibility" attribute by also setting the rule's
- * visibility with {@link Rule#setVisibility}.
- *
- * <p>Checks that {@code nativeAttrVal} is an allowed value via {@link #checkAllowedValues}.
- */
- private static void setRuleAttributeValue(
- Rule rule,
- EventHandler eventHandler,
- Attribute attr,
- Object nativeAttrVal,
- boolean explicit) {
- if (attr.getName().equals("visibility")) {
- @SuppressWarnings("unchecked")
- List<Label> attrList = (List<Label>) nativeAttrVal;
- if (!attrList.isEmpty()
- && ConstantRuleVisibility.LEGACY_PUBLIC_LABEL.equals(attrList.get(0))) {
- rule.reportError(
- rule.getLabel() + ": //visibility:legacy_public only allowed in package declaration",
- eventHandler);
- }
- try {
- rule.setVisibility(PackageUtils.getVisibility(rule.getLabel(), attrList));
- } catch (EvalException e) {
- rule.reportError(rule.getLabel() + " " + e.getMessage(), eventHandler);
- }
- }
- rule.setAttributeValue(attr, nativeAttrVal, explicit);
- checkAllowedValues(rule, attr, eventHandler);
- }
-
- /**
* Converts the build-language-typed {@code buildLangValue} to a native value via {@link
* BuildType#selectableConvert}. Canonicalizes the value's order if it is a {@link List} type and
* {@code attr.isOrderIndependent()} returns {@code true}.
@@ -2465,7 +2467,6 @@
}
}
-
/**
* Verifies that the rule has a valid value for the attribute according to its allowed values.
*
diff --git a/src/main/java/com/google/devtools/build/lib/packages/RuleFactory.java b/src/main/java/com/google/devtools/build/lib/packages/RuleFactory.java
index b579056..0668a78 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/RuleFactory.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/RuleFactory.java
@@ -78,8 +78,7 @@
BuildLangTypedAttributeValuesMap attributeValues,
EventHandler eventHandler,
StarlarkSemantics semantics,
- ImmutableList<StarlarkThread.CallStackEntry> callstack,
- AttributeContainer attributeContainer)
+ ImmutableList<StarlarkThread.CallStackEntry> callstack)
throws InvalidRuleException, InterruptedException {
Preconditions.checkNotNull(ruleClass);
String ruleClassName = ruleClass.getName();
@@ -133,7 +132,6 @@
eventHandler,
generator.location, // see b/23974287 for rationale
callstack,
- attributeContainer,
checkThirdPartyLicenses);
} catch (LabelSyntaxException | CannotPrecomputeDefaultsException e) {
throw new RuleFactory.InvalidRuleException(ruleClass + " " + e.getMessage());
@@ -154,31 +152,22 @@
* creation
* @param semantics the Starlark semantics
* @param callstack the stack of active calls in the Starlark thread
- * @param attributeContainer the {@link AttributeContainer} the rule will contain
* @throws InvalidRuleException if the rule could not be constructed for any reason (e.g. no
* {@code name} attribute is defined)
* @throws NameConflictException if the rule's name or output files conflict with others in this
* package
* @throws InterruptedException if interrupted
*/
- static Rule createAndAddRule(
+ static Rule createAndAddRuleImpl(
Package.Builder pkgBuilder,
RuleClass ruleClass,
BuildLangTypedAttributeValuesMap attributeValues,
EventHandler eventHandler,
StarlarkSemantics semantics,
- ImmutableList<StarlarkThread.CallStackEntry> callstack,
- AttributeContainer attributeContainer)
+ ImmutableList<StarlarkThread.CallStackEntry> callstack)
throws InvalidRuleException, NameConflictException, InterruptedException {
Rule rule =
- createRule(
- pkgBuilder,
- ruleClass,
- attributeValues,
- eventHandler,
- semantics,
- callstack,
- attributeContainer);
+ createRule(pkgBuilder, ruleClass, attributeValues, eventHandler, semantics, callstack);
pkgBuilder.addRule(rule);
return rule;
}
@@ -195,7 +184,6 @@
* a map entry for each non-optional attribute of this class of rule.
* @param loc the location of the rule expression
* @param thread the lexical environment of the function call which declared this rule (optional)
- * @param attributeContainer the {@link AttributeContainer} the rule will contain
* @throws InvalidRuleException if the rule could not be constructed for any reason (e.g. no
* {@code name} attribute is defined)
* @throws NameConflictException if the rule's name or output files conflict with others in this
@@ -207,17 +195,10 @@
RuleClass ruleClass,
BuildLangTypedAttributeValuesMap attributeValues,
StarlarkSemantics semantics,
- ImmutableList<StarlarkThread.CallStackEntry> callstack,
- AttributeContainer attributeContainer)
+ ImmutableList<StarlarkThread.CallStackEntry> callstack)
throws InvalidRuleException, NameConflictException, InterruptedException {
- return createAndAddRule(
- context.pkgBuilder,
- ruleClass,
- attributeValues,
- context.eventHandler,
- semantics,
- callstack,
- attributeContainer);
+ return createAndAddRuleImpl(
+ context.pkgBuilder, ruleClass, attributeValues, context.eventHandler, semantics, callstack);
}
/**
diff --git a/src/main/java/com/google/devtools/build/lib/packages/WorkspaceFactoryHelper.java b/src/main/java/com/google/devtools/build/lib/packages/WorkspaceFactoryHelper.java
index 4c51fb4..f0682e7 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/WorkspaceFactoryHelper.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/WorkspaceFactoryHelper.java
@@ -47,28 +47,14 @@
StoredEventHandler eventHandler = new StoredEventHandler();
BuildLangTypedAttributeValuesMap attributeValues = new BuildLangTypedAttributeValuesMap(kwargs);
Rule rule =
- RuleFactory.createRule(
- pkg,
- ruleClass,
- attributeValues,
- eventHandler,
- semantics,
- callstack,
- new AttributeContainer(ruleClass));
+ RuleFactory.createRule(pkg, ruleClass, attributeValues, eventHandler, semantics, callstack);
pkg.addEvents(eventHandler.getEvents());
pkg.addPosts(eventHandler.getPosts());
overwriteRule(pkg, rule);
for (Map.Entry<String, Label> entry :
ruleClass.getExternalBindingsFunction().apply(rule).entrySet()) {
Label nameLabel = Label.parseAbsolute("//external:" + entry.getKey(), ImmutableMap.of());
- addBindRule(
- pkg,
- bindRuleClass,
- nameLabel,
- entry.getValue(),
- semantics,
- callstack,
- new AttributeContainer(bindRuleClass));
+ addBindRule(pkg, bindRuleClass, nameLabel, entry.getValue(), semantics, callstack);
}
return rule;
}
@@ -138,8 +124,7 @@
Label virtual,
Label actual,
StarlarkSemantics semantics,
- ImmutableList<StarlarkThread.CallStackEntry> callstack,
- AttributeContainer attributeContainer)
+ ImmutableList<StarlarkThread.CallStackEntry> callstack)
throws RuleFactory.InvalidRuleException, Package.NameConflictException, InterruptedException {
Map<String, Object> attributes = Maps.newHashMap();
@@ -153,8 +138,7 @@
BuildLangTypedAttributeValuesMap attributeValues =
new BuildLangTypedAttributeValuesMap(attributes);
Rule rule =
- RuleFactory.createRule(
- pkg, bindRuleClass, attributeValues, handler, semantics, callstack, attributeContainer);
+ RuleFactory.createRule(pkg, bindRuleClass, attributeValues, handler, semantics, callstack);
overwriteRule(pkg, rule);
rule.setVisibility(ConstantRuleVisibility.PUBLIC);
}
diff --git a/src/main/java/com/google/devtools/build/lib/packages/WorkspaceGlobals.java b/src/main/java/com/google/devtools/build/lib/packages/WorkspaceGlobals.java
index f76dd8a..47e344d 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/WorkspaceGlobals.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/WorkspaceGlobals.java
@@ -284,8 +284,7 @@
nameLabel,
actual == NONE ? null : Label.parseAbsolute((String) actual, ImmutableMap.of()),
thread.getSemantics(),
- thread.getCallStack(),
- new AttributeContainer(ruleClass));
+ thread.getCallStack());
} catch (InvalidRuleException | Package.NameConflictException | LabelSyntaxException e) {
throw Starlark.errorf("%s", e.getMessage());
}
diff --git a/src/test/java/com/google/devtools/build/lib/packages/AttributeContainerTest.java b/src/test/java/com/google/devtools/build/lib/packages/AttributeContainerTest.java
index a632f10..57b12e0 100644
--- a/src/test/java/com/google/devtools/build/lib/packages/AttributeContainerTest.java
+++ b/src/test/java/com/google/devtools/build/lib/packages/AttributeContainerTest.java
@@ -45,25 +45,17 @@
}
@Test
- public void testAttributeSettingAndRetrievalByName() throws Exception {
+ public void testAttributeSettingAndRetrieval() throws Exception {
Object someValue1 = new Object();
Object someValue2 = new Object();
- container.setAttributeValueByName(attribute1.getName(), someValue1);
- container.setAttributeValueByName(attribute2.getName(), someValue2);
+ container.setAttributeValue(attribute1, someValue1, /*explicit=*/ true);
+ container.setAttributeValue(attribute2, someValue2, /*explicit=*/ true);
assertThat(container.getAttr(attribute1.getName())).isEqualTo(someValue1);
assertThat(container.getAttr(attribute2.getName())).isEqualTo(someValue2);
assertThat(container.getAttr("nomatch")).isNull();
}
@Test
- public void testExplicitSpecificationsByName() throws Exception {
- // Name-based setters are automatically considered explicit.
- container.setAttributeValueByName(attribute1.getName(), new Object());
- assertThat(container.isAttributeValueExplicitlySpecified(attribute1)).isTrue();
- assertThat(container.isAttributeValueExplicitlySpecified("nomatch")).isFalse();
- }
-
- @Test
public void testExplicitSpecificationsByInstance() throws Exception {
Object someValue = new Object();
container.setAttributeValue(attribute1, someValue, true);
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 fc9477f..b50181e 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
@@ -800,7 +800,6 @@
reporter,
location,
callstack,
- new AttributeContainer(ruleClass),
/*checkThirdPartyRulesHaveLicenses=*/ true);
}
diff --git a/src/test/java/com/google/devtools/build/lib/packages/RuleFactoryTest.java b/src/test/java/com/google/devtools/build/lib/packages/RuleFactoryTest.java
index f89a2df..403b402 100644
--- a/src/test/java/com/google/devtools/build/lib/packages/RuleFactoryTest.java
+++ b/src/test/java/com/google/devtools/build/lib/packages/RuleFactoryTest.java
@@ -70,14 +70,13 @@
RuleClass ruleClass = provider.getRuleClassMap().get("cc_library");
Rule rule =
- RuleFactory.createAndAddRule(
+ RuleFactory.createAndAddRuleImpl(
pkgBuilder,
ruleClass,
new BuildLangTypedAttributeValuesMap(attributeValues),
new Reporter(new EventBus()),
StarlarkSemantics.DEFAULT,
- DUMMY_STACK,
- new AttributeContainer(ruleClass));
+ DUMMY_STACK);
assertThat(rule.getAssociatedRule()).isSameInstanceAs(rule);
@@ -128,14 +127,13 @@
RuleClass ruleClass = provider.getRuleClassMap().get("bind");
Rule rule =
- RuleFactory.createAndAddRule(
+ RuleFactory.createAndAddRuleImpl(
pkgBuilder,
ruleClass,
new BuildLangTypedAttributeValuesMap(attributeValues),
new Reporter(new EventBus()),
StarlarkSemantics.DEFAULT,
- DUMMY_STACK,
- new AttributeContainer(ruleClass));
+ DUMMY_STACK);
assertThat(rule.containsErrors()).isFalse();
}
@@ -157,14 +155,13 @@
assertThrows(
RuleFactory.InvalidRuleException.class,
() ->
- RuleFactory.createAndAddRule(
+ RuleFactory.createAndAddRuleImpl(
pkgBuilder,
ruleClass,
new BuildLangTypedAttributeValuesMap(attributeValues),
new Reporter(new EventBus()),
StarlarkSemantics.DEFAULT,
- DUMMY_STACK,
- new AttributeContainer(ruleClass)));
+ DUMMY_STACK));
assertThat(e).hasMessageThat().contains("must be in the WORKSPACE file");
}
@@ -186,14 +183,13 @@
assertThrows(
RuleFactory.InvalidRuleException.class,
() ->
- RuleFactory.createAndAddRule(
+ RuleFactory.createAndAddRuleImpl(
pkgBuilder,
ruleClass,
new BuildLangTypedAttributeValuesMap(attributeValues),
new Reporter(new EventBus()),
StarlarkSemantics.DEFAULT,
- DUMMY_STACK,
- new AttributeContainer(ruleClass)));
+ DUMMY_STACK));
assertThat(e).hasMessageThat().contains("cannot be in the WORKSPACE file");
}
@@ -227,14 +223,13 @@
assertThrows(
RuleFactory.InvalidRuleException.class,
() ->
- RuleFactory.createAndAddRule(
+ RuleFactory.createAndAddRuleImpl(
pkgBuilder,
ruleClass,
new BuildLangTypedAttributeValuesMap(attributeValues),
new Reporter(new EventBus()),
StarlarkSemantics.DEFAULT,
- DUMMY_STACK,
- new AttributeContainer(ruleClass)));
+ DUMMY_STACK));
assertWithMessage(e.getMessage())
.that(e.getMessage().contains("output file name can't be equal '.'"))
.isTrue();
