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.

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

Support instance member overriding a static member with union types #48

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
13 changes: 8 additions & 5 deletions java/jsinterop/generator/visitor/MembersClassCleaner.java
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
package jsinterop.generator.visitor;

import static jsinterop.generator.model.AnnotationType.JS_METHOD;
import static jsinterop.generator.model.AnnotationType.JS_OVERLAY;
import static jsinterop.generator.model.AnnotationType.JS_PROPERTY;
import static jsinterop.generator.model.EntityKind.METHOD;

Expand Down Expand Up @@ -163,12 +164,14 @@ private <T extends Entity> void renameSameEntities(
String name = entity.getName();
entity.setName(name + "_STATIC");

// TODO(dramaix): add a javadoc above the field explaining why it was renamed
AnnotationType annotationType =
entity.getKind() == METHOD ? JS_METHOD : JS_PROPERTY;
if (!entity.hasAnnotation(JS_OVERLAY)) {
// TODO(dramaix): add a javadoc above the field explaining why it was renamed
AnnotationType annotationType =
entity.getKind() == METHOD ? JS_METHOD : JS_PROPERTY;

ModelHelper.addAnnotationNameAttributeIfNotEmpty(
entity, name, annotationType, true);
ModelHelper.addAnnotationNameAttributeIfNotEmpty(
entity, name, annotationType, true);
}
}
}
}
Expand Down
27 changes: 27 additions & 0 deletions java/jsinterop/generator/visitor/UnionTypeHelperTypeCreator.java
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@
import jsinterop.generator.model.Annotation;
import jsinterop.generator.model.ArrayTypeReference;
import jsinterop.generator.model.CastExpression;
import jsinterop.generator.model.Entity;
import jsinterop.generator.model.Field;
import jsinterop.generator.model.InstanceOfExpression;
import jsinterop.generator.model.JavaTypeReference;
Expand Down Expand Up @@ -78,14 +79,34 @@ public void applyTo(Program program) {
new AbstractRewriter() {
private final Deque<String> currentNameStack = new ArrayDeque<>();

private final Deque<Type> currentTypeStack = new ArrayDeque<>();

@Override
public boolean shouldProcessType(Type node) {
currentTypeStack.push(node);
return super.shouldProcessType(node);
}

@Override
public Type rewriteType(Type node) {
currentTypeStack.pop();
return node;
}

@Override
public boolean shouldProcessField(Field field) {
if (field.isStatic() && currentTypeStack.peek().getFields().stream().filter(f -> !f.equals(field)).anyMatch(f -> f.getName().equals(field.getName()))) {
currentNameStack.push("Static");
}
Copy link
Member

@jDramaix jDramaix Jul 22, 2020

Choose a reason for hiding this comment

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

I need to think more about that. This is a breaking change. every static fields/methods with an union type will see a change in the name of their UnionType helper.

Copy link
Contributor Author

@niloc132 niloc132 Jul 22, 2020

Choose a reason for hiding this comment

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

Agreed, I don't love it either, but was trying to take my cue from the entity.setName(name + "_STATIC"); line in MembersClassCleaner, which at least only adds the suffix if necessary. I think was a reason I had to put this in the visitor instead of the loop at the end, but I've forgotten now what it was, I'll try it again to see if I can remember, or if I can move to that loop and only change already-broken code, rather than modify all static fields, methods.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The case I'm specifically working on here is that the protobuf impl for JS offers instance and static methods with the same names, and the same overloads, so the exact same union gets generated twice. In a specific case like that, it could even be possible to observe that the union and the method name are the same, so share the union type rather than generate it again.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed this code a bit - it isn't pretty, but hoping it touches the basics of what you were thinking of? In short, if the field is static, and there is another field with the same name, then we need the Static suffix - naturally this won't affect any existing code, since it would have been affected by this bug.

For methods it is slightly more complicated, we need to only test the non-overlay methods, since those just call the one "real" method, which takes the union type.

I havent updated tests yet, but presently all existing and new tests pass - once we settle on the approach I'll get those in line with what you are requesting there too?

Copy link
Member

Choose a reason for hiding this comment

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

I think we should take another approach. The MembersClassCleaner is doing too much thing: it's renaming methods/fields, removing constructors/methods or fields and modifying method's generics. We should split that in different visitors: I think at least two in order to modify the method generics in a separate visitor.
The visitor that will rename class members can be run before the UnionTypeMethodParameterHandler and the one that modify method's generics need to be called after the FixReferencesToDuplicatedTypes visitor.
I think it will solve all the issues.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I'll close this for now - it will probably be a few months until I have something to show here, I'm going on personal leave sometime in the next week and will not be in a position any more to take on work like this.

Copy link
Member

@jDramaix jDramaix Jul 30, 2020

Choose a reason for hiding this comment

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

No problem and thanks your patch and opening the issue. I can take over the work. Enjoy your leave!!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi, checking back on this, are you still able to resolve this some other way?

currentNameStack.push(toCamelUpperCase(field.getName()));
return true;
}

@Override
public Field rewriteField(Field field) {
if (field.isStatic() && currentTypeStack.peek().getFields().stream().filter(f -> !f.equals(field)).anyMatch(f -> f.getName().equals(field.getName()))) {
currentNameStack.pop();
}
currentNameStack.pop();
return field;
}
Expand All @@ -95,6 +116,9 @@ public boolean shouldProcessMethod(Method method) {
// JsFunction type only have one method named onInvoke, don't use the method name
// because it doesn't give us any much more information.
if (!isCurrentTypeJsFunction()) {
if (method.isStatic() && currentTypeStack.peek().getMethods().stream().filter(m -> !m.equals(method)).filter(m -> !m.hasAnnotation(JS_OVERLAY)).anyMatch(m -> m.getName().equals(method.getName()))) {
currentNameStack.push("Static");
}
currentNameStack.push(
method.getKind() == CONSTRUCTOR
? "Constructor"
Expand All @@ -106,6 +130,9 @@ public boolean shouldProcessMethod(Method method) {
@Override
public Method rewriteMethod(Method method) {
if (!isCurrentTypeJsFunction()) {
if (method.isStatic() && currentTypeStack.peek().getMethods().stream().filter(m -> !m.equals(method)).filter(m -> !m.hasAnnotation(JS_OVERLAY)).anyMatch(m -> m.getName().equals(method.getName()))) {
currentNameStack.pop();
}
currentNameStack.pop();
}

Expand Down
22 changes: 22 additions & 0 deletions javatests/jsinterop/generator/externs/overloads/BUILD
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
# Description:
# Tests conversion of "overloaded" members, static and non-static
#

load(
"//javatests/jsinterop/generator:jsinterop_generator_test.bzl",
"jsinterop_generator_test",
)

package(
default_visibility = ["//:__subpackages__"],
# Apache2
licenses = ["notice"],
)

jsinterop_generator_test(
name = "overloads",
srcs = ["overloads.js"],
expected_output = [
"Foo.java.txt",
],
)
172 changes: 172 additions & 0 deletions javatests/jsinterop/generator/externs/overloads/Foo.java.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,172 @@
package jsinterop.generator.externs.overloads;

import jsinterop.annotations.JsFunction;
import jsinterop.annotations.JsMethod;
import jsinterop.annotations.JsOverlay;
import jsinterop.annotations.JsPackage;
import jsinterop.annotations.JsProperty;
import jsinterop.annotations.JsType;
import jsinterop.base.Js;

@JsType(isNative = true, namespace = JsPackage.GLOBAL)
public class Foo {
@JsFunction
public interface FunctionPropertyFn {
void onInvoke();
}

@JsFunction
public interface FunctionPropertyFn0 {
void onInvoke();
}

@JsType(isNative = true, name = "?", namespace = JsPackage.GLOBAL)
public interface StaticUnionArgMethodArgUnionType {
@JsOverlay
static Foo.StaticUnionArgMethodArgUnionType of(Object o) {
return Js.cast(o);
}

@JsOverlay
default double asDouble() {
return Js.asDouble(this);
}

@JsOverlay
default String asString() {
return Js.asString(this);
}

@JsOverlay
default boolean isDouble() {
return (Object) this instanceof Double;
}

@JsOverlay
default boolean isString() {
return (Object) this instanceof String;
}
}

@JsType(isNative = true, name = "?", namespace = JsPackage.GLOBAL)
public interface StaticUnionPropertyUnionType {
@JsOverlay
static Foo.StaticUnionPropertyUnionType of(Object o) {
return Js.cast(o);
}

@JsOverlay
default double asDouble() {
return Js.asDouble(this);
}

@JsOverlay
default String asString() {
return Js.asString(this);
}

@JsOverlay
default boolean isDouble() {
return (Object) this instanceof Double;
}

@JsOverlay
default boolean isString() {
return (Object) this instanceof String;
}
}

@JsType(isNative = true, name = "?", namespace = JsPackage.GLOBAL)
public interface UnionArgMethodArgUnionType {
@JsOverlay
static Foo.UnionArgMethodArgUnionType of(Object o) {
return Js.cast(o);
}

@JsOverlay
default double asDouble() {
return Js.asDouble(this);
}

@JsOverlay
default String asString() {
return Js.asString(this);
}

@JsOverlay
default boolean isDouble() {
return (Object) this instanceof Double;
}

@JsOverlay
default boolean isString() {
return (Object) this instanceof String;
}
}

@JsType(isNative = true, name = "?", namespace = JsPackage.GLOBAL)
public interface UnionPropertyUnionType {
@JsOverlay
static Foo.UnionPropertyUnionType of(Object o) {
return Js.cast(o);
}

@JsOverlay
default double asDouble() {
return Js.asDouble(this);
}

@JsOverlay
default String asString() {
return Js.asString(this);
}

@JsOverlay
default boolean isDouble() {
return (Object) this instanceof Double;
}

@JsOverlay
default boolean isString() {
return (Object) this instanceof String;
}
}

@JsProperty(name = "functionProperty")
public static Foo.FunctionPropertyFn functionProperty_STATIC;

@JsProperty(name = "unionProperty")
public static Foo.StaticUnionPropertyUnionType unionProperty_STATIC;

@JsMethod(name = "noArgMethod")
public static native Object noArgMethod_STATIC();

public static native Object unionArgMethod(Foo.StaticUnionArgMethodArgUnionType arg);

@JsOverlay
public static final Object unionArgMethod_STATIC(String arg) {
return unionArgMethod(Js.<Foo.StaticUnionArgMethodArgUnionType>uncheckedCast(arg));
}

@JsOverlay
public static final Object unionArgMethod_STATIC(double arg) {
return unionArgMethod(Js.<Foo.StaticUnionArgMethodArgUnionType>uncheckedCast(arg));
}

public Foo.FunctionPropertyFn0 functionProperty;
public Foo.UnionPropertyUnionType unionProperty;

public native Object noArgMethod();

@JsOverlay
public final Object unionArgMethod(String arg) {
return unionArgMethod(Js.<Foo.UnionArgMethodArgUnionType>uncheckedCast(arg));
}

public native Object unionArgMethod(Foo.UnionArgMethodArgUnionType arg);

@JsOverlay
public final Object unionArgMethod(double arg) {
return unionArgMethod(Js.<Foo.UnionArgMethodArgUnionType>uncheckedCast(arg));
}
}
41 changes: 41 additions & 0 deletions javatests/jsinterop/generator/externs/overloads/overloads.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@

/**
* @fileoverview Test "overloaded" method conventions
* @externs
*/

/**
* @constructor
*/
function Foo() {}

Foo.noArgMethod = function() {};
Foo.prototype.noArgMethod = function() {};
Copy link
Member

Choose a reason for hiding this comment

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

You can remove these two line, we already have these tests in simpleclass package

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good - my thinking was to have them all be covered here as well, so that one didn't have to scan all test packages to find other overload tests, but your point makes good sense too.


/**
* @param {string|number} arg
*/
Foo.unionArgMethod = function(arg) {};
/**
* @param {string|number} arg
*/
Foo.prototype.unionArgMethod = function(arg) {};

/**
* @type {string|number}
*/
Foo.unionProperty;
/**
* @type {string|number}
*/
Foo.prototype.unionProperty;
Copy link
Member

Choose a reason for hiding this comment

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

I would put all test with union type into the uniontype test package.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do, I wasn't sure if this justified its own test package or not.


/**
* @type {function():void}
*/
Foo.functionProperty;

/**
* @type {function():void}
*/
Foo.prototype.functionProperty;
Copy link
Member

Choose a reason for hiding this comment

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

same here, move the tests in the function type test package

Copy link
Member

Choose a reason for hiding this comment

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

I think we don;t need these tests for this cl.