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

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

Open
realityforge opened this issue May 21, 2019 · 3 comments
Open

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

realityforge opened this issue May 21, 2019 · 3 comments

Comments

@realityforge
Copy link
Contributor

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 added a commit to realityforge/closure-compiler that referenced this issue May 21, 2019
…overrides of same methods by HTMLDocument

The open(...), close(), write(...) and writeln(...) methods on Document are
now part of the standard and have been moved to w3c_dom1.js to reflect this.

The typing of Document.prototype.open and Document.prototype.close was
previously incomplete. These have been aligned with the specification
and with the methods in HTMLDocument that overrode these methods. This
may impact type checking but that would be very surprising.

The overrides in HTMLDocument have been removed. This is done due to
limitations of the jsinterop-generator tool that is more fully described
in google/jsinterop-generator#35 While the specifications explicitly
indicate that these overrides are present and annotates (requiring the
[OverrideBuiltins] attribute be applied to the containing interface in
WebIDL) it is not expected that this has any practical impact on programs
type checking against the externs.
@realityforge
Copy link
Contributor Author

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

@realityforge realityforge reopened this May 21, 2019
realityforge added a commit to realityforge/closure-compiler that referenced this issue Jun 1, 2019
…overrides of same methods by HTMLDocument

The open(...), close(), write(...) and writeln(...) methods on Document are
now part of the standard and have been moved to w3c_dom1.js to reflect this.

The typing of Document.prototype.close has been aligned with the specification
and with the overriding close methods in HTMLDocument.

The overrides  of close(), write() and writeln() in HTMLDocument have been
removed. This is done due to limitations of the jsinterop-generator tool that
is more fully described in google/jsinterop-generator#35 While the specifications
explicitly indicate that these overrides are present and annotates (requiring the
[OverrideBuiltins] attribute be applied to the containing interface in
WebIDL) it is not expected that this has any practical impact on programs
type checking against the externs.
@jDramaix
Copy link
Member

jDramaix commented Jun 7, 2019

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.

blickly pushed a commit to google/closure-compiler that referenced this issue Jun 7, 2019
…overrides of same methods by HTMLDocument

The open(...), close(), write(...) and writeln(...) methods on Document are
now part of the standard and have been moved to w3c_dom1.js to reflect this.

The typing of Document.prototype.close has been aligned with the specification
and with the overriding close methods in HTMLDocument.

The overrides  of close(), write() and writeln() in HTMLDocument have been
removed. This is done due to limitations of the jsinterop-generator tool that
is more fully described in google/jsinterop-generator#35 While the specifications
explicitly indicate that these overrides are present and annotates (requiring the
[OverrideBuiltins] attribute be applied to the containing interface in
WebIDL) it is not expected that this has any practical impact on programs
type checking against the externs.

Closes #3399.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=251955300
@realityforge
Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants