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

Can we use traits to establish the inheritance hierarchy when importing JS classes and interfaces? #152

Closed
akritrime opened this issue Apr 20, 2018 · 17 comments

Comments

@akritrime
Copy link

akritrime commented Apr 20, 2018

For example in the DOM API, 'Node is an interface from which a number of DOM API object types inherit'(from MDN docs). Now there is no way in wasm-bindgen yet to specify such a relationship between imported types (as far as I know), which leads to lot of repeated codes when trying to import inherited methods for two separate types.

@alexcrichton
Copy link
Contributor

Yeah this is a good point! I haven't thought much about how inheritance hierarchies will work and how it'll be modeled in Rust. I think there's some good precedent in other ecosystems, but I'd just want to take sme time to evaluate it to figure out the best way to tackle this.

@akritrime
Copy link
Author

Fair enough.

@akritrime akritrime changed the title Can we use traits to establish the inheritance hierarchy when importing JS classes and inheritances? Can we use traits to establish the inheritance hierarchy when importing JS classes and interfaces? Apr 21, 2018
@alexcrichton
Copy link
Contributor

Oh sorry I'm actually totally fine leaving this issue open, just wanted to write down my current thoughts. This is definitely something I think that should be tracked and tackled!

@alexcrichton alexcrichton reopened this Apr 21, 2018
@akritrime
Copy link
Author

Ah cool.

@Pauan
Copy link
Contributor

Pauan commented Apr 21, 2018

That's exactly what stdweb does.

It has an IEventTarget trait, and then an INode trait which inherits from IEventTarget, and then an IElement trait which inherits from INode, etc.

This works out well in practice.

I'm deeply involved in stdweb (both in its implementation and also using it for real projects), so I can give advice, and I'm totally interested in helping get wasm-bindgen up to parity with stdweb.

@alexcrichton
Copy link
Contributor

alexcrichton commented Apr 23, 2018

Seems plausible to me! I'd be totally down for a sketch of what codegen may generate

@willmruzek
Copy link

Am I correct that this issue is asking about importing and representing existing JS class hierarchies rather than creating new ones?

I'm trying to figure out how to import a JS class, extend it in Rust, and export it to JS.

@alexcrichton
Copy link
Contributor

@mruzekw correct! I think defining a Rust struct which "inherits" from a JS class would probably be a separate issue

@fitzgen
Copy link
Member

fitzgen commented Jul 11, 2018

Talked with @alexcrichton about this a little yesterday. I think there are two separate axis involved with supporting bindings to JS things that have single inheritance relationships (not broaching cross language inheritance):

  1. Up and down casting between derived and base types
  2. Functions that are generic over some base type (and do they use static or dynamic dispatch for invocations of base types' methods?)

I think we can mostly design and implement these two things separately.

@ohanar
Copy link
Member

ohanar commented Jul 11, 2018

@fitzgen For number 1, I am really quite fond of how stdweb does things: upcasts use the From trait, and downcasts use the TryFrom trait (well, actually a version of TryFrom that they include in their own crate, since the real TryFrom seems to be in continual stabilization hell).

Regarding number 2, one alternative the previous proposal of using a trait hierarchy like stdweb, is to use Deref instead. Of course this would be be dynamic dispatch, rather than static, so we should make sure that we wouldn't be paying too much of a penalty for this.

I personally found with stdweb that I would always forget to import all the traits I needed, and was a bit bit frustrated. I would likely get used to importing these traits after more time using the library, but this felt like a barrier to entry for me. One thing in favor of using traits, is that it is much more forward compatible should the standard ever allow multiple inheritance.

@fitzgen
Copy link
Member

fitzgen commented Jul 11, 2018

@ohanar, deref doesn't actually work for (2). I think I didn't explain clearly enough. Consider the following example:

Some JS:

class Base {
  method() { return 0; }
}

class Derived extends Base {
  method() { return 1; }
}

And some wasm-bindgen bindings to that JS:

#[wasm_bindgen]
extern {
    type Base;

    #[wasm_bindgen(method)]
    fn method(this: &Base) -> i32;

    #[wasm_bindgen(extends = Base)]
    type Derived;

    #[wasm_bindgen(method)]
    fn method(this: &Derived) -> i32;
}

And a function that takes a Base reference and calls its method method:

fn call_method(b: &Base) -> i32 {
    b.method()
}

When we invoke call_method(&derived.upcast_to_base()) do we expect Base::method or Derived::method to be called? There are valid reasons to want either choice in different scenarios. This is basically whether a function is virtual in C++.

The Rust equivalent, assuming we used traits to represent inheritance, would be fn call_method(b: &Base) -> i32 {...} vs fn call_method<B: BaseTrait>(b: &B) -> i32 {...} vs fn call_method(b: &BaseTrait) -> i32 {...}.

@ohanar
Copy link
Member

ohanar commented Jul 11, 2018

@fitzgen Sure, I understand that, but I don't see how that problem exists in this context, at least for targeting WebIDL (which is our primary target here).

I put together a playground with what I think you are suggesting for traits. I put a couple of comments in there, and I'm wondering if you have an example snippet of WebIDL that would result in producing unique code for DerivedType as BaseTrait.

The only bit from the WebIDL spec that I found mentioning how overriding members behave was the following:

Interfaces may specify an interface member that has the same name as one from an inherited interface. Objects that implement the derived interface will expose the member on the derived interface. It is language binding specific whether the overridden member can be accessed on the object.

And, at least how I read it, I don't see ever having a case where we can access an overridden method while we only have a reference to a base interface.

@fitzgen
Copy link
Member

fitzgen commented Jul 11, 2018

Note: I'm just being cautious and exploring the problem space a bit. I'm certainly not trying to make strong statements that anything proposed thus far is fundamentally flawed or anything like that! Just thinking out loud a bit.


First observation: inheritance is widespread in Web interfaces. Consider HTMLAnchorElement; it has this inheritance chain:

HTMLAnchorElement
    |
    V
HTMLElement
    |
    V
Element
    |
    V
Node
    |
    V
EventTarget
    |
    V
Object

Second observation: most of the methods one actually wants to use on HTMLAnchorElement are actually from base classes. And the methods on other things that I would pass this to are going to take references to the base class. For example, if I am writing my own builder type for building up some DOM structure, I am probably going to have a method something like append(&Node) that is going to internally call something like Node::append_child(&Node). I'm not going to have a method that takes a &HTMLAnchorElement.

The way we grab imported methods, we do const ___wbg_thing_we_are_importing = Base.prototype.method;. When one calls method in fn calll_method(b: &Base) { b.method() }, it will always invoke Base.prototype.method because of the non-structural method import. To me, it seems like it will be really easy to accidentally only call the base methods and not the overridden versions, despite perhaps not intending to do that. One can work around this with structural and doing dynamic prototype chain lookups, however then we won't be compatible with the faster-than-js, pre-validated DOM methods future with the host bindings proposal that we are aiming for.

All that said, I don't know how often methods get overridden in practice in Web APIs. I can't think of any off the top of my head.

Some quick and dirty bash scripting shows that there are a bunch of duplicate method names, although I don't know whether there is inheritance and overriding involved in these:

$ grep -rnE '^\s*\w* \w*\(.*\);$' crates/web-sys/webidls \
    | cut -d ' ' -f 4 \
    | cut -d '(' -f 1 \
    | sort \
    | uniq -c \
    | sort -nr
Output
    321
     17 close
     16 send
     12 clear
      8 reportValidity
      8 remove
      8 postMessage
      8 checkValidity
      7 setCustomValidity
      5 scrollTo
      5 scrollBy
      5 replaceItem
      5 removeItem
      5 or
      5 insertItemBefore
      5 initialize
      5 appendItem
      5 append
      5 abort
      4 scroll
      4 resume
      4 reload
      3 supports
      3 start
      3 setInternal
      3 set
      3 pause
      3 open
      3 has
      3 getElementsByTagNameNS
      3 getElementsByTagName
      3 getElementsByClassName
      3 dump
      3 drawImage
      3 disconnect
      3 deleteInternal
      3 delete
      3 createTouchList
      3 contains
      3 clearInternal
      3 add
<snip>

I put together a playground with what I think you are suggesting for traits.

Yep, this is pretty much what I expect we will eventually do. However, I am not proposing anything yet because I don't think we've fully explored the design space yet.

I'm wondering if you have an example snippet of WebIDL that would result in producing unique code for DerivedType as BaseTrait.

I'm not sure I understand what you mean by "producing unique code".

In general, I'm not worried about the code emitted by wasm-bindgen, so much as generic code people try to write that tries to leverage the inheritance relationships to abstract over types, and how that might go wrong with which bindings that generic code invokes.


To move forward, I think we can do two orthogonal things:

  1. Write an RFC for up-casting and down-casting between concrete types in an inheritance relationship. Define how to describe inheritance by-hand for the proc-macro frontend. I think this is all pretty uncontroversial. Implement it.

  2. Explore some more how to expose inherited methods on derived interfaces, how to make writing code that is generic over all derived types from some base work well, etc. Write up another RFC for this. I think this has more trade offs, subtleties, and moving parts than (1) does, so it is best to separate it out so that we don't block progress on (1) here.

@fitzgen
Copy link
Member

fitzgen commented Jul 12, 2018

RFC for defining single inheritance relationships, upcasting, and downcasting: rustwasm/rfcs#2

(aka bullet (1) from my last comment)

@fitzgen
Copy link
Member

fitzgen commented Jul 12, 2018

All that said, I don't know how often methods get overridden in practice in Web APIs.

Data! Boriz Zbarsky very kindly hacked up Firefox's WebIDL code generator to dump information about overrides. Here is what he passed along to me:

I excluded the following cases:

  1. Static things (that live on constructors).
  2. Writable attr overriding readonly attr.
  • AudioBufferSourceNode.start overrides AudioScheduledSourceNode.start
  • HTMLFormControlsCollection.namedItem overrides HTMLCollection.namedItem
  • HTMLSelectElement.remove overrides Element.remove
  • PerformanceNavigationTiming.toJSON overrides PerformanceResourceTiming.toJSON
  • PerformanceNavigationTiming.toJSON overrides PerformanceEntry.toJSON
  • PerformanceResourceTiming.toJSON overrides PerformanceEntry.toJSON
  • SVGElement.id overrides Element.id
  • SVGElement.className overrides Element.className
  • ShadowRoot.getElementById overrides DocumentFragment.getElementById
  • WebKitCSSMatrix.setMatrixValue overrides DOMMatrix.setMatrixValue
  • WebKitCSSMatrix.multiply overrides DOMMatrixReadOnly.multiply
  • WebKitCSSMatrix.inverse overrides DOMMatrixReadOnly.inverse
  • WebKitCSSMatrix.translate overrides DOMMatrixReadOnly.translate
  • WebKitCSSMatrix.scale overrides DOMMatrixReadOnly.scale
  • WebKitCSSMatrix.rotate overrides DOMMatrixReadOnly.rotate
  • WebKitCSSMatrix.rotateAxisAngle overrides DOMMatrixReadOnly.rotateAxisAngle
  • WebKitCSSMatrix.skewX overrides DOMMatrixReadOnly.skewX
  • WebKitCSSMatrix.skewY overrides DOMMatrixReadOnly.skewY

Some notes about exclusions:

  1. Static things (that live on constructors).

Excluded because static methods/attributes don't really suffer the same issues with overrides due to the way they are invoked.

  1. Writable attr overriding readonly attr.

These are situations where:

interface ReadOnlyThing {
    readonly attribute long x;
}

interface Thing : ReadOnlyThing {
     // this property receives the exact same getter as ReadOnlyThing, but _also_ a setter
    inherit attribute long x;
}

Excluded because it doesn't seem footgun-able either.


Digging into the overrides that do exist.

  • HTMLSelectElement.remove overrides Element.remove

This is pretty terrifying from an API design perspective. The select method returns a completely different type than the element method. I think these are logically two different functions that happen to share a name and are on interfaces that inherit from each other, and are accidentally an override.

  • WebKitCSSMatrix.skewY overrides DOMMatrixReadOnly.skewY

I think we can safely ignore WebKit* prefixed things. I understand why browsers have to implement it for compatibility, but we can move forward past this. Not concerned here.

  • PerformanceNavigationTiming.toJSON overrides PerformanceResourceTiming.toJSON

These toJSON methods extend the emitted JSON to have more specific information when more specific overrides are used. Not the end of the world if the wrong one is used.

  • AudioBufferSourceNode.start overrides AudioScheduledSourceNode.start

These methods have completely different parameter sets :(


Overall, it seems like there are a few cases where calling the base method with a derived thing is going to be a foot gun, but it's not common.

I think we can basically stop worrying too much about this for the most part.

@alexcrichton
Copy link
Contributor

Nice investigation! I'd reach a similar conclusion too :)

@alexcrichton
Copy link
Contributor

With rustwasm/rfcs#2 now in FCP I'm gonna close this in favor of that RFC, any more discussion should definitely happen there!

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

6 participants