google / jsinterop-generator

Generates Java annotated with JsInterop from JavaScript extern sources
Apache License 2.0
78 stars 24 forks source link

Error when @override method takes a type union as a parameter #35

Open realityforge opened 5 years ago

realityforge commented 5 years ago

As part of an attempt to whittle down the remaining errors in google/elemental2 builds I tried to define the standardized Document.prototype.write in an extern accessible via Elemental2.

The extern looks something like:

/**
 * @param {!TrustedHTML|string} text
 * @return {undefined}
 * @see https://html.spec.whatwg.org/multipage/dynamic-markup-insertion.html#dom-document-write
 */
Document.prototype.write = function(text) {};

This is overridden in w3c_dom2.js with

/**
 * @param {!TrustedHTML|string} text
 * @return {undefined}
 * @see http://www.w3.org/TR/2000/CR-DOM-Level-2-20000510/html.html#ID-75233634
 * @override
 */
HTMLDocument.prototype.write = function(text) {};

Unfortunately jsinterop-generator generates the overlay methods for union types in Document class like

  @JsOverlay
  public final void write(TrustedHTML text) {
    write(Js.<Document.WriteTextUnionType>uncheckedCast(text));
  }

and attempts to define a similar but different overlay in HTMLDocument like

  @JsOverlay
  public final void write(TrustedHTML text) {
    write(Js.<HTMLDocument.WriteTextUnionType>uncheckedCast(text));
  }

There does not seem to get the overriding method to use the union type and thus the overlay methods declared by the overridden method. Thus a compile error.

realityforge commented 5 years ago

I should note that this is not a blocker (assuming google/closure-compiler#3399 is merged) as the typing of the override method is identical to overridden method and thus I attempted to just remove the overridden method.

However I am leaving this here to track the bug in case it is an issue in the future

jDramaix commented 5 years ago

I don't think we will fix that. The error occurs because there is an problem in the extern file. You should not override a method with an identical signature. It's not really a bug in the extern file but if we want to keep them clean this kind of error in elemental2 plays the role of gatekeeper.

I keep it open in case of there is another side effect I'm not thinking of.

realityforge commented 5 years ago

It is perfectly valid closure typing to tighten the types in an override. i.e. you imagine going from !TrustedHTML|string in the base class to !TrustedHTML in a subclass and this would trigger a similar issue. However this pattern does not exist in current externs so I don't think it is work fixing it at this stage.

Particularly as the only way to fix this probably involves searching type hierarchies or externalizing these synthesized types whichs is a lot of work for something that is not currently used.