Fix native JsMethods with varargs called from Java varargs methods (#…

…9957)

Native jsinterop varargs calls of the form `instance.method(argsArray)`
sometimes implemented using `instance.method.apply(instance,
argsArray)`, where instance represents some expression.
JsUtils.createApplyInvocation assumes that ImplementJsVarargs has
already created a local variable to ensure that no side effects can take
place when cloning the "instance", but ImplementJsVarargs doesn't
actually check if a side effect was able to happen.

This fix only adds an additional check, creating a local variable in the
case where the instance expression has side effects, and clarifies that
a native JsProperty might have sideeffects (as it could call a JS
property accessor).

An additional fix is made to JsSafeCloner, to let it successfully clone
a qualified reference (e.g. `$wnd.console`) rather than produce an
invalid JS AST. This ended up not being strictly required for the fix,
but appears to be more correct and will at least generate code rather
than a NullPointerException in a later location.

Fixes #9932

dev/core/src/com/google/gwt/dev/jjs/impl/GwtAstBuilder.java

@@ -3315,13 +3315,13 @@ private List<JExpression> popCallArguments(SourceInfo info, Expression[] argumen
       List<JExpression> args = pop(arguments);
       for (int i = 0; i < args.size(); i++) {
         // Account for varargs parameter.
-        int parameterIndex = Math.min(i, methodBinding.parameters.length - 1);
+        int parameterIndex = Math.min(i, methodBinding.original().parameters.length - 1);
         args.set(i, maybeBoxOrUnbox(
             args.get(i),
             arguments[i].implicitConversion,
             isDoNotAutoBoxParameter(methodBinding, parameterIndex)));
       }
-      if (!methodBinding.isVarargs()) {
+      if (!methodBinding.original().isVarargs()) {
         return args;
       }
 
@@ -3331,7 +3331,8 @@ private List<JExpression> popCallArguments(SourceInfo info, Expression[] argumen
         args = Lists.newArrayListWithCapacity(1);
       }
 
-      TypeBinding[] params = methodBinding.parameters;
+      TypeBinding[] params = methodBinding.isVarargs() ? methodBinding.parameters :
+              methodBinding.original().parameters;
       int varArg = params.length - 1;
 
       // See if there's a single varArg which is already an array.

dev/core/src/com/google/gwt/dev/jjs/impl/ImplementJsVarargs.java

@@ -296,7 +296,7 @@ public boolean visit(JMethod x, Context ctx) {
       varargsParameter = Iterables.getLast(x.getParams());
       varargsIndex = x.getParams().size() - 1;
 
-      // JsVarargs parameter can be assumend not null in the implementing method
+      // JsVarargs parameter can be assumed not null in the implementing method
       varargsParameter.setType(varargsParameter.getType().strengthenToNonNull());
 
       argumentsCopyVariable = null;
@@ -462,8 +462,8 @@ public void endVisit(JMethodCall x, Context ctx) {
       // Passed as an array to varargs method will result in an apply call, in which case hoist the
       // qualifier to make sure it is only evaluated once.
       JExpression instance = x.getInstance();
-      if (x.getTarget().needsDynamicDispatch() && !x.isStaticDispatchOnly()
-          && instance != null && !(instance instanceof JVariableRef)) {
+      if (x.getTarget().needsDynamicDispatch() && !x.isStaticDispatchOnly() && instance != null
+              && !(instance instanceof JVariableRef && !instance.hasSideEffects())) {
         // Move the potentially sideffecting qualifier to a temporary variable so that
         // the code generation for calls that need .apply don't need to hande the case.
         SourceInfo sourceInfo = x.getSourceInfo();

dev/core/src/com/google/gwt/dev/js/JsSafeCloner.java

@@ -143,13 +143,17 @@ public void endVisit(JsNameOf x, JsContext ctx) {
      */
     @Override
     public void endVisit(JsNameRef x, JsContext ctx) {
-      if (x.getQualifier() == null && x.getIdent() == "arguments") {
+      if (x.getQualifier() == null && "arguments".equals(x.getIdent())) {
         // References to the arguments object can not be hoisted.
         successful = false;
         stack.push(null);
       }
-      JsNameRef toReturn = new JsNameRef(x.getSourceInfo(), x.getName());
-
+      final JsNameRef toReturn;
+      if (x.getName() == null) {
+        toReturn = new JsNameRef(x.getSourceInfo(), x.getIdent(), x.getQualifier());
+      } else {
+        toReturn = new JsNameRef(x.getSourceInfo(), x.getName());
+      }
       if (x.getQualifier() != null) {
         toReturn.setQualifier(stack.pop());
       }

user/test/com/google/gwt/core/interop/JsTypeVarargsTest.java

@@ -20,10 +20,12 @@
 import com.google.gwt.core.client.ScriptInjector;
 import com.google.gwt.junit.client.GWTTestCase;
 
+import java.util.Objects;
 import javaemul.internal.annotations.DoNotInline;
 import jsinterop.annotations.JsFunction;
 import jsinterop.annotations.JsMethod;
 import jsinterop.annotations.JsPackage;
+import jsinterop.annotations.JsProperty;
 import jsinterop.annotations.JsType;
 
 /**
@@ -321,4 +323,111 @@ public static int varargJsMethodUninstantiatedVararg(
   public native void testVarargsCall_uninstantiatedVararg() /*-{
     @GWTTestCase::assertEquals(II)(0, $global.varargJsMethodUninstantiatedVararg());
   }-*/;
+
+  // https://github.com/gwtproject/gwt/issues/9932
+  public void testVarargsFromJavaToJsinterop() {
+    assertEquals(3, nonNativeMethod("A", "B", "C"));
+  }
+
+  // Java declaration of globally available instance method that takes varargs
+  @JsType(namespace = JsPackage.GLOBAL)
+  public static class VarArgsQualifiedInstanceMethod {
+    @JsProperty(namespace = JsPackage.GLOBAL)
+    public static VarArgsQualifiedInstanceMethod INSTANCE = new VarArgsQualifiedInstanceMethod();
+    public int getLength(Object... values) {
+      return values.length;
+    }
+  }
+
+  // Declaring this type lets us use jsinterop to call the above method.
+  @JsType(isNative = true, namespace = JsPackage.GLOBAL, name = "VarArgsQualifiedInstanceMethod")
+  public static class VarArgsFromJava {
+    @JsProperty(namespace = JsPackage.GLOBAL)
+    public static VarArgsFromJava INSTANCE;
+    public native int getLength(Object... values);
+  }
+
+  // This plain Java method accepts varargs, and tries to pass them into jsinterop.
+  private static int nonNativeMethod(Object... values) {
+    return VarArgsFromJava.INSTANCE.getLength(values);
+  }
+
+  public void testVarargsObjects() {
+    assertEquals(new VarargsSummary<>(1, null, null),
+            varargInstance().acceptsObjects((Object) null));
+    assertEquals(new VarargsSummary<>(0, null, null),
+            varargInstance().acceptsObjects());
+    assertEquals(new VarargsSummary<>(1, String.class, null),
+            varargInstance().acceptsObjects("hello"));
+    assertEquals(new VarargsSummary<>(2, String.class, null),
+            varargInstance().acceptsObjects("hello", "world"));
+    // noinspection ConfusingArgumentToVarargsMethod
+    assertEquals(new VarargsSummary<>(2, String.class, null),
+            varargInstance().acceptsObjects(new String[]{"hello", "world"}));
+    assertEquals(new VarargsSummary<>(2, String.class, null),
+            varargInstance().acceptsObjects(new Object[]{"hello", "world"}));
+  }
+
+    private static VarargMethodHolderFromJava varargInstance() {
+    return new VarargMethodHolderFromJava();
+  }
+
+  // Java impl of the jsinterop type we'll call below.
+  @JsType(namespace = JsPackage.GLOBAL)
+  public static class VarargMethodHolder {
+    public VarargsSummary<Void> acceptsObjects(Object... values) {
+      // Note that unlike the VarargsTest version, values can never be null, and the values
+      // array is always Object[] since it is spliced from js's "arguments".
+      assertNotNull(values);
+      return new VarargsSummary<>(
+              values.length,
+              (values.length == 0 || values[0] == null) ? null : values[0].getClass(),
+              null);
+    }
+  }
+
+  // Declaring this type lets us use jsinterop to call the above method.
+  @JsType(isNative = true, namespace = JsPackage.GLOBAL, name = "VarargMethodHolder")
+  public static class VarargMethodHolderFromJava {
+    public native VarargsSummary<Void> acceptsObjects(Object... values);
+  }
+
+  @JsType
+  public static final class VarargsSummary<T> {
+    private final int count;
+    private final Class<?> firstItemType;
+    private final T value;
+
+    public VarargsSummary(int count, Class<?> firstItemType, T value) {
+      this.count = count;
+      this.firstItemType = firstItemType;
+      this.value = value;
+    }
+
+    @Override
+    public boolean equals(Object o) {
+      if (this == o) {
+        return true;
+      }
+      if (o == null || getClass() != o.getClass()) {
+        return false;
+      }
+      VarargsSummary<?> that = (VarargsSummary<?>) o;
+      return count == that.count
+              && Objects.equals(firstItemType, that.firstItemType)
+              && Objects.equals(value, that.value);
+    }
+
+    @Override
+    public int hashCode() {
+      return Objects.hash(count, firstItemType, value);
+    }
+
+    @Override
+    public String toString() {
+      return "count=" + count +
+              ", firstItemType=" + firstItemType +
+              ", value=" + value;
+    }
+  }
 }

user/test/com/google/gwt/dev/jjs/test/VarargsTest.java

@@ -18,6 +18,7 @@
 import com.google.gwt.junit.client.GWTTestCase;
 
 import java.util.Arrays;
+import java.util.Objects;
 
 /**
  * Tests the new JDK 1.5 varargs functionality.
@@ -87,4 +88,153 @@ private Foo[] fooIdent(Foo[] args) {
   private int[] varargUnboxed(int... args) {
     return args;
   }
+
+  public void testVarargsObjects() {
+    assertEquals(new VarargsSummary<>(1, Object[].class, null),
+            acceptsObjects((Object) null));
+    // noinspection ConfusingArgumentToVarargsMethod
+    assertEquals(new VarargsSummary<>(-1, null, null),
+            acceptsObjects(null));
+    assertEquals(new VarargsSummary<>(-1, null, null),
+            acceptsObjects((Object[]) null));
+    assertEquals(new VarargsSummary<>(0, Object[].class, null),
+            acceptsObjects());
+    assertEquals(new VarargsSummary<>(1, Object[].class, null),
+            acceptsObjects("hello"));
+    assertEquals(new VarargsSummary<>(2, Object[].class, null),
+            acceptsObjects("hello", "world"));
+    // noinspection ConfusingArgumentToVarargsMethod
+    assertEquals(new VarargsSummary<>(2, String[].class, null),
+            acceptsObjects(new String[]{"hello", "world"}));
+    assertEquals(new VarargsSummary<>(2, Object[].class, null),
+            acceptsObjects(new Object[]{"hello", "world"}));
+  }
+
+  private VarargsSummary<Void> acceptsObjects(Object... values) {
+    return new VarargsSummary<>(
+            values == null ? -1 : values.length,
+            values == null ? null : values.getClass(),
+            null);
+  }
+
+  public void testVarargsObjectsWithOtherParam() {
+    assertEquals(new VarargsSummary<>(1, Object[].class, 1),
+            acceptsObjectsAndOtherParam(1, (Object) null));
+    // noinspection ConfusingArgumentToVarargsMethod
+    assertEquals(new VarargsSummary<>(-1, null, 2),
+            acceptsObjectsAndOtherParam(2, null));
+    assertEquals(new VarargsSummary<>(-1, null, 3),
+            acceptsObjectsAndOtherParam(3, (Object[]) null));
+    assertEquals(new VarargsSummary<>(0, Object[].class, 4),
+            acceptsObjectsAndOtherParam(4));
+    assertEquals(new VarargsSummary<>(1, Object[].class, 5),
+            acceptsObjectsAndOtherParam(5, "hello"));
+    assertEquals(new VarargsSummary<>(2, Object[].class, 6),
+            acceptsObjectsAndOtherParam(6, "hello", "world"));
+    // noinspection ConfusingArgumentToVarargsMethod
+    assertEquals(new VarargsSummary<>(2, String[].class, 7),
+            acceptsObjectsAndOtherParam(7, new String[]{"hello", "world"}));
+    assertEquals(new VarargsSummary<>(2, Object[].class, 8),
+            acceptsObjectsAndOtherParam(8, new Object[]{"hello", "world"}));
+  }
+
+  private VarargsSummary<Integer> acceptsObjectsAndOtherParam(int number, Object... values) {
+    return new VarargsSummary<>(
+            values == null ? -1 : values.length,
+            values == null ? null : values.getClass(),
+            number);
+  }
+
+  public void testObjectsWithOtherVarargsParam() {
+    assertEquals(new VarargsSummary<>(1, Object[].class, 1),
+            acceptsObjectsAndGenericParam(1, (Object) null));
+    // noinspection ConfusingArgumentToVarargsMethod
+    assertEquals(new VarargsSummary<>(-1, null, "2"),
+            acceptsObjectsAndGenericParam("2", null));
+    assertEquals(new VarargsSummary<>(-1, null, null),
+            acceptsObjectsAndGenericParam(null, (Object[]) null));
+    assertEquals(new VarargsSummary<>(0, Object[].class, 4),
+            acceptsObjectsAndGenericParam(4));
+    assertEquals(new VarargsSummary<>(1, Object[].class, 5),
+            acceptsObjectsAndGenericParam(5, "hello"));
+    assertEquals(new VarargsSummary<>(2, Object[].class, 6),
+            acceptsObjectsAndGenericParam(6, "hello", "world"));
+    // noinspection ConfusingArgumentToVarargsMethod
+    assertEquals(new VarargsSummary<>(2, String[].class, 7),
+            acceptsObjectsAndGenericParam(7, new String[]{"hello", "world"}));
+    assertEquals(new VarargsSummary<>(2, Object[].class, 8),
+            acceptsObjectsAndGenericParam(8, new Object[]{"hello", "world"}));
+  }
+
+  private <T> VarargsSummary<T> acceptsObjectsAndGenericParam(T generic, Object... values) {
+    return new VarargsSummary<>(
+            values == null ? -1 : values.length,
+            values == null ? null : values.getClass(),
+            generic);
+  }
+
+  public void testGenericVarargsWithOtherParam() {
+    assertEquals(new VarargsSummary<>(1, Object[].class, 1),
+            acceptsGenericVarargsAndOtherParam(1, (Object) null));
+    // noinspection ConfusingArgumentToVarargsMethod
+    assertEquals(new VarargsSummary<>(-1, null, 2),
+            acceptsGenericVarargsAndOtherParam(2, null));
+    assertEquals(new VarargsSummary<>(-1, null, 3),
+            acceptsGenericVarargsAndOtherParam(3, (Object[]) null));
+    assertEquals(new VarargsSummary<>(0, Object[].class, 4),
+            acceptsGenericVarargsAndOtherParam(4));
+    assertEquals(new VarargsSummary<>(1, String[].class, 5),
+            acceptsGenericVarargsAndOtherParam(5, "hello"));
+    assertEquals(new VarargsSummary<>(2, String[].class, 6),
+            acceptsGenericVarargsAndOtherParam(6, "hello", "world"));
+    assertEquals(new VarargsSummary<>(2, String[].class, 7),
+            acceptsGenericVarargsAndOtherParam(7, new String[]{"hello", "world"}));
+    assertEquals(new VarargsSummary<>(2, Object[].class, 8),
+            acceptsGenericVarargsAndOtherParam(8, new Object[]{"hello", "world"}));
+  }
+
+  private <T> VarargsSummary<Integer> acceptsGenericVarargsAndOtherParam(int number, T... values) {
+    return new VarargsSummary<>(
+            values == null ? -1 : values.length,
+            values == null ? null : values.getClass(),
+            number);
+  }
+
+  public static final class VarargsSummary<T> {
+    private final int count;
+    private final Class<?> paramType;
+    private final T value;
+
+    public VarargsSummary(int count, Class<?> paramType, T value) {
+      this.count = count;
+      this.paramType = paramType;
+      this.value = value;
+    }
+
+    @Override
+    public boolean equals(Object o) {
+      if (this == o) {
+        return true;
+      }
+      if (o == null || getClass() != o.getClass()) {
+        return false;
+      }
+      VarargsSummary<?> that = (VarargsSummary<?>) o;
+      return count == that.count
+              && Objects.equals(paramType, that.paramType)
+              && Objects.equals(value, that.value);
+    }
+
+    @Override
+    public int hashCode() {
+      return Objects.hash(count, paramType, value);
+    }
+
+    @Override
+    public String toString() {
+      return "count=" + count +
+              ", paramType=" + paramType +
+              ", value=" + value;
+    }
+  }
 }