Clarify non-native @JsType support for Java 16 records

[niloc132]

Does not attempt to address native support - like enums, if this is to be supported, it will likely require its own annotation and semantics.

Fixes #6

[gkdn]

I haven't checked what we would see in the AST but I thought they would be observed like they are annotated with a @JSmethod since they are public methods. Isn't that the case?

[niloc132]

Thanks for taking a look.

There might have been a miscommunication between us on #6, where I proposed that this is the desired behavior - this isn't a limitation of what would be seen in the AST at all, but what I believe to be the idiomatic way to express this sort of object in JS - I deliberately left the isNative=true unsolved, and you replied "I think it generally makes sense though not sure what to do with native types."

Briefly rehashing the logic:

• A closure record/struct (or plain js object with properties) idiomatically has properties, rather than methods. ES6 class's get/set makes this simple to express as well, without needing/wanting a "real" method to use as an accessor
• In contrast, the idiomatic way in Java is still to use methods. Unlike beans, these "component accessors" don't start with a get- or is- (or sometimes has-) prefix. (Action item here for my patch, also, update JsProperty to explicitly call this out).
• With JsProperty updated to allow being placed on a record component accessor, a non-native JsType record with N components, the only correct way to export it as an idiomatic "object with properties" is to flesh out the component accessor method, going from 1 line of code, to 2 + N * 4 lines.

I argue that the default assumption should be that since the record is explicitly being made available to JS, it should be easy to do in an idiomatic way, rather than making the developer effectively write the full "bean" implementation. If for some reason it is desired to have a component accessor be exported as a no-arg method, that should be considered the exception.

As a compromise between these, I also proposed allowing @JsProperty to be put on the components themselves, with the special rules that it would not apply to the field (as the JEP says it must). I would personally argue that it is inconsistent to argue a) we're uncomfortable with JsType having its rules bent, but b) JsProperty can be redefined in this context to mean something non-standard ("doesn't apply to fields, even though it normally would"), and that it would be more idiomatic to just allow @JsMethod on the components (which will correctly be propagated to the public accessor methods, but not the private final fields).

[gkdn]

Got it.
Property accessor are generally a code size / performance concern for ES5 transpiling. I don't know about performance impact on more recent ES versions. This will also affect potential AutoValue to Record migrations. We need to put more thought on to this.
I guess in short we need to start designing around record types to make this commitment.

[niloc132]

Any additional thoughts on this?

My quick surprise here is that closure-compiler cannot optimize get accessor methods into regular methods, nor inline them. That is, according to the closure appspot service,

class A {
  get foo() {
    return 1;
  }
}

console.log(new A().foo() + 1)

is compiled to

 'use strict';class a{get g(){return 1}}console.log((new a).g()+1);

but replace get foo with getFoo:

class A {
  getFoo() {
    return 1;
  }
}

console.log(new A().getFoo() + 1)

and we do get better optimized output:

 'use strict';console.log(2);

This might not generalize well, but if it does, it surprises me to be so stark.

In contrast, just counting bytes of unoptimized but minified code, while get foo() could be rewritten to something like get a(), a method getFoo() could be rewritten to a() - clearly savings there of 4 bytes in the declaration.

We seem to make it back quickly in usage: invoking a.a(1) (or a(a,1) if made static) as a setter method is a byte longer than a.a=1, so any set accessor used more than 4 times would break even compared to a setter method.

Am I missing something, or does it really boil down to closure not optimizing accessors?

[gkdn]

If it hasn't changed recently, Closure compiler doesn't optimize set/get properties. However J2CL has specific passes in Closure compiler to optimize some of them (can't remember in which cases we fallback though).

re. inlining decisions; Closure compiler heuristic put into account the number of usages when making inlining decision.

re. the design of record JsType in general; unfortunately that will need to wait longer - we will like to come back to it in 1-2 quarters. (Though I'm currently leaning towards preserving the contract by default for migration reasons)

[niloc132]

Note that I made a mistake in the above JS samples, but the outcome is presently the same - trivial property access is never optimized out by closure at this time:

class A {
  get foo() {
    return 1;
  }
}

console.log(new A().foo + 1)// no "()"s after "foo"

 'use strict';class a{get g(){return 1}}console.log((new a).g+1);

[niloc132]

I've rewritten this PR to only modify JsProperty, so that they make sense on record accessors.

record Point(@JsProperty int x, @JsProperty int y) {}

In the above example, without this change, the @JsProperty annotation applies both to the private final int x and public int x() {return this.x;} that Java generates for you, which would be wrong for two reasons - the method doesn't follow JavaBean naming conventions (and must not), and both field and method have the same name (which would collide in JS).

[niloc132]

Any thoughts on this downscoped set of changes to resolve the contradiction presented by annotations on record components?

[gkdn]

For the essence of proposed change: yes we wouldn't let such conflict happen. Being said that talking about record types without supporting them would b e misleading at the moment. I think we should evaluate this all pieces together and update the annotations/documentation etc with the right wording when we actually start working on supporting record types.