hobofan / ambassador

Delegation of trait implementations via procedural macros
Apache License 2.0
251 stars 13 forks source link

Should `ambassador` support delegating to Deref::deref? #36

Closed vi closed 2 years ago

vi commented 2 years ago

Should delegating to Deref::deref be in purview or ambassador?

If yes, how API should look like? Should it be a special mode of #[delegate_to_methods]? Something specified on the trait itself (i.e. a clause in #[delegatable_trait]).

Here is example code with a snippet that is supposed to be ambassadorable ```rust extern crate ambassador; use std::{ops::Deref, rc::Rc, sync::Arc}; use ambassador::{delegatable_trait}; #[delegatable_trait] pub trait Shout { fn shout(&self, input: &str) -> String; } pub struct Cat; impl Shout for Cat { fn shout(&self, input: &str) -> String { format!("{} - meow!", input) } } pub struct Dog; impl Shout for Dog { fn shout(&self, input: &str) -> String { format!("{} - wuff!", input) } } // I want ambassador to generate this: impl> Shout for T { fn shout(&self, input: &str) -> String { Deref::deref(self).shout(input) } } // Auto-generated code stops here fn use_it (shouter: &T) { println!("{}", shouter.shout("BAR")); } pub fn main() { let foo_animal : Rc = Rc::new(Cat); use_it(&foo_animal); let bar_animal : Arc = Arc::new(Dog); use_it(&bar_animal); let baz_animal : &dyn Shout = &Cat; use_it(&baz_animal); let plain_animal : Dog = Dog; use_it(&plain_animal); } ```

Note that it cannot be just a binary flag i.e. "please also derive Deref while we are at it", as Send/Sync business should be manually specifiable by user.

rrbutani commented 2 years ago

@vi does the macro described in #37 work for your use case?

The example on that PR uses a slightly more general delegated impl but it's possible to restrict it to dyn Shout + 'static as in your example:

#[delegate_to_remote_methods]
#[delegate(Shout, target_ref = "deref")]
impl<T: Deref<Target = dyn Shout + 'static>> T {
    fn deref(&self) -> &(dyn Shout + 'static);
}
vi commented 2 years ago

I checked and it works for Deref and target_ref (though I obviously need to change &S to &dyn Shout or & (dyn Shout + Send + Sync).

But when I try to extend it to DerefMut (after changing Shout trait to require &mut self), like this:

#[ambassador::delegate_to_remote_methods]
#[delegate(Shout, target_mut = "deref_mut")]
impl<T: DerefMut<Target = dyn Shout + Send + Sync + 'static>> T {
    fn deref_mut(&mut self) -> & mut (dyn Shout + Send + Sync);
}

, it complains that I need to specify target_ref (error: target_ref was not specified but was needed). But to implement it I need to use Deref, which is in a different impl block.


By the way, does abmassador support (or should support) methods beyond self, &self and &mut self, such as self: Pin<Arc<Self>> - those things are sometimes needed for async trait objects. Shall this be tracked in a separate Github issue?

rrbutani commented 2 years ago

though I obviously need to change &S to &dyn Shout or `& (dyn Shout + Send + Sync

oh whoops, good catch!

But when I try to extend it to DerefMut (after changing Shout trait to require &mut self), like this:

#[ambassador::delegate_to_remote_methods]
#[delegate(Shout, target_mut = "deref_mut")]
impl<T: DerefMut<Target = dyn Shout + Send + Sync + 'static>> T {
    fn deref_mut(&mut self) -> & mut (dyn Shout + Send + Sync);
}

, it complains that I need to specify target_ref (error: target_ref was not specified but was needed)

I believe this is because your Shout trait still has a method that has a &self receiver.

But to implement it I need to use Deref, which is in a different impl block.

This is okay; the impl block ambassador::delegate_to_remote_methods is placed on doesn't have to mirror any actual impl block. It can have method signatures whose actual implementations are in completely different places.

DerefMut implies Deref so you just need to add a deref method signature to the impl block (and a target_ref attr):

#[ambassador::delegate_to_remote_methods]
#[delegate(Shout, target_mut = "deref_mut", target_ref = "deref")]
impl<T: DerefMut<Target = dyn Shout + Send + Sync + 'static>> T {
    fn deref_mut(&mut self) -> &mut (dyn Shout + Send + Sync + 'static);
    fn deref(&self) -> &(dyn Shout + Send + Sync + 'static);
}
Altogether: ```rust extern crate ambassador; use std::ops::DerefMut; use ambassador::{delegatable_trait, delegate_to_remote_methods}; #[delegatable_trait] pub trait Shout { fn shout(&self, input: &str) -> String; fn speak(&mut self) { } } pub struct Cat; impl Shout for Cat { fn shout(&self, input: &str) -> String { format!("{} - meow!", input) } } pub struct Dog; impl Shout for Dog { fn shout(&self, input: &str) -> String { format!("{} - wuff!", input) } } #[delegate_to_remote_methods] #[delegate(Shout, target_mut = "deref_mut", target_ref = "deref")] impl> T { fn deref_mut(&mut self) -> &mut (dyn Shout + Send + Sync + 'static); fn deref(&self) -> &(dyn Shout + Send + Sync + 'static); } fn use_it (shouter: &mut T) { println!("{}", shouter.shout("BAR")); shouter.speak() } pub fn main() { let mut a = Cat; use_it(&mut a); let mut b: &mut (dyn Shout + Send + Sync + 'static) = &mut a; use_it(&mut b); let mut c: Box = Box::new(a); use_it(&mut c); } ```

By the way, does abmassador support (or should support) methods beyond self, &self and &mut self, such as self: Pin<Arc<Self>> - those things are sometimes needed for async trait objects. Shall this be tracked in a separate Github issue?

This definitely is not currently supported — I'll leave it to the project's maintainers to say whether or not this is in scope for the project.

dewert99 commented 2 years ago

Thanks, @rrbutani for providing the explanations and examples, I think with these I can close this issue. As for other receiver types, I think it might be worth adding if people find it useful, but it probably should be its own issue.