JsObject.assign(Object,Object...) should have @DoNotAutobox on the second parameter

[realityforge]

The JsObject.assign method should have the @DoNotAutobox added to the second parameter (at least under GWT2.x). Otherwise, the runtime casts it to a java array which will ultimately cause incorrect behavior.

i.e.

public class JsObject {
    @JsOverlay
    public static final JsObject assign(Object target, Object... var_args) {...}
   ...
}

should look like

public class JsObject {
    @JsOverlay
    public static final JsObject assign(Object target, @DoNotAutobox Object... var_args) {...}
   ...
}

I haven't tracked down where this needs to be fixed (presumably jsinterop-generator) but I just wanted to record the issue so it did not get lost in the ether.

[niloc132]

In j2cl the semantics are different IIRC, so that a plain Java Object[] is actually implemented as a JS array (with some predictable annoyances, like JsArray.toString() spitting out the Object[].toString() results instead of JS's Array.prototype.toString).

But while we're talking about the annotation (and this might be better in another ticket)... how do you indicate "do not autobox any of the args in this varargs"? If you pass an int as one of the args, it is boxed up to Integer, which isn't what is intended - console.log(...) is particularly annoying here.

[gkdn]

DoNotAutobox doesn't prevent Java array and Java arrays only different by metadata so I don't expect any incorrect behavior.

[realityforge]

Oh - I didn't realize that @DoNoAutobox would not work in this context. I am not sure what the correct fix is then.

If we have the following code

final JsPropertyMap<Object> value1 = JsPropertyMap.of( "A", "a" );
final JsObject value2 = Js.uncheckedCast( value1 );

DomGlobal.console.log( JsObject.assign( JsPropertyMap.of(), value1 ) );
DomGlobal.console.log( JsObject.assign( JsPropertyMap.of(), value2 ) );
DomGlobal.console.log( JsObject.assign( JsPropertyMap.of(), Js.uncheckedCast( value1 ) ) );

we get the following in GWT2

{0: {…}, ___clazz: Class, castableTypeMap: {…}, __elementTypeId$: 1, __elementTypeCategory$: 5, typeMarker: ƒ}
{0: {…}, ___clazz: Class, castableTypeMap: {…}, __elementTypeId$: 1, __elementTypeCategory$: 5, typeMarker: ƒ}
{A: "a"}

As you predicted, adding @DoNotAutobox does not have any impact.

FWIW the relevant elemental2 code looks something like:

@JsType( isNative = true, name = "Object", namespace = JsPackage.GLOBAL )
public static class JsObject
{
...
  public static native JsObject assign( JsObject target, JsObject... var_args );

  @JsOverlay
  public static final JsObject assign( Object target, Object... var_args )
  {
    return assign( Js.<JsObject>uncheckedCast( target ), Js.<JsObject>uncheckedCast( var_args ) );
  }
}

So is there any way to make the translation of javascript varargs to java varargs "safe"? I tried several different mechanisms but none seemed to work.

[gkdn]

Ok thanks for detail report; now it makes all sense.

The overlay method is the cause of the problem which effective result in converting a var-arg API to an Array API and then pass it directly.

And the overload is needed to introduce cast so JsCompiler doesn't complain.

The solution is complicated with current design and for this to work assign should be called with apply since we cannot use spread-operator in Java,

Or we can introduce some magic method supported by compiler to do spreads.

If anyone can think of some clever alternatives, please shoot it.

[rluble]

This seems like a bug in GWT. When we call a JsVarags (i.e. a JsMethod with varargs) we actually handle it, i.e. in J2CL we do the spread and do not pass the array directly and in GWT we do apply so that the array is passed as individual values.

So in your example the JsObject.assign overlay calls directly the native assign but passes the array directly instead of using the apply. You should confirm this by looking at the generated code.

If that is so there is probably a bug on the decision whether to use apply or not.

Here is a pointer of the line where the decision is made to use apply because of varargs:

https://source.corp.google.com/piper///depot/google3/third_party/java_src/gwt/svn/trunk/dev/core/src/com/google/gwt/dev/js/JsUtils.java;rcl=310791750;l=271

and the relevant cal is when handling the JMethodCall in GenrateJavaScriptAst.transformMethodCall.

If you want to debug a contribute the fix I'll be happy to review.

On Mon, May 11, 2020 at 1:51 PM Goktug Gokdogan notifications@github.com wrote:

Ok thanks for detail report; now it makes all sense.

The overlay method is the cause of the problem which effective result in converting a var-arg API to an Array API and then pass it directly.

And the overload is needed to introduce cast so JsCompiler doesn't complain.

The solution is complicated with current design and for this to work assign should be called with apply since we cannot use spread-operator in Java,

Or we can introduce some magic method supported by compiler to do spreads.

If anyone can think of some clever alternatives, please shoot it.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/google/elemental2/issues/139#issuecomment-626956074, or unsubscribe https://github.com/notifications/unsubscribe-auth/AA73NFH6OKZF4WKDWU4HBN3RRBQORANCNFSM4M4CKJHQ .

[gkdn]

If we are already auto-spreading with arrays we should just fix the call-site. The method call is passing just an object.

On Mon, May 11, 2020 at 3:35 PM Roberto Lublinerman < notifications@github.com> wrote:

This seems like a bug in GWT. When we call a JsVarags (i.e. a JsMethod with varargs) we actually handle it, i.e. in J2CL we do the spread and do not pass the array directly and in GWT we do apply so that the array is passed as individual values.

So in your example the JsObject.assign overlay calls directly the native assign but passes the array directly instead of using the apply. You should confirm this by looking at the generated code.

If that is so there is probably a bug on the decision whether to use apply or not.

Here is a pointer of the line where the decision is made to use apply because of varargs:

https://source.corp.google.com/piper///depot/google3/third_party/java_src/gwt/svn/trunk/dev/core/src/com/google/gwt/dev/js/JsUtils.java;rcl=310791750;l=271

and the relevant cal is when handling the JMethodCall in GenrateJavaScriptAst.transformMethodCall.

If you want to debug a contribute the fix I'll be happy to review.

On Mon, May 11, 2020 at 1:51 PM Goktug Gokdogan notifications@github.com wrote:

Ok thanks for detail report; now it makes all sense.

The overlay method is the cause of the problem which effective result in converting a var-arg API to an Array API and then pass it directly.

And the overload is needed to introduce cast so JsCompiler doesn't complain.

The solution is complicated with current design and for this to work assign should be called with apply since we cannot use spread-operator in Java,

Or we can introduce some magic method supported by compiler to do spreads.

If anyone can think of some clever alternatives, please shoot it.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <https://github.com/google/elemental2/issues/139#issuecomment-626956074 , or unsubscribe < https://github.com/notifications/unsubscribe-auth/AA73NFH6OKZF4WKDWU4HBN3RRBQORANCNFSM4M4CKJHQ

.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/google/elemental2/issues/139#issuecomment-627001936, or unsubscribe https://github.com/notifications/unsubscribe-auth/AA7MB6GE4OOEVH6VY62R66LRRB4TTANCNFSM4M4CKJHQ .

[rluble]

The fix should be that the @Overlay calls with a Js.<JsObject[]>uncheckedCast (i.e. a cast to a JsObject[] instead of a JsObject[]). Without that, per Java semantics, it will just send an array one argument, instead of passing the array as the varargs array.

On Mon, May 11, 2020 at 4:08 PM Goktug Gokdogan notifications@github.com wrote:

If we are already auto-spreading with arrays we should just fix the call-site. The method call is passing just an object.

On Mon, May 11, 2020 at 3:35 PM Roberto Lublinerman < notifications@github.com> wrote:

This seems like a bug in GWT. When we call a JsVarags (i.e. a JsMethod with varargs) we actually handle it, i.e. in J2CL we do the spread and do not pass the array directly and in GWT we do apply so that the array is passed as individual values.

So in your example the JsObject.assign overlay calls directly the native assign but passes the array directly instead of using the apply. You should confirm this by looking at the generated code.

If that is so there is probably a bug on the decision whether to use apply or not.

Here is a pointer of the line where the decision is made to use apply because of varargs:

https://source.corp.google.com/piper///depot/google3/third_party/java_src/gwt/svn/trunk/dev/core/src/com/google/gwt/dev/js/JsUtils.java;rcl=310791750;l=271

and the relevant cal is when handling the JMethodCall in GenrateJavaScriptAst.transformMethodCall.

If you want to debug a contribute the fix I'll be happy to review.

On Mon, May 11, 2020 at 1:51 PM Goktug Gokdogan < notifications@github.com> wrote:

Ok thanks for detail report; now it makes all sense.

The overlay method is the cause of the problem which effective result in converting a var-arg API to an Array API and then pass it directly.

And the overload is needed to introduce cast so JsCompiler doesn't complain.

The solution is complicated with current design and for this to work assign should be called with apply since we cannot use spread-operator in Java,

Or we can introduce some magic method supported by compiler to do spreads.

If anyone can think of some clever alternatives, please shoot it.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub < https://github.com/google/elemental2/issues/139#issuecomment-626956074 , or unsubscribe <

https://github.com/notifications/unsubscribe-auth/AA73NFH6OKZF4WKDWU4HBN3RRBQORANCNFSM4M4CKJHQ

.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub <https://github.com/google/elemental2/issues/139#issuecomment-627001936 , or unsubscribe < https://github.com/notifications/unsubscribe-auth/AA7MB6GE4OOEVH6VY62R66LRRB4TTANCNFSM4M4CKJHQ

.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/google/elemental2/issues/139#issuecomment-627012726, or unsubscribe https://github.com/notifications/unsubscribe-auth/AA73NFDT74GWF2IR3XPW3LLRRCAQJANCNFSM4M4CKJHQ .

[rluble]

The error was in the jsinterop_generator which generates the @JsOverlays. The fix is already in. So elemental2 will be fixed in the next release.

[realityforge]

Great - thanks!

[niloc132]

I think this can be closed, since 1.1.0 shipped.