Glob arguments 'exclude' and 'exclude_directories' must be named

Unamed arguments are confusing, e.g.
  glob(["*.java"], ["testing/*.java"])
The second list is actually excluded.

RELNOTES: Glob arguments 'exclude' and 'exclude_directories' must be named

--
MOS_MIGRATED_REVID=127190991

3 files changed, 65 insertions, 26 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
index f242aebb1a..c05e178b35 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java
@@ -477,35 +477,62 @@ public final class PackageFactory {
     return packageArguments.build();
   }
 
-  /****************************************************************************
-   * Environment function factories.
+  /**
+   * ************************************************************************** Environment function
+   * factories.
    */
 
   /**
    * Returns a function-value implementing "glob" in the specified package context.
    *
-   * @param async if true, start globs in the background but don't block on their completion.
-   *        Only use this for heuristic preloading.
+   * @param async if true, start globs in the background but don't block on their completion. Only
+   *     use this for heuristic preloading.
    */
-  @SkylarkSignature(name = "glob", objectType = Object.class, returnType = SkylarkList.class,
-      doc = "Returns a list of files that match glob search pattern",
-      parameters = {
-        @Param(name = "include", type = SkylarkList.class, generic1 = String.class,
-            doc = "a list of strings specifying patterns of files to include."),
-        @Param(name = "exclude", type = SkylarkList.class, generic1 = String.class,
-            defaultValue = "[]",
-            doc = "a list of strings specifying patterns of files to exclude."),
-        // TODO(bazel-team): migrate all existing code to use boolean instead?
-        @Param(name = "exclude_directories", type = Integer.class, defaultValue = "1",
-            doc = "a integer that if non-zero indicates directories should not be matched.")},
-      documented = false, useAst = true, useEnvironment = true)
+  @SkylarkSignature(
+    name = "glob",
+    objectType = Object.class,
+    returnType = SkylarkList.class,
+    doc = "Returns a list of files that match glob search pattern",
+    parameters = {
+      @Param(
+        name = "include",
+        type = SkylarkList.class,
+        generic1 = String.class,
+        doc = "a list of strings specifying patterns of files to include."
+      ),
+      @Param(
+        name = "exclude",
+        type = SkylarkList.class,
+        generic1 = String.class,
+        defaultValue = "[]",
+        positional = false,
+        named = true,
+        doc = "a list of strings specifying patterns of files to exclude."
+      ),
+      // TODO(bazel-team): migrate all existing code to use boolean instead?
+      @Param(
+        name = "exclude_directories",
+        type = Integer.class,
+        defaultValue = "1",
+        positional = false,
+        named = true,
+        doc = "a integer that if non-zero indicates directories should not be matched."
+      )
+    },
+    documented = false,
+    useAst = true,
+    useEnvironment = true
+  )
   private static final BuiltinFunction.Factory newGlobFunction =
       new BuiltinFunction.Factory("glob") {
         public BuiltinFunction create(final PackageContext originalContext, final boolean async) {
           return new BuiltinFunction("glob", this) {
             public SkylarkList invoke(
-                SkylarkList include, SkylarkList exclude, Integer excludeDirectories,
-                FuncallExpression ast, Environment env)
+                SkylarkList include,
+                SkylarkList exclude,
+                Integer excludeDirectories,
+                FuncallExpression ast,
+                Environment env)
                 throws EvalException, ConversionException, InterruptedException {
               return callGlob(
                   originalContext, async, include, exclude, excludeDirectories != 0, ast, env);
diff --git a/src/test/java/com/google/devtools/build/lib/packages/PackageFactoryTest.java b/src/test/java/com/google/devtools/build/lib/packages/PackageFactoryTest.java
index 943c21f929..6feb959a10 100644
--- a/src/test/java/com/google/devtools/build/lib/packages/PackageFactoryTest.java
+++ b/src/test/java/com/google/devtools/build/lib/packages/PackageFactoryTest.java
@@ -581,7 +581,7 @@ public class PackageFactoryTest extends PackageFactoryTestBase {
         scratch.file(
             "/rg/BUILD",
             "cc_library(name = 'ri', srcs = glob(['**/*.cc']))",
-            "cc_library(name = 're', srcs = glob(['*.cc'], ['**/*.c']))");
+            "cc_library(name = 're', srcs = glob(['*.cc'], exclude=['**/*.c']))");
     Package pkg = packages.eval("rg", file);
     events.assertNoWarningsOrErrors();
 
@@ -663,33 +663,43 @@ public class PackageFactoryTest extends PackageFactoryTestBase {
     assertGlobFails(
         "glob()",
         "insufficient arguments received by glob(include: sequence of strings, "
-            + "exclude: sequence of strings = [], exclude_directories: int = 1) "
+            + "*, exclude: sequence of strings = [], exclude_directories: int = 1) "
             + "(got 0, expected at least 1)");
   }
 
   @Test
+  public void testGlobUnamedExclude() throws Exception {
+    events.setFailFast(false);
+    assertGlobFails(
+        "glob(['a'], ['b'])",
+        "too many (2) positional arguments in call to glob(include: sequence of strings, "
+            + "*, exclude: sequence of strings = [], exclude_directories: int = 1)");
+  }
+
+  @Test
   public void testTooManyArgumentsGlobErrors() throws Exception {
     events.setFailFast(false);
     assertGlobFails(
         "glob(1,2,3,4)",
         "too many (4) positional arguments in call to glob(include: sequence of strings, "
-            + "exclude: sequence of strings = [], exclude_directories: int = 1)");
+            + "*, exclude: sequence of strings = [], exclude_directories: int = 1)");
   }
 
   @Test
   public void testGlobEnforcesListArgument() throws Exception {
     events.setFailFast(false);
     assertGlobFails(
-        "glob(1,2)",
-        "Method glob(include: sequence of strings, exclude: sequence of strings, "
-            + "exclude_directories: int) is not applicable for arguments (int, int, int)");
+        "glob(1, exclude=2)",
+        "Method glob(include: sequence of strings, *, exclude: sequence of strings, "
+            + "exclude_directories: int) is not applicable for arguments (int, int, int): "
+            + "'include' is int, but should be sequence");
   }
 
   @Test
   public void testGlobEnforcesListOfStringsArguments() throws Exception {
     events.setFailFast(false);
     assertGlobFails(
-        "glob(['a', 'b'],['c', 42])",
+        "glob(['a', 'b'], exclude=['c', 42])",
         "expected value of type 'string' for element 1 of 'glob' argument, but got 42 (int)");
   }
 
diff --git a/src/test/java/com/google/devtools/build/lib/packages/util/PackageFactoryTestBase.java b/src/test/java/com/google/devtools/build/lib/packages/util/PackageFactoryTestBase.java
index 800dc19b92..872c4e2bfb 100644
--- a/src/test/java/com/google/devtools/build/lib/packages/util/PackageFactoryTestBase.java
+++ b/src/test/java/com/google/devtools/build/lib/packages/util/PackageFactoryTestBase.java
@@ -235,7 +235,9 @@ public abstract class PackageFactoryTestBase {
     Path file =
         scratch.file(
             "/globs/BUILD",
-            Printer.format("result = glob(%r, %r, %r)", includes, excludes, excludeDirs ? 1 : 0),
+            Printer.format(
+                "result = glob(%r, exclude=%r, exclude_directories=%r)",
+                includes, excludes, excludeDirs ? 1 : 0),
             resultAssertion);
 
     return packages.evalAndReturnGlobCache("globs", file, packages.ast(file));