oracle / graaljs

GraalJS – A high-performance, ECMAScript compliant, and embeddable JavaScript runtime for Java
https://www.graalvm.org/javascript/
Universal Permissive License v1.0
1.82k stars 191 forks source link

Different behavior for upper bounded wildcard generics on a PolyglotMap between versions 24.0.2 and 24.1.0 #861

Closed notdryft closed 1 week ago

notdryft commented 1 month ago

Hello,

I'm facing some issues with PolyglotMap's key types but I'm not sure whether it is an actual bug or changed/fixed behavior going from version 24.0.2 (using GraalVM 22.0.2) to 24.1.0 (using GraalVM 23.0.0).

So, I'm using the interop to use Java methods with this kind of signature:

void wasWorkingTest(Map<? extends CharSequence, String> map); // just prints the content of the map

With Graal JS version 24.0.2, it did work. The following code prints what's expected of it and no exceptions are thrown:

var context = Context.newBuilder().allowAllAccess(true).build();
context.eval("js", """
  const r = Java.type("org.dryft.CharSequenceReproducer");
  r.wasWorkingTest({ should: "work" });""");

However, with Graal JS version 24.1.0, the same code now throws an exception:

Exception in thread "main" TypeError: invokeMember (wasWorkingTest) on org.dryft.CharSequenceReproducer failed due to: Unsupported Map key type: interface java.lang.CharSequence
    at <js> :program(Unnamed:2:57-92)
    at org.graalvm.polyglot.Context.eval(Context.java:428)
    at org.dryft.CharSequenceReproducer.main(CharSequenceReproducer.java:22)

Given the exception says CharSequence is not a supported key type, I extended the test code with a second method:

void wasWorkingTest(Map<? extends CharSequence, String> map); // just prints the content of the map
void alwaysFailedTest(Map<CharSequence, String> map); // does the same as the previous method

The evaluation code is now:

var context = Context.newBuilder().allowAllAccess(true).build();
context.eval("js", """
  const r = Java.type("org.dryft.CharSequenceReproducer");
  r.wasWorkingTest({ should: "work" });
  r.alwaysFailedTest({ should: "not work" });""");

With Graal JS version 24.0.2, it prints:

should: work
Exception in thread "main" TypeError: invokeMember (alwaysFailedTest) on org.dryft.CharSequenceReproducer failed due to: Unsupported Map key type: interface java.lang.CharSequence
    at <js> :program(Unnamed:3:95-136)
    at org.graalvm.polyglot.Context.eval(Context.java:428)
    at org.dryft.CharSequenceReproducer.main(CharSequenceReproducer.java:22)

And with 24.1.0:

Exception in thread "main" TypeError: invokeMember (wasWorkingTest) on org.dryft.CharSequenceReproducer failed due to: Unsupported Map key type: interface java.lang.CharSequence
    at <js> :program(Unnamed:2:57-92)
    at org.graalvm.polyglot.Context.eval(Context.java:428)
    at org.dryft.CharSequenceReproducer.main(CharSequenceReproducer.java:22)

Which confirms CharSequences were never supported as a key type to begin with. But then, why did the generic version work previously? Is the change intended? What am I missing?

The only thing I could find is this pull request from the Graal repo which was applied in patch 24.1.0: GR-51402 Improved Polyglot guest function to host interface proxies to infer the expected return type from the corresponding type argument (R) of the generic target type (e.g. BiFunction<T,U,R>).

In case it's needed, I pushed the previous code to a repository here: https://github.com/notdryft/charsequence

For some context, the real use case is for Gatling JS which relies on the Java DSL of Gatling. In this DSL, one method has a Map<? extends CharSequence as an argument, which is how I stumbled upon the previous issue: https://github.com/gatling/gatling/blob/main/gatling-http-java/src/main/java/io/gatling/javaapi/http/HttpProtocolBuilder.java#L280

iamstolis commented 1 month ago

My understanding is that the change between versions 24.0.2 and 24.1.0 is a bug-fix. Even in 24.0.2 it was not the intention to support CharSequence. Unfortunately, due to a bug, ? extends CharSequence was interpreted as Object key type. This has been fixed/changed by the pull-request that you have mentioned.

A potential workaround is to use a helper method that accepts Map<String,Object> and invokes the original method (with Map<? extends CharSequence,Object> parameter).

woess commented 2 weeks ago

Indeed it was only working by accident. I, too, would suggest to use Map<String, Object>. We could allow CharSequence as a Map key type (and treat it like String), but I'm not yet convinced it is a good idea. /cc @chumer

notdryft commented 1 week ago

I'm not convinced too it would be a good idea to add support to CharSequence keys so I'm closing this issue! Thanks for confirming our suspicions.