changeset 635:64e841576c68

8026397: Fix ambiguity with array conversion, including passing JS NativeArrays in Java variable arity methods' vararg array position Reviewed-by: jlaskey, sundar
author attila
date Tue, 15 Oct 2013 15:57:14 +0200
parents d155c4a7703c
children aa452eb4a5d0
files src/jdk/internal/dynalink/beans/SingleDynamicMethod.java src/jdk/internal/dynalink/support/Guards.java src/jdk/internal/dynalink/support/messages.properties src/jdk/nashorn/internal/codegen/CodeGenerator.java src/jdk/nashorn/internal/runtime/linker/JavaArgumentConverters.java src/jdk/nashorn/internal/runtime/linker/NashornLinker.java test/src/jdk/nashorn/api/javaaccess/ArrayConversionTest.java
diffstat 7 files changed, 203 insertions(+), 37 deletions(-) [+]
line wrap: on
line diff
--- 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
--- 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);
--- 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
--- 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;
             }
--- 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<Class<?>, MethodHandle> CONVERTERS = new ConcurrentHashMap<>();
+    private static final Map<Class<?>, 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);
 
--- 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<MethodHandle> ARRAY_CONVERTERS = new ClassValue<MethodHandle>() {
+        @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);
--- 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));
+    }
 }