Proper error messages when built-in rule attributes are overridden #1811
--
MOS_MIGRATED_REVID=135241715
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/BaseRuleClasses.java b/src/main/java/com/google/devtools/build/lib/analysis/BaseRuleClasses.java
index 48cb299..eb49821 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/BaseRuleClasses.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/BaseRuleClasses.java
@@ -161,6 +161,8 @@
*/
public static RuleClass.Builder commonCoreAndSkylarkAttributes(RuleClass.Builder builder) {
return builder
+ .add(attr("name", STRING)
+ .nonconfigurable("Rule name"))
// The visibility attribute is special: it is a nodep label, and loading the
// necessary package groups is handled by {@link LabelVisitor#visitTargetVisibility}.
// Package groups always have the null configuration so that they are not duplicated
diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryModule.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryModule.java
index a4feb03..4f84481 100644
--- a/src/main/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryModule.java
+++ b/src/main/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryModule.java
@@ -109,6 +109,9 @@
// We'll set the name later, pass the empty string for now.
Builder builder = new Builder("", RuleClassType.WORKSPACE, true);
+ builder.addOrOverrideAttribute(attr("$local", BOOLEAN).defaultValue(local).build());
+ BaseRuleClasses.commonCoreAndSkylarkAttributes(builder);
+ builder.add(attr("expect_failure", STRING));
if (attrs != Runtime.NONE) {
for (Map.Entry<String, Descriptor> attr :
castMap(attrs, String.class, Descriptor.class, "attrs").entrySet()) {
@@ -119,9 +122,6 @@
builder.addOrOverrideAttribute(attrBuilder.build(attrName));
}
}
- builder.addOrOverrideAttribute(attr("$local", BOOLEAN).defaultValue(local).build());
- BaseRuleClasses.commonCoreAndSkylarkAttributes(builder);
- builder.add(attr("expect_failure", STRING));
builder.setConfiguredTargetFunction(implementation);
builder.setRuleDefinitionEnvironment(funcallEnv);
builder.setWorkspaceOnly();
diff --git a/src/test/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryIntegrationTest.java b/src/test/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryIntegrationTest.java
index 5365d85..2692f5d 100644
--- a/src/test/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryIntegrationTest.java
+++ b/src/test/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryIntegrationTest.java
@@ -335,4 +335,30 @@
invalidatePackages();
assertThat(getRuleContext(getConfiguredTarget("//:data")).getWorkspaceName()).isEqualTo("bleh");
}
+
+ @Test
+ public void testSkylarkRepositoryCannotOverrideBuiltInAttribute() throws Exception {
+ scratch.file(
+ "def.bzl",
+ "def _impl(ctx):",
+ " print(ctx.attr.name)",
+ "",
+ "repo = repository_rule(",
+ " implementation=_impl,",
+ " attrs={'name': attr.string(mandatory=True)})");
+ scratch.file(rootDirectory.getRelative("BUILD").getPathString());
+ scratch.overwriteFile(
+ rootDirectory.getRelative("WORKSPACE").getPathString(),
+ "load('//:def.bzl', 'repo')",
+ "repo(name='foo')");
+
+ invalidatePackages();
+ try {
+ getConfiguredTarget("@foo//:bar");
+ fail();
+ } catch (AssertionError e) {
+ assertThat(e.getMessage()).contains("There is already a built-in attribute 'name' "
+ + "which cannot be overridden");
+ }
+ }
}
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 03590c7..2367da2 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
@@ -97,13 +97,28 @@
Assert.fail("Expected error '"
+ "There is already a built-in attribute 'tags' which cannot be overridden"
+ "' but got no error");
- } catch (IllegalArgumentException | EvalException e) {
+ } catch (EvalException e) {
assertThat(e).hasMessage(
"There is already a built-in attribute 'tags' which cannot be overridden");
}
}
@Test
+ public void testCannotOverrideBuiltInAttributeName() throws Exception {
+ ev.setFailFast(true);
+ try {
+ evalAndExport(
+ "def impl(ctx): return", "r = rule(impl, attrs = {'name': attr.string()})");
+ Assert.fail("Expected error '"
+ + "There is already a built-in attribute 'name' which cannot be overridden"
+ + "' but got no error");
+ } catch (EvalException e) {
+ assertThat(e).hasMessage(
+ "There is already a built-in attribute 'name' which cannot be overridden");
+ }
+ }
+
+ @Test
public void testImplicitArgsAttribute() throws Exception {
evalAndExport(
"def _impl(ctx):",
