Proper error messages when built-in rule attributes are overridden #1811

--
MOS_MIGRATED_REVID=134823021

4 files changed, 47 insertions, 4 deletions

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 48cb299a74..eb49821885 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 class BaseRuleClasses {
    */
   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 a4feb031ac..4f84481c99 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 @@ public class SkylarkRepositoryModule {
           // 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 @@ public class SkylarkRepositoryModule {
               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 5365d855a3..2692f5de0b 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 @@ public class SkylarkRepositoryIntegrationTest extends BuildViewTestCase {
     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 03590c7979..2367da2cf5 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 @@ public class SkylarkRuleClassFunctionsTest extends SkylarkTestCase {
       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):",