Remove #[async_trait]

[DrewMcArthur]

closes #4

This removes the dependency on the async-trait crate, and refactors trait definitions to return impl Future<Output=T> + Send, complying with the compiler warning async_fn_in_trait mentioned here and here.

Seems like tests pass, but opening this as a draft PR so it can undergo some scrutiny before it's merged.

A nice-to-add piece would be something for the documentation - if you look at the docs.rs page for the FromStream trait, you can see the definition is the expanded code, which makes it a little tricky to know how to define the compatible function. This will clean that up a bit, but we'd still have the discrepancy where the trait function is defined as

trait Foo<T> {
    fn bar() -> impl Future<Output=T> + Send;
}

while the implementation looks like

struct Baz;
impl<T> Foo<T> for Baz {
    async fn bar() -> T {
        todo!()
    }
}

so it would be cool if the docs mentioned this & made it clear what the "right" way to implement the traits is! cheers!

[js2xxx]

Manually specifying the Send bound to the returned futures may not be a good idea, since it eliminates the existence of non-Send Decoders (though I wonder whether it's acceptable @haydnv). The async fn approach appears to be more generic, and its return type bound can be specified at the call site via a yet-unstable feature Return Type Notation.

[haydnv]

Why do the implementations use fn ... -> impl Future<RT> rather than async fn ... -> RT? My understanding is that async is still allowed in trait implementations and this would reduce the amount of code that has to be updated while maintaining the clarity of the return signature: https://blog.rust-lang.org/2023/12/21/async-fn-rpit-in-traits.html#can-i-mix-async-fn-and-impl-trait (same below)

[DrewMcArthur]

the latter (async fn ... -> RT) is only equivalent to fn ... -> impl Future<Output=RT>, and that Future is not Send, so you can't use that function across threads, i linked some discussion that felt relevant in the description, e.g. this forum post and this SO question

an alternative would be to follow the example you linked, and do something like

#[trait_variant::make]
trait MyTrait {
    async fn MyAsync() -> i32;
}

adding that attribute would be equivalent to removing the async and making the return type impl Future<Output=i32> + Send (and I think we could configure it to make both a Send and not-Send version).

does that track?

[haydnv]

Yes thank you for clarifying. Per the discussion in #5 it sounds like the #[trait_variant] is the way to go.

[haydnv]

Thanks @DrewMcArthur ! Would you mind merging this into the 0.9 branch rather than main? Since this is a breaking change I would at least like support it in destream_json and tbon before merging it into main.

@js2xxx I agree the async fn notation would be better but can you clarify why using the unstable RTN feature would be necessary?

[js2xxx]

It's not necessary though, since the current version already restricts the returned futures to be Send. My considerations lie in the possible thread-local/non-Send usage, as mentioned in the RFC.

RTN allows the downstream users of decoders to choose whether they should be Send at the call site instead of being restricted to be Send at the definition site. This enables decoder implementations to be non-Send, e.g. thread-local-only, which allows them to adopt non-Send/Sync data structures and smart pointers such as RefCell<T>, Rc<T>, etc.

In short, RTN makes the core traits more general and extends their applications to some extent.

[DrewMcArthur]

@js2xxx thanks for bringing this up, I think after staring at it for a bit I'm beginning to understand - I made these changes because a) of the warning i got, and b) downstream when was implementing these traits, i got an error that the Future wasn't Send, so this was my workaround.

It seems like the way forward is to replace the async fn, and add #[trait_variant::make(SendFromStream: Send)] above these traits (although that feels a bit like we're replacing one proc-macro with another, would that still be an improvement?)

the last paragraph of MVP part 2 here mentions that RTN ends up being really verbose for traits with many methods, like the Visitor trait. which is why i lean towards leveraging #[trait_variant]

[haydnv]

Thank you for clarifying, my preference is for now to retain the Send bound until such time as there is a specific need to remove it in order to minimize the work needed to implement the trait and maintain the implementation. IIUC we should still expect a performance improvement from replacing #[async_trait] with #[trait_variant] because it will effectively replace a Box<dyn Future<...>> with an impl Future from the compiler's perspective.

Does that sound right?

[js2xxx]

Yes, it sounds good to me. :)

[DrewMcArthur]

i've made that change to use #[trait_variant::make(Send)] rather than having fn() -> impl Future<...> + Send, but i'm running into a compilation issue.

I just added a workflow that should show the errors i'm getting directly on the PR once it's approved

alternatively you can check the same PR in my repo and see the error i'm getting.

any hints?

[DrewMcArthur]

looks like i just needed to add some async { } wrappers in the trait's default impls of async functions. is that acceptable?

edit: yeah this is a workaround for rust-lang/impl-trait-utils#17. implementations should work fine though

[haydnv]

@DrewMcArthur I understand that this is necessary in the default method implementations due to a quirk of #[trait_variant]. Is it safe to assume other implementors (e.g. destream_json) won't need to worry about quirks like this? If so I think this PR is ready to merge in.

[DrewMcArthur]

other implementors (e.g. destream_json) won't need to worry about quirks like this? If so I think this PR is ready to merge in.

@haydnv I believe so, but I'd like to get a PR for destream_json working before we merge this, to be sure 😄

[DrewMcArthur]

just opened haydnv/destream_json#11 !