Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
Byron opened this issue May 13, 2021 · 5 comments
Closed

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

Byron opened this issue May 13, 2021 · 5 comments

Comments

@Byron
Copy link

Byron commented May 13, 2021

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:

  • there are no default features
  • no features means is_async
  • A feature called blocking-client makes the existing blocking client code available

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.

  • there are no default features, meaning there is neither blocking nor non-blocking client code as the user should choose.
  • with the async-client, we also require maybe-async/is_async and async-trait
  • with blocking-client, we require maybe-async/is_sync
  • if compiled with --all-features, we pick one of the above to be the dominant one

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
Copy link

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
Copy link
Author

Byron commented May 16, 2021

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
Copy link
Owner

fMeow commented May 16, 2021

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
Copy link
Author

Byron commented May 17, 2021

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
Copy link
Author

Byron commented Jun 7, 2021

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).

@Byron Byron closed this as completed Jun 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants