Refactor FuncallExpression to allow for complex function terms later.

RELNOTES: None.
PiperOrigin-RevId: 164981071

6 files changed, 71 insertions, 66 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryModule.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryModule.java
index f64781b93b..9d123ca6a3 100644
--- a/src/main/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryModule.java
+++ b/src/main/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryModule.java
@@ -42,6 +42,7 @@ import com.google.devtools.build.lib.syntax.Runtime;
 import com.google.devtools.build.lib.syntax.SkylarkDict;
 import com.google.devtools.build.lib.syntax.SkylarkList;
 import com.google.devtools.build.lib.syntax.SkylarkSignatureProcessor;
+import com.google.devtools.build.lib.util.Preconditions;
 import java.util.Map;
 
 /**
@@ -155,7 +156,7 @@ public class SkylarkRepositoryModule {
     public Object call(
         Object[] args, FuncallExpression ast, com.google.devtools.build.lib.syntax.Environment env)
         throws EvalException, InterruptedException {
-      String ruleClassName = ast.getFunction().getName();
+      String ruleClassName = Preconditions.checkNotNull(ast.getFunctionNameIfPossible());
       try {
         RuleClass ruleClass = builder.build(ruleClassName);
         PackageContext context = PackageFactory.getContext(env, ast);
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 ab68d65b9e..6bfa55ad1d 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
@@ -186,24 +186,48 @@ public final class FuncallExpression extends Expression {
     }
   }
 
-  @Nullable private final Expression object;
-
-  private final Identifier function;
+  private final Expression function;
 
   private final List<Argument.Passed> arguments;
 
   private final int numPositionalArgs;
 
-  public FuncallExpression(@Nullable Expression object, Identifier function,
-                           List<Argument.Passed> arguments) {
-    this.object = object;
-    this.function = function;
-    this.arguments = arguments;
+  public FuncallExpression(Expression function, List<Argument.Passed> arguments) {
+    this.function = Preconditions.checkNotNull(function);
+    this.arguments = Preconditions.checkNotNull(arguments);
     this.numPositionalArgs = countPositionalArguments();
   }
 
-  public FuncallExpression(Identifier function, List<Argument.Passed> arguments) {
-    this(null, function, arguments);
+  /** Returns the function that is called. */
+  public Expression getFunction() {
+    return this.function;
+  }
+
+  /**
+   * Returns the name of the called function if it's available in the AST.
+   *
+   * <p>It may not be available in cases like `list[0](arg1, arg2)`.
+   */
+  @Nullable
+  public String getFunctionNameIfPossible() {
+    Identifier ident = getFunctionIdentifierIfPossible();
+    return ident == null ? null : ident.getName();
+  }
+
+  /**
+   * Returns the identifier of the called function if it's available in the AST.
+   *
+   * <p>It may not be available in cases like `list[0](arg1, arg2)`.
+   */
+  @Nullable
+  public Identifier getFunctionIdentifierIfPossible() {
+    if (function instanceof Identifier) {
+      return (Identifier) function;
+    }
+    if (function instanceof DotExpression) {
+      return ((DotExpression) function).getField();
+    }
+    return null;
   }
 
   /**
@@ -220,22 +244,6 @@ public final class FuncallExpression extends Expression {
   }
 
   /**
-   * Returns the function expression.
-   */
-  public Identifier getFunction() {
-    return function;
-  }
-
-  /**
-   * Returns the object the function called on.
-   * It's null if the function is not called on an object.
-   */
-  @Nullable
-  public Expression getObject() {
-    return object;
-  }
-
-  /**
    * Returns an (immutable, ordered) list of function arguments. The first n are
    * positional and the remaining ones are keyword args, where n =
    * getNumPositionalArguments().
@@ -254,10 +262,6 @@ public final class FuncallExpression extends Expression {
 
    @Override
    public void prettyPrint(Appendable buffer) throws IOException {
-     if (object != null) {
-       object.prettyPrint(buffer);
-       buffer.append('.');
-     }
      function.prettyPrint(buffer);
      buffer.append('(');
      String sep = "";
@@ -272,9 +276,6 @@ public final class FuncallExpression extends Expression {
   @Override
   public String toString() {
     Printer.LengthLimitedPrinter printer = new Printer.LengthLimitedPrinter();
-    if (object != null) {
-      printer.append(object.toString()).append(".");
-    }
     printer.append(function.toString());
     printer.printAbbreviatedList(arguments, "(", ", ", ")", null,
         Printer.SUGGESTED_CRITICAL_LIST_ELEMENTS_COUNT,
@@ -381,7 +382,8 @@ public final class FuncallExpression extends Expression {
                   getLocation(),
                   String.format(
                       "type '%s' has multiple matches for function %s",
-                      EvalUtils.getDataTypeNameFromClass(objClass), formatMethod(args, kwargs)));
+                      EvalUtils.getDataTypeNameFromClass(objClass),
+                      formatMethod(methodName, args, kwargs)));
             }
           }
         }
@@ -396,14 +398,15 @@ public final class FuncallExpression extends Expression {
         errorMessage =
             String.format(
                 "type '%s' has no method %s",
-                EvalUtils.getDataTypeNameFromClass(objClass), formatMethod(args, kwargs));
+                EvalUtils.getDataTypeNameFromClass(objClass),
+                formatMethod(methodName, args, kwargs));
 
       } else {
         errorMessage =
             String.format(
                 "%s, in method %s of '%s'",
                 argumentListConversionResult.getError(),
-                formatMethod(args, kwargs),
+                formatMethod(methodName, args, kwargs),
                 EvalUtils.getDataTypeNameFromClass(objClass));
       }
       throw new EvalException(getLocation(), errorMessage);
@@ -513,9 +516,9 @@ public final class FuncallExpression extends Expression {
     return ArgumentListConversionResult.fromArgumentList(builder.build());
   }
 
-  private String formatMethod(List<Object> args, Map<String, Object> kwargs) {
+  private static String formatMethod(String name, List<Object> args, Map<String, Object> kwargs) {
     StringBuilder sb = new StringBuilder();
-    sb.append(function.getName()).append("(");
+    sb.append(name).append("(");
     boolean first = true;
     for (Object obj : args) {
       if (!first) {
@@ -713,7 +716,7 @@ public final class FuncallExpression extends Expression {
         addKeywordArg(kwargs, arg.getName(), value, duplicates);
       }
     }
-    checkDuplicates(duplicates, function.getName(), getLocation());
+    checkDuplicates(duplicates, function.toString(), getLocation());
   }
 
   @VisibleForTesting
@@ -724,14 +727,20 @@ public final class FuncallExpression extends Expression {
 
   @Override
   Object doEval(Environment env) throws EvalException, InterruptedException {
-    return (object != null) ? invokeObjectMethod(env) : invokeGlobalFunction(env);
+    if (function instanceof DotExpression) {
+      return invokeObjectMethod(env, (DotExpression) function);
+    }
+    if (function instanceof Identifier) {
+      return invokeGlobalFunction(env);
+    }
+    throw new EvalException(
+        getLocation(), Printer.format("cannot evaluate function '%s'", function));
   }
 
-  /**
-   * Invokes object.function() and returns the result.
-   */
-  private Object invokeObjectMethod(Environment env) throws EvalException, InterruptedException {
-    Object objValue = object.eval(env);
+  /** Invokes object.function() and returns the result. */
+  private Object invokeObjectMethod(Environment env, DotExpression dot)
+      throws EvalException, InterruptedException {
+    Object objValue = dot.getObject().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
@@ -739,7 +748,7 @@ public final class FuncallExpression extends Expression {
     Map<String, Object> kwargs = new LinkedHashMap<>();
     evalArguments(posargs, kwargs, env);
     return invokeObjectMethod(
-        function.getName(), posargs.build(), ImmutableMap.copyOf(kwargs), this, env);
+        dot.getField().getName(), posargs.build(), ImmutableMap.copyOf(kwargs), this, env);
   }
 
   /**
@@ -789,11 +798,7 @@ public final class FuncallExpression extends Expression {
 
   @Override
   void validate(ValidationEnvironment env) throws EvalException {
-    if (object != null) {
-      object.validate(env);
-    } else {
-      function.validate(env);
-    }
+    function.validate(env);
     for (Argument.Passed arg : arguments) {
       arg.getValue().validate(env);
     }
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/Parser.java b/src/main/java/com/google/devtools/build/lib/syntax/Parser.java
index e3b14aa32d..8f5e301fde 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/Parser.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/Parser.java
@@ -447,7 +447,8 @@ public class Parser {
     if (function.getLocation() == null) {
       function = setLocation(function, start, end);
     }
-    return setLocation(new FuncallExpression(receiver, function, args), start, end);
+    Expression fun = receiver == null ? function : new DotExpression(receiver, function);
+    return setLocation(new FuncallExpression(fun, args), start, end);
   }
 
   // arg ::= IDENTIFIER '=' nontupleexpr
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/SyntaxTreeVisitor.java b/src/main/java/com/google/devtools/build/lib/syntax/SyntaxTreeVisitor.java
index 345b4bbe96..b23b048473 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/SyntaxTreeVisitor.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/SyntaxTreeVisitor.java
@@ -56,9 +56,6 @@ public class SyntaxTreeVisitor {
   }
 
   public void visit(FuncallExpression node) {
-    if (node.getObject() != null) {
-      visit(node.getObject());
-    }
     visit(node.getFunction());
     visitAll(node.getArguments());
   }
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 dd30a41410..becaab2ac8 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
@@ -694,7 +694,7 @@ public class EvaluationTest extends EvaluationTestCase {
 
   @Test
   public void testDictKeysDuplicateKeyArgs() throws Exception {
-    newTest().testIfExactError("duplicate keywords 'arg', 'k' in call to keys",
+    newTest().testIfExactError("duplicate keywords 'arg', 'k' in call to {\"a\": 1}.keys",
         "{'a': 1}.keys(arg='abc', arg='def', k=1, k=2)");
   }
 
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 fc5e5867dc..c616b2ad78 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
@@ -176,7 +176,7 @@ public class ParserTest extends EvaluationTestCase {
   public void testFuncallExpr() throws Exception {
     FuncallExpression e = (FuncallExpression) parseExpression("foo(1, 2, bar=wiz)");
 
-    Identifier ident = e.getFunction();
+    Identifier ident = (Identifier) e.getFunction();
     assertThat(ident.getName()).isEqualTo("foo");
 
     assertThat(e.getArguments()).hasSize(3);
@@ -199,8 +199,8 @@ public class ParserTest extends EvaluationTestCase {
     FuncallExpression e =
       (FuncallExpression) parseExpression("foo.foo(1, 2, bar=wiz)");
 
-    Identifier ident = e.getFunction();
-    assertThat(ident.getName()).isEqualTo("foo");
+    DotExpression dotExpression = (DotExpression) e.getFunction();
+    assertThat(dotExpression.getField().getName()).isEqualTo("foo");
 
     assertThat(e.getArguments()).hasSize(3);
     assertThat(e.getNumPositionalArguments()).isEqualTo(2);
@@ -222,8 +222,8 @@ public class ParserTest extends EvaluationTestCase {
     FuncallExpression e =
       (FuncallExpression) parseExpression("foo.replace().split(1)");
 
-    Identifier ident = e.getFunction();
-    assertThat(ident.getName()).isEqualTo("split");
+    DotExpression dotExpr = (DotExpression) e.getFunction();
+    assertThat(dotExpr.getField().getName()).isEqualTo("split");
 
     assertThat(e.getArguments()).hasSize(1);
     assertThat(e.getNumPositionalArguments()).isEqualTo(1);
@@ -244,8 +244,8 @@ public class ParserTest extends EvaluationTestCase {
   public void testStringMethExpr() throws Exception {
     FuncallExpression e = (FuncallExpression) parseExpression("'foo'.foo()");
 
-    Identifier ident = e.getFunction();
-    assertThat(ident.getName()).isEqualTo("foo");
+    DotExpression dotExpression = (DotExpression) e.getFunction();
+    assertThat(dotExpression.getField().getName()).isEqualTo("foo");
 
     assertThat(e.getArguments()).isEmpty();
   }
@@ -291,7 +291,8 @@ public class ParserTest extends EvaluationTestCase {
 
     FuncallExpression e = (FuncallExpression) parseExpression(
         "'FOO.CC'.lower()[1:].startswith('oo')");
-    assertThat(e.getFunction().getName()).isEqualTo("startswith");
+    DotExpression dotExpression = (DotExpression) e.getFunction();
+    assertThat(dotExpression.getField().getName()).isEqualTo("startswith");
     assertThat(e.getArguments()).hasSize(1);
 
     s = (SliceExpression) parseExpression("'FOO.CC'[1:][:2]");
@@ -336,7 +337,7 @@ public class ParserTest extends EvaluationTestCase {
 
     // Test that the actual parameters are: (1, $error$, 3):
 
-    Identifier ident = e.getFunction();
+    Identifier ident = (Identifier) e.getFunction();
     assertThat(ident.getName()).isEqualTo("f");
 
     assertThat(e.getArguments()).hasSize(3);