public methods are too eagerly optimized away; need ability to declare "@export" annotation
kohlschuetter opened this issue · 9 comments
Describe the bug
I want to export a transpiled Java class to other JavaScript code in my application (i.e., outside the influence of j2cl/closure).
However, Java methods clearly marked as "public" are optimized away by the closure compiler, probably as they're not marked with a JavaScript @export annotation. When I manually run closure with a modified version of the transpiled code (adding @export to the corresponding JavaScript methods), everything works as expected.
I wonder if adding @export in this case should be the default behavior. Alternatively, introducing a @JsExport annotation (or similar) would also be helpful.
To Reproduce
Please try send us the minimal Java code snippet that reproduces the problem.
- Add the following method to
HelloWorldinsamples/helloworld/src/main/java/com/google/j2cl/samples/helloworldlib/HelloWorld.java:
public String getHelloWorld1() {
return "Hello World!";
}
- Add the following code below
sayHello()insamples/helloworld/src/main/java/com/google/j2cl/samples/helloworld/app.js:
const HelloWorld = goog.require('com.google.j2cl.samples.helloworldlib.HelloWorld');
goog.exportSymbol("hello.world", HelloWorld);
-
Run
build_test_samples.sh -
Check the generated JavaScript for the presence of the method (even if renamed, we should get a match for the returned string)
cat bazel-bin/src/main/java/com/google/j2cl/samples/helloworld/helloworld.js | grep World
(Note that due to the bug this doesn't return anything!)
Bazel version
Bazelisk version: v1.18.0
Build label: 5.4.0
Build target: bazel-out/darwin_arm64-opt/bin/src/main/java/com/google/devtools/build/lib/bazel/BazelServer_deploy.jar
Build time: Thu Dec 15 16:14:44 2022 (1671120884)
Build timestamp: 1671120884
Build timestamp as int: 1671120884
Expected behavior
The generated JavaScript file should contain a method that returns "Hello World!", and after executing the JavaScript, the following code should return it as well:
var hw = new hello.world();
hw.getHelloWorld1();
Workaround
Option 1: Manually add @export annotation after code generation but before invoking closure compiler:
class HelloWorld extends j_l_Object {
//...
/** @return {?string} @export */
getHelloWorld1() {
return 'Hello World!';
}
Option 2: Modify JavaScriptImplGenerator.java:
in getJsDoc, add the following else if after if (!methodDescriptor.canBeReferencedExternally()
} else if (methodDescriptor.getJsVisibility() == Visibility.PUBLIC && methodDescriptor.getOrigin() == MethodOrigin.SOURCE) {
jsDocBuilder.append(" @export");
}
Option 3: Properly add support for a @JsExport Java annotation that is translated to @export in JavaScript.
Another option: https://github.com/treblereel/gwt3-processors experimentally makes use of an annotation processor to emit a .native.js (or .js file? its been a while, I don't recall) to emit goog.exportSymbol/goog.exportProperty calls, which are functionally equivalent to the @export jsdoc.
Emitting @export when generating JS is risky (as in option 2/3), since you are only transpiling a java library in isolation, with no knowledge of how it will be used in the downstream "application". GWT 2.x doesn't do this either btw, any @JsType(isNative=false) will not be exported unless -generateJsInteropExports is passed at compile time. The goog.exportSymbol/goog.exportProperty calls do not need to be in the same JS file where the type/member was declared, allowing this decision to be made "downstream" of where the @JsType was defined.
Some care must be taken - exportProperty (or even @export) effectively doesn't seem to work on actual properties, as it exports the current value rather than a reference to the existing field/accessor. We've experimented in trying to find a better way to resolve this, but not yet talked to the closure-compiler folks about what might be wrong here.
@niloc132 From my tests, google.exportSymbol on the class is not equivalent to @export on the class methods, as the exportSymbol call doesn't transitively apply to its members (it would have to apply to the prototype members since no instance of the class was instantiated).
The point is exactly that I want these accessible regardless of what j2cl thinks is being used at runtime. This is essential for building libraries (the final product may still be optimized through a separate pipeline)
For my current testing, I'm using a combination of google.exportSymbol (for the class) plus option 2 (modifying JavaScriptImplGenerator). I'd actually prefer proper code to be generated instead of depending on google.exportSymbol, which under the hood uses execScript to declare the path components. Generated JavaScript code could optimize that, too.
So, ideally, one would have a Java annotation @JsExport that can be used for the class as well as for any fields and methods, and j2cl would automatically generate code as requested.
The annotation could have a boolean property that indicates whether members are automatically exported as well, or not, and a boolean property to disallow export (i.e., export=false).
It could also have a property to specify the name, which could default to the Java class name.
Something like this:
@Retention(RetentionPolicy.RUNTIME)
@Target({ElementType.METHOD, ElementType.FIELD, ElementType.TYPE})
@Documented
public @interface JsExport {
boolean export() default true;
boolean includeMembers() default false;
String name() default ""; // use Java name by default (for classes, use fully-qualified name)
}Unreachable public methods are optimized away by design. Applications with J2CL + jscompiler are compiled under a whole world assumption to produce small "binaries". Otherwise lots of public methods in the libraries and all the code reachable to those would be in the binary, defeating the purpose.
You can export out of the binary as mentioned above using goog.exportSymbol and goog.exportProperty. That being said I have no experience in writing application code.
@rluble Would you be open to a PR?
goog.exportSymbol does NOT solve the issue, as I explained above.
@kohlschuetter goog.exportSymbol("hello.world", HelloWorld); exports the constructor object, not its properties. Each property is individually considered for dead code removal.
Please see details here.
@gkdn Yes, exactly, that's the problem.
Right now, we throw the problem from j2cl over the fence to closure-compiler, where we rely that whatever dependency logic remains from closure-library will do "the right thing", which it obviously can't because from its perspective it doesn't even know Java exists...
I had hoped that there would be an easy way to declare what I want to export from the Java side, by means of annotation.
Having said that, it's probably quite easy for me to fix this for my fork. I just wonder if this is something you'd accept as a fix.
Sorry I don't understand what the problem is and how it is related to JsCompiler doesn't understand from Java (though actually that is not true, JsCompiler understands J2CL). The code there is doing the right thing; the behavior in handwritten JavaScript and J2CL/Java generated code are same; each class and its properties are different candidates for dead code pruning; that has nothing to do with Java or JavaScript.
Also keep in mind that you don't need to export anything explicitly unless they are used outside of your application (javascript+java) boundary. What you are doing with goog.exports is telling the optimizer what should be accessible from outside of your app (i.e. due to the code not seen by optimizer). If your javascript code is seen with the optimizer, you also don't need that.
(And we cannot accept such a PR, since it will completely defeats dead code pruning)
OK, I understand this is out of scope for your project. I'm trying to use j2cl to build JavaScript code that can be reused beyond a monolithic j2cl-controlled application, that's all. Once I have a working environment that demonstrates the utility of this annotation, I will keep you posted in this issue. (I'm not proposing to use the solution in Option 2, by the way, that approach is obviously too broad)