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 48cb299a743496..eb498218859254 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 Metadata getMetadata() { */ 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 a4feb031acb073..4f84481c99675e 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 BaseFunction invoke( // 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 attr : castMap(attrs, String.class, Descriptor.class, "attrs").entrySet()) { @@ -119,9 +122,6 @@ public BaseFunction invoke( 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 5365d855a33c49..2692f5de0bb018 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 void testLoadDoesNotHideWorkspaceFunction() throws Exception { 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 03590c79794786..2367da2cf57afc 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,12 +97,27 @@ public void testCannotOverrideBuiltInAttribute() throws Exception { 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(