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.

Sign up for GitHub

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

Clarify non-native @JsType support for Java 16 records #7

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Clarify non-native @JsType support for Java 16 records #7

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
90bcc64
Clarify non-native @JsType support for Java 16 records
niloc132 Apr 7, 2024
ed6445a
Revert existing changes, update JsProperty instead to make sense with
niloc132 May 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion java/jsinterop/annotations/JsType.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@
*
* <p>Marking an object with JsType is similar to marking each public member of the class with
* {@link JsProperty}/{@link JsMethod}/{@link JsConstructor} respectively. In order for this to work
* correctly the JavaScript name needs to be unique for each member. Some unobvious ways to cause
* correctly the JavaScript name needs to be unique for each member. Some nonobvious ways to cause
* name collisions are:
*
* <ul>
Expand All @@ -48,6 +48,9 @@
* <p>For native interfaces with no particular JavaScript type associated with them (e.g. structural
* types) it is recommended to use {@code namespace = JsPackage.GLOBAL} and {@code name = '?'}.
*
* <p>If a Java {@code record} is marked a "non-native" JsType public accessor methods are treated
* as though they are annotated with {@link JsProperty}.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

*
* <p><b>Instanceof and Castability:</b>
*
* <p>If the JsType is native, the generated code will try to mimic Javascript semantics.
Expand Down