`core::error`, `no_std`

[jordens]

error_in_core has been stabilized in 1.81. This PR builds on the work of #64/#211/#244.

• Introduce a std feature (default enabled) to gate the std components (currently only std::path)
• Private re-export of std::error or core::error to avoid bumping MSRV for std. no_std does require 1.81.
• Tests/docs/CI updates
• Macro hygiene: impl/expand.rs uses full absolute paths.

It's unclear to me whether it's better to bump the MSRV (1.56 -> 1.81) or use a private re-export of either std::error or core::error (as done here) to keep support for rustc < 1.81.

[jstarks]

Given the wide use of this crate, it seems very unlikely that an MSRV bump would be accepted.

[jordens]

@dtolnay what's your take on the PR, especially regarding the MSRV question?

It's unclear to me whether it's better to bump the MSRV (1.56 -> 1.81 as done in this draft) or use a private re-export of either std::error or core::error (as done previously) to keep support for rustc < 1.81.

[umgefahren]

What if one is just able to somehow pass core::error instead of std::error?

[kayabaNerve]

My personal belief is default-features = false should be MSRV 1.81 and features = ["std"] should be MSRV 1.56.

[jordens]

@dtolnay any guidance on how you'd like to see it done?

[kayabaNerve]

PR LGTM with edge case that I don't believe you can import thiserror with a distinct package name with this path (I'm unsure if you could already).

[jordens]

@dtolnay I know this is at least the third PR attempting it and this has come up many times in the past. But now that your condition can be met, what's missing? And what can we do to get this in?

[ognevny]

this code doesn't work with my #![no_std] lib...

60 | #[derive(Debug, Error, PartialEq, Eq)]
   |                 ^^^^^
   |                 |
   |                 use of undeclared crate or module `std`
   |                 in this derive macro expansion
   |
  ::: ~/.cargo/git/checkouts/thiserror-8e8b6321d051832b/c9cd3b9/impl/src/lib.rs:33:1
   |
33 | pub fn derive_error(input: TokenStream) -> TokenStream {
   | ------------------------------------------------------ in this expansion of `#[derive(Error)]`
   |
help: consider importing this module
   |
5  + use core::error;

[ZhekaS]

this code doesn't work with my #![no_std] lib...

I am getting a similar one for a bin crate:

error[E0433]: failed to resolve: use of undeclared crate or module `std`
  --> src/********:49:17
   |
49 | #[derive(Debug, Error)]
   |                 ^^^^^ use of undeclared crate or module `std`
   |
   = note: this error originates in the derive macro `Error` (in Nightly builds, run with -Z macro-backtrace for more info)
help: consider importing this module
   |
1  + use core::error;
   |

For more information about this error, try `rustc --explain E0433`.

Looks like it only happens when fn source is derived. Then the macro expansion looks like:

fn source(&self) -> ::core::option::Option<&(dyn std::error::Error + 'static)> ....

Note the std there... Can be fixed by:

diff --git a/impl/src/expand.rs b/impl/src/expand.rs
index 7c1e351..7e05e8e 100644
--- a/impl/src/expand.rs
+++ b/impl/src/expand.rs
@@ -267,7 +267,7 @@ fn impl_enum(input: Enum) -> TokenStream {
             }
         });
         Some(quote! {
-            fn source(&self) -> ::core::option::Option<&(dyn std::error::Error + 'static)> {
+            fn source(&self) -> ::core::option::Option<&(dyn ::thiserror::__private::error::Error + 'static)> {
                 use thiserror::__private::AsDynError as _;
                 #[allow(deprecated)]
                 match self {

[jdygert-spok]

PR LGTM with edge case that I don't believe you can import thiserror with a distinct package name with this path (I'm unsure if you could already).

Looks like you already couldn't, at least when using certain attributes, since there's existing use of the __private module.

this code doesn't work with my #![no_std] lib...

Missed one: https://github.com/dtolnay/thiserror/pull/304/files#diff-bb7dbcae2958da4ff43be22e512a8abfb22393983d7e49717d5a8ec25ea7abaeR270

I think this PR would be more likely to be merged if CI would catch this.

[ZhekaS]

Missed one: https://github.com/dtolnay/thiserror/pull/304/files#diff-bb7dbcae2958da4ff43be22e512a8abfb22393983d7e49717d5a8ec25ea7abaeR270

Works like a charm with the above fix on a somewhat large no_std project. Really need it to get in ASAP.

[jordens]

Fixed in the commit above.

[ognevny]

now it fails to publish my lib into crates-io

error[E0433]: failed to resolve: use of undeclared crate or module `std`
  --> src/....rs:62:17
   |
62 | #[derive(Debug, Error, PartialEq, Eq)]
   |                 ^^^^^
   |                 |
   |                 use of undeclared crate or module `std`
   |                 in this derive macro expansion
   |
  ::: ~/.cargo/registry/src/index.crates.io-6f17d22bba15001f/thiserror-impl-1.0.63/src/lib.rs:33:1
   |
33 | pub fn derive_error(input: TokenStream) -> TokenStream {
   | ------------------------------------------------------ in this expansion of `#[derive(Error)]`
   |
help: consider importing this module
   |
5  + use core::error;
   |

error[E0433]: failed to resolve: use of undeclared crate or module `std`
  --> src/....rs:68:13
   |
68 |     #[error(transparent)]
   |             ^^^^^^^^^^^ use of undeclared crate or module `std`
   |
help: consider importing one of these items
   |
5  + use core::error::Error;
   |
5  + use core::fmt::Error;
   |
5  + use regex::Error;
   |

error[E0433]: failed to resolve: use of undeclared crate or module `std`
  --> src/....rs:9:17
   |
9  | #[derive(Debug, Error, PartialEq, Eq)]
   |                 ^^^^^
   |                 |
   |                 use of undeclared crate or module `std`
   |                 in this derive macro expansion
   |
  ::: ~/.cargo/registry/src/index.crates.io-6f17d22bba15001f/thiserror-impl-1.0.63/src/lib.rs:33:1
   |
33 | pub fn derive_error(input: TokenStream) -> TokenStream {
   | ------------------------------------------------------ in this expansion of `#[derive(Error)]`
   |
help: consider importing this module
   |
5  + use core::error;
   |

For more information about this error, try `rustc --explain E0433`.
error: could not compile `...` (lib) due to 3 previous errors

while tests without default features (std disabled) compile and run successfully

[jordens]

You can't publish if you have git dependencies.

[jmwample]

It might be more palatable if this PR made no-std a feature such that enabling it bumps the MSRV to 1.81 instead of forcing an MSRV bump for the library. You could make no-std default for compilers 1.81+ and then an organic bump could be done when the library actually wants to drop support for older versions.

[kayabaNerve]

Features are intended to be additive. no-std disables some functionality so it isn't additive. std is additive, and when enabled, maintains the existing MSRV.

[jmwample]

I recognize that typically you want an additive std or alloc feature to enable broader functionality than the scope of the original package, but I feel like this is a special case because it is a widely used, existing crate. I would argue that (at least for now) no-std is additive in that it adds a new mode of usage, and does so without a breaking change for packages already using thiserror.

In my view this all depends on the type of release that this change goes out in. If it is a major or minor version bump then msrv 1.81 with backward support via feature is probably fine. If it is just a patch though, it should stay msrv 1.56 w/ a no-std feature.

[kayabaNerve]

Enabling a no-std feature would be a breaking change for some packages already using thiserror.

no-std disables some functionality

If this had a no-std feature, it'd be possible for one dependency to enable it in a way which irrecoverably breaks another dependency who relies on the std functionality. This cannot happen with the std feature present here.

Since this is a new feature, semver rules would mean this should be done with a minor version bump (satisfying your preferences) yet AFAIK, dtolnay does not follow semver for their crates (at least not all of them). I'd presume this to be released as 1.0.64 accordingly.

[dhedey]

@jordens - What do we think about using build.rs to effectively force the use of std::core functionality on rustc < 1.81 ?

We could do this by introducing a cargo:rustc-cfg=thiserror_no_core_error rustc-cfg flag, in a very similar manner to dtolnay did in his equivalent PR in anyhow https://github.com/dtolnay/anyhow/pull/383/files ?

[jordens]

I'd like to hear about the prospects of this PR first.

[dhedey]

I certainly support this PR.

That said, and please correct me if I’m wrong, but it is my understanding that if we release this current PR, we immediately break the builds of anyone compiling with rustc < 1.81 which imports thiserror with default-features = false.

Given the widespread usage of thiserror, I would imagine such a breakage is not tenable.

I have suggested a simple tweak to this PR which I believe avoids this issue in this comment by effectively always enabling an equivalent-to-std feature before the Error trait is available in core.

I assume given dtolnay was the author of the equivalent PR I linked to in anyhow, they would favour such a solution, and given that I assume the merging of any such PR is blocked on dtolnay’s approval, it seems likely we require such a change; tested with this particular import case.

But maybe I’m missing something which means thiserror should be given different treatment?

[jstarks]

I certainly support this PR.

That said, and please correct me if I’m wrong, but it is my understanding that if we release this current PR, we immediately break the builds of anyone compiling with rustc < 1.81 which imports thiserror with default-features = false.

Given the widespread usage of thiserror, I would imagine such a breakage is not tenable.

I have suggested a simple tweak to this PR which I believe avoids this issue in this comment by effectively always enabling an equivalent-to-std feature before the Error trait is available in core.

I assume given dtolnay was the author of the equivalent PR I linked to in anyhow, they would favour such a solution, and given that I assume the merging of any such PR is blocked on dtolnay’s approval, it seems likely we require such a change; tested with this particular import case.

But maybe I’m missing something which means thiserror should be given different treatment?

There are a few places where std::path::Path is referenced, as part of an impl on some trait. Path is not and will never be in core. So these references have to be gated behind a feature.

[dhedey]

There are a few places where std::path::Path is referenced, as part of an impl on some trait. Path is not and will never be in core. So these references have to be gated behind a feature.

I'm not suggesting we remove the std feature - I'm just suggesting it's effectively auto-enabled pre rust 1.81, to minimize compilation/CI breaks for people using a rust version before it supports core::error.

The current PR I believe will break compilation for anyone who is building on rustc < 81 and has imported thiserror with default-features = false.

Instead, I suggest similar in execution to this PR on anyhow we:

• Introduce in build.rs, if rustc < 81 then println!("cargo:rustc-cfg=thiserror_no_core_error_force_use_std"); (and an equivalent line to avoid lint errors in rustc >= 81)
• In code, replace feature = "std" with any(feature = "std", thiserror_no_core_error_force_use_std),

After this change, I believe the breaks are minimized to people who:

• Are on rustc >= 81
• Use default-features = false
• Rely on std features in this crate

Which I imagine is a sufficiently small crowd that this might be deemed acceptable, given the fix is to just add the std feature. It would be even nicer if we could detect such breakages, and advise in the compilation error that they need to use the std feature.

For what it's worth, I think this is particularly a problem because thiserror can be used by libraries rather than applications, and so may affect library consumers who don't have a direct thiserror dependency themselves; but just weren't using a lock file or started a fresh project with a slightly outdated rust version. Such users may be less experienced, and may find it frustrating to have to fix a compilation error due to an indirect dependency breaking.

[dhedey]

Actually, I'm a little wrong, apologies. I think I misinterpreted the PR on anyhow.

The PR on anyhow simply avoids outputting a core error at all if anyhow_no_core_error is true.

So, when upgrading to anyhow = { version = "1.0.89", default-features = false } with rustc at 1.80, it appears the crate itself compiles, but downstream crates using the auto-conversions are broken because it no longer outputs conversions from std::error::Error to anyhow::Error:

fn main() -> Result<()> {
    let config = std::fs::read_to_string("cluster.json")?;
    Ok(())
}

Compilation Error

error[E0277]: `?` couldn't convert the error to `anyhow::Error`
 --> src/main.rs:4:57
  |
3 | fn main() -> Result<()> {
  |              ---------- expected `anyhow::Error` because of this
4 |     let config = std::fs::read_to_string("cluster.json")?;
  |                  ---------------------------------------^ the trait `From<std::io::Error>` is not implemented for `anyhow::Error`, which is required by `Result<(), anyhow::Error>: FromResidual<Result<Infallible, std::io::Error>>`
  |                  |
  |                  this can't be annotated with `?` because it has type `Result<_, std::io::Error>`
  |
  = note: the question mark operation (`?`) implicitly performs a conversion on the error value using the `From` trait
  = help: the following other types implement trait `FromResidual<R>`:
            <Result<T, F> as FromResidual<Result<Infallible, E>>>
            <Result<T, F> as FromResidual<Yeet<E>>>
  = note: required for `Result<(), anyhow::Error>` to implement `FromResidual<Result<Infallible, std::io::Error>>`

That said, unlike anyhow, this crate (and indeed the output from the macro) simply wouldn't compile on 1.80 with default-features = false, so I think personally I'd still advocate for effectively force-enabling the std feature on older rustc.

But I accept others may disagree, and I don't think it's a blocker.

Maybe I'm strange to default to using default-features = false for all imports and only turning on what I need :+1: .

[jdygert-spok]

To be clear, anyhow already had an std feature enabled by default, meaning that PR strictly added functionality when core::error::Error is available. thiserror does NOT currently have an std feature, meaning this is a breaking change, depending on how you view default features.

[kayabaNerve]

I think all of this can be argued bikeshedding and is best left to a further PR. Someone can even now prepare a PR, descending from these commits, ready to be merged after this PR is merged if it's acceptably done and desirable. It doesn't have to be a blocker here.

[domenukk]

As another reference point, rust-url - with a similar amount of users on crates.io - just accepted a PR that adds a std feature. Just as in this PR, MSRV for std stayed the same, no_std requires 1.81. https://github.com/servo/rust-url/pull/831

TL;DR: Would be great to get this PR merged as well :)

[clabby]

Any updates here? This PR looks good to go, other popular crates have adopted this same pattern. @dtolnay, any notes on what's blocking this on your end?

[juliankrieger]

Is there any work left to do here? I would love to help.

[zdivelbiss]

I'd love to use this crate in a project of mine, and it seems there hasn't been much activity in describing what needs to be done / changed.

@dtolnay Any input on how this can proceed to being merged?