Compile function call expressions.

Mostly reuses the interpreters argument checking and helper functions.

--
MOS_MIGRATED_REVID=107395974

17 files changed, 402 insertions, 127 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/syntax/AbstractComprehension.java b/src/main/java/com/google/devtools/build/lib/syntax/AbstractComprehension.java
index 962f2fcb31..9ceb880118 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/AbstractComprehension.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/AbstractComprehension.java
@@ -84,7 +84,8 @@ public abstract class AbstractComprehension extends Expression {
         VariableScope scope,
         DebugInfo debugInfo,
         ASTNode node,
-        AstAccessors debugAccessors) throws EvalException;
+        AstAccessors debugAccessors)
+        throws EvalException;
 
     abstract void validate(ValidationEnvironment env) throws EvalException;
 
@@ -234,7 +235,8 @@ public abstract class AbstractComprehension extends Expression {
         VariableScope scope,
         DebugInfo debugInfo,
         ASTNode node,
-        AstAccessors debugAccessors) throws EvalException {
+        AstAccessors debugAccessors)
+        throws EvalException {
       List<ByteCodeAppender> code = new ArrayList<>();
       LabelAdder nopeLabel = new LabelAdder();
       // compile condition and convert to boolean
@@ -387,7 +389,8 @@ public abstract class AbstractComprehension extends Expression {
       VariableScope scope,
       InternalVariable collection,
       DebugInfo debugInfo,
-      AstAccessors debugAccessors) throws EvalException;
+      AstAccessors debugAccessors)
+      throws EvalException;
 
   /**
    * Add byte code which finalizes and loads the collection on the stack.
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/AssignmentStatement.java b/src/main/java/com/google/devtools/build/lib/syntax/AssignmentStatement.java
index e3dc7d1788..ed4b127332 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/AssignmentStatement.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/AssignmentStatement.java
@@ -77,7 +77,7 @@ public final class AssignmentStatement extends Statement {
   @Override
   ByteCodeAppender compile(
       VariableScope scope, Optional<LoopLabels> loopLabels, DebugInfo debugInfo)
-          throws EvalException {
+      throws EvalException {
     return new ByteCodeAppender.Compound(
         expression.compile(scope, debugInfo),
         lvalue.compileAssignment(this, debugInfo.add(this), scope));
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/BaseFunction.java b/src/main/java/com/google/devtools/build/lib/syntax/BaseFunction.java
index 48ff7f668f..1ca624e933 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/BaseFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/BaseFunction.java
@@ -23,6 +23,9 @@ import com.google.common.collect.Sets;
 import com.google.devtools.build.lib.events.Location;
 import com.google.devtools.build.lib.syntax.SkylarkList.Tuple;
 import com.google.devtools.build.lib.syntax.Type.ConversionException;
+import com.google.devtools.build.lib.syntax.compiler.ByteCodeUtils;
+
+import net.bytebuddy.implementation.bytecode.StackManipulation;
 
 import java.util.ArrayList;
 import java.util.HashMap;
@@ -402,6 +405,15 @@ public abstract class BaseFunction implements SkylarkValue {
     return parent;
   }
 
+  public static final StackManipulation call =
+      ByteCodeUtils.invoke(
+          BaseFunction.class,
+          "call",
+          List.class,
+          Map.class,
+          FuncallExpression.class,
+          Environment.class);
+
   /**
    * The outer calling convention to a BaseFunction.
    *
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/BinaryOperatorExpression.java b/src/main/java/com/google/devtools/build/lib/syntax/BinaryOperatorExpression.java
index 386fd8dc35..8676c36a4a 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/BinaryOperatorExpression.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/BinaryOperatorExpression.java
@@ -481,15 +481,15 @@ public final class BinaryOperatorExpression extends Expression {
    * <p> The method must be named exactly as the lower case name of the operator and in addition to
    * the operands require an Environment and Location.
    */
-  private static StackManipulation callImplementation(VariableScope scope,
-      AstAccessors debugAccessors, Operator operator) {
+  private static StackManipulation callImplementation(
+      VariableScope scope, AstAccessors debugAccessors, Operator operator) {
     Class<?>[] parameterTypes =
         new Class<?>[] {Object.class, Object.class, Environment.class, Location.class};
     return new StackManipulation.Compound(
         scope.loadEnvironment(),
         debugAccessors.loadLocation,
         ByteCodeUtils.invoke(
-        BinaryOperatorExpression.class, operator.name().toLowerCase(), parameterTypes));
+            BinaryOperatorExpression.class, operator.name().toLowerCase(), parameterTypes));
   }
 
   /**
@@ -498,14 +498,13 @@ public final class BinaryOperatorExpression extends Expression {
    * <p> The method must be named exactly as the lower case name of the operator and in addition to
    * the operands require a Location.
    */
-  private static StackManipulation callImplementation(AstAccessors debugAccessors,
-      Operator operator) {
-    Class<?>[] parameterTypes =
-        new Class<?>[] {Object.class, Object.class, Location.class};
+  private static StackManipulation callImplementation(
+      AstAccessors debugAccessors, Operator operator) {
+    Class<?>[] parameterTypes = new Class<?>[] {Object.class, Object.class, Location.class};
     return new StackManipulation.Compound(
         debugAccessors.loadLocation,
         ByteCodeUtils.invoke(
-        BinaryOperatorExpression.class, operator.name().toLowerCase(), parameterTypes));
+            BinaryOperatorExpression.class, operator.name().toLowerCase(), parameterTypes));
   }
 
   /**
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/DictComprehension.java b/src/main/java/com/google/devtools/build/lib/syntax/DictComprehension.java
index 51995bf8ed..c85cc234c8 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/DictComprehension.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/DictComprehension.java
@@ -68,7 +68,8 @@ public class DictComprehension extends AbstractComprehension {
       VariableScope scope,
       InternalVariable collection,
       DebugInfo debugInfo,
-      AstAccessors debugAccessors) throws EvalException {
+      AstAccessors debugAccessors)
+      throws EvalException {
     List<ByteCodeAppender> code = new ArrayList<>();
     append(code, collection.load());
     code.add(keyExpression.compile(scope, debugInfo));
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/DotExpression.java b/src/main/java/com/google/devtools/build/lib/syntax/DotExpression.java
index 84805e9724..4bc35865a4 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/DotExpression.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/DotExpression.java
@@ -137,14 +137,23 @@ public final class DotExpression extends Expression {
         name,
         debugInfo.add(this).loadLocation,
         scope.loadEnvironment(),
-        ByteCodeUtils.invoke(DotExpression.class, "eval", Object.class, String.class,
-            Location.class, Environment.class),
+        ByteCodeUtils.invoke(
+            DotExpression.class,
+            "eval",
+            Object.class,
+            String.class,
+            Location.class,
+            Environment.class),
         // at this point we have the value of obj and the result of eval on the stack
         name,
         debugInfo.add(this).loadLocation,
-        ByteCodeUtils.invoke(DotExpression.class, "checkResult", Object.class, Object.class,
-            String.class, Location.class)
-        );
+        ByteCodeUtils.invoke(
+            DotExpression.class,
+            "checkResult",
+            Object.class,
+            Object.class,
+            String.class,
+            Location.class));
     return ByteCodeUtils.compoundAppender(code);
   }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/Expression.java b/src/main/java/com/google/devtools/build/lib/syntax/Expression.java
index 5e6a716ac3..d6090255d1 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/Expression.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/Expression.java
@@ -79,8 +79,7 @@ public abstract class Expression extends ASTNode {
    * @throws EvalException for any error that would have occurred during evaluation of the
    *    function definition that contains this statement, e.g. type errors.
    */
-  ByteCodeAppender compile(VariableScope scope, DebugInfo debugInfo)
-      throws EvalException {
+  ByteCodeAppender compile(VariableScope scope, DebugInfo debugInfo) throws EvalException {
     throw new UnsupportedOperationException(this.getClass().getSimpleName() + " unsupported.");
   }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/ExpressionStatement.java b/src/main/java/com/google/devtools/build/lib/syntax/ExpressionStatement.java
index 5c9a03aded..21f71b4d58 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/ExpressionStatement.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/ExpressionStatement.java
@@ -59,7 +59,7 @@ public final class ExpressionStatement extends Statement {
   @Override
   ByteCodeAppender compile(
       VariableScope scope, Optional<LoopLabels> loopLabels, DebugInfo debugInfo)
-          throws EvalException {
+      throws EvalException {
     return expr.compile(scope, debugInfo);
   }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/ForStatement.java b/src/main/java/com/google/devtools/build/lib/syntax/ForStatement.java
index 41415ac0ca..f3b4c85d21 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/ForStatement.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/ForStatement.java
@@ -150,7 +150,7 @@ public final class ForStatement extends Statement {
   @Override
   ByteCodeAppender compile(
       VariableScope scope, Optional<LoopLabels> outerLoopLabels, DebugInfo debugInfo)
-          throws EvalException {
+      throws EvalException {
     AstAccessors debugAccessors = debugInfo.add(this);
     List<ByteCodeAppender> code = new ArrayList<>();
     InternalVariable originalIterable =
@@ -162,7 +162,8 @@ public final class ForStatement extends Statement {
     append(code, debugAccessors.loadLocation, EvalUtils.toIterable, Duplication.SINGLE);
     // save it for later concurrent modification check
     code.add(originalIterable.store());
-    append(code,
+    append(
+        code,
         ByteCodeMethodCalls.BCImmutableList.copyOf,
         ByteCodeMethodCalls.BCImmutableList.iterator);
     code.add(iterator.store());
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/FuncallExpression.java b/src/main/java/com/google/devtools/build/lib/syntax/FuncallExpression.java
index cbe5564ec6..e5678cb93c 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/FuncallExpression.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/FuncallExpression.java
@@ -14,6 +14,8 @@
 
 package com.google.devtools.build.lib.syntax;
 
+import static com.google.devtools.build.lib.syntax.compiler.ByteCodeUtils.append;
+
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Joiner;
 import com.google.common.cache.CacheBuilder;
@@ -24,8 +26,22 @@ import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.Lists;
 import com.google.devtools.build.lib.events.Location;
 import com.google.devtools.build.lib.syntax.EvalException.EvalExceptionWithJavaCause;
+import com.google.devtools.build.lib.syntax.compiler.ByteCodeMethodCalls;
+import com.google.devtools.build.lib.syntax.compiler.ByteCodeUtils;
+import com.google.devtools.build.lib.syntax.compiler.DebugInfo;
+import com.google.devtools.build.lib.syntax.compiler.DebugInfo.AstAccessors;
+import com.google.devtools.build.lib.syntax.compiler.NewObject;
+import com.google.devtools.build.lib.syntax.compiler.Variable.InternalVariable;
+import com.google.devtools.build.lib.syntax.compiler.VariableScope;
 import com.google.devtools.build.lib.util.StringUtilities;
 
+import net.bytebuddy.description.type.TypeDescription;
+import net.bytebuddy.implementation.bytecode.ByteCodeAppender;
+import net.bytebuddy.implementation.bytecode.Removal;
+import net.bytebuddy.implementation.bytecode.StackManipulation;
+import net.bytebuddy.implementation.bytecode.assign.TypeCasting;
+import net.bytebuddy.implementation.bytecode.constant.TextConstant;
+
 import java.lang.reflect.InvocationTargetException;
 import java.lang.reflect.Method;
 import java.lang.reflect.Modifier;
@@ -294,7 +310,7 @@ public final class FuncallExpression extends Expression {
     List<String> names = new ArrayList<>();
     for (List<MethodDescriptor> methods : methodCache.get(objClass).values()) {
       for (MethodDescriptor method : methods) {
-        // TODO(bazel-team): store the Skylark name in the MethodDescriptor. 
+        // TODO(bazel-team): store the Skylark name in the MethodDescriptor.
         String name = method.annotation.name();
         if (name.isEmpty()) {
           name = StringUtilities.toPythonStyleFunctionName(method.method.getName());
@@ -410,33 +426,215 @@ public final class FuncallExpression extends Expression {
   }
 
   /**
+   * A {@link StackManipulation} invoking addKeywordArg.
+   * <p>Kept close to the definition of the method to avoid reflection errors when changing it.
+   */
+  private static final StackManipulation addKeywordArg =
+      ByteCodeUtils.invoke(
+          FuncallExpression.class,
+          "addKeywordArg",
+          Map.class,
+          String.class,
+          Object.class,
+          ImmutableList.Builder.class);
+
+  /**
    * Add one argument to the keyword map, registering a duplicate in case of conflict.
+   *
+   * <p>public for reflection by the compiler and calls from compiled functions
    */
-  private void addKeywordArg(Map<String, Object> kwargs, String name,
-      Object value,  ImmutableList.Builder<String> duplicates) {
+  public static void addKeywordArg(
+      Map<String, Object> kwargs,
+      String name,
+      Object value,
+      ImmutableList.Builder<String> duplicates) {
     if (kwargs.put(name, value) != null) {
       duplicates.add(name);
     }
   }
 
   /**
+   * A {@link StackManipulation} invoking addKeywordArgs.
+   * <p>Kept close to the definition of the method to avoid reflection errors when changing it.
+   */
+  private static final StackManipulation addKeywordArgs =
+      ByteCodeUtils.invoke(
+          FuncallExpression.class,
+          "addKeywordArgs",
+          Map.class,
+          Object.class,
+          ImmutableList.Builder.class,
+          Location.class);
+
+  /**
    * Add multiple arguments to the keyword map (**kwargs), registering duplicates
+   *
+   * <p>public for reflection by the compiler and calls from compiled functions
    */
-  private void addKeywordArgs(Map<String, Object> kwargs,
-      Object items, ImmutableList.Builder<String> duplicates) throws EvalException {
+  public static void addKeywordArgs(
+      Map<String, Object> kwargs,
+      Object items,
+      ImmutableList.Builder<String> duplicates,
+      Location location)
+      throws EvalException {
     if (!(items instanceof Map<?, ?>)) {
-      throw new EvalException(getLocation(),
+      throw new EvalException(
+          location,
           "Argument after ** must be a dictionary, not " + EvalUtils.getDataTypeName(items));
     }
     for (Map.Entry<?, ?> entry : ((Map<?, ?>) items).entrySet()) {
       if (!(entry.getKey() instanceof String)) {
-        throw new EvalException(getLocation(),
-            "Keywords must be strings, not " + EvalUtils.getDataTypeName(entry.getKey()));
+        throw new EvalException(
+            location, "Keywords must be strings, not " + EvalUtils.getDataTypeName(entry.getKey()));
       }
       addKeywordArg(kwargs, (String) entry.getKey(), entry.getValue(), duplicates);
     }
   }
 
+  /**
+   * A {@link StackManipulation} invoking checkCallable.
+   * <p>Kept close to the definition of the method to avoid reflection errors when changing it.
+   */
+  private static final StackManipulation checkCallable =
+      ByteCodeUtils.invoke(FuncallExpression.class, "checkCallable", Object.class, Location.class);
+
+  /**
+   * Checks whether the given object is a {@link BaseFunction}.
+   *
+   * <p>Public for reflection by the compiler and access from generated byte code.
+   *
+   * @throws EvalException If not a BaseFunction.
+   */
+  public static BaseFunction checkCallable(Object functionValue, Location location)
+      throws EvalException {
+    if (functionValue instanceof BaseFunction) {
+      return (BaseFunction) functionValue;
+    } else {
+      throw new EvalException(
+          location, "'" + EvalUtils.getDataTypeName(functionValue) + "' object is not callable");
+    }
+  }
+
+  /**
+   * A {@link StackManipulation} invoking checkDuplicates.
+   * <p>Kept close to the definition of the method to avoid reflection errors when changing it.
+   */
+  private static final StackManipulation checkDuplicates =
+      ByteCodeUtils.invoke(
+          FuncallExpression.class,
+          "checkDuplicates",
+          ImmutableList.Builder.class,
+          String.class,
+          Location.class);
+
+  /**
+   * Check the list from the builder and report an {@link EvalException} if not empty.
+   *
+   * <p>public for reflection by the compiler and calls from compiled functions
+   */
+  public static void checkDuplicates(
+      ImmutableList.Builder<String> duplicates, String function, Location location)
+      throws EvalException {
+    List<String> dups = duplicates.build();
+    if (!dups.isEmpty()) {
+      throw new EvalException(
+          location,
+          "duplicate keyword"
+              + (dups.size() > 1 ? "s" : "")
+              + " '"
+              + Joiner.on("', '").join(dups)
+              + "' in call to "
+              + function);
+    }
+  }
+
+  /**
+   * A {@link StackManipulation} invoking invokeObjectMethod.
+   * <p>Kept close to the definition of the method to avoid reflection errors when changing it.
+   */
+  private static final StackManipulation invokeObjectMethod =
+      ByteCodeUtils.invoke(
+          FuncallExpression.class,
+          "invokeObjectMethod",
+          String.class,
+          ImmutableList.class,
+          ImmutableMap.class,
+          FuncallExpression.class,
+          Environment.class);
+
+  /**
+   * Call a method depending on the type of an object it is called on.
+   *
+   * <p>Public for reflection by the compiler and access from generated byte code.
+   *
+   * @param positionals The first object is expected to be the object the method is called on.
+   * @param call the original expression that caused this call, needed for rules especially
+   */
+  public static Object invokeObjectMethod(
+      String method,
+      ImmutableList<Object> positionals,
+      ImmutableMap<String, Object> keyWordArgs,
+      FuncallExpression call,
+      Environment env)
+      throws EvalException, InterruptedException {
+    Location location = call.getLocation();
+    Object value = positionals.get(0);
+    ImmutableList<Object> positionalArgs = positionals.subList(1, positionals.size());
+    BaseFunction function = Runtime.getFunction(EvalUtils.getSkylarkType(value.getClass()), method);
+    if (function != null) {
+      if (!isNamespace(value.getClass())) {
+        // Use self as an implicit parameter in front.
+        positionalArgs = positionals;
+      }
+      return function.call(
+          positionalArgs, ImmutableMap.<String, Object>copyOf(keyWordArgs), call, env);
+    } else if (value instanceof ClassObject) {
+      Object fieldValue = ((ClassObject) value).getValue(method);
+      if (fieldValue == null) {
+        throw new EvalException(location, String.format("struct has no method '%s'", method));
+      }
+      if (!(fieldValue instanceof BaseFunction)) {
+        throw new EvalException(
+            location, String.format("struct field '%s' is not a function", method));
+      }
+      function = (BaseFunction) fieldValue;
+      return function.call(
+          positionalArgs, ImmutableMap.<String, Object>copyOf(keyWordArgs), call, env);
+    } else if (env.isSkylark()) {
+      // Only allow native Java calls when using Skylark
+      // When calling a Java method, the name is not in the Environment,
+      // so evaluating 'func' would fail.
+      Class<?> objClass;
+      Object obj;
+      if (value instanceof Class<?>) {
+        // Static call
+        obj = null;
+        objClass = (Class<?>) value;
+      } else {
+        obj = value;
+        objClass = value.getClass();
+      }
+      MethodDescriptor methodDescriptor = call.findJavaMethod(objClass, method, positionalArgs);
+      if (!keyWordArgs.isEmpty()) {
+        throw new EvalException(
+            call.func.getLocation(),
+            String.format(
+                "Keyword arguments are not allowed when calling a java method"
+                    + "\nwhile calling method '%s' for type %s",
+                method,
+                EvalUtils.getDataTypeNameFromClass(objClass)));
+      }
+      return callMethod(methodDescriptor, method, obj, positionalArgs.toArray(), location, env);
+    } else {
+      throw new EvalException(
+          location,
+          String.format(
+              "%s is not defined on object of type '%s'",
+              call.functionName(),
+              EvalUtils.getDataTypeName(value)));
+    }
+  }
+
   @SuppressWarnings("unchecked")
   private void evalArguments(ImmutableList.Builder<Object> posargs, Map<String, Object> kwargs,
       Environment env)
@@ -455,18 +653,12 @@ public final class FuncallExpression extends Expression {
           posargs.addAll((Iterable<Object>) value);
         }
       } else if (arg.isStarStar()) {  // expand the kwargs
-        addKeywordArgs(kwargs, value, duplicates);
+        addKeywordArgs(kwargs, value, duplicates, getLocation());
       } else {
         addKeywordArg(kwargs, arg.getName(), value, duplicates);
       }
     }
-    List<String> dups = duplicates.build();
-    if (!dups.isEmpty()) {
-      throw new EvalException(getLocation(),
-          "duplicate keyword" + (dups.size() > 1 ? "s" : "") + " '"
-          + Joiner.on("', '").join(dups)
-          + "' in call to " + func);
-    }
+    checkDuplicates(duplicates, func.getName(), getLocation());
   }
 
   @VisibleForTesting
@@ -486,68 +678,13 @@ public final class FuncallExpression extends Expression {
   private Object invokeObjectMethod(Environment env) throws EvalException, InterruptedException {
     Object objValue = obj.eval(env);
     ImmutableList.Builder<Object> posargs = new ImmutableList.Builder<>();
+    posargs.add(objValue);
     // We copy this into an ImmutableMap in the end, but we can't use an ImmutableMap.Builder, or
     // we'd still have to have a HashMap on the side for the sake of properly handling duplicates.
     Map<String, Object> kwargs = new HashMap<>();
-
-    // Strings, lists and dictionaries (maps) have functions that we want to use in
-    // MethodLibrary.
-    // For other classes, we can call the Java methods.
-    BaseFunction function =
-        Runtime.getFunction(EvalUtils.getSkylarkType(objValue.getClass()), func.getName());
-    if (function != null) {
-      if (!isNamespace(objValue.getClass())) {
-        // Add self as an implicit parameter in front.
-        posargs.add(objValue);
-      }
-      evalArguments(posargs, kwargs, env);
-      return function.call(posargs.build(), ImmutableMap.<String, Object>copyOf(kwargs), this, env);
-    } else if (objValue instanceof ClassObject) {
-      Object fieldValue = ((ClassObject) objValue).getValue(func.getName());
-      if (fieldValue == null) {
-        throw new EvalException(
-            getLocation(), String.format("struct has no method '%s'", func.getName()));
-      }
-      if (!(fieldValue instanceof BaseFunction)) {
-        throw new EvalException(
-            getLocation(), String.format("struct field '%s' is not a function", func.getName()));
-      }
-      function = (BaseFunction) fieldValue;
-      evalArguments(posargs, kwargs, env);
-      return function.call(posargs.build(), ImmutableMap.<String, Object>copyOf(kwargs), this, env);
-    } else if (env.isSkylark()) {
-      // Only allow native Java calls when using Skylark
-      // When calling a Java method, the name is not in the Environment,
-      // so evaluating 'func' would fail.
-      evalArguments(posargs, kwargs, env);
-      Class<?> objClass;
-      Object obj;
-      if (objValue instanceof Class<?>) {
-        // Static call
-        obj = null;
-        objClass = (Class<?>) objValue;
-      } else {
-        obj = objValue;
-        objClass = objValue.getClass();
-      }
-      String name = func.getName();
-      ImmutableList<Object> args = posargs.build();
-      MethodDescriptor method = findJavaMethod(objClass, name, args);
-      if (!kwargs.isEmpty()) {
-        throw new EvalException(
-            func.getLocation(),
-            String.format(
-                "Keyword arguments are not allowed when calling a java method"
-                + "\nwhile calling method '%s' for type %s",
-                name, EvalUtils.getDataTypeNameFromClass(objClass)));
-      }
-      return callMethod(method, name, obj, args.toArray(), getLocation(), env);
-    } else {
-      throw new EvalException(
-          getLocation(),
-          String.format("%s is not defined on object of type '%s'", functionName(),
-              EvalUtils.getDataTypeName(objValue)));
-    }
+    evalArguments(posargs, kwargs, env);
+    return invokeObjectMethod(
+        func.getName(), posargs.build(), ImmutableMap.<String, Object>copyOf(kwargs), this, env);
   }
 
   /**
@@ -559,14 +696,9 @@ public final class FuncallExpression extends Expression {
     // We copy this into an ImmutableMap in the end, but we can't use an ImmutableMap.Builder, or
     // we'd still have to have a HashMap on the side for the sake of properly handling duplicates.
     Map<String, Object> kwargs = new HashMap<>();
-    if ((funcValue instanceof BaseFunction)) {
-      BaseFunction function = (BaseFunction) funcValue;
-      evalArguments(posargs, kwargs, env);
-      return function.call(posargs.build(), ImmutableMap.<String, Object>copyOf(kwargs), this, env);
-    } else {
-      throw new EvalException(
-          getLocation(), "'" + EvalUtils.getDataTypeName(funcValue) + "' object is not callable");
-    }
+    BaseFunction function = checkCallable(funcValue, getLocation());
+    evalArguments(posargs, kwargs, env);
+    return function.call(posargs.build(), ImmutableMap.<String, Object>copyOf(kwargs), this, env);
   }
 
   /**
@@ -611,4 +743,119 @@ public final class FuncallExpression extends Expression {
   protected boolean isNewScope() {
     return true;
   }
+
+  @Override
+  ByteCodeAppender compile(VariableScope scope, DebugInfo debugInfo) throws EvalException {
+    AstAccessors debugAccessors = debugInfo.add(this);
+    List<ByteCodeAppender> code = new ArrayList<>();
+    if (obj != null) {
+      compileObjectMethodCall(scope, debugInfo, debugAccessors, code);
+    } else {
+      compileGlobalFunctionCall(scope, debugInfo, debugAccessors, code);
+    }
+    return ByteCodeUtils.compoundAppender(code);
+  }
+
+  /**
+   * Add code that compiles the argument expressions.
+   *
+   * <p>The byte code leaves the arguments on the stack in order of:
+   * positional arguments, key word arguments, this FuncallExpression, Environment
+   * This is the order required by {@link #invokeObjectMethod} and
+   *  {@link BaseFunction#call(List, Map, FuncallExpression, Environment)}.
+   */
+  private void compileArguments(
+      VariableScope scope,
+      DebugInfo debugInfo,
+      AstAccessors debugAccessors,
+      List<ByteCodeAppender> code)
+      throws EvalException {
+    InternalVariable positionalsBuilder = scope.freshVariable(ImmutableList.Builder.class);
+    append(code, ByteCodeMethodCalls.BCImmutableList.builder);
+    code.add(positionalsBuilder.store());
+
+    InternalVariable keyWordArgs = scope.freshVariable(Map.class);
+    append(code, NewObject.fromConstructor(HashMap.class).arguments());
+    code.add(keyWordArgs.store());
+
+    InternalVariable duplicatesBuilder =
+        scope.freshVariable(new TypeDescription.ForLoadedType(ImmutableList.Builder.class));
+    append(code, ByteCodeMethodCalls.BCImmutableList.builder);
+    code.add(duplicatesBuilder.store());
+
+    StackManipulation builderAdd =
+        new StackManipulation.Compound(
+            ByteCodeMethodCalls.BCImmutableList.Builder.add, Removal.SINGLE);
+
+    // add an object the function is called on first
+    if (obj != null) {
+      append(code, positionalsBuilder.load());
+      code.add(obj.compile(scope, debugInfo));
+      append(code, builderAdd);
+    }
+    // add all arguments to their respective builder/map
+    for (Argument.Passed arg : args) {
+      ByteCodeAppender value = arg.getValue().compile(scope, debugInfo);
+      if (arg.isPositional()) {
+        append(code, positionalsBuilder.load());
+        code.add(value);
+        append(code, builderAdd);
+      } else if (arg.isStar()) {
+        // expand the starArg by adding all it's elements to the builder
+        append(code, positionalsBuilder.load());
+        code.add(value);
+        append(
+            code,
+            TypeCasting.to(new TypeDescription.ForLoadedType(Iterable.class)),
+            ByteCodeMethodCalls.BCImmutableList.Builder.addAll,
+            Removal.SINGLE);
+      } else if (arg.isStarStar()) {
+        append(code, keyWordArgs.load());
+        code.add(value);
+        append(code, duplicatesBuilder.load(), debugAccessors.loadLocation, addKeywordArgs);
+      } else {
+        append(code, keyWordArgs.load(), new TextConstant(arg.getName()));
+        code.add(value);
+        append(code, duplicatesBuilder.load(), addKeywordArg);
+      }
+    }
+    append(
+        code,
+        // check for duplicates in the key word arguments
+        duplicatesBuilder.load(),
+        new TextConstant(func.getName()),
+        debugAccessors.loadLocation,
+        checkDuplicates,
+        // load the arguments in the correct order for invokeObjectMethod and BaseFunction.call
+        positionalsBuilder.load(),
+        ByteCodeMethodCalls.BCImmutableList.Builder.build,
+        keyWordArgs.load(),
+        ByteCodeMethodCalls.BCImmutableMap.copyOf,
+        debugAccessors.loadAstNode,
+        TypeCasting.to(new TypeDescription.ForLoadedType(FuncallExpression.class)),
+        scope.loadEnvironment());
+  }
+
+  private void compileObjectMethodCall(
+      VariableScope scope,
+      DebugInfo debugInfo,
+      AstAccessors debugAccessors,
+      List<ByteCodeAppender> code)
+      throws EvalException {
+    append(code, new TextConstant(func.getName()));
+    compileArguments(scope, debugInfo, debugAccessors, code);
+    append(code, invokeObjectMethod);
+  }
+
+  private void compileGlobalFunctionCall(
+      VariableScope scope,
+      DebugInfo debugInfo,
+      AstAccessors debugAccessors,
+      List<ByteCodeAppender> code)
+      throws EvalException {
+    code.add(func.compile(scope, debugInfo));
+    append(code, debugAccessors.loadLocation, checkCallable);
+    compileArguments(scope, debugInfo, debugAccessors, code);
+    append(code, BaseFunction.call);
+  }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/IfStatement.java b/src/main/java/com/google/devtools/build/lib/syntax/IfStatement.java
index a40a4daf37..09003a637d 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/IfStatement.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/IfStatement.java
@@ -83,7 +83,7 @@ public final class IfStatement extends Statement {
     @Override
     ByteCodeAppender compile(
         VariableScope scope, Optional<LoopLabels> loopLabels, DebugInfo debugInfo)
-            throws EvalException {
+        throws EvalException {
       List<ByteCodeAppender> code = new ArrayList<>();
       for (Statement statement : stmts) {
         code.add(statement.compile(scope, loopLabels, debugInfo));
@@ -160,7 +160,7 @@ public final class IfStatement extends Statement {
   @Override
   ByteCodeAppender compile(
       VariableScope scope, Optional<LoopLabels> loopLabels, DebugInfo debugInfo)
-          throws EvalException {
+      throws EvalException {
     List<ByteCodeAppender> code = new ArrayList<>();
     LabelAdder after = new LabelAdder();
     LabelAdder nextConditionalOrElse;
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/LValue.java b/src/main/java/com/google/devtools/build/lib/syntax/LValue.java
index 1b6cf65ec5..22a3299afe 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/LValue.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/LValue.java
@@ -140,8 +140,8 @@ public class LValue implements Serializable {
    *
    * <p>The value to possibly destructure and assign must already be on the stack.
    */
-  public ByteCodeAppender compileAssignment(ASTNode node, AstAccessors debugAccessors,
-      VariableScope scope) throws EvalException {
+  public ByteCodeAppender compileAssignment(
+      ASTNode node, AstAccessors debugAccessors, VariableScope scope) throws EvalException {
     List<ByteCodeAppender> code = new ArrayList<>();
     compileAssignment(node, debugAccessors, expr, scope, code);
     return ByteCodeUtils.compoundAppender(code);
@@ -155,21 +155,19 @@ public class LValue implements Serializable {
       AstAccessors debugAccessors,
       Expression leftValue,
       VariableScope scope,
-      List<ByteCodeAppender> code) throws EvalException {
+      List<ByteCodeAppender> code)
+      throws EvalException {
     if (leftValue instanceof Identifier) {
       code.add(compileAssignment(scope, (Identifier) leftValue));
     } else if (leftValue instanceof ListLiteral) {
       List<Expression> lValueExpressions = ((ListLiteral) leftValue).getElements();
       compileAssignment(node, debugAccessors, scope, lValueExpressions, code);
     } else {
-      String message = String.format(
-          "Can't assign to expression '%s', only to variables or nested tuples of variables",
-          leftValue);
-      throw new EvalExceptionWithStackTrace(
-          new EvalException(
-              node.getLocation(),
-              message),
-              node);
+      String message =
+          String.format(
+              "Can't assign to expression '%s', only to variables or nested tuples of variables",
+              leftValue);
+      throw new EvalExceptionWithStackTrace(new EvalException(node.getLocation(), message), node);
     }
   }
 
@@ -182,14 +180,16 @@ public class LValue implements Serializable {
       AstAccessors debugAccessors,
       VariableScope scope,
       List<Expression> lValueExpressions,
-      List<ByteCodeAppender> code) throws EvalException {
+      List<ByteCodeAppender> code)
+      throws EvalException {
     InternalVariable objects = scope.freshVariable(Collection.class);
     InternalVariable iterator = scope.freshVariable(Iterator.class);
     // convert the object on the stack into a collection and store it to a variable for loading
     // multiple times below below
     code.add(new ByteCodeAppender.Simple(debugAccessors.loadLocation, EvalUtils.toCollection));
     code.add(objects.store());
-    append(code,
+    append(
+        code,
         // check that we got exactly the amount of objects in the collection that we need
         IntegerConstant.forValue(lValueExpressions.size()),
         objects.load(),
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/ListComprehension.java b/src/main/java/com/google/devtools/build/lib/syntax/ListComprehension.java
index 9573076b99..749b056f84 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/ListComprehension.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/ListComprehension.java
@@ -65,7 +65,8 @@ public final class ListComprehension extends AbstractComprehension {
       VariableScope scope,
       InternalVariable collection,
       DebugInfo debugInfo,
-      AstAccessors debugAccessors) throws EvalException {
+      AstAccessors debugAccessors)
+      throws EvalException {
     List<ByteCodeAppender> code = new ArrayList<>();
     append(code, collection.load());
     code.add(outputExpression.compile(scope, debugInfo));
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/ReturnStatement.java b/src/main/java/com/google/devtools/build/lib/syntax/ReturnStatement.java
index c1161554d0..fa1aac406f 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/ReturnStatement.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/ReturnStatement.java
@@ -84,7 +84,7 @@ public class ReturnStatement extends Statement {
   @Override
   ByteCodeAppender compile(
       VariableScope scope, Optional<LoopLabels> loopLabels, DebugInfo debugInfo)
-          throws EvalException {
+      throws EvalException {
     ByteCodeAppender compiledExpression = returnExpression.compile(scope, debugInfo);
     return new ByteCodeAppender.Compound(
         compiledExpression, new ByteCodeAppender.Simple(MethodReturn.REFERENCE));
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/Statement.java b/src/main/java/com/google/devtools/build/lib/syntax/Statement.java
index 0d8498ea8c..93f2a3a6ef 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/Statement.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/Statement.java
@@ -78,7 +78,7 @@ public abstract class Statement extends ASTNode {
    */
   ByteCodeAppender compile(
       VariableScope scope, Optional<LoopLabels> loopLabels, DebugInfo debugInfo)
-          throws EvalException {
+      throws EvalException {
     throw new UnsupportedOperationException(this.getClass().getSimpleName() + " unsupported.");
   }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/UserDefinedFunction.java b/src/main/java/com/google/devtools/build/lib/syntax/UserDefinedFunction.java
index ededb16999..cb6691c433 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/UserDefinedFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/UserDefinedFunction.java
@@ -79,10 +79,12 @@ public class UserDefinedFunction extends BaseFunction {
   private static File debugFolder;
   public static boolean enableCompiler = false;
 
-  protected UserDefinedFunction(Identifier function,
+  protected UserDefinedFunction(
+      Identifier function,
       FunctionSignature.WithValues<Object, SkylarkType> signature,
-      ImmutableList<Statement> statements, Environment.Frame definitionGlobals)
-    throws EvalException {
+      ImmutableList<Statement> statements,
+      Environment.Frame definitionGlobals)
+      throws EvalException {
     super(function.getName(), signature, function.getLocation());
     this.statements = statements;
     this.definitionGlobals = definitionGlobals;
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/compiler/Jump.java b/src/main/java/com/google/devtools/build/lib/syntax/compiler/Jump.java
index 7ab6379d19..f1b861370c 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/compiler/Jump.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/compiler/Jump.java
@@ -14,6 +14,7 @@
 package com.google.devtools.build.lib.syntax.compiler;
 
 import com.google.devtools.build.lib.syntax.Operator;
+
 import net.bytebuddy.implementation.Implementation.Context;
 import net.bytebuddy.implementation.bytecode.StackManipulation;