Add support for defaulted methods #20

mendess · 2023-12-27T15:24:27Z

Resolves #17

trait-variant/src/variant.rs

compiler-errors · 2023-12-29T16:26:06Z

trait-variant/src/variant.rs

+            fn_item
+                .default
+                .as_ref()
+                .map(|b| syn::parse2(quote! { { async move #b } }).unwrap()),


Just noting that the correct desugaring would involve moving every argument into the block as well.

@mendess I believe what @compiler-errors means here is that every argument of the trait fn should also be moved into the block. eg

async fn defaulted_mut(&mut self, x: u32) -> i32 { self.make(x, "10").await }

becomes

fn defaulted_mut<'the_self_lt>(&'the_self_lt mut self, x: u32) -> impl ::core::future::Future<Output = i32> + Send where &'the_self_lt mut Self: Send, { async move { let self = self; let x = x; self.make(x, "10").await } }

Added a commit that does this. Sorry for misunderstanding the original comment

mendess · 2023-12-29T19:37:51Z

@compiler-errors so I gave your suggestions a try with these modifications to the example trait

#[trait_variant::make(IntFactory: Send)]
pub trait LocalIntFactory {
    const NAME: &'static str;

    type MyFut<'a>: Future
    where
        Self: 'a;

    async fn make(&self, x: u32, y: &str) -> i32;
    async fn make_mut(&mut self);
    fn stream(&self) -> impl Iterator<Item = i32>;
    fn call(&self) -> u32;
    fn another_async(&self, input: Result<(), &str>) -> Self::MyFut<'_>;
    async fn defaulted(&self) -> i32 {
        self.make(10, "10").await
    }
    async fn defaulted_mut(&mut self) -> i32 {
        self.make(10, "10").await
    }
    async fn defaulted_mut_2(&mut self) {
        self.make_mut().await
    }
    async fn defaulted_move(self) -> i32
    where
        Self: Sized,
    {
        self.make(10, "10").await
    }
}

And I generated something like this

pub trait IntFactory: Send {
    const NAME: &'static str;
    type MyFut<'a>: Future
    where
        Self: 'a;
    fn make(&self, x: u32, y: &str) -> impl ::core::future::Future<Output = i32> + Send;
    fn make_mut(&mut self) -> impl ::core::future::Future<Output = ()> + Send;
    fn stream(&self) -> impl Iterator<Item = i32> + Send;
    fn call(&self) -> u32;
    fn another_async(&self, input: Result<(), &str>) -> Self::MyFut<'_>;
    fn defaulted<'the_self_lt>(&'the_self_lt self) -> impl ::core::future::Future<Output = i32> + Send
    where
        &'the_self_lt Self: Send,
    {
        async { self.make(10, "10").await }
    }
    fn defaulted_mut<'the_self_lt>(&'the_self_lt mut self) -> impl ::core::future::Future<Output = i32> + Send
    where
        &'the_self_lt mut Self: Send, // error[E0277]: `Self` cannot be shared between threads safely
    {
        async { self.make(10, "10").await }
    }
    fn defaulted_mut_2<'the_self_lt>(&'the_self_lt mut self) -> impl ::core::future::Future<Output = ()> + Send
    where
        &'the_self_lt mut Self: Send,
    {
        async { self.make_mut().await }
    }
    fn defaulted_move(self) -> impl ::core::future::Future<Output = i32> + Send
    where
        Self: Sized,
        Self: Send, // error[E0277]: `Self` cannot be shared between threads safely
    {
        async { self.make(10, "10").await }
    }
}

The errors are because the async block of those particular default implementations don't "capture" self the same way the method does. For example, default_mut doesn't actually need &mut self for it's implementation. Hence the compiler requires Self: Sync.

Do you still think it's worth trying to generate very precise bounds like I tried to do here?

andrewtoth · 2023-12-29T22:09:54Z

@mendess I believe in your generated example you are missing the move keyword after async. Using async move {...} fixes the compile errors.

Your explanation of the error makes sense, hence why it is required to move the actual receiver type into the block instead of just borrowing &Self.

mendess · 2023-12-30T12:49:58Z

@andrewtoth I remove the move keyword of what @compiler-errors mentioned, but maybe I'm misunderstood them

compiler-errors · 2023-12-30T13:54:24Z

I didn't mention removing the move keyword--that is definitely needed.

mendess · 2024-01-01T13:35:47Z

Alright, edited the codegen to have more precise bounds for Self

This is what it looks like now

use std::future::Future;

#[allow(async_fn_in_trait)]
pub trait LocalIntFactory {
    const NAME: &'static str;

    type MyFut<'a>: Future
    where
        Self: 'a;

    async fn make(&self, x: u32, y: &str) -> i32;
    async fn make_mut(&mut self);
    fn stream(&self) -> impl Iterator<Item = i32>;
    fn call(&self) -> u32;
    fn another_async(&self, input: Result<(), &str>) -> Self::MyFut<'_>;
    async fn defaulted(&self) -> i32 {
        self.make(10, "10").await
    }
    async fn defaulted_mut(&mut self) -> i32 {
        self.make(10, "10").await
    }
    async fn defaulted_mut_2(&mut self) {
        self.make_mut().await
    }
    async fn defaulted_move(self) -> i32
    where
        Self: Sized,
    {
        self.make(10, "10").await
    }
}
pub trait IntFactory: Send {
    const NAME: &'static str;
    type MyFut<'a>: Future
    where
        Self: 'a;
    fn make(&self, x: u32, y: &str) -> impl ::core::future::Future<Output = i32> + Send;
    fn make_mut(&mut self) -> impl ::core::future::Future<Output = ()> + Send;
    fn stream(&self) -> impl Iterator<Item = i32> + Send;
    fn call(&self) -> u32;
    fn another_async(&self, input: Result<(), &str>) -> Self::MyFut<'_>;
    fn defaulted<'the_self_lt>(&'the_self_lt self) -> impl ::core::future::Future<Output = i32> + Send
    where
        &'the_self_lt Self: Send,
    {
        async move { self.make(10, "10").await }
    }
    fn defaulted_mut<'the_self_lt>(&'the_self_lt mut self) -> impl ::core::future::Future<Output = i32> + Send
    where
        &'the_self_lt mut Self: Send,
    {
        async move { self.make(10, "10").await }
    }
    fn defaulted_mut_2<'the_self_lt>(&'the_self_lt mut self) -> impl ::core::future::Future<Output = ()> + Send
    where
        &'the_self_lt mut Self: Send,
    {
        async move { self.make_mut().await }
    }
    fn defaulted_move(self) -> impl ::core::future::Future<Output = i32> + Send
    where
        Self: Sized,
        Self: Send,
    {
        async move { self.make(10, "10").await }
    }
}
impl<T> LocalIntFactory for T
where
    T: IntFactory,
    Self: Sync // this is only added if there are defaulted methods
{
    const NAME: &'static str = <Self as IntFactory>::NAME;
    type MyFut<'a> = <Self as IntFactory>::MyFut<'a> where Self: 'a;
    async fn make(&self, x: u32, y: &str) -> i32 {
        <Self as IntFactory>::make(self, x, y).await
    }
    async fn make_mut(&mut self) {
        <Self as IntFactory>::make_mut(self).await
    }
    fn stream(&self) -> impl Iterator<Item = i32> {
        <Self as IntFactory>::stream(self)
    }
    fn call(&self) -> u32 {
        <Self as IntFactory>::call(self)
    }
    fn another_async(&self, input: Result<(), &str>) -> Self::MyFut<'_> {
        <Self as IntFactory>::another_async(self, input)
    }
    async fn defaulted(&self) -> i32 {
        <Self as IntFactory>::defaulted(self).await
    }
    async fn defaulted_mut(&mut self) -> i32 {
        <Self as IntFactory>::defaulted_mut(self).await
    }
    async fn defaulted_mut_2(&mut self) {
        <Self as IntFactory>::defaulted_mut_2(self).await
    }
    async fn defaulted_move(self) -> i32
    where
        Self: Sized,
    {
        <Self as IntFactory>::defaulted_move(self).await
    }
}

andrewtoth · 2024-01-03T14:11:41Z

trait-variant/src/variant.rs

@@ -164,8 +182,26 @@ fn mk_blanket_impl(attrs: &Attrs, tr: &ItemTrait) -> TokenStream {
    let orig = &tr.ident;
    let variant = &attrs.variant.name;
    let items = tr.items.iter().map(|item| blanket_impl_item(item, variant));
+    let self_is_sync = tr


This is no longer necessary if we are adding the bounds on each function.

It is still needed. This is for the blanket impl, if there are defaulted methods in the trait they basically require Self: Sync so in order for the impl to be valid it can only "blanket" over types that are Sync

I see, but would it make more sense to add to add the bound for<'a> &'a Self: Send, which is what the compiler is complaining about?
Also, not sure why it complains about only about the defaulted(&self) function and not the default_mut(&mut self) and defaulted_move(self) functions? What is different about the mut and move ones?

I'm not sure what the difference is, maybe there is some implied bound? 🤔

andrewtoth · 2024-01-03T14:49:18Z

trait-variant/src/variant.rs

+            fn_item
+                .default
+                .as_ref()
+                .map(|b| syn::parse2(quote! { { async move #b } }).unwrap()),


@mendess I believe what @compiler-errors means here is that every argument of the trait fn should also be moved into the block. eg

async fn defaulted_mut(&mut self, x: u32) -> i32 { self.make(x, "10").await }

becomes

fn defaulted_mut<'the_self_lt>(&'the_self_lt mut self, x: u32) -> impl ::core::future::Future<Output = i32> + Send where &'the_self_lt mut Self: Send, { async move { let self = self; let x = x; self.make(x, "10").await } }

ryo33 · 2024-01-15T10:52:19Z

trait-variant/src/variant.rs

+                syn::parse2(quote! { { async move { #(#items)* #block} } })
+                    .expect("valid async block")


Though I'm not confident, using parse_quote instead seems idiomatic in this case.

parse_quote! { { async move { #(#items)* #block} } }

mendess · 2024-01-16T13:30:23Z

Edited the codegen again and this is what it looks like now

impl<TraitVariantBlanketType> LocalIntFactory for TraitVariantBlanketType
where
    TraitVariantBlanketType: IntFactory,
    for<'s> &'s Self: Send, // no longer bounds `Self: Sync`
{
    const NAME: &'static str = <Self as IntFactory>::NAME;
    type MyFut<'a> = <Self as IntFactory<>>::MyFut<'a> where Self: 'a;
    async fn make(&self, x: u32, y: &str) -> i32 {
        <Self as IntFactory>::make(self, x, y).await
    }
    async fn make_mut(&mut self) {
        <Self as IntFactory>::make_mut(self).await
    }
    fn stream(&self) -> impl Iterator<Item = i32> {
        <Self as IntFactory>::stream(self)
    }
    fn call(&self) -> u32 {
        <Self as IntFactory>::call(self)
    }
    fn another_async(&self, input: Result<(), &str>) -> Self::MyFut<'_> {
        <Self as IntFactory>::another_async(self, input)
    }
    async fn defaulted(&self, x: u32) -> i32 {
        <Self as IntFactory>::defaulted(self, x).await
    }
    async fn defaulted_mut(&mut self) -> i32 {
        <Self as IntFactory>::defaulted_mut(self).await
    }
    async fn defaulted_mut_2(&mut self) {
        <Self as IntFactory>::defaulted_mut_2(self).await
    }
    async fn defaulted_move(self) -> i32
    where
        Self: Sized,
    {
        <Self as IntFactory>::defaulted_move(self).await
    }
}

sargarass · 2024-02-14T12:46:21Z

@mendess can you please rebase it onto new main.

mendess · 2024-02-14T23:08:02Z

@sargarass Done!

mendess · 2024-02-27T14:36:21Z

@compiler-errors could I get a re-review? Maybe get this merged? 👀

sargarass · 2024-03-05T14:07:20Z

@tmandry could you also take a look? We are using the Mendess branch in our closed-source project and would like to have it merged.

tmandry

Thanks, I looked it over. I think I'd rather require that the needed bounds are already listed on the trait itself than silently add them to individual methods. My goal is for uses/expansions of this macro to eventually be replaced with trait aliases. It isn't possible to have a trait alias that adds where clauses to individual methods.

For the common case of Send bounds, we can require that Sync is also listed. In the general case, we unfortunately can't check for the user and will have to let the compiler give an error on the generated code. (Once #[diagnostic::on_unimplemented] is stabilized in the next release I think we can do something clever with that to improve the user experience, in the general case.)

tmandry · 2024-03-18T21:26:16Z

trait-variant/src/variant.rs

+                    }
+                    FnArg::Typed(PatType { pat, .. }) => match pat.as_ref() {
+                        Pat::Ident(PatIdent { ident, .. }) => quote! { let #ident = #ident; },
+                        _ => todo!(),


Report an error

tmandry · 2024-03-18T21:28:02Z

trait-variant/src/variant.rs

+                ..sig.clone()
+            },
+            fn_item.default.as_ref().map(|b| {
+                let items = sig.inputs.iter().map(|i| match i {


Suggested change

let items = sig.inputs.iter().map(|i| match i {

let move_args = sig.inputs.iter().map(|i| match i {

tmandry · 2024-03-18T21:31:15Z

trait-variant/examples/variant.rs

    fn stream(&self) -> impl Iterator<Item = i32>;
    fn call(&self) -> u32;
    fn another_async(&self, input: Result<(), &str>) -> Self::MyFut<'_>;
+    async fn defaulted(&self, x: u32) -> i32 {


I think we're outgrowing using this example as a test. Feel free to keep one as an actual example, but can you move the remaining cases to a new test case under trait-variant/tests? See #30 for an example.

tmandry · 2024-03-18T23:16:46Z

trait-variant/src/variant.rs

+        .unwrap_or_default();
+
+    if self_is_sync {
+        blanket_where_clause.push(parse_quote! { for<'s> &'s Self: Send });


Here and below: This assumes the user is using Send as a bound, but the macro is designed to be more general than that. I don't have a problem with special casing Send to provide a better user experience, but we cannot just assume that's what the bound is.

loichyan · 2024-03-25T03:16:29Z

I tested this PR and found that calling an async function that returns a static reference in the defaulted method leads to a compilation error:

#[trait_variant::make(MyTrait: Send)]
// 1. lifetime may not live long enough
//    returning this value requires that `'the_self_lt` must outlive `'static`
// 2. implementation of `std::marker::Send` is not general enough
//    `&'0 Self` must implement `std::marker::Send`, for any lifetime `'0`...
//    ...but `std::marker::Send` is actually implemented for the type `&'the_self_lt Self`
// 3. to declare that `impl std::future::Future<Output = i32> + std::marker::Send` captures data from argument `self`, you can add an explicit `'the_self_lt` lifetime bound: ` + 'the_self_lt`
// 4. lifetime `'the_self_lt` defined here
pub trait LocalMyTrait {
    async fn make(&self, x: u32, y: &str) -> i32;

    async fn defaulted_with_static(&self, x: u32, y: &str) -> i32 {
        let _t = get_global_var().await;
        self.make(x, y).await
    }
}

async fn get_global_var() -> &'static i32 {
    todo!()
}

The expanded code:

#[allow(async_fn_in_trait)]
pub trait LocalMyTrait {
    async fn make(&self, x: u32, y: &str) -> i32;
    async fn defaulted_with_static(&self, x: u32, y: &str) -> i32 {
        let _t = get_global_var().await;
        self.make(x, y).await
    }
}
pub trait MyTrait: Send {
    fn make(&self, x: u32, y: &str) -> impl ::core::future::Future<Output = i32> + Send;
    fn defaulted_with_static<'the_self_lt>(
        &'the_self_lt self,
        x: u32,
        y: &str,
    ) -> impl ::core::future::Future<Output = i32> + Send // Add 'the_self_lt bound here, it compiles
    where
        &'the_self_lt Self: Send,
    {
        async move {
            let __self = self;
            let x = x;
            let y = y;
            {
                let _t = get_global_var().await;
                __self.make(x, y).await
            }
        }
    }
}
impl<TraitVariantBlanketType: MyTrait> LocalMyTrait for TraitVariantBlanketType
where
    for<'s> &'s Self: Send,
{
    async fn make(&self, x: u32, y: &str) -> i32 {
        <Self as MyTrait>::make(self, x, y).await
    }
    async fn defaulted_with_static(&self, x: u32, y: &str) -> i32 {
        <Self as MyTrait>::defaulted_with_static(self, x, y).await
    }
}

loichyan · 2024-03-25T06:42:55Z

It also results in a lifetime mismatch error when overriding the default method:

#[trait_variant::make(MyTrait: Send)]
pub trait LocalMyTrait {
    async fn defaulted(&self, _x: u32, _y: &str) -> i32 {
        todo!()
    }
}

struct MyImpl;

impl MyTrait for MyImpl {
    // 1. lifetime parameters or bounds on method `defaulted` do not match the trait declaration
    // lifetimes do not match method in trait [E0195]
    async fn defaulted(&self, _x: u32, _y: &str) -> i32 {
        todo!()
    }
}

loichyan · 2024-03-25T07:54:52Z

... an async function that returns a static reference in the defaulted method leads to a compilation error
It also results in a lifetime mismatch error when overriding the default method

A possible solution for the two issues is to replace the &'the_self_lt Self: Send bound with Self: Sync.

Sytten · 2024-04-29T14:17:21Z

Any movement?

tmandry · 2024-07-30T00:20:28Z

A possible solution for the two issues is to replace the &'the_self_lt Self: Send bound with Self: Sync.

That is one way, and might lead to better error messages in the Send case specifically.

In the general case we can replace &'the_self_lt Self: Send with for<'a> &'a Self: Send and it seems to work.

mendess force-pushed the support-defaulted-methods branch 3 times, most recently from d1db272 to 4ab0022 Compare December 27, 2023 18:20

girstenbrei mentioned this pull request Dec 28, 2023

Support syn::Pat::Wild pattern in method signatures #21

Open

andrewtoth mentioned this pull request Dec 29, 2023

Add support for traits with generic parameters #19

Merged

compiler-errors requested changes Dec 29, 2023

View reviewed changes

mendess force-pushed the support-defaulted-methods branch 3 times, most recently from b3706eb to d279a3c Compare January 1, 2024 13:31

andrewtoth reviewed Jan 3, 2024

View reviewed changes

mendess requested review from compiler-errors and andrewtoth January 3, 2024 16:39

ryo33 reviewed Jan 15, 2024

View reviewed changes

mendess force-pushed the support-defaulted-methods branch 4 times, most recently from 58351ab to 68944f5 Compare January 16, 2024 13:28

mendess requested a review from ryo33 January 16, 2024 13:30

mendess added 2 commits February 14, 2024 23:05

Add support for defaulted methods

f719ebe

Explicitly move all arguments into the async block

5fb6904

mendess force-pushed the support-defaulted-methods branch from 68944f5 to 5fb6904 Compare February 14, 2024 23:07

ryo33 approved these changes Mar 5, 2024

View reviewed changes

tmandry requested changes Mar 18, 2024

View reviewed changes

AlvaroSierra mentioned this pull request Jun 4, 2024

async function with default implementation won't compile #17

Open

tmandry mentioned this pull request Jul 30, 2024

feat: support default implement method #28

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for defaulted methods #20

Add support for defaulted methods #20

mendess commented Dec 27, 2023

compiler-errors Dec 29, 2023

andrewtoth Jan 3, 2024 •

edited

Loading

mendess Jan 3, 2024

mendess commented Dec 29, 2023

andrewtoth commented Dec 29, 2023 •

edited

Loading

mendess commented Dec 30, 2023

compiler-errors commented Dec 30, 2023

mendess commented Jan 1, 2024 •

edited

Loading

andrewtoth Jan 3, 2024

mendess Jan 3, 2024

andrewtoth Jan 4, 2024 •

edited

Loading

mendess Jan 16, 2024

andrewtoth Jan 3, 2024 •

edited

Loading

ryo33 Jan 15, 2024

mendess commented Jan 16, 2024

sargarass commented Feb 14, 2024

mendess commented Feb 14, 2024

mendess commented Feb 27, 2024

sargarass commented Mar 5, 2024

tmandry left a comment

tmandry Mar 18, 2024

tmandry Mar 18, 2024

tmandry Mar 18, 2024

tmandry Mar 18, 2024

loichyan commented Mar 25, 2024

loichyan commented Mar 25, 2024

loichyan commented Mar 25, 2024

Sytten commented Apr 29, 2024

tmandry commented Jul 30, 2024

		syn::parse2(quote! { { async move { #(#items)* #block} } })
		.expect("valid async block")

	let items = sig.inputs.iter().map(\|i\| match i {
	let move_args = sig.inputs.iter().map(\|i\| match i {

Add support for defaulted methods #20

Are you sure you want to change the base?

Add support for defaulted methods #20

Conversation

mendess commented Dec 27, 2023

Choose a reason for hiding this comment

andrewtoth Jan 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mendess commented Dec 29, 2023

andrewtoth commented Dec 29, 2023 • edited Loading

mendess commented Dec 30, 2023

compiler-errors commented Dec 30, 2023

mendess commented Jan 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewtoth Jan 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewtoth Jan 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mendess commented Jan 16, 2024

sargarass commented Feb 14, 2024

mendess commented Feb 14, 2024

mendess commented Feb 27, 2024

sargarass commented Mar 5, 2024

tmandry left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

loichyan commented Mar 25, 2024

loichyan commented Mar 25, 2024

loichyan commented Mar 25, 2024

Sytten commented Apr 29, 2024

tmandry commented Jul 30, 2024

andrewtoth Jan 3, 2024 •

edited

Loading

andrewtoth commented Dec 29, 2023 •

edited

Loading

mendess commented Jan 1, 2024 •

edited

Loading

andrewtoth Jan 4, 2024 •

edited

Loading

andrewtoth Jan 3, 2024 •

edited

Loading