Should now error on wasm (not on emscripten or wasi) without wasmbind?

[alexkazik]

I've written a program for wasm and everything worked.
After some time I decided to disable all default features on the dependencies.
I had to enable some features to get it to compile ("clock" for chrono).
But then it worked (it compiled and seem to work).

Though a call to Local::now() did now panics (was fine before, it calls something in std which always panics on my target).
Now it's clear to me that I should have enabled the feature wasmbind.

But it would've been better if the now() function just was not available in my configuration.

Removing the line feature = "wasmbind", in Utc::now() seems to have that effect.

The result is that on wasm (no emscripten or wasi) with wasmbind the replaced function is used. And if not on wasm or with emscripten or wasi the std function is used. And on wasm (no emscripten or wasi) without wasmbind the function is not available.

Of course Utc::today(), Local::now() and Local::today() have also to be disabled in that case, the following does just that.

#[cfg(any(
    not(target_arch = "wasm32"),
    feature = "wasmbind",
    all(target_arch = "wasm32", any(target_os = "emscripten", target_os = "wasi")),
))]

This would be of course a breaking change.

And there are probably other targets which also always panic (though I don't know).

I can try to write a PR if that is wanted.

Edit:
As an alternative or first step, all the mentioned functions could be marked as deprecated with a comment which says that it will always panic (of course only for the problematic configuration).

Example:

#[cfg_attr(
    all(
        target_arch = "wasm32",
        not(feature = "wasmbind"),
        not(any(target_os = "emscripten", target_os = "wasi"))
    ),
    deprecated(since = "?", note = "will always panic")
)]

[pitdicker]

The clock feature is already pretty much the thing that controls if now() and Local are available.

If I remember right wasm support was quickly added to the standard library to give rust a presence in that space since the beginning. But a lot of things were just stubs that panic on use. It was the first (and maybe only?) target where the standard library lies about its capabilities.

We could remove now on wasm without the wasi target or wasmbind feature. That would indeed be a breaking change.

But if an ecosystem is build on 'pretend you support some features so you can compile, but avoid actually using xyz', it seems chrono is following the conventions for that ecosystem. What do you think?

[alexkazik]

I'm ok with how it is, I wanted to make it easier for newcomers, especially if wasmbind get's disabled by default #1164.

[ramosbugs]

First off, thanks for all the work maintaining this excellent crate!

This issue came up in the oauth2 (ramosbugs/oauth2-rs#230) and openidconnect (ramosbugs/openidconnect-rs#127) crates, which support WASM but initially panicked until we enabled chrono's wasmbind feature. Now we have a PR to disable the wasmbind feature for non-WASM builds (ramosbugs/oauth2-rs#262), and it's creating a burden to add a WASM test to make sure the wasmbind feature is still enabled in WASM builds to prevent future regressions.

But if an ecosystem is build on 'pretend you support some features so you can compile, but avoid actually using xyz', it seems chrono is following the conventions for that ecosystem. What do you think?

This may have been the case in the early WASM days, but this doesn't seem like a viable long-term approach. The current behavior is the equivalent of sprinkling .unwrap() throughout the code, which is an anti-pattern that leads to surprises at runtime.

Most of the recent deprecations and breaking changes to chrono have been about replacing infallible interfaces that might panic with fallible ones that force the caller to consider what should happen in cases like integer overflow. Following the same logic, functions like Utc::now() should be fallible if there's a chance they might panic at runtime. Of course, that would be a pretty inconvenient interface for the majority of use cases. I think some preferable alternatives would be:

1. automatically enable wasmbind on WASM targets when the clock feature is enabled (i.e., get rid of wasmbind as an explicit feature altogether)
2. error out at compile time if clock is enabled without wasmbind on WASM targets
3. remove interfaces like Utc::now() on WASM targets without wasmbind (i.e., only expose them if both features are enabled)

All of these options would be breaking changes, although option 1 could be implemented without breaking changes by deprecating the wasmbind feature (making it a no-op) instead of removing it altogether. I think option 1 would make the most sense since downstream users are already signaling their desire to have a functioning clock by enabling the clock feature. It would be nice if this crate automatically included whatever dependencies are needed for the clock to work on each supported target, and failed to compile on targets that don't support a clock (if any).

Panicking at runtime is a foot gun, and the Rust ecosystem generally does its best to remove foot guns.

[pitdicker]

@ramosbugs Thank you for the good comment.

For 0.5 we are not going to enable the wasmbind feature by default, see #1164 and #1472.
Option 2 and 3 are possible and will both result in a compile time error.

Would you be willing to make a PR for 2 or 3 against the 0.5.x branch?

[ramosbugs]

Reading #1164 and #1472, it seems like the motivation was to support WASM targets without the JS interface. Option 2 would break compilation for those targets when the default features (which include clock) are enabled. Given that context, I think Option 3 makes the most sense, allowing compilation to succeed as long as the downstream code doesn't actually call functions like Utc::now(), which would for sure fail at runtime anyway.

I'll work on a PR in the next week or so.

[pitdicker]

Thank you!