New class hierarchy for Skylark functions

* New hierarchy BaseFunction > UserModeFunction, BuiltinFunction.
  The old hierarchy still exists for now, to be deleted after migration:
  Function > AbstractFunction > MixedModeFunction >
    (UserModeFunction, SkylarkFunction > SimpleSkylarkFunction)
  (UserModeFunction is already migrated, and
  BaseFunction implements Function, for now.)

* Function supports *args and **kwargs when calling functions, and
  mandatory named-only parameters in the style of Python 3.
  Notable difference with Python: *args binds the variable to a tuple,
  because a Skylark list would have to be monomorphic.

* A better, simpler, safer FFI using reflection with BuiltinFunction.
  Handles typechecking, passes parameters in a more Java style.
  (Not used for now, will be used later.)

* A new annotation @SkylarkSignature, intended to replace @SkylarkBuiltin,
  supports the full function call protocol, including default arguments.

* Support for annotating function Factory-s rather than functions.

--
MOS_MIGRATED_REVID=88958581

3 files changed, 208 insertions, 13 deletions

diff --git a/src/test/java/com/google/devtools/build/lib/syntax/BaseFunctionTest.java b/src/test/java/com/google/devtools/build/lib/syntax/BaseFunctionTest.java
new file mode 100644
index 0000000000..6833622e28
--- /dev/null
+++ b/src/test/java/com/google/devtools/build/lib/syntax/BaseFunctionTest.java
@@ -0,0 +1,160 @@
+// Copyright 2006-2015 Google Inc. 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.syntax;
+
+import static com.google.common.truth.Truth.assertThat;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.fail;
+
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.JUnit4;
+
+import java.util.Arrays;
+import java.util.Map;
+import java.util.TreeMap;
+
+/**
+ * Tests for {@link BaseFunction}.
+ * This tests the argument processing by BaseFunction
+ * between the outer call(posargs, kwargs, ast, env) and the inner call(args, ast, env).
+ */
+@RunWith(JUnit4.class)
+public class BaseFunctionTest extends AbstractEvaluationTestCase {
+
+  private Environment singletonEnv(String id, Object value) {
+    Environment env = new Environment();
+    env.update(id, value);
+    return env;
+  }
+
+  /**
+   * Handy implementation of {@link BaseFunction} that returns all its args as a list.
+   * (We'd use SkylarkList.tuple, but it can't handle null.)
+   */
+  private static class TestingBaseFunction extends BaseFunction {
+    TestingBaseFunction(FunctionSignature signature) {
+      super("mixed", signature);
+    }
+
+    @Override
+    public Object call(Object[] arguments, FuncallExpression ast, Environment env) {
+      return Arrays.asList(arguments);
+    }
+  }
+
+  private void checkBaseFunction(BaseFunction func, String callExpression, String expectedOutput)
+      throws Exception {
+    Environment env = singletonEnv(func.getName(), func);
+
+    if (expectedOutput.charAt(0) == '[') { // a tuple => expected to pass
+      assertEquals("Wrong output for " + callExpression,
+          expectedOutput, eval(callExpression, env).toString());
+
+    } else { // expected to fail with an exception
+      try {
+        eval(callExpression, env);
+        fail();
+      } catch (EvalException e) {
+        assertEquals("Wrong exception for " + callExpression,
+            expectedOutput, e.getMessage());
+      }
+    }
+  }
+
+  private static final String[] BASE_FUNCTION_EXPRESSIONS = {
+    "mixed()",
+    "mixed(1)",
+    "mixed(1, 2)",
+    "mixed(1, 2, 3)",
+    "mixed(1, 2, wiz=3, quux=4)",
+    "mixed(foo=1)",
+    "mixed(foo=1, bar=2)",
+    "mixed(bar=2, foo=1)",
+    "mixed(2, foo=1)",
+    "mixed(bar=2, foo=1, wiz=3)",
+  };
+
+  public void checkBaseFunctions(boolean onlyNamedArguments,
+      String expectedSignature, String... expectedResults) throws Exception {
+    BaseFunction func = new TestingBaseFunction(
+        onlyNamedArguments
+        ? FunctionSignature.namedOnly(1, "foo", "bar")
+        : FunctionSignature.of(1, "foo", "bar"));
+
+    assertThat(func.toString()).isEqualTo(expectedSignature);
+
+    for (int i = 0; i < BASE_FUNCTION_EXPRESSIONS.length; i++) {
+      String expr = BASE_FUNCTION_EXPRESSIONS[i];
+      String expected = expectedResults[i];
+      checkBaseFunction(func, expr, expected);
+    }
+  }
+
+  @Test
+  public void testNoSurplusArguments() throws Exception {
+    checkBaseFunctions(false, "mixed(foo, bar = ?)",
+        "insufficient arguments received by mixed(foo, bar = ?) (got 0, expected at least 1)",
+        "[1, null]",
+        "[1, 2]",
+        "too many (3) positional arguments in call to mixed(foo, bar = ?)",
+        "unexpected keywords 'quux', 'wiz' in call to mixed(foo, bar = ?)",
+        "[1, null]",
+        "[1, 2]",
+        "[1, 2]",
+        "argument 'foo' passed both by position and by name in call to mixed(foo, bar = ?)",
+        "unexpected keyword 'wiz' in call to mixed(foo, bar = ?)");
+  }
+
+  @Test
+  public void testOnlyNamedArguments() throws Exception {
+    checkBaseFunctions(true, "mixed(*, foo = ?, bar)",
+        "missing mandatory keyword arguments in call to mixed(*, foo = ?, bar)",
+        "mixed(*, foo = ?, bar) does not accept positional arguments, but got 1",
+        "mixed(*, foo = ?, bar) does not accept positional arguments, but got 2",
+        "mixed(*, foo = ?, bar) does not accept positional arguments, but got 3",
+        "mixed(*, foo = ?, bar) does not accept positional arguments, but got 2",
+        "missing mandatory named-only argument 'bar' while calling mixed(*, foo = ?, bar)",
+        "[1, 2]",
+        "[1, 2]",
+        "mixed(*, foo = ?, bar) does not accept positional arguments, but got 1",
+        "unexpected keyword 'wiz' in call to mixed(*, foo = ?, bar)");
+  }
+
+  @Test
+  @SuppressWarnings("unchecked")
+  public void testKwParam() throws Exception {
+    Environment env = new SkylarkEnvironment(syntaxEvents.collector());
+    exec(parseFileForSkylark(
+        "def foo(a, b, c=3, d=4, *args, e, f, g=7, h=8, **kwargs):\n"
+        + "  return (a, b, c, d, e, f, g, h, args, kwargs)\n"
+        + "v1 = foo(1, 2, e=5, f=6)\n"
+        + "v2 = foo(1, *['x', 'y', 'z', 't'], h=9, e=5, f=6, i=0)\n"
+        + "def bar(**kwargs):\n"
+        + "  return kwargs\n"
+        + "b1 = bar(name='foo', type='jpg', version=42)\n"
+        + "b2 = bar()\n"), env);
+
+    assertThat(EvalUtils.prettyPrintValue(env.lookup("v1")))
+        .isEqualTo("(1, 2, 3, 4, 5, 6, 7, 8, (), {})");
+    assertThat(EvalUtils.prettyPrintValue(env.lookup("v2")))
+        .isEqualTo("(1, \"x\", \"y\", \"z\", 5, 6, 7, 9, (\"t\",), {\"i\": 0})");
+
+    // NB: the conversion to a TreeMap below ensures the keys are sorted.
+    assertThat(EvalUtils.prettyPrintValue(
+        new TreeMap<String, Object>((Map<String, Object>) env.lookup("b1"))))
+        .isEqualTo("{\"name\": \"foo\", \"type\": \"jpg\", \"version\": 42}");
+    assertThat(EvalUtils.prettyPrintValue(env.lookup("b2"))).isEqualTo("{}");
+  }
+}
diff --git a/src/test/java/com/google/devtools/build/lib/syntax/FunctionTest.java b/src/test/java/com/google/devtools/build/lib/syntax/FunctionTest.java
index 4ce41cb7f3..2eea35a1d9 100644
--- a/src/test/java/com/google/devtools/build/lib/syntax/FunctionTest.java
+++ b/src/test/java/com/google/devtools/build/lib/syntax/FunctionTest.java
@@ -89,7 +89,7 @@ public class FunctionTest extends AbstractEvaluationTestCase {
   }
 
   private void createOuterFunction(Environment env, final List<Object> params) {
-    Function outerFunc = new AbstractFunction("outer_func") {
+    BaseFunction outerFunc = new BaseFunction("outer_func") {
 
       @Override
       public Object call(List<Object> args, Map<String, Object> kwargs, FuncallExpression ast,
@@ -362,7 +362,8 @@ public class FunctionTest extends AbstractEvaluationTestCase {
 
   @Test
   public void testDefaultArgumentsInsufficientArgNum() throws Exception {
-    checkError("func(a, b = null, c = null) received insufficient arguments",
+    checkError("insufficient arguments received by func(a, b = \"b\", c = \"c\") "
+        + "(got 0, expected at least 1)",
         "def func(a, b = 'b', c = 'c'):",
         "  return a + b + c",
         "func()");
@@ -371,16 +372,18 @@ public class FunctionTest extends AbstractEvaluationTestCase {
   @Test
   public void testKwargs() throws Exception {
     List<Statement> input = parseFileForSkylark(
-        "def foo(a, b = 'b', c = 'c'):\n"
-      + "  return a + b + c\n"
+        "def foo(a, b = 'b', *, c, d = 'd'):\n"
+      + "  return a + b + c + d\n"
       + "args = {'a': 'x', 'c': 'z'}\n"
       + "v1 = foo(**args)\n"
-      + "v2 = foo('x', **{'b': 'y'})\n"
-      + "v3 = foo(c = 'z', a = 'x', **{'b': 'y'})");
+      + "v2 = foo('x', c = 'c', d = 'e', **{'b': 'y'})\n"
+      + "v3 = foo(c = 'z', a = 'x', **{'b': 'y', 'd': 'f'})");
     exec(input, env);
-    assertEquals("xbz", env.lookup("v1"));
-    assertEquals("xyc", env.lookup("v2"));
-    assertEquals("xyz", env.lookup("v3"));
+    assertEquals("xbzd", env.lookup("v1"));
+    assertEquals("xyce", env.lookup("v2"));
+    assertEquals("xyzf", env.lookup("v3"));
+    UserDefinedFunction foo = (UserDefinedFunction) env.lookup("foo");
+    assertEquals("foo(a, b = \"b\", *, d = \"d\", c)", foo.toString());
   }
 
   @Test
@@ -401,7 +404,7 @@ public class FunctionTest extends AbstractEvaluationTestCase {
 
   @Test
   public void testKwargsCollision() throws Exception {
-    checkError("func(a, b) got multiple values for keyword argument 'b'",
+    checkError("argument 'b' passed both by position and by name in call to func(a, b)",
         "def func(a, b):",
         "  return a + b",
         "func('a', 'b', **{'b': 'foo'})");
@@ -452,6 +455,26 @@ public class FunctionTest extends AbstractEvaluationTestCase {
     assertEquals("a12", env.lookup("v4"));
   }
 
+  @Test
+  public void testStarParam() throws Exception {
+    List<Statement> input = parseFileForSkylark(
+        "def f(name, value = '1', *rest, mandatory, optional = '2'):\n"
+        + "  r = name + value + mandatory + optional + '|'\n"
+        + "  for x in rest: r += x\n"
+        + "  return r\n"
+        + "v1 = f('a', 'b', mandatory = 'z')\n"
+        + "v2 = f('a', 'b', 'c', 'd', mandatory = 'z')\n"
+        + "v3 = f('a', *['b', 'c', 'd'], mandatory = 'y', optional = 'z')\n"
+        + "v4 = f(*['a'], **{'value': 'b', 'mandatory': 'c'})\n"
+        + "v5 = f('a', 'b', 'c', *['d', 'e'], mandatory = 'f', **{'optional': 'g'})\n");
+    exec(input, env);
+    assertEquals("abz2|", env.lookup("v1"));
+    assertEquals("abz2|cd", env.lookup("v2"));
+    assertEquals("abyz|cd", env.lookup("v3"));
+    assertEquals("abc2|", env.lookup("v4"));
+    assertEquals("abfg|cde", env.lookup("v5"));
+  }
+
   private void checkError(String msg, String... lines)
       throws Exception {
     try {
diff --git a/src/test/java/com/google/devtools/build/lib/syntax/ParserTest.java b/src/test/java/com/google/devtools/build/lib/syntax/ParserTest.java
index f97282e5e1..1707b3aa51 100644
--- a/src/test/java/com/google/devtools/build/lib/syntax/ParserTest.java
+++ b/src/test/java/com/google/devtools/build/lib/syntax/ParserTest.java
@@ -20,6 +20,7 @@ import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertTrue;
 import static org.junit.Assert.fail;
 
+import com.google.common.collect.ImmutableList;
 import com.google.devtools.build.lib.events.Location;
 import com.google.devtools.build.lib.syntax.DictionaryLiteral.DictionaryEntryLiteral;
 
@@ -968,9 +969,20 @@ public class ParserTest extends AbstractParserTestCase {
   @Test
   public void testUnnamedStar() throws Exception {
     syntaxEvents.setFailFast(false);
-    parseFileForSkylark(
-        "def func(a, b1=2, b2=3, *, c1, c2, d=4): return a + b1 + b2 + c1 + c2 + d\n");
-    syntaxEvents.assertContainsEvent("no star, star-star or named-only parameters (for now)");
+    List<Statement> statements = parseFileForSkylark(
+        "def func(a, b1=2, b2=3, *, c1, d=4, c2): return a + b1 + b2 + c1 + c2 + d\n");
+    assertThat(statements).hasSize(1);
+    assertThat(statements.get(0)).isInstanceOf(FunctionDefStatement.class);
+    FunctionDefStatement stmt = (FunctionDefStatement) statements.get(0);
+    FunctionSignature sig = stmt.getArgs().getSignature();
+    // Note the reordering of mandatory named-only at the end.
+    assertThat(sig.getNames()).isEqualTo(ImmutableList.<String>of(
+        "a", "b1", "b2", "d", "c1", "c2"));
+    FunctionSignature.Shape shape = sig.getShape();
+    assertThat(shape.getMandatoryPositionals()).isEqualTo(1);
+    assertThat(shape.getOptionalPositionals()).isEqualTo(2);
+    assertThat(shape.getMandatoryNamedOnly()).isEqualTo(2);
+    assertThat(shape.getOptionalNamedOnly()).isEqualTo(1);
   }
 
   @Test