shepmaster / snafu

Easily assign underlying errors into domain-specific errors while adding context
https://docs.rs/snafu/
Apache License 2.0
1.45k stars 61 forks source link

Misleading backtrace documentation on docs.rs #332

Open kosta opened 2 years ago

kosta commented 2 years ago

This line: https://github.com/shepmaster/snafu/blob/6159ea9f3a3e189f1faa2f63904402f04dea54ec/src/backtrace_shim.rs#L6

Gets rendered into docs.rs here: https://docs.rs/snafu/latest/snafu/struct.Backtrace.html

Which made me think that I don't need to enable the feature backtraces to actually get backtraces. Cost me a few minutes to figure this out :)

I think we should make it more clear that Backtraces are only available if the backtraces feature is enabled.

shepmaster commented 2 years ago

It is enabled on docs.rs though.

https://github.com/shepmaster/snafu/blob/6159ea9f3a3e189f1faa2f63904402f04dea54ec/Cargo.toml#L25

What do you propose as an alternate?

kosta commented 2 years ago

I think the tokio docs make it pretty clear that a feature is needed. A bit of digging showed that they use #[cfg(doc)] together with #[cfg(docsrs)].

I tried to prototype this here for snafu. You need to build that branch with RUSTDOCFLAGS="--cfg docsrs" DOCS_RS=1 cargo +nightly doc --open --features backtraces to see the results.

I find it a bit ugly to be honest, but it makes it much more clear.

A lighter alternative would be just use #[cfg(docsrs)] to switch between the phrasing of "Backtrace functionality is currently enabled" to "To use these features, enable the feature backtraces". Because the user doesn't care what is enabled on docs.rs, they care about what is enabled in their local repo.

8573 commented 2 years ago

Respectfully, how much advantage comes from using conditional compilation at all rather than a static statement that "This functionality is available only if the Cargo feature backtrace is enabled."?

shepmaster commented 2 years ago

SNAFU's use of features with respect to Backtrace is a bit different from other crates. The reason is that any user of SNAFU should feel comfortable including a Backtrace in their error, but that only the user producing the final binary should be able to decide if and how backtraces will actually be active.

To that end, the Backtrace type has four possible states:

  1. inert. This is what you get by default.
  2. active and opaque. This is what adding backtraces does.
  3. active and transparent; backtrace crate is used. This is what adding backtraces-impl-backtrace-crate does.
  4. active and transparent; std crate is used. This is what adding unstable-backtraces-impl-std does.

Those last two are mutually exclusive — a type alias can only point to one concrete type 😉. This is another reason that those feature flags should only be chosen by the binary.

In a world where the standard library has stabilized a Backtrace type, this might just collapse into nothingness — no feature flags, just re-exporting std::whatever::Backtrace. It might need the inert shim for no-std environments though...

Circling back to the #[cfg(doc)] idea, it'd be a little strange to show "Available on crate feature backtraces only" for (active) Backtrace but there still be a page for Backtrace even when it's not enabled. Maybe not though.

kosta commented 2 years ago

My point is just that I confused the message on docs.rs "Backtrace functionality is currently enabled." with "The backtraces feature is enabled by default".

I think we can do the most precise phrasing using #[cfg]. However a static phrasing might works as well, such as

In order to use Backtrace, you need to enable the backtraces feature which is not enabled by default. If you generated these docs locally: Backtrace functionality is currently enabled.

But that is a bit weird I must say :)