Skip to content

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.

Sign up for GitHub

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

create_async_runtime: Make cfg checks exhaustive #2937

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2bdb037
`create_async_runtime`: Make `cfg` checks exhaustive
magodo Aug 21, 2025
61048cd
remove return
magodo Aug 21, 2025
d65a3f1
update
magodo Aug 21, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion sdk/typespec/typespec_client_core/src/async_runtime/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,6 +185,11 @@ pub fn set_async_runtime(runtime: Arc<dyn AsyncRuntime>) -> crate::Result<()> {
}

fn create_async_runtime() -> Arc<dyn AsyncRuntime> {
#[cfg(all(feature = "wasm_bindgen", feature = "tokio"))]
compile_error!(
"feature \"wasm_bindgen\" and feature \"tokio\" cannot be enabled at the same time"
);

#[cfg(all(target_arch = "wasm32", feature = "wasm_bindgen"))]
{
Arc::new(web_runtime::WasmBindgenRuntime) as Arc<dyn AsyncRuntime>
Expand All @@ -193,7 +198,10 @@ fn create_async_runtime() -> Arc<dyn AsyncRuntime> {
{
Arc::new(tokio_runtime::TokioRuntime) as Arc<dyn AsyncRuntime>
}
#[cfg(not(any(feature = "tokio", feature = "wasm_bindgen")))]
#[cfg(not(any(
feature = "tokio",
all(target_arch = "wasm32", feature = "wasm_bindgen")
Copy link
Member

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?

Copy link
Contributor Author

@magodo magodo Aug 25, 2025 •
edited
Loading

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.

Copy link
Member

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.

Copy link
Member

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.

Copy link
Member

@heaths heaths Aug 25, 2025 •
edited
Loading

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.

Copy link
Contributor Author

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

)))]
{
Arc::new(standard_runtime::StdRuntime) as Arc<dyn AsyncRuntime>
}
Expand Down
Loading