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 bazelbuild#6437 RELNOTES: Attribute names are going to be restricted and must be syntactically valid identifiers. bazelbuild#6437 PiperOrigin-RevId: 255986287
irengrig · Jul 1, 2019 · e6ab3c4 · e6ab3c4
1 parent 6748ccb
commit e6ab3c4
Show file tree

Hide file tree

Showing 6 changed files with 104 additions and 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
@@ -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<Pair<String, SkylarkAttr.Descriptor>> attributes =
-        attrObjectToAttributesList(attrs, ast);
+        attrObjectToAttributesList(attrs, ast.getLocation(), funcallEnv);
 
     if (skylarkTestable) {
       builder.setSkylarkTestable();
@@ -408,16 +409,29 @@ public BaseFunction rule(
     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) {
       for (Map.Entry<String, Descriptor> attr :
           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<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
@@ -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
@@ -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<String> 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
@@ -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
@@ -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
@@ -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 =