From 5e5ec766e7244c7684d058f3c22dec96a27e6656 Mon Sep 17 00:00:00 2001 From: laurentlb Date: Mon, 1 Jul 2019 10:43:57 -0700 Subject: [PATCH] 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 --- .../skylark/SkylarkRuleClassFunctions.java | 24 ++++++++--- .../packages/StarlarkSemanticsOptions.java | 13 ++++++ .../devtools/build/lib/syntax/Identifier.java | 27 ++++++++++++- .../build/lib/syntax/StarlarkSemantics.java | 5 +++ .../SkylarkSemanticsConsistencyTest.java | 2 + .../SkylarkRuleClassFunctionsTest.java | 40 +++++++++++++++++++ 6 files changed, 104 insertions(+), 7 deletions(-) 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 42982dd8c825e1..2737aa2f4496f1 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 @@ public BaseFunction rule( // We'll set the name later, pass the empty string for now. RuleClass.Builder builder = new RuleClass.Builder("", type, true, parent); ImmutableList> attributes = - attrObjectToAttributesList(attrs, ast); + attrObjectToAttributesList(attrs, ast.getLocation(), funcallEnv); if (skylarkTestable) { builder.setSkylarkTestable(); @@ -408,8 +409,20 @@ public BaseFunction rule( return new SkylarkRuleFunction(builder, type, attributes, ast.getLocation()); } - protected static ImmutableList> 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> attrObjectToAttributesList( + Object attrs, Location loc, Environment env) throws EvalException { ImmutableList.Builder> attributes = ImmutableList.builder(); if (attrs != Runtime.NONE) { @@ -417,7 +430,8 @@ protected static ImmutableList> attrObjectToAttributesL 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 @@ public SkylarkAspect aspect( } ImmutableList> descriptors = - attrObjectToAttributesList(attrs, ast); + attrObjectToAttributesList(attrs, ast.getLocation(), funcallEnv); ImmutableList.Builder attributes = ImmutableList.builder(); ImmutableSet.Builder requiredParams = ImmutableSet.builder(); for (Pair 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 3509a0c0056060..c521b6d74cdc0d 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 @@ -570,6 +570,18 @@ public class StarlarkSemanticsOptions extends OptionsBase implements Serializabl + "only specifiable positionally (and not by keyword).") 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", @@ -648,6 +660,7 @@ public StarlarkSemantics toSkylarkSemantics() { .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 29858d56767457..6cf89a75aa8a1e 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 @@ private EvalException getSpecialException() { 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 @@ EvalException createInvalidIdentifierException(Set symbols) { 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 0ca38f0f90cfad..140b12eb468ad3 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 boolean flagValue(FlagIdentifier flagIdentifier) { public abstract boolean incompatibleRemoveNativeMavenJar(); + public abstract boolean incompatibleRestrictAttributeNames(); + public abstract boolean incompatibleRestrictNamedParams(); public abstract boolean incompatibleRunShellCommandString(); @@ -272,6 +274,7 @@ public static Builder builderWithDefaults() { .incompatibleRemapMainRepo(false) .incompatibleRemoveNativeMavenJar(false) .incompatibleRunShellCommandString(false) + .incompatibleRestrictAttributeNames(false) .incompatibleRestrictNamedParams(false) .incompatibleStringJoinRequiresStrings(true) .internalSkylarkFlagTestCanary(false) @@ -354,6 +357,8 @@ public abstract Builder incompatibleDisallowRuleExecutionPlatformConstraintsAllo 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 6fd4db7afd4b6c..749c2d8ba910c5 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 @@ private static StarlarkSemanticsOptions buildRandomOptions(Random rand) throws E "--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 @@ private static StarlarkSemantics buildRandomSemantics(Random rand) { .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 91b54274b41395..54ca28393739a2 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 @@ -197,6 +197,46 @@ public void testAttrAllowedFileTypesWrongType() throws Exception { "attr.label_list(allow_files = 18)"); } + @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 =