koute / stdweb

A standard library for the client-side Web
Apache License 2.0
3.44k stars 177 forks source link

Replace traits with Deref #293

Open Pauan opened 5 years ago

Pauan commented 5 years ago

I had created an RFC for adding in traits to wasm-bindgen (similar to what stdweb is doing), however it has been superseded by an RFC which uses Deref instead. It is highly likely that the Deref RFC will win.

Although I was the biggest proponent for stdweb-style traits (and I still think it's a solid system), I've since come around to Deref, it has several advantages that traits don't have:

(There are some downsides as well, such as that users might not realize that a function/method which accepts a Foo also accepts a Bar or Qux (they might think it only accepts a Foo). This can largely be solved with solid documentation.)

For the sake of gaining those advantages, and also for more consistency with the wasm-bindgen ecosystem, I think we should replace our traits-based system with a Deref-based system.

We will need a new attribute which generates the Deref implementation, something like this:

#[reference(child_of = Foo)]
struct Bar( Reference );

That will generate this code:

impl Deref for Bar {
    type Target = Foo;

    #[inline]
    fn deref( &self ) -> &Self::Target {
        let reference: &::stdweb::Reference = self.as_ref();
        unsafe {
            <Self as ::stdweb::ReferenceType>::from_reference_unchecked_ref( reference )
        }
    }
}

(This also requires adding a new unsafe fn from_reference_unchecked_ref( reference: &Reference ) -> &Self; method on ReferenceType)

Root classes should have #[reference(child_of = Object)], and Object should probably have #[reference(child_of = Reference)] (should Reference have #[reference(child_of = Value)]?)

We should also change subclass_of so that it generates AsRef<Foo> implementations, so that way it's still possible to use AsRef<Foo> in generics.

We will also need a migration path: first we add in Deref and deprecate traits, and then in the next major release we remove the traits.

I'm willing to make a PR that does all of that. But we should wait until the Deref RFC is accepted and implemented in wasm-bindgen.

Pauan commented 5 years ago

Alternatively, we could have a system where sub-classes don't contain a Reference, instead they contain the super-class, like this:

#[derive(Clone, Debug, PartialEq, Eq, ReferenceType)]
#[reference(instance_of = "Foo")]
struct Foo( Reference );

#[derive(Clone, Debug, PartialEq, Eq, ReferenceType)]
#[reference(instance_of = "Bar")]
#[reference(subclass_of(Foo))]
#[reference(child_of = Foo)]
struct Bar( Foo );

#[derive(Clone, Debug, PartialEq, Eq, ReferenceType)]
#[reference(instance_of = "Qux")]
#[reference(subclass_of(Foo, Bar))]
#[reference(child_of = Bar)]
struct Qux( Bar );

This makes it possible to convert from a &Bar to a &Foo without using ReferenceType::from_reference_unchecked_ref.

NeverGivinUp commented 5 years ago

I wonder, if this should be considered separately from or combined with the long term goal of making stdweb compatible to (or building stdweb upon) wasm-bindgen, web-sys and js-sys. @Pauan's first proposition seems to aim at the compatibility goal saying

for more consistency with the wasm-bindgen ecosystem

, but shys away from introducing strong dependencies like inheriting from js-sys' Object struct. @Pauan's second proposition is different to wasm-bindgen's approach and does not mention the subject of compatibility (and personally I'm to ignorant to judge it myself).

Pauan commented 5 years ago

@NeverGivinUp It's both.

The long-term goal of compatibility with wasm-bindgen will require a lot of changes (including breaking changes). It will take a long time and a lot of work.

So it must be done incrementally, in small chunks. This is one of those chunks. And it can be implemented independently of the other chunks.

Also, this change to Deref is still useful even without wasm-bindgen (due to the advantages I listed).

shys away from introducing strong dependencies like inheriting from js-sys' Object struct.

That's a very big change, requiring a dependency on wasm-bindgen and js-sys. Right now it's not even possible to combine stdweb and wasm-bindgen in the same Rust project, so that's not a feasible change right now (though it is the long-term goal).

second proposition is different to wasm-bindgen's approach and does not mention the subject of compatibility (and personally I'm to ignorant to judge it myself).

Of course it's different from wasm-bindgen: it's the same system that stdweb uses right now (except with Reference replaced with the super-class).

It's not a second proposition, it's describing a small implementation detail (as an alternative to using ReferenceType::from_reference_unchecked_ref).

The user API is the same either way (in both cases it uses Deref).

NeverGivinUp commented 5 years ago

Thanks a lot for clarifying @Pauan. Very helpful, as always (:

Obviously I didn't know what needs do be done for wasm-bindgen-compatibility and how this fits in the grand scheme of things. Is there a list of tasks?

Pauan commented 5 years ago

Is there a list of tasks?

I don't think so, and it's a bit hard to make such a list, since we're not even sure what all the incompatibilities are.

As a first step, wasm-bindgen will need to support some sort of js! macro, as described in this wasm-bindgen issue.

After that it will be a matter of replacing stdweb's runtime with wasm-bindgen's runtime, replacing Value and Reference with the wasm-bindgen equivalents, replacing stdweb's ad-hoc type system with the wasm_bindgen macro, and then rewriting literally every single stdweb API to use the wasm_bindgen macro.

Plus various other changes that will need to happen to cargo-web (which I'm not very familiar with).

It's basically a full rewrite of stdweb from the ground up. This is because the underlying philosophy, implementation, and APIs are very different between wasm-bindgen and stdweb.

koute commented 5 years ago

Thanks for the proposal @Pauan!

Hmm... I would probably be fine with this, however it does make me somewhat uneasy that this wouldn't support multiple interfaces/mixins (as Deref can point to only one type). Are we sure this won't be a problem?

It also feels like the current trait based system is more Rust-y. Many people do think that using Deref to emulate inheritance is a bad practice. Even Rust's official docs says this:

Deref should only be implemented for smart pointers

So it's a little surprising to me that a Deref-based approach is likely to win.

That said, while I probably like trait based system more as it feels more Rust-y and is more flexible, I don't have super strong opinions that it's objectively better, although it does feel to me like it is.

You're right that if we do this it'd be the best to do this progressively. Perhaps instead of adding #[reference(child_of=...)] attribute we could look just inside the structure and if we see struct Foo(Bar) then we assume Bar is the parent and generate a Deref for it? That seems somewhat more natural and doesn't require any extra syntax.

Obviously I didn't know what needs do be done for wasm-bindgen-compatibility and how this fits in the grand scheme of things. Is there a list of tasks?

As Pauan said, there isn't. However I would envision roughly something like this:

1) Get wasm-bindgen to support a basic js_raw_asm! macro. 2) Port our internal __js_raw_asm! and js! macros to procedural macros. 3) Make our JS macros switchable between cargo-web-based implementation and wasm-bindgen one. Now it should be possible to use stdweb in wasm-bindgen projects, although still using our own runtime. 4) Port the internals to use js-sys and web-sys and trim down our runtime. This is extra complicated by the fact that I do still want to support Emscripten for the foreseeable future. Initially this could probably be achieved by writing our own wasm_bindgen procedural attribute macro which would be compatible with the upstream wasm_bindgen macro on a source code level (we could probably reuse most of the parser and just replace the codegen part), but would generate bindings in exactly the same way as we do now. Then we could switch between wasm-bindgen-based runtime (which is wasm32-unknown-unknown only) and the current cargo-web-based runtime (which supports all of the targets) with a feature flag.

Pauan commented 5 years ago

however it does make me somewhat uneasy that this wouldn't support multiple interfaces/mixins

That's a good point, I should have addressed that in my original post.

The simple fact is that JavaScript doesn't support multiple interfaces/mixins, it only has single inheritance.

The interfaces and mixins in the WebIDL spec are purely for the editor's convenience, in actuality the methods will be duplicated in JavaScript.

It sucks to generate duplicate methods in Rust (though macros can help with that), but it is the more semantically precise thing to do.

Long term, once stdweb is using web-sys, mixins/interfaces won't be our concern anymore (since they're handled by web-sys). To be more specific, web-sys does indeed duplicate the methods (and it can do so easily because it auto-generates the bindings, unlike stdweb).

It also feels like the current trait based system is more Rust-y. Many people do think that using Deref to emulate inheritance is a bad practice.

I agree! But that doesn't change the inherent disadvantages of traits (and advantages of Deref). It may not be the intended usage of Deref, but it does seem to work out well regardless.

I think that the advice of avoiding Deref is good for Rust code, but we're providing JavaScript bindings, and as much as I don't like to admit it, Deref does fit in smoothly with JavaScript's semantics.

Also keep in mind that generic situations will use AsRef<Foo>, not Deref.

So it's a little surprising to me that a Deref-based approach is likely to win.

I'll be honest, I was surprised as well. At first most people were on board with traits (with Alex being the only real exception).

But after I carefully and thoroughly documented all the pros and cons of traits vs Deref (and as some people pointed out some use cases I hadn't considered), I slowly came to the realization that Deref is probably better.

It wasn't an easy realization for me to make, and I'm still not 100% sold on it, but it's probably the best option we have (until Rust gets a proper inheritance mechanism).

Perhaps instead of adding #[reference(child_of=...)] attribute we could look just inside the structure and if we see struct Foo(Bar) then we assume Bar is the parent and generate a Deref for it? That seems somewhat more natural and doesn't require any extra syntax.

That idea seems quite reasonable. I'll try some experiments to see whether the struct Foo(Bar) approach will work, or whether it'll require big changes.


I think a good first step is to add in a Deref-based implementation (in addition to the current trait system), and put it under an experimental feature flag. That will let us play around with it and see how well it works, what the migration costs are, etc.

Assuming that works out, then we can enable the Deref implementation by default, and deprecate the trait system, and then later remove the trait system.

Also, I'd like to note that it is possible to have both traits and Deref at the same time. Though this has the issue of causing confusion for users ("should I use traits? Deref? both?")

koute commented 5 years ago

I think a good first step is to add in a Deref-based implementation (in addition to the current trait system), and put it under an experimental feature flag. That will let us play around with it and see how well it works, what the migration costs are, etc.

Sounds good to me!

Pauan commented 5 years ago

Just a heads up that the Deref RFC was accepted (and the traits RFC rejected), just as I predicted it would.

derekdreery commented 5 years ago

It's worth reading the discussion thread for Deref RFC, it goes through quite a lot of the advantages/disadvantages of each approach.