oracle / graal

GraalVM compiles Java applications into native executables that start instantly, scale fast, and use fewer compute resources 🚀
https://www.graalvm.org
Other
20k stars 1.6k forks source link

[GR-54995] Should '0' should not be able to map to int? type coercion #9096

Open in-fke opened 1 month ago

in-fke commented 1 month ago

Describe GraalVM and your environment :

Have you verified this issue still happens when using the latest snapshot? You can find snapshot builds here: https://github.com/graalvm/graalvm-ce-dev-builds/releases No, but I have looked at the code and it's the same: https://github.com/oracle/graal/blob/master/truffle/src/com.oracle.truffle.host/src/com/oracle/truffle/host/HostUtil.java#L141

Describe the issue Rhino can "cast" to int, while Graal / Truffel does not, see code below.

Get ClassCastException with Cannot convert 'com.oracle.truffle.api.strings.TruffleString'(language: Java, type: com.oracle.truffle.api.strings.TruffleString) to Java type 'int': Invalid or lossy primitive coercion. Code snippet or code repository that reproduces the issue

    public static class TakesInt {
        @Export
        public void method(int arg) {};
    }

    @Test
    public void testTakesInt() {
        Context context = Context.create();
        context.getBindings("js").putMember("takesInt", new TakesInt());
        context.eval("js", "takesInt.method('0');");
    }

Steps to reproduce the issue Run the above test.

Expected behavior I would assume that Truffle would cast that (as long as it's compliant?). Caller is com.oracle.truffle.host.HostToTypeNode.convertImpl() Maybe https://github.com/oracle/graal/blob/master/truffle/src/com.oracle.truffle.host/src/com/oracle/truffle/host/HostUtil.java#L141 should support other target types (such as int?).

Additional context Add any other context about the problem here. Specially important are stack traces or log output. Feel free to link to gists or to screenshots if necesary

Details ``` TypeError: invokeMember (method) on ScriptingServiceGraalStandaloneTest$TakesInt failed due to: Cannot convert 'com.oracle.truffle.api.strings.TruffleString'(language: Java, type: com.oracle.truffle.api.strings.TruffleString) to Java type 'int': Invalid or lossy primitive coercion. at :program(Unnamed:1:0-19) at org.graalvm.polyglot.Context.eval(Context.java:429) at ScriptingServiceGraalStandaloneTest.testTakesInt(ScriptingServiceGraalStandaloneTest.java:144) ```
in-fke commented 1 month ago

https://github.com/oracle/graaljs/issues/272

in-fke commented 4 weeks ago

I will probably use this workaround (targetTypeMapping), question is: is it valid, that Graal does not do this out-of-the-box?

    public static class TakesInt {
        @Export
        public void method(int arg) {};
    }

    @Test
    public void testTakesIntWithCoercion() {
        // https://github.com/oracle/graal/issues/9096
        AtomicInteger predicateSucceededCount = new AtomicInteger();
        AtomicInteger predicateFailedCount = new AtomicInteger();
        AtomicInteger conversionCount = new AtomicInteger();        
        HostAccess hostAccess = HostAccess.newBuilder(HostAccess.ALL)
                // Rhino would do the same!?
                .targetTypeMapping(String.class, Integer.class, target -> {
                    boolean result;
                    try {
                        double d = Double.valueOf(target);
                        if (Math.floor(d) != Math.ceil(d)) {
                            result = false;
                        } else if (d > Integer.MAX_VALUE) {
                            result = false;
                        } else if (d < Integer.MIN_VALUE) {
                            result = false;
                        } else {
                            result = true;
                        }
                    } catch (NumberFormatException e) {
                        result = false;
                    }
                    if (result) {
                        predicateSucceededCount.incrementAndGet();
                    } else {
                        predicateFailedCount.incrementAndGet();
                    }
                    return result;
                }, target -> {
                    conversionCount.incrementAndGet();
                    return Double.valueOf(target).intValue();
                })
                //
                .build();        
        Context context = Context.newBuilder("js")
                .allowHostAccess(hostAccess)
                // required to declare class with Java.type
                .allowHostClassLookup(s -> true).build();                
        Value bindings = context.getBindings("js");
        bindings.putMember("takesInt", new TakesInt());
        context.eval("js", "const Integer = Java.type('" + Integer.class.getName() + "');");
        // these must not throw
        context.eval("js", "takesInt.method('0');");
        context.eval("js", "takesInt.method('' + Integer.MAX_VALUE);");
        context.eval("js", "takesInt.method('' + Integer.MIN_VALUE);");
        assertEquals(3, predicateSucceededCount.get());
        assertEquals(0, predicateFailedCount.get());        
        assertEquals(3, conversionCount.get());
        List.of(
                // too small
                "takesInt.method('' + (Integer.MIN_VALUE - 1));",
                // too large
                "takesInt.method('' + (Integer.MAX_VALUE + 1));",
                // not integer
                "takesInt.method('1.5');")
                //
                .forEach((script) -> {
                    try {
                        context.eval("js", script);
                        fail("for script [" + script + "]: expected excpetion to be thrown");
                    } catch (Exception e) {
                        assertEquals("for script [" + script + "]: exception class mismatch", PolyglotException.class, e.getClass());
                        assertTrue("for script [" + script + "]: unexpected message: [" + e.getMessage() + "]", StringUtils.contains(e.getMessage(), "Invalid or lossy primitive coercion"));
                    }
                });
        assertEquals(3, predicateSucceededCount.get());
        assertEquals(3, predicateFailedCount.get());        
        assertEquals(3, conversionCount.get());
    }