8068573: POJO setter using [] syntax throws an exception

Reviewed-by: lagergren, jlaskey

6 files changed, 79 insertions, 18 deletions

diff --git a/src/jdk/internal/dynalink/beans/AbstractJavaLinker.java b/src/jdk/internal/dynalink/beans/AbstractJavaLinker.java
index b8e8a3df..cf1b11d3 100644
--- a/src/jdk/internal/dynalink/beans/AbstractJavaLinker.java
+++ b/src/jdk/internal/dynalink/beans/AbstractJavaLinker.java
@@ -491,8 +491,9 @@ abstract class AbstractJavaLinker implements GuardingDynamicLinker {
 
                 // We want setters that conform to "Object(O, V)". Note, we aren't doing "R(O, V)" as it might not be
                 // valid for us to convert return values proactively. Also, since we don't know what setters will be
-                // invoked, we'll conservatively presume Object return type.
-                final MethodType type = callSiteDescriptor.getMethodType().changeReturnType(Object.class);
+                // invoked, we'll conservatively presume Object return type. The one exception is void return.
+                final MethodType origType = callSiteDescriptor.getMethodType();
+                final MethodType type = origType.returnType() == void.class ? origType : origType.changeReturnType(Object.class);
 
                 // What's below is basically:
                 //   foldArguments(guardWithTest(isNotNull, invoke, null|nextComponent.invocation),
@@ -508,7 +509,7 @@ abstract class AbstractJavaLinker implements GuardingDynamicLinker {
                 // Bind property setter handle to the expected setter type and linker services. Type is
                 // MethodHandle(Object, String, Object)
                 final MethodHandle boundGetter = MethodHandles.insertArguments(getPropertySetterHandle, 0,
-                        CallSiteDescriptorFactory.dropParameterTypes(callSiteDescriptor, 1, 2), linkerServices);
+                        callSiteDescriptor.changeMethodType(setterType), linkerServices);
 
                 // Cast getter to MethodHandle(O, N, V)
                 final MethodHandle typedGetter = linkerServices.asType(boundGetter, type.changeReturnType(
diff --git a/src/jdk/internal/dynalink/beans/OverloadedMethod.java b/src/jdk/internal/dynalink/beans/OverloadedMethod.java
index 70ec495a..c8d50072 100644
--- a/src/jdk/internal/dynalink/beans/OverloadedMethod.java
+++ b/src/jdk/internal/dynalink/beans/OverloadedMethod.java
@@ -123,7 +123,6 @@ class OverloadedMethod {
         varArgMethods = new ArrayList<>(methodHandles.size());
         final int argNum = callSiteType.parameterCount();
         for(MethodHandle mh: methodHandles) {
-            mh = mh.asType(mh.type().changeReturnType(commonRetType));
             if(mh.isVarargsCollector()) {
                 final MethodHandle asFixed = mh.asFixedArity();
                 if(argNum == asFixed.type().parameterCount()) {
diff --git a/src/jdk/internal/dynalink/support/TypeUtilities.java b/src/jdk/internal/dynalink/support/TypeUtilities.java
index bf4771b2..78d42ab1 100644
--- a/src/jdk/internal/dynalink/support/TypeUtilities.java
+++ b/src/jdk/internal/dynalink/support/TypeUtilities.java
@@ -118,17 +118,13 @@ public class TypeUtilities {
     public static Class<?> getCommonLosslessConversionType(final Class<?> c1, final Class<?> c2) {
         if(c1 == c2) {
             return c1;
+        } else if (c1 == void.class || c2 == void.class) {
+            return Object.class;
         } else if(isConvertibleWithoutLoss(c2, c1)) {
             return c1;
         } else if(isConvertibleWithoutLoss(c1, c2)) {
             return c2;
-        }
-        if(c1 == void.class) {
-            return c2;
-        } else if(c2 == void.class) {
-            return c1;
-        }
-        if(c1.isPrimitive() && c2.isPrimitive()) {
+        } else if(c1.isPrimitive() && c2.isPrimitive()) {
             if((c1 == byte.class && c2 == char.class) || (c1 == char.class && c2 == byte.class)) {
                 // byte + char = int
                 return int.class;
@@ -268,20 +264,24 @@ public class TypeUtilities {
     }
 
     /**
-     * Determines whether a type can be converted to another without losing any
-     * precision.
+     * Determines whether a type can be converted to another without losing any precision. As a special case,
+     * void is considered convertible only to Object and void, while anything can be converted to void. This
+     * is because a target type of void means we don't care about the value, so the conversion is always
+     * permissible.
      *
      * @param sourceType the source type
      * @param targetType the target type
      * @return true if lossless conversion is possible
      */
     public static boolean isConvertibleWithoutLoss(final Class<?> sourceType, final Class<?> targetType) {
-        if(targetType.isAssignableFrom(sourceType)) {
+        if(targetType.isAssignableFrom(sourceType) || targetType == void.class) {
             return true;
         }
         if(sourceType.isPrimitive()) {
             if(sourceType == void.class) {
-                return false; // Void can't be losslessly represented by any type
+                // Void should be losslessly representable by Object, either as null or as a custom value that
+                // can be set with DynamicLinkerFactory.setAutoConversionStrategy.
+                return targetType == Object.class;
             }
             if(targetType.isPrimitive()) {
                 return isProperPrimitiveLosslessSubtype(sourceType, targetType);
diff --git a/src/jdk/nashorn/internal/runtime/linker/Bootstrap.java b/src/jdk/nashorn/internal/runtime/linker/Bootstrap.java
index 173bfc34..cd4dd3b7 100644
--- a/src/jdk/nashorn/internal/runtime/linker/Bootstrap.java
+++ b/src/jdk/nashorn/internal/runtime/linker/Bootstrap.java
@@ -68,6 +68,8 @@ public final class Bootstrap {
 
     private static final MethodHandleFunctionality MH = MethodHandleFactory.getFunctionality();
 
+    private static final MethodHandle VOID_TO_OBJECT = MH.constant(Object.class, ScriptRuntime.UNDEFINED);
+
     /**
      * The default dynalink relink threshold for megamorphisism is 8. In the case
      * of object fields only, it is fine. However, with dual fields, in order to get
@@ -481,14 +483,16 @@ public final class Bootstrap {
     private static MethodHandle unboxReturnType(final MethodHandle target, final MethodType newType) {
         final MethodType targetType = target.type();
         final Class<?> oldReturnType = targetType.returnType();
+        final Class<?> newReturnType = newType.returnType();
         if (TypeUtilities.isWrapperType(oldReturnType)) {
-            final Class<?> newReturnType = newType.returnType();
             if (newReturnType.isPrimitive()) {
                 // The contract of setAutoConversionStrategy is such that the difference between newType and targetType
                 // can only be JLS method invocation conversions.
                 assert TypeUtilities.isMethodInvocationConvertible(oldReturnType, newReturnType);
                 return MethodHandles.explicitCastArguments(target, targetType.changeReturnType(newReturnType));
             }
+        } else if (oldReturnType == void.class && newReturnType == Object.class) {
+            return MethodHandles.filterReturnValue(target, VOID_TO_OBJECT);
         }
         return target;
     }
diff --git a/test/script/basic/JDK-8020324.js.EXPECTED b/test/script/basic/JDK-8020324.js.EXPECTED
index 500f46b9..9ebae802 100644
--- a/test/script/basic/JDK-8020324.js.EXPECTED
+++ b/test/script/basic/JDK-8020324.js.EXPECTED
@@ -17,7 +17,7 @@ bean.readWrite: 17
 bean.readWrite = 18: 18
 obj1.readWrite: 18
 obj1.getReadWrite(): 18
-obj1.setReadWrite(19): null
+obj1.setReadWrite(19): undefined
 obj1.readWrite: 19
 bean.readWrite: 19
 
@@ -52,7 +52,7 @@ PropertyBind.staticReadWrite: 25
 PropertyBind.staticReadWrite = 26: 26
 obj2.staticReadWrite: 26
 obj2.getStaticReadWrite(): 26
-obj2.setStaticReadWrite(27): null
+obj2.setStaticReadWrite(27): undefined
 obj2.staticReadWrite: 27
 PropertyBind.staticReadWrite: 27
 
diff --git a/test/script/basic/JDK-8068573.js b/test/script/basic/JDK-8068573.js
new file mode 100644
index 00000000..73f71582
--- /dev/null
+++ b/test/script/basic/JDK-8068573.js
@@ -0,0 +1,57 @@
+/*
+ * Copyright (c) 2014 Oracle and/or its affiliates. All rights reserved.
+ * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+ * 
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ * 
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ * 
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ * 
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ */
+
+/**
+ * JDK-8068573: POJO setter using [] syntax throws an exception
+ *
+ * @test
+ * @run
+ */
+
+// Invoke a setter using []. It's important that the setter returns void.
+var pb = new (Java.type("jdk.nashorn.test.models.PropertyBind"))
+var n = "writeOnly";
+pb[n] = 2;
+Assert.assertEquals(pb.peekWriteOnly(), 2);
+
+// Invoke an overloaded setter using []. It's important that one of the 
+// overloads returns void.
+var os = new (Java.type("jdk.nashorn.test.models.OverloadedSetter"))
+var n2 = "color";
+os[n2] = 3; // exercise int overload
+Assert.assertEquals(os.peekColor(), "3");
+os[n2] = "blue";  // exercise string overload
+Assert.assertEquals(os.peekColor(), "blue");
+for each(var x in [42, "42"]) {
+  os[n2] = x; // exercise both overloads in the same call site
+  Assert.assertEquals(os.peekColor(), "42");
+}
+
+// Invoke an overloaded method using [], repeatedly in the same call 
+// site. It's important that one of the overloads returns void.
+var n3="foo";
+var param=["xyz", 1, "zyx", 2];
+var expected=["boo", void 0, "boo", void 0];
+for(var i in param) {
+  Assert.assertEquals(os[n3](param[i]), expected[i]);
+}