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

Replace traits with Deref #293

Open
Pauan opened this issue Oct 19, 2018 · 10 comments
Open

Replace traits with Deref #293

Pauan opened this issue Oct 19, 2018 · 10 comments

Comments

@Pauan
Copy link
Contributor

Pauan commented Oct 19, 2018

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:

  • It is much simpler to implement, and simpler to use (no need to import traits, no need to use generics).

  • It doesn't cause monomorphization bloat, because it's not using generics.

  • With traits you have to choose between these two styles:

    fn foo<A: IFoo>(x: &A) { ... }
    fn foo(x: &Foo) { ... }

    There are various trade-offs between these two styles, which makes it difficult to choose.

    But with Deref, you just always use fn foo(x: &Foo) { ... }, and it will always have the correct behavior.

  • Within the documentation it lists all of the methods that the type supports (right now most of the methods are implemented within traits, so they're not obviously visible in the docs).

(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
Copy link
Contributor Author

Pauan commented Oct 19, 2018

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
Copy link

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
Copy link
Contributor Author

Pauan commented Oct 19, 2018

@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
Copy link

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
Copy link
Contributor Author

Pauan commented Oct 19, 2018

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
Copy link
Owner

koute commented Oct 21, 2018

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
Copy link
Contributor Author

Pauan commented Oct 22, 2018

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
Copy link
Owner

koute commented Oct 24, 2018

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
Copy link
Contributor Author

Pauan commented Nov 9, 2018

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

@richard-uk1
Copy link

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

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

4 participants