google / jsinterop-base

Utilities for GWT and J2CL to interact with JavaScript beyond JsInterop
Apache License 2.0
67 stars 16 forks source link

@DoNotAutobox is absent from several Js methods - why? #12

Closed realityforge closed 5 years ago

realityforge commented 5 years ago

I am just trying to understand why several of the methods in the Js class do not have the @DoNotAutobox annotation on their object parameters when presumably they should. Examples include the "as" methods and supporting infrastructure. i.e. Js.asBoolean(obj) and thus InternalJsUtil.asBoolean(obj), as well as Js.coerceToDouble(obj).

Is it just because it is assumed that humans invoking this are smart enough to not misuse them and the code is unlikely to be used in code generators? What would be the penalty of adding them?

tbroyer commented 5 years ago

Boolean and Double are never boxed/unboxed, as boolean/java.lang.Boolean and double/java.lang.Double are interchangeable and always map to native JS boolean and number values respectively. That might be the reason.

gkdn commented 5 years ago

@DoNotAutobox is only useful for numeric values like char, short, int, long etc. and if you are using Js.asBoolean with those, it is certain that you are breaking the type system and it is a bug to call those methods in such cases. It is also completely useless to call Js.coerceToDouble with such types since Java handles the conversion already.

(We should have error-prone check for those and error out while compiling but we don't have it yet.)

niloc132 commented 5 years ago

The autoboxing that is happening around boolean->Boolean seems to cause a clinit to be emitted even though this shouldn't be required. In EventTarget, we have this:

 @JsOverlay
  default void addEventListener(String type, EventListener listener, boolean options) {
    addEventListener(
        type, listener, Js.<EventTarget.AddEventListenerOptionsUnionType>uncheckedCast(options));
  }

This then generates this static java method, with the Boolean clinit, and inlined valueOf operation or whatnot to box:

   function Yg(a,b,c,d){
      a.addEventListener(b,c,($g(),d?true:false))
   }

Do we actually expect that, or shouldn't this be generated as only a.addEventListener(b, c, d)? I realize that from JS's perspective, boolean and Boolean might as well be the same thing, but unless I'm mistaken about what $g is or that ternary is for, the compiler is still generating the unnecessary boxing code.

gkdn commented 5 years ago

As I said in the other thread, Js.uncheckedCast already has a DoNotAutobox.

niloc132 commented 5 years ago

Ah, my mistake - I'm still on the latest stable release.