Skylark: fix auto type conversion between the BUILD language and Skylark. SkylarkList#toList() behaves consistently for all implementations.

--
MOS_MIGRATED_REVID=87817550

2 files changed, 25 insertions, 11 deletions

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 3ef5973d44..fc228227c5 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
@@ -470,11 +470,13 @@ public final class FuncallExpression extends Expression {
     // we'd still have to have a HashMap on the side for the sake of properly handling duplicates.
     Map<String, Object> kwargs = new HashMap<>();
 
+    final Object returnValue;
+    final Function function;
     if (obj != null) { // obj.func(...)
       Object objValue = obj.eval(env);
       // Strings, lists and dictionaries (maps) have functions that we want to use in MethodLibrary.
       // For other classes, we can call the Java methods.
-      Function function =
+      function =
           env.getFunction(EvalUtils.getSkylarkType(objValue.getClass()), func.getName());
       if (function != null) {
         if (!isNamespace(objValue.getClass())) {
@@ -482,8 +484,8 @@ public final class FuncallExpression extends Expression {
           posargs.add(objValue);
         }
         evalArguments(posargs, kwargs, env, function);
-        return EvalUtils.checkNotNull(this,
-            function.call(posargs.build(), ImmutableMap.<String, Object>copyOf(kwargs), this, env));
+        returnValue = function.call(
+            posargs.build(), ImmutableMap.<String, Object>copyOf(kwargs), this, env);
       } else if (env.isSkylarkEnabled()) {
         // Only allow native Java calls when using Skylark
         // When calling a Java method, the name is not in the Environment,
@@ -496,7 +498,8 @@ public final class FuncallExpression extends Expression {
                   func.getName(), objValue, EvalUtils.getDataTypeName(objValue)));
         }
         if (objValue instanceof Class<?>) {
-          // Static Java method call
+          // Static Java method call. We can return the value from here directly because
+          // invokeJavaMethod() has special checks.
           return invokeJavaMethod(null, (Class<?>) objValue, func.getName(), posargs.build());
         } else {
           return invokeJavaMethod(objValue, objValue.getClass(), func.getName(), posargs.build());
@@ -509,16 +512,24 @@ public final class FuncallExpression extends Expression {
     } else { // func(...)
       Object funcValue = func.eval(env);
       if ((funcValue instanceof Function)) {
-        Function function = (Function) funcValue;
+        function = (Function) funcValue;
         evalArguments(posargs, kwargs, env, function);
-        return EvalUtils.checkNotNull(this,
-            function.call(posargs.build(), ImmutableMap.<String, Object>copyOf(kwargs), this, env));
+        returnValue = function.call(
+            posargs.build(), ImmutableMap.<String, Object>copyOf(kwargs), this, env);
       } else {
         throw new EvalException(getLocation(),
             "'" + EvalUtils.getDataTypeName(funcValue)
             + "' object is not callable");
       }
     }
+
+    EvalUtils.checkNotNull(this, returnValue);
+    if (!env.isSkylarkEnabled()) {
+      // The call happens in the BUILD language. Note that accessing "BUILD language" functions in
+      // Skylark should never happen.
+      return SkylarkType.convertFromSkylark(returnValue);
+    }
+    return returnValue;
   }
 
   private ArgConversion getArgConversion(Function function) {
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkList.java b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkList.java
index 9e9788a1d4..42cf163f85 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkList.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkList.java
@@ -86,7 +86,8 @@ public abstract class SkylarkList implements Iterable<Object> {
   // TODO(bazel-team): we should be very careful using this method. Check and remove
   // auto conversions on the Java-Skylark interface if possible.
   /**
-   * Converts this Skylark list to a Java list.
+   * Returns a mutable Java list copy of this SkylarkList if it's a list or an
+   * immutable copy if it's a tuple.
    */
   public abstract List<Object> toList();
 
@@ -235,7 +236,8 @@ public abstract class SkylarkList implements Iterable<Object> {
 
     @Override
     public List<Object> toList() {
-      return getList();
+      ImmutableList<Object> result = getList();
+      return isTuple() ? result : Lists.newArrayList(result);
     }
 
     private ImmutableList<Object> getList() {
@@ -291,8 +293,9 @@ public abstract class SkylarkList implements Iterable<Object> {
     }
 
     @Override
-    public ImmutableList<Object> toList() {
-      return ImmutableList.<Object>builder().addAll(left).addAll(right).build();
+    public List<Object> toList() {
+      List<Object> result = ImmutableList.<Object>builder().addAll(left).addAll(right).build();
+      return isTuple() ? result : Lists.newArrayList(result);
     }
   }