feat: add `BorshSerializeAsync`/`BorshDeserializeAsync` (WIP) #337

DanikVitek · 2025-01-21T23:20:10Z

The conversation started in #150.
async-generic PR: scouten/async-generic#17

This implementation should be more flexible, as it allows for different runtimes and provides traits for manual implementation in case of any uncommon runtime.

…parameters needing `Send` bound

DanikVitek · 2025-01-22T11:19:13Z

Should the BorshDeserializeAsync and BorshSerializeAsync extend their sync counterparts and contain only async functions, or should they copy synchronous functions?

DanikVitek · 2025-01-22T18:09:31Z

To support deserialize_with functionality for #[derive(BorshDeserializeAsync)], I guess, there should be a separate deserialize_with_async param. Same for the serialize counterpart

dj8yfo · 2025-01-22T18:35:06Z

@DanikVitek is this doable without async_trait import?
Not that's its too important to get rid of it, just want to know what are limitations not allowing to get rid of it, what are potential tradeoffs, etc.

# Conflicts: # Cargo.toml

DanikVitek · 2025-01-22T18:58:12Z

@DanikVitek is this doable without async_trait import? Not that's its too important to get rid of it, just want to know what are limitations not allowing to get rid of it, what are potential tradeoffs, etc.

I guess, if there is no need for the traits to be dyn-compatible, then the only difficulty is making the async_generic generate async functions as sync functions, which return a custom impl Future, because the set MSRV does not yet support async functions in traits. As I can see, the BorshSerialize is not dyn-compatible, and there is no need for BorshDeserialize to be dyn-compatible, so I guess, the same could be true for the async counterparts.

If the MSRV bump is not an issue, then it's even easier

DanikVitek · 2025-01-22T19:26:35Z

Ok, no. Both async in traits and -> impl Trait in traits (and hence, impl Future) require the MSRV 1.75. So all return types would still need to be pin-boxed and now we're just reimplementing the async_trait at this point

dj8yfo · 2025-01-23T11:31:04Z

@DanikVitek can you support your assertion that the return types will need to be pin-boxed anyway by doing a minimal example in a repo outside of this pr with the right trait (BorshSerializeAsync/BorshDeserializeAsync) signatures and msrv 1.75? I think the common convention is that you cannot bump MSRV in patch releases, but in minor version releases you're free to do so.

EDIT: if so (if they need to be pin-boxed anyway), i think we'll stick with async_trait approach for now, as we'll mark this feature unstable__async anyway to have some freedom in modifying it in the near future and middle-term

dj8yfo · 2025-01-23T14:00:20Z

Should the BorshDeserializeAsync and BorshSerializeAsync extend their sync counterparts and contain only async functions, or should they copy synchronous functions?

I think the traits should be completely separate and not extend sync counterparts and lib user should be free to only implement/derive a subset or all of them if he/she desires. I was under the impression it's the case now, maybe i'm mistaken.

dj8yfo · 2025-01-23T14:23:37Z

To support deserialize_with functionality for #[derive(BorshDeserializeAsync)], I guess, there should be a separate deserialize_with_async param. Same for the serialize counterpart

Before diving into doing derive for new traits, i'd stop and do some tests with snapshots. @DanikVitek, can you choose some subset of types, which you consider core to borsh , and make sure the async counterparts serialize/deserialize to the same snapthos that the sync interface produces in tests? You're free to modify test code, as long you double check that existing snapshots aren't touched/renamed. Ideally this should be done by pure addition (no replacements/deletions) of test code (not modifying existing tests) and just checking the new snapshots are equal to older ones. This way it would be easy to verify during review that existing serialization format, provided by stable borsh subset, hasn't changed. If the existing test code changes, it kind of needs more attention to verify the old tests still run at all.

It might be somewhat tedious to cover all existing (snapshot) tests at once, but that's one of the reason the feature should be made unstable__ for a while to kind of stress that sync and async interfaces may produce different results due to bugs/typos.

dj8yfo · 2025-01-23T14:29:09Z

To support deserialize_with functionality for #[derive(BorshDeserializeAsync)], I guess, there should be a separate deserialize_with_async param. Same for the serialize counterpart

It appears that all of bounds, serialize_with, deserialize_with proc-macro sub-attributes should be cloned to _async counterparts, maybe sharing some part of implementation with their sync versions. Maybe some other sub-attributes too, but these are the ones that come off the top of my head.

frol · 2025-01-23T15:42:19Z

@DanikVitek Amazing job! I'd like to also invite you to join @race-of-sloths

DanikVitek · 2025-01-23T15:50:50Z

@race-of-sloths sounds nice

race-of-sloths · 2025-01-23T15:51:27Z

@DanikVitek Thank you for your contribution! Your pull request is now a part of the Race of Sloths!
New Sloth joined the Race! Welcome!

Current status: waiting for scoring

We're waiting for maintainer to score this pull request with @race-of-sloths score [0,1,2,3,5,8,13] command. Alternatively, autoscoring [1,2] will be applied for this pull request

What is the Race of Sloths

Race of Sloths is a friendly competition where you can participate in challenges and compete with other open-source contributors within your normal workflow

For contributors:

Tag @race-of-sloths inside your pull requests
Wait for the maintainer to review and score your pull request
Check out your position in the Leaderboard
Keep weekly and monthly streaks to reach higher positions
Boast your contributions with a dynamic picture of your Profile

For maintainers:

Score pull requests that participate in the Race of Sloths and receive a reward
Engage contributors with fair scoring and fast responses so they keep their streaks
Promote the Race to the point where the Race starts promoting you
Grow the community of your contributors

Feel free to check our website for additional details!

Bot commands

For contributors
- Include a PR: @race-of-sloths include to enter the Race with your PR
For maintainers:
- Invite contributor @race-of-sloths invite to invite the contributor to participate in a race or include it, if it's already a runner.
- Assign points: @race-of-sloths score [1/2/3/5/8/13] to award points based on your assessment.
- Reject this PR: @race-of-sloths exclude to send this PR back to the drawing board.
- Exclude repo: @race-of-sloths pause to stop bot activity in this repo until @race-of-sloths unpause command is called

DanikVitek · 2025-01-23T22:01:39Z

@DanikVitek can you support your assertion that the return types will need to be pin-boxed anyway by doing a minimal example in a repo outside of this pr with the right trait (BorshSerializeAsync/BorshDeserializeAsync) signatures and msrv 1.75? I think the common convention is that you cannot bump MSRV in patch releases, but in minor version releases you're free to do so.

@dj8yfo
I've created a separate branch with an example implementation of BorshDeserializeAsync, and it was literally a manual expansion of async_trait. Also, here's the warning I've got from the compiler. It makes sense, because async fn in traits is a syntactic sugar for fn () -> impl Future<Output=...>, so there is no way to specify additional bounds like Send or lifetimes. Therefore I think, that for performance reasons it is better to use the impl Future rather then Pin<Box<dyn Future>> as the BorshSerialize/Deserialize traits are not ment to be dyn-compatible, and implement the boilerplate code duplication via the async_generic macro

dj8yfo · 2025-01-23T22:28:37Z

@DanikVitek i believe the lint https://doc.rust-lang.org/nightly/nightly-rustc/rustc_lint/async_fn_in_trait/static.ASYNC_FN_IN_TRAIT.html is just a reminder to do #[trait_variant::make(Send)] .
i checked

#[trait_variant::make(Send)]
pub trait Handler {
    async fn handle(&mut self, input: &Input) -> Result<String, String>;
    async fn handle2(self, input: &'static str) -> Result<String, String>;
}

struct Type {
    pub field: String,
}

impl Handler for Type {
    async fn handle(&mut self, input: &Input) -> Result<String, String> {
        let local_var = "fsdfs sdf sdf".to_owned();
        self.field.push_str(&input.field);
        self.field.push_str(local_var.as_str());

        Ok(self.field.clone())
    }

    async fn handle2(mut self, input: &'static str) -> Result<String, String> {
        let local_var = "fsdfs sdf sdf".to_owned();
        self.field.push_str(&input);
        self.field.push_str(local_var.as_str());

        Ok(self.field)
    }
}

and it appears to run. Not hundred % sure it's gonna work with async borsh traits, but looks like Box::pin in async-trait is needed to return a future from an async block + stay dyn-compatible.

`

DanikVitek · 2025-01-23T22:32:40Z

Box::pin in async-trait is needed to return a future from an async block + stay dyn-compatible.

Yes.

To be more precise, the async block declares an anonymous type that implements Future, and every .await call transforms into an enum variant, that holds all the variables, that are used across this .await boundary. So when we box the future, the actual type gets erased and, of course, a layer of indirection is added.

I don't completely understand the behaviour of .poll(), but it performs a non-blocking check of the future's current state

DanikVitek · 2025-01-23T22:42:52Z

trait_variant looks interesting. An official workaround crate

DanikVitek · 2025-02-22T17:54:25Z

@dj8yfo async-generic does support dispatching attributes on sync and async items separately, including doc comments. All features of the async-generic will be documented.
Could you, please, mention all the syntax propositions in this PR scouten/async-generic#17, for them to not get lost among the review comments?

…. Use more readable async-generic syntax

borsh/src/de/mod.rs

DanikVitek · 2025-02-23T19:34:24Z

borsh-derive/src/lib.rs

@@ -35,6 +35,19 @@ fn check_attrs_get_cratename(input: &TokenStream) -> Result<Path, Error> {
 /// moved to docs of **Derive Macro** `BorshSerialize` in `borsh` crate


This should have proper docs for convenience

plz clarify what you mean. these were moved to borsh to have doc-tests runnable in CI and avoid circular dev-dependency on borsh for borsh-derive.

Hm.. I'll look into it, because it's a bit inconvenient to look up the actual docs

external users 80% of time know nothing about borsh-derive and just use re-exports from borsh.
For them it's just an irrelevant implementation detail. the use borsh::BorshSerialize; #[derive(BorshSerialize)] ...
pattern kind of suggests that by itself. Probably the only reason it needs to be a crate and not a submodule is it being a proc-macro crate.

(this is about access to doc at docs.rs)

as far as access to docs from an ide is concerned ,

/// Details of [`BorshSerialize`](macro@crate::BorshSerialize) derive macro.

line of backlink could be added here

so that anyone, who has clickable links in doc-port in an ide can jump there.

.
In the case of the paragon of virtue serde, it's not much difference there:

And it's derive macro's doc isn't rich with information either: https://docs.rs/serde/latest/serde/derive.Serialize.html

actually, vscode performs a smart rewrite of this link

/// Details of [`BorshSerialize`](macro@crate::BorshSerialize) derive macro.

but this contradicts the logic of how rustdoc perceives this link:

So i'd rather not try to adjust to a bug of a single editor in this spot,
as the correct link here is https://docs.rs/borsh/1.5.5/borsh/derive.BorshSerialize.html (what rustdoc generates),
vs https://docs.rs/borsh-derive/1.5.5/borsh_derive/derive.BorshSerialize.html (vscode version) .

borsh/Cargo.toml

borsh/src/de/mod.rs

dj8yfo · 2025-03-07T17:14:58Z

borsh/docs/rustdoc_include/borsh_crate_top_level.md

-  Gates implementation of [BorshSerialize] and [BorshDeserialize]
+* **`derive`** -
+  Gates derive macros of [`BorshSerialize`] and
+  [`BorshDeserialize`] traits, as well as of [`BorshSchema`],


, as well as of [BorshSchema] can be removed

dj8yfo · 2025-03-07T17:18:34Z

borsh-derive/src/internals/deserialize/mod.rs

+            if IS_ASYNC {
+                cfg_if! {
+                    if #[cfg(feature = "async")] {
+                        parsed.deserialize_with_async
+                    } else {
+                        None
+                    }
+                }
+            } else {
+                parsed.deserialize_with
+            },


this looks like it can be extracted to a method get_deser_with, similar to internals::attributes::field::Attributes::get_bounds

dj8yfo · 2025-03-07T17:26:58Z

borsh-derive/src/internals/serialize/enums/mod.rs

+            if IS_ASYNC {
+                cfg_if! {
+                    if #[cfg(feature = "async")] {
+                        parsed.serialize_with_async
+                    } else {
+                        None
+                    }
+                }
+            } else {
+                parsed.serialize_with
+            },


this looks like it can be extracted to a method get_ser_with, similar to internals::attributes::field::Attributes::get_bounds

dj8yfo · 2025-03-07T19:08:13Z

borsh-derive/src/internals/serialize/structs/mod.rs

+        quote! { io::Write }
+    };
+    let r#async = IS_ASYNC.then(|| Token![async](Span::call_site()));
+    let lifetime = IS_ASYNC.then(|| Lifetime::new("'async_variant", Span::call_site()));


suggest changing/shortening this to __async in the style of #260, but this change isn't mandatory/important

dj8yfo · 2025-03-07T21:42:49Z

all of remaining comments should be resolved + an async-generic version published to crates.io and pinned to

DanikVitek added 4 commits January 21, 2025 14:46

WIP: implement support for async I/O. FIXME: resolve issue with type …

d4d92f7

…parameters needing `Send` bound

resolved issue with type parameters needing Send bound

23a292f

remove redundant cfg_attrs

5b7f19d

Implement async I/O for all standard types. TODO: derive, etc.

cf2807c

DanikVitek requested review from frol, dj8yfo, PolyProgrammist and akorchyn as code owners January 21, 2025 23:20

DanikVitek marked this pull request as draft January 21, 2025 23:21

remove async-generic version

447d7bb

dj8yfo marked this pull request as ready for review January 22, 2025 06:49

dj8yfo marked this pull request as draft January 22, 2025 06:50

Merge remote-tracking branch 'upstream/master'

984b16d

# Conflicts: # Cargo.toml

Remove dependency on sync counterparts

31a4c24

DanikVitek added 9 commits February 22, 2025 17:33

fmt

c5b99e5

Fix doc links

9f4c5e3

Use monospace for types and features

f23ed33

Fix tests

57a0a40

Revert changes

584f36f

Add docs to the async_io declaration

bc70f92

Fix lifetimes

a5e027e

fmt

018c356

Fix clippy & fmt warnings

19e1571

DanikVitek added 2 commits February 22, 2025 20:31

Fix docs and doc tests

66447cb

Fix docs and checks in CI. Fix clippy warnings about mixed attributes…

7c3eb77

…. Use more readable async-generic syntax

DanikVitek commented Feb 22, 2025

View reviewed changes

borsh/src/de/mod.rs Show resolved Hide resolved

DanikVitek added 2 commits February 23, 2025 01:08

Readability fixes. Add test for async array deserialization

0a1aab4

Readability fix

d624435

DanikVitek commented Feb 23, 2025

View reviewed changes

dj8yfo reviewed Feb 28, 2025

View reviewed changes

borsh/Cargo.toml Outdated Show resolved Hide resolved

DanikVitek added 3 commits February 28, 2025 19:41

Fix lifetimes. Add ascii feature to docs metadata

6c52ef9

clippy

da103d3

fmt

253b868

dj8yfo reviewed Feb 28, 2025

View reviewed changes

borsh/src/de/mod.rs Outdated Show resolved Hide resolved

make captures module public

e58f2b9

dj8yfo reviewed Mar 4, 2025

View reviewed changes

borsh/src/de/mod.rs Outdated Show resolved Hide resolved

revert remove inline

611e8b7

dj8yfo reviewed Mar 7, 2025

View reviewed changes

dj8yfo marked this pull request as draft March 7, 2025 21:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add `BorshSerializeAsync`/`BorshDeserializeAsync` (WIP) #337

feat: add `BorshSerializeAsync`/`BorshDeserializeAsync` (WIP) #337

DanikVitek commented Jan 21, 2025

DanikVitek commented Jan 22, 2025

DanikVitek commented Jan 22, 2025 •

edited

Loading

dj8yfo commented Jan 22, 2025

DanikVitek commented Jan 22, 2025 •

edited

Loading

DanikVitek commented Jan 22, 2025 •

edited

Loading

dj8yfo commented Jan 23, 2025 •

edited

Loading

dj8yfo commented Jan 23, 2025

dj8yfo commented Jan 23, 2025 •

edited

Loading

dj8yfo commented Jan 23, 2025

frol commented Jan 23, 2025

DanikVitek commented Jan 23, 2025

race-of-sloths commented Jan 23, 2025

DanikVitek commented Jan 23, 2025

dj8yfo commented Jan 23, 2025

DanikVitek commented Jan 23, 2025 •

edited

Loading

DanikVitek commented Jan 23, 2025

DanikVitek commented Feb 22, 2025 •

edited

Loading

DanikVitek Feb 23, 2025

dj8yfo Feb 24, 2025

DanikVitek Feb 26, 2025

dj8yfo Feb 28, 2025 •

edited

Loading

dj8yfo Feb 28, 2025

dj8yfo Feb 28, 2025 •

edited

Loading

dj8yfo Mar 7, 2025

dj8yfo Mar 7, 2025

dj8yfo Mar 7, 2025

dj8yfo Mar 7, 2025

dj8yfo commented Mar 7, 2025

		@@ -35,6 +35,19 @@ fn check_attrs_get_cratename(input: &TokenStream) -> Result<Path, Error> {
		/// moved to docs of Derive Macro `BorshSerialize` in `borsh` crate

feat: add BorshSerializeAsync/BorshDeserializeAsync (WIP) #337

Are you sure you want to change the base?

feat: add BorshSerializeAsync/BorshDeserializeAsync (WIP) #337

Conversation

DanikVitek commented Jan 21, 2025

DanikVitek commented Jan 22, 2025

DanikVitek commented Jan 22, 2025 • edited Loading

dj8yfo commented Jan 22, 2025

DanikVitek commented Jan 22, 2025 • edited Loading

DanikVitek commented Jan 22, 2025 • edited Loading

dj8yfo commented Jan 23, 2025 • edited Loading

dj8yfo commented Jan 23, 2025

dj8yfo commented Jan 23, 2025 • edited Loading

dj8yfo commented Jan 23, 2025

frol commented Jan 23, 2025

DanikVitek commented Jan 23, 2025

race-of-sloths commented Jan 23, 2025

DanikVitek commented Jan 23, 2025

dj8yfo commented Jan 23, 2025

DanikVitek commented Jan 23, 2025 • edited Loading

DanikVitek commented Jan 23, 2025

DanikVitek commented Feb 22, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dj8yfo Feb 28, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dj8yfo Feb 28, 2025 • edited Loading

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

dj8yfo commented Mar 7, 2025

feat: add `BorshSerializeAsync`/`BorshDeserializeAsync` (WIP) #337

feat: add `BorshSerializeAsync`/`BorshDeserializeAsync` (WIP) #337

DanikVitek commented Jan 22, 2025 •

edited

Loading

DanikVitek commented Jan 22, 2025 •

edited

Loading

DanikVitek commented Jan 22, 2025 •

edited

Loading

dj8yfo commented Jan 23, 2025 •

edited

Loading

dj8yfo commented Jan 23, 2025 •

edited

Loading

DanikVitek commented Jan 23, 2025 •

edited

Loading

DanikVitek commented Feb 22, 2025 •

edited

Loading

dj8yfo Feb 28, 2025 •

edited

Loading

dj8yfo Feb 28, 2025 •

edited

Loading