fMeow / maybe-async-rs

A procedure macro to unify SYNC and ASYNC implementation for downstream application/crates
https://docs.rs/maybe-async
MIT License
146 stars 17 forks source link

[Q&A] Would it be possible to have an `is_async` feature as well? #8

Closed Byron closed 3 years ago

Byron commented 3 years ago

The reason I am asking might very well be ignorance, but here I go.

When transforming the git-transport crate to also support async, I am starting out with blocking code. Right now I use feature toggles as follows:

The problem I am having with this is my inability to conditionally not compile the async-trait crate, as it's required for the async code, but not for the blocking code. However, cargo features are additive and can't be turned off, leaving the blocking code with the need to compile async-trait.

This issue is certainly minor, but if valid might point at an opportunity.

With a feature flag like maybe-async/is_async, I could have the following configuration.

While only having taken a glance at how the is_sync feature toggle is used in this crate, I have a feeling that the addition of is_async would be very possible. If so, I would be happy to contribute it, too.

I am curious to hear what you are thinking. Thanks a lot.

GeoffClements commented 3 years ago

I agree that this would be useful. I am in the process of moving a crate to an optional async. The current crate owner wants a minimally invasive change so default has to be is_async.

I would guess that this will be the same for many as there's a lot of blocking code out there that could be converted to async.

Byron commented 3 years ago

That's great to hear! Making a choice for default features should be fine as they can be turned off easily.

Something that came to mind is the choice to be made when both of the mutually exclusive feature toggles are set, i.e. --all-features compiles turn on is_async and is_sync. In my case, I choose the blocking code to emerge out victorious, but I see how there is no right answer. It's just something that maybe_async would have to make configurable too or else the crate using maybe_async probably won't compile.

In any case, I am already and happily using it in tests to consolidate test cases to work with async and sync code paths and prevent drift in the distinct implementations. Once maybe_async can also handle is_async it would be possible to get rid of some duplication in the crate code itself as well.

Maybe it's interesting to you, so here is how maybe_async is used in tests for deduplication. Currently it won't remove await invocations that are nested in macros, that's quite alright from my experience. Is this related to how deep in the AST the await is located, or is it related to macros?

fMeow commented 3 years ago

Yeah! I agree with the idea to add an is_async feature gate to conditionally eliminate the async-trait dependency. And for the --all-features issue, we can just throw an compiler error and leave the choice to users.

Currently it won't remove await invocations that are nested in macros, that's quite alright from my experience. Is this related to how deep in the AST the await is located, or is it related to macros?

The answer is the former. I am affraid this is how procedural macro works. Code inside a declarative macro is not regarded as regular rust code in AST.

Byron commented 3 years ago

Awesome, I will be looking forward to it as it will enable me to use it in non-test code as well.

And for the --all-features issue, we can just throw an compiler error and leave the choice to users.

In a way I like it as it doesn't select something automatically. Thus far I was avoiding doing this because of the expectation that is set in the cargo documentation:

A consequence of this is that features should be additive. That is, enabling a feature should not disable functionality, and it should usually be safe to enable any combination of features. A feature should not introduce a SemVer-incompatible change.

The way we use features here already go against their intended use, so maybe it's time to go 'all in' and error if mutually exclusive features are selected.

The answer is the former. I am affraid this is how procedural macro works. Code inside a declarative macro is not regarded as regular rust code in AST.

Interesting, that sounds like plugins see the code prior to expansion.

Byron commented 3 years ago

When choosing to not allow both blocking and async feature toggles to exist there is no need for configuring anything in maybe-async.

Sacrificing running cargo _ --all-features is probably ok as users of the crate will always have to make a conscious choice about their IO, and silently defaulting to blocking IO is probably surprising in the worst and inefficient in the best case as more dependencies are pulled in.

Hence I close this issue.

Context

Thus far I managed to avoid maybe-async and its dependencies for the lower level crates and only used it in tests. There it was absolutely required to be able to share tests to validate a partially diverged codebase with some duplication in it to accommodate for async and blocking implementations.

Having arrived at a higher level crate I am looking at about 200 lines of code which are full of logic and full of IO, with only a few tests to cover the happy paths. It's probably code that is not quite stable yet either and imagining me maintaining both seems like an unwelcome chore. Now it appears worth it to pull in maybe-async and pay the cost in compile time knowing that typical applications will only pay for maybe-async itself, not for its dependencies as they will depend on other proc-macros somewhere using the same dependency tree.

Maybe one day I will feel comfortable trowing the blocking codepath entirely, too, but more tests about compile speed and binary size will have to be conducted (and assuming that performance is the same or better).