From aaa8416dd848b5c7ce494244c75682dc0be8fb9d Mon Sep 17 00:00:00 2001 From: attila Date: Wed, 14 Jan 2015 15:54:18 +0100 Subject: 8068573: POJO setter using [] syntax throws an exception Reviewed-by: lagergren, jlaskey --- .../dynalink/beans/AbstractJavaLinker.java | 7 +-- .../internal/dynalink/beans/OverloadedMethod.java | 1 - .../internal/dynalink/support/TypeUtilities.java | 22 ++++----- .../nashorn/internal/runtime/linker/Bootstrap.java | 6 ++- test/script/basic/JDK-8020324.js.EXPECTED | 4 +- test/script/basic/JDK-8068573.js | 57 ++++++++++++++++++++++ 6 files changed, 79 insertions(+), 18 deletions(-) create mode 100644 test/script/basic/JDK-8068573.js 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]); +} -- cgit v1.2.3