-
Notifications
You must be signed in to change notification settings - Fork 308
create_async_runtime
: Make cfg
checks exhaustive
#2937
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
base: main
Are you sure you want to change the base?
Conversation
Thank you for your contribution @magodo! We will review the pull request and get back to you soon. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes a configuration gap in the create_async_runtime()
function by making the cfg
checks exhaustive. The change prevents a scenario where enabling the wasm_bindgen
feature but building for a non-WASM target would result in no matching configuration, causing the function to return ()
instead of a proper runtime.
- Made
cfg
conditions mutually exclusive by adding early returns - Updated the fallback condition to properly exclude the WASM+wasm_bindgen combination
- Ensures all possible feature/target combinations are handled correctly
#[cfg(not(any(feature = "tokio", feature = "wasm_bindgen")))] | ||
#[cfg(not(any( | ||
feature = "tokio", | ||
all(target_arch = "wasm32", feature = "wasm_bindgen") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this way - with mutually-exclusive features as added above - instead of how I had it? Basically, if you were on wasm but didn't specify wasm_bindgen
, you'd get the standard runtime which, yes, would fail on wasm anyway. But it would fail with the compile_error
now so nothing has changed except that with how I had it, if - somehow - standard did support wasm (highly doubtful, but imagine a world for std
did support this) it would just work.
Is your goal here only to provide a better error message?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@heaths It is a leak of the check here in case you have specified wasm_bindgen
but not targeting wasm32
. In this case, this function will return a ()
(instead of one of the runtime instance). By doing this change, this case will be caught by this cfg
, and return the standard runtime as expected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which won't compile, which is fine. It's an invalid combination - and one that is causing me to rethink whether we should have this wasm_bindgen
feature at all and instead rely solely on target_arch = "wasm"
or, where the difference matters, target = "wasm32-unknown-unknown"
.
Can you remind me again why we actually needed a separate feature? Was it only to provide an AsyncRuntime
for WASM based on bindgen
- much like we default to reqwest
for our default HTTP stack but don't necessitate it?
If that's the case, maybe we need to manage this in a more central place e.g., in src/lib.rs
we have something like:
#[cfg(all(feature = "wasm_bindgen", not(target_arch = "wasm")))]
compile_error!("nuh uh uh");
Should we have to add other features - which I want to avoid parity features - in the future, we can just add in an any()
for feature names.
@LarryOsterman what do you think? I'd rather not hide this impossible combination check deep down. Or maybe we can solve this in another way but nothing comes to mind. I'd rather not add a build script just for this because it increases build times and we can achieve this in lib.rs
already.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would much rather see this at the absolute top level in lib.rs if at all possible rather than in create_async_runtime
.
To be honest, I'd love to see the existance of the wasm_bindgen feature be conditional in the cargo.toml, but I don't believe that you can tie features to a specific architecture.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, but I imagine platform-specific features would only push the difficulties upstream even if they were supported...and probably one reason they're not (probably many reasons).
We probably just have to think of this like reqwest
but with the added caveat to be useful it has to be on WASM.
That said, it did make me think of an issue: if customers are building out for wasm32-unknown-unknown
or wasm32-wasip2
plus typical PC targets, we might not want this compile_error!()
. It means they can't just take a platform-agnostic dependency on azure_core = { version = "*", features = ["wasm_bindgen"] }
and have it just work for whichever platform. They'd need platform-specific targets e.g.,
[dependencies]
azure_core.version = "1.0.0"
[target.'cfg(target_arch = "wasm32")'.dependencies]
azure_core = { version = "1.0.0", features = ["wasm_bindgen"] }
Not the most approachable. So maybe we terminate create_async_runtime
with the compile_error!()
instead. I realize I just flipped-flopped, but had some time to think about more use cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@heaths This is when we wanted to introduce a feature: #2770 (comment).
This PR makes the
cfg
checks exhaustive in thecreate_async_runtime()
, to avoid a case that a package depending ontypespec_client_core
, enabledwasm_bindgen
feature, but build for native target (e.g. x86-84). This will hit no check conditions in thecreate_async_runtime()
, makes it reutrn()
. Instead, this shall use the standard runtime.In fact, mutually exclusive features are not encouraged: https://doc.rust-lang.org/cargo/reference/features.html#mutually-exclusive-features. While I don't have any better idea in this case..
Related to: #2838.