Ensure attribute name lengths are reasonable
Puts a sensible, but generous, bound on attribute name length in order to
prevent (most probably accidental) runaway name lengths. The expectation is
that no reasonable human is or should be typing out 128 char attribute
names. Instead anything getting to this length is likely machine generated,
and should be checked for accidental unbounded growth.
While this is technically a breaking change, it's expected that the allowance
is generous enough that it's a noop. Do note that the previous max was
MAX_INT or the JVM OOMing, whichever came first, which is something we clearly
shouldn't allow.
RELNOTES: A maximum attribute name length is 128 is enforced
PiperOrigin-RevId: 305086330
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 93ae0b3..bfbbecd 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
@@ -125,12 +125,19 @@
public class RuleClass {
/**
- * Maximum attributes per RuleClass. Current value was chosen high enough to be considered a
+ * Maximum attributes per RuleClass. Current value was chosen to be 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 = 200;
+ /**
+ * Maximum attribute name length. Chosen to accommodate existing and prevent extreme outliers from
+ * forming - extreme values create bloat, both in memory usage and various outputs, including but
+ * not limited to, query output.
+ */
+ private static final int MAX_ATTRIBUTE_NAME_LENGTH = 128;
+
@AutoCodec
static final Function<? super Rule, Map<String, Label>> NO_EXTERNAL_BINDINGS =
Functions.<Map<String, Label>>constant(ImmutableMap.<String, Label>of());
@@ -794,14 +801,8 @@
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);
+ checkAttributes(name, type, attributes);
- type.checkAttributes(attributes);
Preconditions.checkState(
(type == RuleClassType.ABSTRACT)
== (configuredTargetFactory == null && configuredTargetFunction == null),
@@ -877,6 +878,30 @@
buildSetting);
}
+ private static void checkAttributes(
+ String ruleClassName, RuleClassType ruleClassType, Map<String, Attribute> attributes) {
+ Preconditions.checkArgument(
+ attributes.size() <= MAX_ATTRIBUTES,
+ "Rule class %s declared too many attributes (%s > %s)",
+ ruleClassName,
+ attributes.size(),
+ MAX_ATTRIBUTES);
+
+ for (String attributeName : attributes.keySet()) {
+ // TODO(b/151171037): This check would make more sense at Attribute creation time, but the
+ // use of unchecked exceptions in these APIs makes it brittle.
+ Preconditions.checkArgument(
+ attributeName.length() <= MAX_ATTRIBUTE_NAME_LENGTH,
+ "Attribute %s.%s's name is too long (%s > %s)",
+ ruleClassName,
+ attributeName,
+ attributeName.length(),
+ MAX_ATTRIBUTE_NAME_LENGTH);
+ }
+
+ ruleClassType.checkAttributes(attributes);
+ }
+
private void assertSkylarkRuleClassHasImplementationFunction() {
Preconditions.checkState(
(type == RuleClassType.NORMAL || type == RuleClassType.TEST)
@@ -1161,6 +1186,9 @@
*
* <p>Typically rule classes should only declare a handful of attributes - this expectation is
* enforced when the instance is built.
+ *
+ * <p>Attribute names should be meaningful but short; overly long names are rejected at
+ * instantiation.
*/
public <TYPE> Builder add(Attribute.Builder<TYPE> attr) {
addAttribute(attr.build());
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 0571e4c..ad21913 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
@@ -32,6 +32,7 @@
import com.google.common.base.Function;
import com.google.common.base.Predicate;
import com.google.common.base.Predicates;
+import com.google.common.base.Strings;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
@@ -1221,4 +1222,21 @@
.hasMessageThat()
.isEqualTo("Rule class myclass declared too many attributes (201 > 200)");
}
+
+ @Test
+ public void testBuildTooLongAttributeNameRejected() {
+ IllegalArgumentException expected =
+ assertThrows(
+ IllegalArgumentException.class,
+ () ->
+ new RuleClass.Builder("myclass", RuleClassType.NORMAL, /*skylark=*/ false)
+ .factory(DUMMY_CONFIGURED_TARGET_FACTORY)
+ .add(attr("tags", STRING_LIST))
+ .add(attr(Strings.repeat("x", 150), STRING))
+ .build());
+
+ assertThat(expected)
+ .hasMessageThat()
+ .matches("Attribute myclass\\.x{150}'s name is too long \\(150 > 128\\)");
+ }
}
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 f653a64..fe1e6f6 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
@@ -18,6 +18,7 @@
import static org.junit.Assert.assertThrows;
import com.google.common.base.Joiner;
+import com.google.common.base.Strings;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.Iterables;
@@ -236,6 +237,21 @@
}
@Test
+ public void testRuleClassTooLongAttributeName() throws Exception {
+ ev.setFailFast(false);
+
+ evalAndExport(
+ "def impl(ctx): return;",
+ "r = rule(impl, attrs = { '" + Strings.repeat("x", 150) + "': attr.int() })");
+
+ assertThat(ev.getEventCollector()).hasSize(1);
+ Event event = ev.getEventCollector().iterator().next();
+ assertThat(event.getKind()).isEqualTo(EventKind.ERROR);
+ assertThat(event.getMessage())
+ .matches("Attribute r\\.x{150}'s name is too long \\(150 > 128\\)");
+ }
+
+ @Test
public void testDisableDeprecatedParams() throws Exception {
setSkylarkSemanticsOptions("--incompatible_disable_deprecated_attr_params=true");
