# HG changeset patch # User attila # Date 1381845434 -7200 # Node ID 64e841576c6837a869d55af477ed35b1a2fce91b # Parent d155c4a7703cec44d49ad6fb7dcfbbe70d07e17a 8026397: Fix ambiguity with array conversion, including passing JS NativeArrays in Java variable arity methods' vararg array position Reviewed-by: jlaskey, sundar diff -r d155c4a7703c -r 64e841576c68 src/jdk/internal/dynalink/beans/SingleDynamicMethod.java --- a/src/jdk/internal/dynalink/beans/SingleDynamicMethod.java Mon Oct 14 12:41:11 2013 +0200 +++ b/src/jdk/internal/dynalink/beans/SingleDynamicMethod.java Tue Oct 15 15:57:14 2013 +0200 @@ -91,6 +91,7 @@ import jdk.internal.dynalink.CallSiteDescriptor; import jdk.internal.dynalink.linker.LinkerServices; import jdk.internal.dynalink.support.Guards; +import jdk.internal.dynalink.support.Lookup; /** * Base class for dynamic methods that dispatch to a single target Java method or constructor. Handles adaptation of the @@ -100,6 +101,9 @@ * @version $Id: $ */ abstract class SingleDynamicMethod extends DynamicMethod { + + private static final MethodHandle CAN_CONVERT_TO = Lookup.findOwnStatic(MethodHandles.lookup(), "canConvertTo", boolean.class, LinkerServices.class, Class.class, Object.class); + SingleDynamicMethod(String name) { super(name); } @@ -201,23 +205,69 @@ return createConvertingInvocation(target, linkerServices, callSiteType).asVarargsCollector( callSiteLastArgType); } - if(!linkerServices.canConvert(callSiteLastArgType, varArgType)) { - // Call site signature guarantees the argument can definitely not be an array (i.e. it is primitive); - // link immediately to a vararg-packing method handle. - return createConvertingInvocation(collectArguments(fixTarget, argsLen), linkerServices, callSiteType); + + // This method handle takes the single argument and packs it into a newly allocated single-element array. It + // will be used when the incoming argument can't be converted to the vararg array type (the "vararg packer" + // method). + final MethodHandle varArgCollectingInvocation = createConvertingInvocation(collectArguments(fixTarget, + argsLen), linkerServices, callSiteType); + + // Is call site type assignable from an array type (e.g. Object:int[], or Object[]:String[]) + final boolean isAssignableFromArray = callSiteLastArgType.isAssignableFrom(varArgType); + // Do we have a custom conversion that can potentially convert the call site type to an array? + final boolean isCustomConvertible = linkerServices.canConvert(callSiteLastArgType, varArgType); + if(!isAssignableFromArray && !isCustomConvertible) { + // Call site signature guarantees the argument can definitely not be converted to an array (i.e. it is + // primitive), and no conversion can help with it either. Link immediately to a vararg-packing method + // handle. + return varArgCollectingInvocation; } - // Call site signature makes no guarantees that the single argument in the vararg position will be - // compatible across all invocations. Need to insert an appropriate guard and fall back to generic vararg - // method when it is not. - return MethodHandles.guardWithTest(Guards.isInstance(varArgType, fixParamsLen, callSiteType), - createConvertingInvocation(fixTarget, linkerServices, callSiteType), - createConvertingInvocation(collectArguments(fixTarget, argsLen), linkerServices, callSiteType)); + + // This method handle employs language-specific conversions to convert the last argument into an array of + // vararg type. + final MethodHandle arrayConvertingInvocation = createConvertingInvocation(MethodHandles.filterArguments( + fixTarget, fixParamsLen, linkerServices.getTypeConverter(callSiteLastArgType, varArgType)), + linkerServices, callSiteType); + + // This method handle determines whether the value can be converted to the array of vararg type using a + // language-specific conversion. + final MethodHandle canConvertArgToArray = MethodHandles.insertArguments(CAN_CONVERT_TO, 0, linkerServices, + varArgType); + + // This one adjusts the previous one for the location of the argument and the call site type. + final MethodHandle canConvertLastArgToArray = MethodHandles.dropArguments(canConvertArgToArray, 0, + MethodType.genericMethodType(fixParamsLen).parameterList()).asType(callSiteType.changeReturnType(boolean.class)); + + // This one takes the previous ones and combines them into a method handle that converts the argument into + // a vararg array when it can, otherwise falls back to the vararg packer. + final MethodHandle convertToArrayWhenPossible = MethodHandles.guardWithTest(canConvertLastArgToArray, + arrayConvertingInvocation, varArgCollectingInvocation); + + if(isAssignableFromArray) { + return MethodHandles.guardWithTest( + // Is incoming parameter already a compatible array? + Guards.isInstance(varArgType, fixParamsLen, callSiteType), + // Yes: just pass it to the method + createConvertingInvocation(fixTarget, linkerServices, callSiteType), + // No: either go through a custom conversion, or if it is not possible, go directly to the + // vararg packer. + isCustomConvertible ? convertToArrayWhenPossible : varArgCollectingInvocation); + } + + // Just do the custom conversion with fallback to the vararg packer logic. + assert isCustomConvertible; + return convertToArrayWhenPossible; } // Remaining case: more than one vararg. return createConvertingInvocation(collectArguments(fixTarget, argsLen), linkerServices, callSiteType); } + @SuppressWarnings("unused") + private static boolean canConvertTo(final LinkerServices linkerServices, Class to, Object obj) { + return obj == null ? false : linkerServices.canConvert(obj.getClass(), to); + } + /** * Creates a method handle out of the original target that will collect the varargs for the exact component type of * the varArg array. Note that this will nicely trigger language-specific type converters for exactly those varargs diff -r d155c4a7703c -r 64e841576c68 src/jdk/internal/dynalink/support/Guards.java --- a/src/jdk/internal/dynalink/support/Guards.java Mon Oct 14 12:41:11 2013 +0200 +++ b/src/jdk/internal/dynalink/support/Guards.java Tue Oct 15 15:57:14 2013 +0200 @@ -88,6 +88,7 @@ import java.lang.invoke.MethodType; import java.util.logging.Level; import java.util.logging.Logger; +import jdk.internal.dynalink.DynamicLinker; import jdk.internal.dynalink.linker.LinkerServices; /** @@ -115,11 +116,11 @@ public static MethodHandle isOfClass(Class clazz, MethodType type) { final Class declaredType = type.parameterType(0); if(clazz == declaredType) { - LOG.log(Level.WARNING, "isOfClassGuardAlwaysTrue", new Object[] { clazz.getName(), 0, type }); + LOG.log(Level.WARNING, "isOfClassGuardAlwaysTrue", new Object[] { clazz.getName(), 0, type, DynamicLinker.getLinkedCallSiteLocation() }); return constantTrue(type); } if(!declaredType.isAssignableFrom(clazz)) { - LOG.log(Level.WARNING, "isOfClassGuardAlwaysFalse", new Object[] { clazz.getName(), 0, type }); + LOG.log(Level.WARNING, "isOfClassGuardAlwaysFalse", new Object[] { clazz.getName(), 0, type, DynamicLinker.getLinkedCallSiteLocation() }); return constantFalse(type); } return getClassBoundArgumentTest(IS_OF_CLASS, clazz, 0, type); @@ -152,11 +153,11 @@ public static MethodHandle isInstance(Class clazz, int pos, MethodType type) { final Class declaredType = type.parameterType(pos); if(clazz.isAssignableFrom(declaredType)) { - LOG.log(Level.WARNING, "isInstanceGuardAlwaysTrue", new Object[] { clazz.getName(), pos, type }); + LOG.log(Level.WARNING, "isInstanceGuardAlwaysTrue", new Object[] { clazz.getName(), pos, type, DynamicLinker.getLinkedCallSiteLocation() }); return constantTrue(type); } if(!declaredType.isAssignableFrom(clazz)) { - LOG.log(Level.WARNING, "isInstanceGuardAlwaysFalse", new Object[] { clazz.getName(), pos, type }); + LOG.log(Level.WARNING, "isInstanceGuardAlwaysFalse", new Object[] { clazz.getName(), pos, type, DynamicLinker.getLinkedCallSiteLocation() }); return constantFalse(type); } return getClassBoundArgumentTest(IS_INSTANCE, clazz, pos, type); @@ -174,11 +175,11 @@ public static MethodHandle isArray(int pos, MethodType type) { final Class declaredType = type.parameterType(pos); if(declaredType.isArray()) { - LOG.log(Level.WARNING, "isArrayGuardAlwaysTrue", new Object[] { pos, type }); + LOG.log(Level.WARNING, "isArrayGuardAlwaysTrue", new Object[] { pos, type, DynamicLinker.getLinkedCallSiteLocation() }); return constantTrue(type); } if(!declaredType.isAssignableFrom(Object[].class)) { - LOG.log(Level.WARNING, "isArrayGuardAlwaysFalse", new Object[] { pos, type }); + LOG.log(Level.WARNING, "isArrayGuardAlwaysFalse", new Object[] { pos, type, DynamicLinker.getLinkedCallSiteLocation() }); return constantFalse(type); } return asType(IS_ARRAY, pos, type); diff -r d155c4a7703c -r 64e841576c68 src/jdk/internal/dynalink/support/messages.properties --- a/src/jdk/internal/dynalink/support/messages.properties Mon Oct 14 12:41:11 2013 +0200 +++ b/src/jdk/internal/dynalink/support/messages.properties Tue Oct 15 15:57:14 2013 +0200 @@ -76,11 +76,11 @@ # OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF # ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. -isInstanceGuardAlwaysTrue=isInstance guard for {0} in position {1} in method type {2} will always return true -isInstanceGuardAlwaysFalse=isInstance guard for {0} in position {1} in method type {2} will always return false +isInstanceGuardAlwaysTrue=isInstance guard for {0} in position {1} in method type {2} at {3} will always return true +isInstanceGuardAlwaysFalse=isInstance guard for {0} in position {1} in method type {2} at {3} will always return false -isOfClassGuardAlwaysTrue=isOfClass guard for {0} in position {1} in method type {2} will always return true -isOfClassGuardAlwaysFalse=isOfClass guard for {0} in position {1} in method type {2} will always return false +isOfClassGuardAlwaysTrue=isOfClass guard for {0} in position {1} in method type {2} at {3} will always return true +isOfClassGuardAlwaysFalse=isOfClass guard for {0} in position {1} in method type {2} at {3} will always return false -isArrayGuardAlwaysTrue=isArray guard in position {0} in method type {1} will always return true -isArrayGuardAlwaysFalse=isArray guard in position {0} in method type {1} will always return false \ No newline at end of file +isArrayGuardAlwaysTrue=isArray guard in position {0} in method type {1} at {2} will always return true +isArrayGuardAlwaysFalse=isArray guard in position {0} in method type {1} at {2} will always return false \ No newline at end of file diff -r d155c4a7703c -r 64e841576c68 src/jdk/nashorn/internal/codegen/CodeGenerator.java --- a/src/jdk/nashorn/internal/codegen/CodeGenerator.java Mon Oct 14 12:41:11 2013 +0200 +++ b/src/jdk/nashorn/internal/codegen/CodeGenerator.java Tue Oct 15 15:57:14 2013 +0200 @@ -453,7 +453,13 @@ public boolean enterFunctionNode(FunctionNode functionNode) { // function nodes will always leave a constructed function object on stack, no need to load the symbol // separately as in enterDefault() + lc.pop(functionNode); functionNode.accept(codegen); + // NOTE: functionNode.accept() will produce a different FunctionNode that we discard. This incidentally + // doesn't cause problems as we're never touching FunctionNode again after it's visited here - codegen + // is the last element in the compilation pipeline, the AST it produces is not used externally. So, we + // re-push the original functionNode. + lc.push(functionNode); method.convert(type); return false; } diff -r d155c4a7703c -r 64e841576c68 src/jdk/nashorn/internal/runtime/linker/JavaArgumentConverters.java --- a/src/jdk/nashorn/internal/runtime/linker/JavaArgumentConverters.java Mon Oct 14 12:41:11 2013 +0200 +++ b/src/jdk/nashorn/internal/runtime/linker/JavaArgumentConverters.java Tue Oct 15 15:57:14 2013 +0200 @@ -31,10 +31,8 @@ import java.lang.invoke.MethodHandle; import java.lang.invoke.MethodHandles; -import java.util.Deque; -import java.util.List; -import java.util.concurrent.ConcurrentHashMap; -import java.util.concurrent.ConcurrentMap; +import java.util.HashMap; +import java.util.Map; import jdk.internal.dynalink.support.TypeUtilities; import jdk.nashorn.internal.runtime.ConsString; import jdk.nashorn.internal.runtime.JSType; @@ -59,13 +57,7 @@ } static MethodHandle getConverter(final Class targetType) { - MethodHandle converter = CONVERTERS.get(targetType); - if(converter == null && targetType.isArray()) { - converter = MH.insertArguments(JSType.TO_JAVA_ARRAY.methodHandle(), 1, targetType.getComponentType()); - converter = MH.asType(converter, converter.type().changeReturnType(targetType)); - CONVERTERS.putIfAbsent(targetType, converter); - } - return converter; + return CONVERTERS.get(targetType); } @SuppressWarnings("unused") @@ -263,12 +255,9 @@ return MH.findStatic(MethodHandles.lookup(), JavaArgumentConverters.class, name, MH.type(rtype, types)); } - private static final ConcurrentMap, MethodHandle> CONVERTERS = new ConcurrentHashMap<>(); + private static final Map, MethodHandle> CONVERTERS = new HashMap<>(); static { - CONVERTERS.put(List.class, JSType.TO_JAVA_LIST.methodHandle()); - CONVERTERS.put(Deque.class, JSType.TO_JAVA_DEQUE.methodHandle()); - CONVERTERS.put(Number.class, TO_NUMBER); CONVERTERS.put(String.class, TO_STRING); diff -r d155c4a7703c -r 64e841576c68 src/jdk/nashorn/internal/runtime/linker/NashornLinker.java --- a/src/jdk/nashorn/internal/runtime/linker/NashornLinker.java Mon Oct 14 12:41:11 2013 +0200 +++ b/src/jdk/nashorn/internal/runtime/linker/NashornLinker.java Tue Oct 15 15:57:14 2013 +0200 @@ -30,6 +30,8 @@ import java.lang.invoke.MethodHandle; import java.lang.invoke.MethodHandles; import java.lang.reflect.Modifier; +import java.util.Deque; +import java.util.List; import jdk.internal.dynalink.CallSiteDescriptor; import jdk.internal.dynalink.linker.ConversionComparator; import jdk.internal.dynalink.linker.GuardedInvocation; @@ -38,6 +40,8 @@ import jdk.internal.dynalink.linker.LinkerServices; import jdk.internal.dynalink.linker.TypeBasedGuardingDynamicLinker; import jdk.internal.dynalink.support.Guards; +import jdk.nashorn.internal.objects.NativeArray; +import jdk.nashorn.internal.runtime.JSType; import jdk.nashorn.internal.runtime.ScriptFunction; import jdk.nashorn.internal.runtime.ScriptObject; import jdk.nashorn.internal.runtime.Undefined; @@ -47,6 +51,13 @@ * includes {@link ScriptFunction} and its subclasses) as well as {@link Undefined}. */ final class NashornLinker implements TypeBasedGuardingDynamicLinker, GuardingTypeConverterFactory, ConversionComparator { + private static final ClassValue ARRAY_CONVERTERS = new ClassValue() { + @Override + protected MethodHandle computeValue(Class type) { + return createArrayConverter(type); + } + }; + /** * Returns true if {@code ScriptObject} is assignable from {@code type}, or it is {@code Undefined}. */ @@ -103,6 +114,12 @@ if (mh != null) { return new GuardedInvocation(mh, canLinkTypeStatic(sourceType) ? null : IS_NASHORN_OR_UNDEFINED_TYPE); } + + GuardedInvocation inv = getArrayConverter(sourceType, targetType); + if(inv != null) { + return inv; + } + return getSamTypeConverter(sourceType, targetType); } @@ -129,6 +146,41 @@ return null; } + /** + * Returns a guarded invocation that converts from a source type that is NativeArray to a Java array or List or + * Deque type. + * @param sourceType the source type (presumably NativeArray a superclass of it) + * @param targetType the target type (presumably an array type, or List or Deque) + * @return a guarded invocation that converts from the source type to the target type. null is returned if + * either the source type is neither NativeArray, nor a superclass of it, or if the target type is not an array + * type, List, or Deque. + */ + private static GuardedInvocation getArrayConverter(final Class sourceType, final Class targetType) { + final boolean isSourceTypeNativeArray = sourceType == NativeArray.class; + // If source type is more generic than ScriptFunction class, we'll need to use a guard + final boolean isSourceTypeGeneric = !isSourceTypeNativeArray && sourceType.isAssignableFrom(NativeArray.class); + + if (isSourceTypeNativeArray || isSourceTypeGeneric) { + final MethodHandle guard = isSourceTypeGeneric ? IS_NATIVE_ARRAY : null; + if(targetType.isArray()) { + return new GuardedInvocation(ARRAY_CONVERTERS.get(targetType), guard); + } + if(targetType == List.class) { + return new GuardedInvocation(JSType.TO_JAVA_LIST.methodHandle(), guard); + } + if(targetType == Deque.class) { + return new GuardedInvocation(JSType.TO_JAVA_DEQUE.methodHandle(), guard); + } + } + return null; + } + + private static MethodHandle createArrayConverter(final Class type) { + assert type.isArray(); + final MethodHandle converter = MH.insertArguments(JSType.TO_JAVA_ARRAY.methodHandle(), 1, type.getComponentType()); + return MH.asType(converter, converter.type().changeReturnType(type)); + } + private static boolean isAutoConvertibleFromFunction(final Class clazz) { return isAbstractClass(clazz) && !ScriptObject.class.isAssignableFrom(clazz) && JavaAdapterFactory.isAutoConvertibleFromFunction(clazz); @@ -148,7 +200,26 @@ @Override public Comparison compareConversion(final Class sourceType, final Class targetType1, final Class targetType2) { + if(sourceType == NativeArray.class) { + // Prefer lists, as they're less costly to create than arrays. + if(isList(targetType1)) { + if(!isList(targetType2)) { + return Comparison.TYPE_1_BETTER; + } + } else if(isList(targetType2)) { + return Comparison.TYPE_2_BETTER; + } + // Then prefer arrays + if(targetType1.isArray()) { + if(!targetType2.isArray()) { + return Comparison.TYPE_1_BETTER; + } + } else if(targetType2.isArray()) { + return Comparison.TYPE_2_BETTER; + } + } if(ScriptObject.class.isAssignableFrom(sourceType)) { + // Prefer interfaces if(targetType1.isInterface()) { if(!targetType2.isInterface()) { return Comparison.TYPE_1_BETTER; @@ -160,7 +231,12 @@ return Comparison.INDETERMINATE; } + private static boolean isList(Class clazz) { + return clazz == List.class || clazz == Deque.class; + } + private static final MethodHandle IS_SCRIPT_FUNCTION = Guards.isInstance(ScriptFunction.class, MH.type(Boolean.TYPE, Object.class)); + private static final MethodHandle IS_NATIVE_ARRAY = Guards.isOfClass(NativeArray.class, MH.type(Boolean.TYPE, Object.class)); private static final MethodHandle IS_NASHORN_OR_UNDEFINED_TYPE = findOwnMH("isNashornTypeOrUndefined", Boolean.TYPE, Object.class); diff -r d155c4a7703c -r 64e841576c68 test/src/jdk/nashorn/api/javaaccess/ArrayConversionTest.java --- a/test/src/jdk/nashorn/api/javaaccess/ArrayConversionTest.java Mon Oct 14 12:41:11 2013 +0200 +++ b/test/src/jdk/nashorn/api/javaaccess/ArrayConversionTest.java Tue Oct 15 15:57:14 2013 +0200 @@ -87,10 +87,26 @@ } @Test + public void testArrayAmbiguity() throws ScriptException { + runTest("x", "'abc'"); + runTest("x", "['foo', 'bar']"); + } + + @Test public void testListArrays() throws ScriptException { runTest("assertListArray", "[['foo', 'bar'], ['apple', 'orange']]"); } + @Test + public void testVarArgs() throws ScriptException { + // Sole NativeArray in vararg position becomes vararg array itself + runTest("assertVarArg_42_17", "[42, 17]"); + // NativeArray in vararg position becomes an argument if there are more arguments + runTest("assertVarArg_array_17", "[42], 18"); + // Only NativeArray is converted to vararg array, other objects (e.g. a function) aren't + runTest("assertVarArg_function", "function() { return 'Hello' }"); + } + private static void runTest(final String testMethodName, final String argument) throws ScriptException { e.eval("Java.type('" + ArrayConversionTest.class.getName() + "')." + testMethodName + "(" + argument + ")"); } @@ -188,4 +204,32 @@ assertEquals(Arrays.asList("foo", "bar"), array[0]); assertEquals(Arrays.asList("apple", "orange"), array[1]); } + + public static void assertVarArg_42_17(Object... args) throws ScriptException { + assertEquals(2, args.length); + assertEquals(42, ((Number)args[0]).intValue()); + assertEquals(17, ((Number)args[1]).intValue()); + } + + public static void assertVarArg_array_17(Object... args) throws ScriptException { + assertEquals(2, args.length); + e.getBindings(ScriptContext.ENGINE_SCOPE).put("arr", args[0]); + assertTrue((Boolean)e.eval("arr instanceof Array && arr.length == 1 && arr[0] == 42")); + assertEquals(18, ((Number)args[1]).intValue()); + } + + public static void assertVarArg_function(Object... args) throws ScriptException { + assertEquals(1, args.length); + e.getBindings(ScriptContext.ENGINE_SCOPE).put("fn", args[0]); + assertEquals("Hello", e.eval("fn()")); + } + + + + public static void x(String y) { + assertEquals("abc", y); + } + public static void x(String[] y) { + assertTrue(Arrays.equals(new String[] { "foo", "bar"}, y)); + } }