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(
