Introduce flag --incompatible_restrict_attribute_names When the flag is enabled, attribute names must be syntactically valid identifiers. For example, they cannot contain special characters. Fixes https://github.com/bazelbuild/bazel/issues/6437 RELNOTES: Attribute names are going to be restricted and must be syntactically valid identifiers. https://github.com/bazelbuild/bazel/issues/6437 PiperOrigin-RevId: 255986287
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkRuleClassFunctions.java b/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkRuleClassFunctions.java index 42982dd..2737aa2 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkRuleClassFunctions.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkRuleClassFunctions.java
@@ -81,6 +81,7 @@ import com.google.devtools.build.lib.syntax.EvalUtils; import com.google.devtools.build.lib.syntax.FuncallExpression; import com.google.devtools.build.lib.syntax.FunctionSignature; +import com.google.devtools.build.lib.syntax.Identifier; import com.google.devtools.build.lib.syntax.Runtime; import com.google.devtools.build.lib.syntax.SkylarkCallbackFunction; import com.google.devtools.build.lib.syntax.SkylarkDict; @@ -302,7 +303,7 @@ // We'll set the name later, pass the empty string for now. RuleClass.Builder builder = new RuleClass.Builder("", type, true, parent); ImmutableList<Pair<String, SkylarkAttr.Descriptor>> attributes = - attrObjectToAttributesList(attrs, ast); + attrObjectToAttributesList(attrs, ast.getLocation(), funcallEnv); if (skylarkTestable) { builder.setSkylarkTestable(); @@ -408,8 +409,20 @@ return new SkylarkRuleFunction(builder, type, attributes, ast.getLocation()); } - protected static ImmutableList<Pair<String, Descriptor>> attrObjectToAttributesList( - Object attrs, FuncallExpression ast) throws EvalException { + private static void checkAttributeName(Location loc, Environment env, String name) + throws EvalException { + if (env.getSemantics().incompatibleRestrictAttributeNames() && !Identifier.isValid(name)) { + throw new EvalException( + loc, + "attribute name `" + + name + + "` is not a valid identfier. " + + "This check can be disabled with `--incompatible_restrict_attribute_names=false`."); + } + } + + private static ImmutableList<Pair<String, Descriptor>> attrObjectToAttributesList( + Object attrs, Location loc, Environment env) throws EvalException { ImmutableList.Builder<Pair<String, Descriptor>> attributes = ImmutableList.builder(); if (attrs != Runtime.NONE) { @@ -417,7 +430,8 @@ castMap(attrs, String.class, Descriptor.class, "attrs").entrySet()) { Descriptor attrDescriptor = attr.getValue(); AttributeValueSource source = attrDescriptor.getValueSource(); - String attrName = source.convertToNativeName(attr.getKey(), ast.getLocation()); + checkAttributeName(loc, env, attr.getKey()); + String attrName = source.convertToNativeName(attr.getKey(), loc); attributes.add(Pair.of(attrName, attrDescriptor)); } } @@ -502,7 +516,7 @@ } ImmutableList<Pair<String, SkylarkAttr.Descriptor>> descriptors = - attrObjectToAttributesList(attrs, ast); + attrObjectToAttributesList(attrs, ast.getLocation(), funcallEnv); ImmutableList.Builder<Attribute> attributes = ImmutableList.builder(); ImmutableSet.Builder<String> requiredParams = ImmutableSet.builder(); for (Pair<String, Descriptor> nameDescriptorPair : descriptors) {
diff --git a/src/main/java/com/google/devtools/build/lib/packages/StarlarkSemanticsOptions.java b/src/main/java/com/google/devtools/build/lib/packages/StarlarkSemanticsOptions.java index 3509a0c..c521b6d 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/StarlarkSemanticsOptions.java +++ b/src/main/java/com/google/devtools/build/lib/packages/StarlarkSemanticsOptions.java
@@ -571,6 +571,18 @@ public boolean incompatibleRestrictNamedParams; @Option( + name = "incompatible_restrict_attribute_names", + defaultValue = "false", + documentationCategory = OptionDocumentationCategory.STARLARK_SEMANTICS, + effectTags = {OptionEffectTag.LOADING_AND_ANALYSIS}, + metadataTags = { + OptionMetadataTag.INCOMPATIBLE_CHANGE, + OptionMetadataTag.TRIGGERED_BY_ALL_INCOMPATIBLE_CHANGES + }, + help = "If set to true, restrict rule attribute names to valid identifiers") + public boolean incompatibleRestrictAttributeNames; + + @Option( name = "incompatible_depset_for_libraries_to_link_getter", defaultValue = "true", documentationCategory = OptionDocumentationCategory.STARLARK_SEMANTICS, @@ -648,6 +660,7 @@ .incompatibleObjcFrameworkCleanup(incompatibleObjcFrameworkCleanup) .incompatibleRemapMainRepo(incompatibleRemapMainRepo) .incompatibleRemoveNativeMavenJar(incompatibleRemoveNativeMavenJar) + .incompatibleRestrictAttributeNames(incompatibleRestrictAttributeNames) .incompatibleRestrictNamedParams(incompatibleRestrictNamedParams) .incompatibleRunShellCommandString(incompatibleRunShellCommandString) .incompatibleStringJoinRequiresStrings(incompatibleStringJoinRequiresStrings)
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/Identifier.java b/src/main/java/com/google/devtools/build/lib/syntax/Identifier.java index 29858d5..6cf89a7 100644 --- a/src/main/java/com/google/devtools/build/lib/syntax/Identifier.java +++ b/src/main/java/com/google/devtools/build/lib/syntax/Identifier.java
@@ -140,8 +140,8 @@ if (name.equals("REPOSITORY_NAME")) { return new EvalException( getLocation(), - "The value 'REPOSITORY_NAME' has been removed in favor of 'repository_name()', " - + "please use the latter (" + "The value 'REPOSITORY_NAME' has been removed in favor of 'repository_name()', please" + + " use the latter (" + "https://docs.bazel.build/versions/master/skylark/lib/native.html#repository_name)."); } return null; @@ -165,4 +165,27 @@ public static Identifier of(String name) { return new Identifier(name); } + + /** Returns true if the string is a syntactically valid identifier. */ + public static boolean isValid(String name) { + // TODO(laurentlb): Handle Unicode characters. + if (name.isEmpty()) { + return false; + } + for (int i = 0; i < name.length(); i++) { + char c = name.charAt(i); + if ((c >= 'a' && c <= 'z') + || (c >= 'A' && c <= 'Z') + || (c >= '0' && c <= '9') + || (c == '_')) { + continue; + } + return false; + } + if (name.charAt(0) >= '0' && name.charAt(0) <= '9') { + return false; + } + + return true; + } }
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/StarlarkSemantics.java b/src/main/java/com/google/devtools/build/lib/syntax/StarlarkSemantics.java index 0ca38f0..140b12e 100644 --- a/src/main/java/com/google/devtools/build/lib/syntax/StarlarkSemantics.java +++ b/src/main/java/com/google/devtools/build/lib/syntax/StarlarkSemantics.java
@@ -190,6 +190,8 @@ public abstract boolean incompatibleRemoveNativeMavenJar(); + public abstract boolean incompatibleRestrictAttributeNames(); + public abstract boolean incompatibleRestrictNamedParams(); public abstract boolean incompatibleRunShellCommandString(); @@ -272,6 +274,7 @@ .incompatibleRemapMainRepo(false) .incompatibleRemoveNativeMavenJar(false) .incompatibleRunShellCommandString(false) + .incompatibleRestrictAttributeNames(false) .incompatibleRestrictNamedParams(false) .incompatibleStringJoinRequiresStrings(true) .internalSkylarkFlagTestCanary(false) @@ -354,6 +357,8 @@ public abstract Builder incompatibleRemoveNativeMavenJar(boolean value); + public abstract Builder incompatibleRestrictAttributeNames(boolean value); + public abstract Builder incompatibleRestrictNamedParams(boolean value); public abstract Builder incompatibleRunShellCommandString(boolean value);
diff --git a/src/test/java/com/google/devtools/build/lib/packages/SkylarkSemanticsConsistencyTest.java b/src/test/java/com/google/devtools/build/lib/packages/SkylarkSemanticsConsistencyTest.java index 6fd4db7..749c2d8 100644 --- a/src/test/java/com/google/devtools/build/lib/packages/SkylarkSemanticsConsistencyTest.java +++ b/src/test/java/com/google/devtools/build/lib/packages/SkylarkSemanticsConsistencyTest.java
@@ -160,6 +160,7 @@ "--incompatible_objc_framework_cleanup=" + rand.nextBoolean(), "--incompatible_remap_main_repo=" + rand.nextBoolean(), "--incompatible_remove_native_maven_jar=" + rand.nextBoolean(), + "--incompatible_restrict_attribute_names=" + rand.nextBoolean(), "--incompatible_restrict_named_params=" + rand.nextBoolean(), "--incompatible_run_shell_command_string=" + rand.nextBoolean(), "--incompatible_string_join_requires_strings=" + rand.nextBoolean(), @@ -212,6 +213,7 @@ .incompatibleObjcFrameworkCleanup(rand.nextBoolean()) .incompatibleRemapMainRepo(rand.nextBoolean()) .incompatibleRemoveNativeMavenJar(rand.nextBoolean()) + .incompatibleRestrictAttributeNames(rand.nextBoolean()) .incompatibleRestrictNamedParams(rand.nextBoolean()) .incompatibleRunShellCommandString(rand.nextBoolean()) .incompatibleStringJoinRequiresStrings(rand.nextBoolean())
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 91b5427..54ca283 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
@@ -198,6 +198,46 @@ } @Test + public void testAttrNameSpecialCharactersAreForbidden() throws Exception { + ev = + createEvaluationTestCase( + StarlarkSemantics.DEFAULT_SEMANTICS.toBuilder() + .incompatibleRestrictAttributeNames(true) + .build()); + ev.initialize(); + + ev.setFailFast(false); + evalAndExport("def impl(ctx): return", "r = rule(impl, attrs = {'ab$c': attr.int()})"); + ev.assertContainsError("attribute name `ab$c` is not a valid identfier"); + } + + @Test + public void testAttrNameCannotStartWithDigit() throws Exception { + ev = + createEvaluationTestCase( + StarlarkSemantics.DEFAULT_SEMANTICS.toBuilder() + .incompatibleRestrictAttributeNames(true) + .build()); + ev.initialize(); + + ev.setFailFast(false); + evalAndExport("def impl(ctx): return", "r = rule(impl, attrs = {'2_foo': attr.int()})"); + ev.assertContainsError("attribute name `2_foo` is not a valid identfier"); + } + + @Test + public void testAttrNameSpecialCharactersLegacy() throws Exception { + ev = + createEvaluationTestCase( + StarlarkSemantics.DEFAULT_SEMANTICS.toBuilder() + .incompatibleRestrictAttributeNames(false) + .build()); + ev.initialize(); + + evalAndExport("def impl(ctx): return", "r = rule(impl, attrs = {'ab$c': attr.int()})"); + } + + @Test public void testDisableDeprecatedParams() throws Exception { ev = createEvaluationTestCase(