Introduce flag --incompatible_no_kwargs_in_build_files

Implements #8021 When it is enabled, **kwargs and *args are not allowed in BUILD files. Without this flag, the ban on **kwarg and *args is not fully working. RELNOTES: New flag `--incompatible_no_kwargs_in_build_files`. See #8021 PiperOrigin-RevId: 243325744
bazelbuild · Apr 12, 2019 · de41cf8 · de41cf8
1 parent 207528c
commit de41cf8
Show file tree

Hide file tree

Showing 8 changed files with 43 additions and 4 deletions.
diff --git a/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java b/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java
@@ -1770,7 +1770,8 @@ public Package.Builder evaluateBuildFile(
         }
       }
 
-      if (!ValidationEnvironment.checkBuildSyntax(buildFileAST.getStatements(), eventHandler)) {
+      if (!ValidationEnvironment.checkBuildSyntax(
+          buildFileAST.getStatements(), eventHandler, pkgEnv)) {
         pkgBuilder.setContainsErrors();
       }
 

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
@@ -443,6 +443,20 @@ public class StarlarkSemanticsOptions extends OptionsBase implements Serializabl
               + "symbols introduced by load are not implicitly re-exported.")
   public boolean incompatibleNoTransitiveLoads;
 
+  @Option(
+      name = "incompatible_no_kwargs_in_build_files",
+      defaultValue = "false",
+      documentationCategory = OptionDocumentationCategory.STARLARK_SEMANTICS,
+      effectTags = {OptionEffectTag.BUILD_FILE_SEMANTICS},
+      metadataTags = {
+        OptionMetadataTag.INCOMPATIBLE_CHANGE,
+        OptionMetadataTag.TRIGGERED_BY_ALL_INCOMPATIBLE_CHANGES
+      },
+      help =
+          "If set to true, *args and **kwargs are not allowed in BUILD files. See "
+              + "https://github.com/bazelbuild/bazel/issues/8021")
+  public boolean incompatibleNoKwargsInBuildFiles;
+
   @Option(
       name = "incompatible_remap_main_repo",
       defaultValue = "false",
@@ -525,6 +539,7 @@ public StarlarkSemantics toSkylarkSemantics() {
         .incompatibleExpandDirectories(incompatibleExpandDirectories)
         .incompatibleNewActionsApi(incompatibleNewActionsApi)
         .incompatibleNoAttrLicense(incompatibleNoAttrLicense)
+        .incompatibleNoKwargsInBuildFiles(incompatibleNoKwargsInBuildFiles)
         .incompatibleNoOutputAttrDefault(incompatibleNoOutputAttrDefault)
         .incompatibleNoSupportToolsInActionInputs(incompatibleNoSupportToolsInActionInputs)
         .incompatibleNoTargetOutputGroup(incompatibleNoTargetOutputGroup)

diff --git a/src/main/java/com/google/devtools/build/lib/packages/WorkspaceFactory.java b/src/main/java/com/google/devtools/build/lib/packages/WorkspaceFactory.java
@@ -206,7 +206,7 @@ private void execute(
       }
     }
 
-    if (!ValidationEnvironment.checkBuildSyntax(ast.getStatements(), localReporter)
+    if (!ValidationEnvironment.checkBuildSyntax(ast.getStatements(), localReporter, workspaceEnv)
         || !ast.exec(workspaceEnv, localReporter)) {
       localReporter.handle(Event.error("Error evaluating WORKSPACE file"));
     }

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
@@ -160,6 +160,8 @@ public boolean flagValue(FlagIdentifier flagIdentifier) {
 
   public abstract boolean incompatibleNoAttrLicense();
 
+  public abstract boolean incompatibleNoKwargsInBuildFiles();
+
   public abstract boolean incompatibleNoOutputAttrDefault();
 
   public abstract boolean incompatibleNoSupportToolsInActionInputs();
@@ -218,6 +220,7 @@ public static Builder builderWithDefaults() {
           .incompatibleExpandDirectories(true)
           .incompatibleNewActionsApi(false)
           .incompatibleNoAttrLicense(true)
+          .incompatibleNoKwargsInBuildFiles(false)
           .incompatibleNoOutputAttrDefault(false)
           .incompatibleNoSupportToolsInActionInputs(false)
           .incompatibleNoTargetOutputGroup(false)
@@ -280,6 +283,8 @@ public abstract static class Builder {
 
     public abstract Builder incompatibleNewActionsApi(boolean value);
 
+    public abstract Builder incompatibleNoKwargsInBuildFiles(boolean value);
+
     public abstract Builder incompatibleNoAttrLicense(boolean value);
 
     public abstract Builder incompatibleNoOutputAttrDefault(boolean value);

diff --git a/src/main/java/com/google/devtools/build/lib/syntax/ValidationEnvironment.java b/src/main/java/com/google/devtools/build/lib/syntax/ValidationEnvironment.java
@@ -389,7 +389,7 @@ private void closeBlock() {
    * **kwargs. This creates a better separation between code and data.
    */
   public static boolean checkBuildSyntax(
-      List<Statement> statements, final EventHandler eventHandler) {
+      List<Statement> statements, final EventHandler eventHandler, Environment env) {
     // Wrap the boolean inside an array so that the inner class can modify it.
     final boolean[] success = new boolean[] {true};
     // TODO(laurentlb): Merge with the visitor above when possible (i.e. when BUILD files use it).
@@ -442,6 +442,9 @@ public void visit(FuncallExpression node) {
                         + "explicitly.");
               }
             }
+            if (env.getSemantics().incompatibleNoKwargsInBuildFiles()) {
+              super.visit(node);
+            }
           }
         };
     checker.visitAll(statements);

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
@@ -148,6 +148,7 @@ private static StarlarkSemanticsOptions buildRandomOptions(Random rand) throws E
         "--incompatible_expand_directories=" + rand.nextBoolean(),
         "--incompatible_new_actions_api=" + rand.nextBoolean(),
         "--incompatible_no_attr_license=" + rand.nextBoolean(),
+        "--incompatible_no_kwargs_in_build_files=" + rand.nextBoolean(),
         "--incompatible_no_output_attr_default=" + rand.nextBoolean(),
         "--incompatible_no_support_tools_in_action_inputs=" + rand.nextBoolean(),
         "--incompatible_no_target_output_group=" + rand.nextBoolean(),
@@ -192,6 +193,7 @@ private static StarlarkSemantics buildRandomSemantics(Random rand) {
         .incompatibleExpandDirectories(rand.nextBoolean())
         .incompatibleNewActionsApi(rand.nextBoolean())
         .incompatibleNoAttrLicense(rand.nextBoolean())
+        .incompatibleNoKwargsInBuildFiles(rand.nextBoolean())
         .incompatibleNoOutputAttrDefault(rand.nextBoolean())
         .incompatibleNoSupportToolsInActionInputs(rand.nextBoolean())
         .incompatibleNoTargetOutputGroup(rand.nextBoolean())

diff --git a/src/test/java/com/google/devtools/build/lib/syntax/EvaluationTest.java b/src/test/java/com/google/devtools/build/lib/syntax/EvaluationTest.java
@@ -708,4 +708,17 @@ public void testArgsForbiddenInBuild() throws Exception {
     new BuildTest()
         .testIfErrorContains("*args arguments are not allowed in BUILD files", "func(*array)");
   }
+
+  @Test
+  public void testIncompatibleKwargsInBuildFiles() throws Exception {
+    new BuildTest("--incompatible_no_kwargs_in_build_files=true")
+        .testIfErrorContains(
+            "kwargs arguments are not allowed in BUILD files", "len(dict(**{'a': 1}))");
+
+    new BuildTest("--incompatible_no_kwargs_in_build_files=false")
+        .testStatement("len(dict(**{'a': 1}))", 1);
+
+    new SkylarkTest("--incompatible_no_kwargs_in_build_files")
+        .testStatement("len(dict(**{'a': 1}))", 1);
+  }
 }
diff --git a/src/test/java/com/google/devtools/build/lib/syntax/util/EvaluationTestCase.java b/src/test/java/com/google/devtools/build/lib/syntax/util/EvaluationTestCase.java
@@ -201,7 +201,7 @@ public Object eval(String... input) throws Exception {
       return BuildFileAST.eval(env, input);
     }
     BuildFileAST ast = BuildFileAST.parseString(env.getEventHandler(), input);
-    ValidationEnvironment.checkBuildSyntax(ast.getStatements(), env.getEventHandler());
+    ValidationEnvironment.checkBuildSyntax(ast.getStatements(), env.getEventHandler(), env);
     return ast.eval(env);
   }