Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
in-fke opened this issue Jun 12, 2024 · 2 comments
Open

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

in-fke opened this issue Jun 12, 2024 · 2 comments
Assignees
Labels

Comments

@in-fke
Copy link

in-fke commented Jun 12, 2024

Describe GraalVM and your environment :

  • GraalVM version or commit id if built from source: 23.0.4
  • CE or EE: CE
  • JDK version: 17
  • OS and OS Version: Windows
  • Architecture: x64
  • The output of java -Xinternalversion:
<using stock VM>

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 <js> :program(Unnamed:1:0-19)
	at org.graalvm.polyglot.Context.eval(Context.java:429)
	at ScriptingServiceGraalStandaloneTest.testTakesInt(ScriptingServiceGraalStandaloneTest.java:144)
@in-fke in-fke added the truffle label Jun 12, 2024
@in-fke
Copy link
Author

in-fke commented Jun 12, 2024

oracle/graaljs#272

@in-fke
Copy link
Author

in-fke commented Jun 13, 2024

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());
    } 

@fernando-valdez fernando-valdez self-assigned this Jun 14, 2024
@fernando-valdez fernando-valdez changed the title Should '0' should not be able to map to int? type coercion [GR-54995] Should '0' should not be able to map to int? type coercion Jun 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants