Cleanup @SkylarkCallable parameters and their error handling.

This involves enforcing additional compiletime restrictions on Param ordering and semantics.

RELNOTES: None.
PiperOrigin-RevId: 191927206

8 files changed, 259 insertions, 1 deletions

diff --git a/src/test/java/com/google/devtools/build/lib/skylarkinterface/processor/SkylarkCallableProcessorTest.java b/src/test/java/com/google/devtools/build/lib/skylarkinterface/processor/SkylarkCallableProcessorTest.java
index 524b356d73..69a2725e7d 100644
--- a/src/test/java/com/google/devtools/build/lib/skylarkinterface/processor/SkylarkCallableProcessorTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skylarkinterface/processor/SkylarkCallableProcessorTest.java
@@ -179,4 +179,45 @@ public final class SkylarkCallableProcessorTest {
             "Parameter 'a_parameter' has both 'type' and 'allowedTypes' specified."
                 + " Only one may be specified.");
   }
+
+  @Test
+  public void testParamNeitherNamedNorPositional() throws Exception {
+    assertAbout(javaSource())
+        .that(getFile("ParamNeitherNamedNorPositional.java"))
+        .processedWith(new SkylarkCallableProcessor())
+        .failsToCompile()
+        .withErrorContaining(
+            "Parameter 'a_parameter' must be either positional or named");
+  }
+
+  @Test
+  public void testNonDefaultParamAfterDefault() throws Exception {
+    assertAbout(javaSource())
+        .that(getFile("NonDefaultParamAfterDefault.java"))
+        .processedWith(new SkylarkCallableProcessor())
+        .failsToCompile()
+        .withErrorContaining(
+            "Positional parameter 'two' has no default value but is specified "
+                + "after one or more positional parameters with default values");
+  }
+
+  @Test
+  public void testPositionalParamAfterNonPositional() throws Exception {
+    assertAbout(javaSource())
+        .that(getFile("PositionalParamAfterNonPositional.java"))
+        .processedWith(new SkylarkCallableProcessor())
+        .failsToCompile()
+        .withErrorContaining(
+            "Positional parameter 'two' is specified after one or more non-positonal parameters");
+  }
+
+  @Test
+  public void testPositionalOnlyParamAfterNamed() throws Exception {
+    assertAbout(javaSource())
+        .that(getFile("PositionalOnlyParamAfterNamed.java"))
+        .processedWith(new SkylarkCallableProcessor())
+        .failsToCompile()
+        .withErrorContaining(
+            "Positional-only parameter 'two' is specified after one or more named parameters");
+  }
 }
diff --git a/src/test/java/com/google/devtools/build/lib/skylarkinterface/processor/testsources/GoldenCase.java b/src/test/java/com/google/devtools/build/lib/skylarkinterface/processor/testsources/GoldenCase.java
index 1c29312db1..53baa0145c 100644
--- a/src/test/java/com/google/devtools/build/lib/skylarkinterface/processor/testsources/GoldenCase.java
+++ b/src/test/java/com/google/devtools/build/lib/skylarkinterface/processor/testsources/GoldenCase.java
@@ -126,4 +126,30 @@ public class GoldenCase {
       SkylarkSemantics skylarkSemantics) {
     return "baz";
   }
+
+  @SkylarkCallable(
+      name = "many_arg_method_mixing_positional_and_named",
+      documented = false,
+      parameters = {
+          @Param(name = "one", type = String.class, positional = true, named = false),
+          @Param(name = "two", type = String.class, positional = true, named = true),
+          @Param(name = "three", type = String.class, positional = true, named = true,
+              defaultValue = "three"),
+          @Param(name = "four", type = String.class, positional = false, named = true),
+          @Param(name = "five", type = String.class, positional = false, named = true,
+              defaultValue = "five"),
+          @Param(name = "six", type = String.class, positional = false, named = true),
+      },
+      useLocation = true
+  )
+  public String manyArgMethodMixingPositoinalAndNamed(
+      String one,
+      String two,
+      String three,
+      String four,
+      String five,
+      String six,
+      Location location) {
+    return "baz";
+  }
 }
diff --git a/src/test/java/com/google/devtools/build/lib/skylarkinterface/processor/testsources/NonDefaultParamAfterDefault.java b/src/test/java/com/google/devtools/build/lib/skylarkinterface/processor/testsources/NonDefaultParamAfterDefault.java
new file mode 100644
index 0000000000..3e736f8d11
--- /dev/null
+++ b/src/test/java/com/google/devtools/build/lib/skylarkinterface/processor/testsources/NonDefaultParamAfterDefault.java
@@ -0,0 +1,41 @@
+// Copyright 2018 The Bazel Authors. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//    http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.devtools.build.lib.skylarkinterface.processor.testsources;
+
+import com.google.devtools.build.lib.skylarkinterface.Param;
+import com.google.devtools.build.lib.skylarkinterface.SkylarkCallable;
+
+/**
+ * Test case for a SkylarkCallable method which has a positional parameter with no default value
+ * specified after a positional parameter with a default value.
+ */
+public class NonDefaultParamAfterDefault {
+
+  @SkylarkCallable(
+      name = "non_default_after_default",
+      documented = false,
+      parameters = {
+          @Param(name = "one",
+              named = true,
+              defaultValue = "1",
+              positional = true),
+          @Param(name = "two",
+              named = true,
+              positional = true)
+      })
+  public Integer nonDefaultAfterDefault(Integer one, Integer two) {
+    return 42;
+  }
+}
\ No newline at end of file
diff --git a/src/test/java/com/google/devtools/build/lib/skylarkinterface/processor/testsources/ParamNeitherNamedNorPositional.java b/src/test/java/com/google/devtools/build/lib/skylarkinterface/processor/testsources/ParamNeitherNamedNorPositional.java
new file mode 100644
index 0000000000..7c7bb1cb59
--- /dev/null
+++ b/src/test/java/com/google/devtools/build/lib/skylarkinterface/processor/testsources/ParamNeitherNamedNorPositional.java
@@ -0,0 +1,37 @@
+// Copyright 2018 The Bazel Authors. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//    http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.devtools.build.lib.skylarkinterface.processor.testsources;
+
+import com.google.devtools.build.lib.skylarkinterface.Param;
+import com.google.devtools.build.lib.skylarkinterface.SkylarkCallable;
+
+/**
+ * Test case for a SkylarkCallable method which has a parameter with is neither named nor
+ * positional.
+ */
+public class ParamNeitherNamedNorPositional {
+
+  @SkylarkCallable(
+      name = "param_neither_named_nor_positional",
+      documented = false,
+      parameters = {
+          @Param(name = "a_parameter",
+              named = false,
+              positional = false)
+      })
+  public Integer paramUndecidable() {
+    return 42;
+  }
+}
\ No newline at end of file
diff --git a/src/test/java/com/google/devtools/build/lib/skylarkinterface/processor/testsources/PositionalOnlyParamAfterNamed.java b/src/test/java/com/google/devtools/build/lib/skylarkinterface/processor/testsources/PositionalOnlyParamAfterNamed.java
new file mode 100644
index 0000000000..74ae4ea614
--- /dev/null
+++ b/src/test/java/com/google/devtools/build/lib/skylarkinterface/processor/testsources/PositionalOnlyParamAfterNamed.java
@@ -0,0 +1,40 @@
+// Copyright 2018 The Bazel Authors. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//    http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.devtools.build.lib.skylarkinterface.processor.testsources;
+
+import com.google.devtools.build.lib.skylarkinterface.Param;
+import com.google.devtools.build.lib.skylarkinterface.SkylarkCallable;
+
+/**
+ * Test case for a SkylarkCallable method which has a positional-only parameter specified after a
+ * named positional parameter.
+ */
+public class PositionalOnlyParamAfterNamed {
+
+  @SkylarkCallable(
+      name = "positional_only_after_named",
+      documented = false,
+      parameters = {
+          @Param(name = "one",
+              named = true,
+              positional = true),
+          @Param(name = "two",
+              named = false,
+              positional = true)
+      })
+  public Integer positionalOnlyAfterNamed(Integer one, Integer two) {
+    return 42;
+  }
+}
\ No newline at end of file
diff --git a/src/test/java/com/google/devtools/build/lib/skylarkinterface/processor/testsources/PositionalParamAfterNonPositional.java b/src/test/java/com/google/devtools/build/lib/skylarkinterface/processor/testsources/PositionalParamAfterNonPositional.java
new file mode 100644
index 0000000000..713f52e1f0
--- /dev/null
+++ b/src/test/java/com/google/devtools/build/lib/skylarkinterface/processor/testsources/PositionalParamAfterNonPositional.java
@@ -0,0 +1,40 @@
+// Copyright 2018 The Bazel Authors. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//    http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.devtools.build.lib.skylarkinterface.processor.testsources;
+
+import com.google.devtools.build.lib.skylarkinterface.Param;
+import com.google.devtools.build.lib.skylarkinterface.SkylarkCallable;
+
+/**
+ * Test case for a SkylarkCallable method which has a positional parameter specified after a
+ * non positional parameter.
+ */
+public class PositionalParamAfterNonPositional {
+
+  @SkylarkCallable(
+      name = "positional_after_non_positional",
+      documented = false,
+      parameters = {
+          @Param(name = "one",
+              named = true,
+              positional = false),
+          @Param(name = "two",
+              named = true,
+              positional = true)
+      })
+  public Integer positionalAfterNonPositional(Integer one, Integer two) {
+    return 42;
+  }
+}
\ No newline at end of file
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
index a49de3a2e4..c3c18a9482 100644
--- a/src/test/java/com/google/devtools/build/lib/syntax/EvaluationTest.java
+++ b/src/test/java/com/google/devtools/build/lib/syntax/EvaluationTest.java
@@ -667,7 +667,8 @@ public class EvaluationTest extends EvaluationTestCase {
   @Test
   public void testDictKeysTooManyArgs() throws Exception {
     newTest().testIfExactError(
-        "too many arguments, in method call keys(string) of 'dict'", "{'a': 1}.keys('abc')");
+        "expected no more than 0 positional arguments, but got 1, "
+            + "in method call keys(string) of 'dict'", "{'a': 1}.keys('abc')");
   }
 
   @Test
diff --git a/src/test/java/com/google/devtools/build/lib/syntax/SkylarkEvaluationTest.java b/src/test/java/com/google/devtools/build/lib/syntax/SkylarkEvaluationTest.java
index ee6fc425bb..3804a72d53 100644
--- a/src/test/java/com/google/devtools/build/lib/syntax/SkylarkEvaluationTest.java
+++ b/src/test/java/com/google/devtools/build/lib/syntax/SkylarkEvaluationTest.java
@@ -943,6 +943,38 @@ public class SkylarkEvaluationTest extends EvaluationTest {
         .testLookup("b", "with_params(1, true, false, true, false, a)");
   }
 
+  /**
+   * This test verifies an error is raised when a method parameter is set both positionally and
+   * by name.
+   */
+  @Test
+  public void testArgSpecifiedBothByNameAndPosition() throws Exception {
+    // in with_params, 'posOrNamed' is positional parameter index 2. So by specifying both
+    // posOrNamed by name and three positional parameters, there is a conflict.
+    new SkylarkTest()
+        .update("mock", new Mock())
+        .testIfErrorContains("got multiple values for keyword argument 'posOrNamed'",
+            "mock.with_params(1, True, True, posOrNamed=True, named=True)");
+  }
+
+  @Test
+  public void testTooManyPositionalArgs() throws Exception {
+    new SkylarkTest()
+        .update("mock", new Mock())
+        .testIfErrorContains("expected no more than 3 positional arguments, but got 4",
+            "mock.with_params(1, True, True, 'toomany', named=True)");
+
+    new SkylarkTest()
+        .update("mock", new Mock())
+        .testIfErrorContains("expected no more than 3 positional arguments, but got 5",
+            "mock.with_params(1, True, True, 'toomany', 'alsotoomany', named=True)");
+
+    new SkylarkTest()
+        .update("mock", new Mock())
+        .testIfErrorContains("expected no more than 1 positional arguments, but got 2",
+            "mock.is_empty('a', 'b')");
+  }
+
   @Test
   public void testJavaCallWithPositionalAndKwargs() throws Exception {
     new SkylarkTest()