Cap the number of attributes per rule-class at 150
Intention is to prevent hard-crashes due to runaway RuleClass growth -
AttributeContainer has a 254 cutoff for number of attributes, when that's
hit we crash hard.
150 was chosen to be high enough that it's significantly unlikely to affect
any existing rule classes, and low enough to give us the flexibility to lift
the limit if we absolutely must without running into the AttributeContainer
wall.
RELNOTES: A maximum 150 attributes per RuleClass is enforced
PiperOrigin-RevId: 300348762
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 0d9156c..9b5504b 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
@@ -123,6 +123,13 @@
@AutoCodec
public class RuleClass {
+ /**
+ * Maximum attributes per RuleClass. Current value was chosen high enough to be considered a
+ * non-breaking change for reasonable use. It was also chosen to be low enough to give significant
+ * headroom before hitting {@link AttributeContainer}'s limits.
+ */
+ private static final int MAX_ATTRIBUTES = 150;
+
@AutoCodec
static final Function<? super Rule, Map<String, Label>> NO_EXTERNAL_BINDINGS =
Functions.<Map<String, Label>>constant(ImmutableMap.<String, Label>of());
@@ -775,6 +782,14 @@
public RuleClass build(String name, String key) {
Preconditions.checkArgument(this.name.isEmpty() || this.name.equals(name));
type.checkName(name);
+
+ Preconditions.checkArgument(
+ attributes.size() <= MAX_ATTRIBUTES,
+ "Rule class %s declared too many attributes (%s > %s)",
+ name,
+ attributes.size(),
+ MAX_ATTRIBUTES);
+
type.checkAttributes(attributes);
Preconditions.checkState(
(type == RuleClassType.ABSTRACT)
@@ -1123,10 +1138,10 @@
}
/**
- * Builds attribute from the attribute builder and adds it to this rule
- * class.
+ * Builds provided attribute and attaches it to this rule class.
*
- * @param attr attribute builder
+ * <p>Typically rule classes should only declare a handful of attributes - this expectation is
+ * enforced when the instance is built.
*/
public <TYPE> Builder add(Attribute.Builder<TYPE> attr) {
addAttribute(attr.build());
diff --git a/src/main/java/com/google/devtools/build/lib/skylarkbuildapi/SkylarkRuleFunctionsApi.java b/src/main/java/com/google/devtools/build/lib/skylarkbuildapi/SkylarkRuleFunctionsApi.java
index 3d2d8a8..e557592 100644
--- a/src/main/java/com/google/devtools/build/lib/skylarkbuildapi/SkylarkRuleFunctionsApi.java
+++ b/src/main/java/com/google/devtools/build/lib/skylarkbuildapi/SkylarkRuleFunctionsApi.java
@@ -134,7 +134,8 @@
+ "implicitly added and must not be specified. Attributes "
+ "<code>visibility</code>, <code>deprecation</code>, <code>tags</code>, "
+ "<code>testonly</code>, and <code>features</code> are implicitly added and "
- + "cannot be overridden."),
+ + "cannot be overridden. Most rules need only a handful of attributes. To "
+ + "limit memory usage, the rule function imposes a cap on the size of attrs."),
// TODO(bazel-team): need to give the types of these builtin attributes
@Param(
name = "outputs",
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 fda2f34..aedddf5 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
@@ -1163,4 +1163,22 @@
assertThat(stringSetting.hasAttr(SKYLARK_BUILD_SETTING_DEFAULT_ATTR_NAME, LABEL)).isFalse();
}
+
+ @Test
+ public void testBuildTooManyAttributesRejected() {
+ RuleClass.Builder builder =
+ new RuleClass.Builder("myclass", RuleClassType.NORMAL, /*skylark=*/ false)
+ .factory(DUMMY_CONFIGURED_TARGET_FACTORY)
+ .add(attr("tags", STRING_LIST));
+ for (int i = 0; i < 150; i++) {
+ builder.add(attr("attr" + i, STRING));
+ }
+
+ IllegalArgumentException expected =
+ assertThrows(IllegalArgumentException.class, builder::build);
+
+ assertThat(expected)
+ .hasMessageThat()
+ .isEqualTo("Rule class myclass declared too many attributes (151 > 150)");
+ }
}
diff --git a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleClassFunctionsTest.java b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleClassFunctionsTest.java
index 3feba044..594148b 100644
--- a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleClassFunctionsTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleClassFunctionsTest.java
@@ -27,6 +27,8 @@
import com.google.devtools.build.lib.analysis.skylark.SkylarkRuleClassFunctions.SkylarkRuleFunction;
import com.google.devtools.build.lib.analysis.skylark.SkylarkRuleContext;
import com.google.devtools.build.lib.cmdline.Label;
+import com.google.devtools.build.lib.events.Event;
+import com.google.devtools.build.lib.events.EventKind;
import com.google.devtools.build.lib.packages.AdvertisedProviderSet;
import com.google.devtools.build.lib.packages.AspectParameters;
import com.google.devtools.build.lib.packages.Attribute;
@@ -210,6 +212,27 @@
}
@Test
+ public void testRuleClassTooManyAttributes() throws Exception {
+ ev.setFailFast(false);
+
+ ImmutableList.Builder<String> linesBuilder =
+ ImmutableList.<String>builder()
+ .add("def impl(ctx): return")
+ .add("r = rule(impl, attrs = {");
+ for (int i = 0; i < 150; i++) {
+ linesBuilder.add(" 'attr" + i + "': attr.int(),");
+ }
+ linesBuilder.add("})");
+
+ evalAndExport(linesBuilder.build().toArray(new String[0]));
+
+ assertThat(ev.getEventCollector()).hasSize(1);
+ Event event = ev.getEventCollector().iterator().next();
+ assertThat(event.getKind()).isEqualTo(EventKind.ERROR);
+ assertThat(event.getMessage()).contains("Rule class r declared too many attributes");
+ }
+
+ @Test
public void testDisableDeprecatedParams() throws Exception {
setSkylarkSemanticsOptions("--incompatible_disable_deprecated_attr_params=true");
