@JsEnum types cannot be used in a union

[niloc132]

Steps to repro - use an externs file with an enum that is referenced by a union type. Roughly:

/**
 * @enum {number}
 */
foo.bar.SomeEnum = {
  ONE: 0,
  TWO: 1,
  THREE: 2,
  FIVE: 3,
  SEVEN: 4
};

/**
 * @type {foo.bar.SomeEnum|string}
 */
foo.bar.ObjectWithProperties.prototype.thing;

The error is when j2cl tries to transpile the sources into js, it is reported that the union type is attempting an instanceof on the enum instead of just treating it like a number (which in this case is all it is).

Easy workaround: rewrite ObjectWithProperties.thing's type to be {number|string}, though that defeats the purpose a bit.

Proposed fix (not having given it great thought): The union type should instanceof based on the type label: in this case, number. This seems to be a bit difficult to implement though, since the original @enum might be already made into a java type? The jsinterop.generator.model.Type instances don't keep track of the type label at this time, so that would need to be added? It could potentially also be helpful in general, j2cl itself could implement $isInstance with this knowledge if it were part of the @JsEnum annotation?

Easier error reporting option: instead of waiting for j2cl to fail the generated java, something like this could throw when the generator is trying to emit the union's isFoo() method:

index b72a70a..c9d2e7b 100644
--- a/java/jsinterop/generator/visitor/UnionTypeHelperTypeCreator.java
+++ b/java/jsinterop/generator/visitor/UnionTypeHelperTypeCreator.java
@@ -262,6 +262,10 @@ public class UnionTypeHelperTypeCreator implements ModelVisitor {
       return new ArrayTypeReference(OBJECT);
     }
 
+    if (typeReference.getTypeDeclaration() != null && typeReference.getTypeDeclaration().isEnum()) {
+      throw new IllegalStateException("Can't use @enum in instanceof, so cannot construct union type");
+    }
+
     // remove Type parameters
     if (typeReference instanceof ParametrizedTypeReference) {
       return ((ParametrizedTypeReference) typeReference).getMainType();

[niloc132]

Riffing on this idea a bit more:

It could potentially also be helpful in general, j2cl itself could implement $isInstance with this knowledge if it were part of the @JsEnum annotation?

This could be handy too, in cases like

/**
 * @enum {string}
 */
MyEnum = {
 "Orange":"Foo",
 "Blue":"Bar"
}

where the string keys don't match the enum name or the ordinal doesn't match the value, so you can either do the "proper" java thing and use MyEnum.values()[n] or MyEnum.valueOf(str), or you can do the "its JS, I do what I want" thing of Js.<MyEnum>cast("Foo").

[gkdn]

IIRC, we discussed earlier that externs should not have enum definitions. if we are not handling the correctly we should probably have a good warning about it.

I think Closure is willing to accept patches for converting such enums to number.

@jDramaix please correct me if I'm wrong.

[niloc132]

Perhaps jsinterop-generator should drop support for @enum, as it currently (correctly) creates native @JsEnum types? I think treating those enum types as if they were the aliased type could make sense too?

Related: I'm having trouble with @typedef as well, getting errors when those are handled. There appears to be explicit support in jsinterop-generator for this (for example, jsinterop.generator.closure.visitor.AbstractClosureVisitor#acceptTypedef), though I don't see tests for it (so I'm unclear of the typedefs in closure's externs are handled wrong, or written wrong). That said, if this typedef mechanism worked, perhaps @enum code could behave a bit like that?

[jDramaix]

IIRC, we discussed earlier that externs should not have enum definitions. if we are not handling the correctly we should probably have a good warning about it.

Externs used in Elemental2 should not use closure enum as they represent native api. If your extern represents a closure library, it can use closure enum.

[niloc132]

Stupid question: If it is a closure library, then by definition it isn't an extern, right? Wouldn't it just be the closure library as input at that point?

Assuming so, could the jsinterop-generator tool detect the presence/absence of @externs in the file to know how it should generate the enum?

Concrete example of "closure-compiler provided externs, which definitely do not refer to a closure-library": https://github.com/google/closure-compiler/blob/master/contrib/externs/maps/google_maps_api_v3.js#L4713-L4728

[jDramaix]

A closure library can provide an extern file if it does not provide the src but a binary (compiled js). The maps api is the concrete example.
IF you call maps api in you code, thanks to the extern file, the js compiler can do type checking against the calls the maps api inside your code.

[niloc132]

Can you clarify the distinction then you were making, where "externs used in elemental2 should not use closure enum"? I'm afraid I'm not understanding the point you were trying to convey.

[jDramaix]

Elemental2 is the jsinterop version of apis provided by the browser. Browser code is not closure annotated code and they never define a closure type enum.

Google maps is not an api provided by a browser but by a third party javascript library. As this library is written using closure type system, it can define closure enum type.

[gkdn]

I don't think Google Maps vs Browser change things much.
I think it is still controversial to define enum in this case; this is because the extern should never be the source of truth of those values and defining them on enum does that.

[niloc132]

@jDramaix right - I'm filing this issue against jsinterop-generator ("Generates Java annotated with JsInterop from JavaScript extern sources"), not elemental2. My only goal in this is to understand/document/expand the limitations of jsinterop-generator when trying to generate jsinterop sources based on arbitrary externs, not to specifically tailor this work to browser apis.

[niloc132]

So what is the resolution here? Goktug indicated that a change could be made on closure, but I don't quite follow what that change would be:

I think Closure is willing to accept patches for converting such enums to number.

Would this mean that in generating externs, you would get number instead of the @enum type? Would tsickle also be modified to emit number params instead of enums when externs are generated?

And for projects which have closure JS side by side with Java (see #44), is it expected then that @enum would be outright removed? Or is this project not intended to support closure js/ts to be used with Java (not as externs, but to let j2cl java be compatible with those existing closure js types)?

[gkdn]

I'm not following most of your questions/conclusions.

You can just send a patch to closure extern to make it a number.

[niloc132]

@gkdn ok, that was one of the options I was trying to offer, sorry if that was not clear. Are you saying that generally when we find an @enum in closure externs it should be a @const with @type {number} members?

[gkdn]

Yes, that's what we were told earlier.

[niloc132]

Thanks, I appreciate the clarification.