Repository: chronotope/chrono (language: rust)

Issue:
Should `now` error on wasm (not on emscripten or wasi) without wasmbind?
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()](https://github.com/chronotope/chrono/blob/main/src/offset/utc.rs#L84) 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.
```rust
#[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:
```rust
#[cfg_attr(
    all(
        target_arch = "wasm32",
        not(feature = "wasmbind"),
        not(any(target_os = "emscripten", target_os = "wasi"))
    ),
    deprecated(since = "?", note = "will always panic")
)]
```


Discussion / hints:
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?
I'm ok with how it is, I wanted to make it easier for newcomers, especially if wasmbind get's disabled by default [#1164](https://github.com/chronotope/chrono/issues/1164).
First off, thanks for all the work maintaining this excellent crate!

This issue came up in the `oauth2` (https://github.com/ramosbugs/oauth2-rs/pull/230) and `openidconnect` (https://github.com/ramosbugs/openidconnect-rs/pull/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 (https://github.com/ramosbugs/oauth2-rs/pull/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.

@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?
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.
Thank you!

Fix patch:
diff --git a/src/datetime/mod.rs b/src/datetime/mod.rs
index 7f7d9023c4..142ea997fe 100644
--- a/src/datetime/mod.rs
+++ b/src/datetime/mod.rs
@@ -874,11 +874,16 @@ impl DateTime<Utc> {
         feature = "std",
         all(
             feature = "now",
-            not(all(
-                target_arch = "wasm32",
-                feature = "wasmbind",
-                not(any(target_os = "emscripten", target_os = "wasi"))
-            ))
+            any(
+                unix,
+                windows,
+                target_os = "solid_asp3",
+                target_os = "hermit",
+                target_os = "wasi",
+                target_os = "xous",
+                all(target_vendor = "fortanix", target_env = "sgx"),
+                target_os = "teeos",
+            )
         )
     ))]
     pub(crate) fn try_from_system_time(t: std::time::SystemTime) -> Result<DateTime<Utc>, Error> {
diff --git a/src/offset/local/mod.rs b/src/offset/local/mod.rs
index bf476aac76..63dc7ee16c 100644
--- a/src/offset/local/mod.rs
+++ b/src/offset/local/mod.rs
@@ -11,7 +11,7 @@ use rkyv::{Archive, Deserialize, Serialize};
 
 use super::fixed::FixedOffset;
 use super::{MappedLocalTime, TimeZone};
-use crate::{DateTime, NaiveDateTime, Utc};
+use crate::NaiveDateTime;
 
 #[cfg(unix)]
 #[path = "unix.rs"]
@@ -120,8 +120,8 @@ impl Local {
     /// Returns a `DateTime<Local>` which corresponds to the current date, time and offset from
     /// UTC.
     ///
-    /// See also the similar [`Utc::now()`] which returns `DateTime<Utc>`, i.e. without the local
-    /// offset.
+    /// See also the similar [`Utc::now()`](crate::Utc::now) which returns `DateTime<Utc>`, i.e.
+    /// without the local offset.
     ///
     /// # Example
     ///
@@ -144,8 +144,19 @@ impl Local {
     /// let offset = FixedOffset::east(5 * 60 * 60).unwrap();
     /// let now_with_offset = Local::now().with_timezone(&offset);
     /// ```
-    pub fn now() -> DateTime<Local> {
-        Utc::now().with_timezone(&Local)
+    #[cfg(any(
+        unix,
+        windows,
+        target_os = "solid_asp3",
+        target_os = "hermit",
+        target_os = "wasi",
+        target_os = "xous",
+        all(target_vendor = "fortanix", target_env = "sgx"),
+        target_os = "teeos",
+        all(target_arch = "wasm32", feature = "wasmbind")
+    ))]
+    pub fn now() -> crate::DateTime<Local> {
+        crate::Utc::now().with_timezone(&Local)
     }
 }
 
diff --git a/src/offset/utc.rs b/src/offset/utc.rs
index 6efc289cca..ff1d5dcdcf 100644
--- a/src/offset/utc.rs
+++ b/src/offset/utc.rs
@@ -4,23 +4,12 @@
 //! The UTC (Coordinated Universal Time) time zone.
 
 use core::fmt;
-#[cfg(all(
-    feature = "now",
-    not(all(
-        target_arch = "wasm32",
-        feature = "wasmbind",
-        not(any(target_os = "emscripten", target_os = "wasi"))
-    ))
-))]
-use std::time::SystemTime;
 
 #[cfg(any(feature = "rkyv-16", feature = "rkyv-32", feature = "rkyv-64"))]
 use rkyv::{Archive, Deserialize, Serialize};
 
 use super::{FixedOffset, MappedLocalTime, Offset, TimeZone};
 use crate::naive::NaiveDateTime;
-#[cfg(feature = "now")]
-use crate::DateTime;
 #[cfg(all(feature = "now", doc))]
 use crate::OutOfRange;
 
@@ -82,14 +71,25 @@ impl Utc {
     /// Panics if the system clock is set to a time in the extremely distant past or future, such
     /// that it is out of the range representable by `DateTime<Utc>`. It is assumed that this
     /// crate will no longer be in use by that time.
-    #[cfg(not(all(
-        target_arch = "wasm32",
-        feature = "wasmbind",
-        not(any(target_os = "emscripten", target_os = "wasi"))
-    )))]
+    // Covers the platforms with `SystemTime::time()` supported by the Rust Standard Library as of
+    // Rust 1.78. See:
+    //   https://github.com/rust-lang/rust/blob/5e9bed7b1e1524936001b37b6ecbf406179d8ebe/library/std/src/sys/pal/mod.rs
+    // Note that some platforms listed in the PAL table do not support `SystemTime::time()` (e.g.,
+    // `zkvm` and `wasm`). When updating this configuration, also be sure to update `Local::now()`
+    // and `DateTime::try_from_system_time()`.
+    #[cfg(any(
+        unix,
+        windows,
+        target_os = "solid_asp3",
+        target_os = "hermit",
+        target_os = "wasi",
+        target_os = "xous",
+        all(target_vendor = "fortanix", target_env = "sgx"),
+        target_os = "teeos",
+    ))]
     #[must_use]
-    pub fn now() -> DateTime<Utc> {
-        DateTime::try_from_system_time(SystemTime::now()).expect(
+    pub fn now() -> crate::DateTime<Utc> {
+        crate::DateTime::try_from_system_time(std::time::SystemTime::now()).expect(
             "system clock is set to a time extremely far into the past or future; cannot convert",
         )
     }
@@ -101,9 +101,9 @@ impl Utc {
         not(any(target_os = "emscripten", target_os = "wasi"))
     ))]
     #[must_use]
-    pub fn now() -> DateTime<Utc> {
+    pub fn now() -> crate::DateTime<Utc> {
         let now = js_sys::Date::new_0();
-        DateTime::<Utc>::from(now)
+        crate::DateTime::<Utc>::from(now)
     }
 }
 


Test patch: