`core::error`, `no_std` #304

jordens · 2024-06-14T13:18:26Z

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.

* master: Release 1.0.61 Format PR 299 with rustfmt also {std -> core}::panic::UnwindSafe use core::fmt instead of std::fmt

jstarks · 2024-06-14T17:19:19Z

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

jordens · 2024-07-02T16:30:17Z

@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 · 2024-08-11T14:35:37Z

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

* upstream/master: Upload CI Cargo.lock for reproducing failures Work around new dead code warning in test Release 1.0.63 Update documentation of #[from] and #[backtrace] attributes Release 1.0.62 Support .0.0 nested tuple index Add regression test for issue 309 No need for dead code if struct fields are public Ignore warning on unused struct in test Fill in ignore reasons in all #[ignore] attributes

kayabaNerve · 2024-09-05T19:19:11Z

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

jordens · 2024-09-06T08:15:50Z

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

This reverts commit 8cbdc4e.

This reverts commit 3e792e0.

This reverts commit 61b1c17.

* let integration tests require `std` where necessary * don't use error_in_core anymore

don't warn if more than one of the `ignore` conditions hold

kayabaNerve · 2024-09-06T20:28:52Z

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

dhedey · 2024-09-24T17:03:01Z

@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 ?

* upstream/master: Release 1.0.64 Mark #[automatically_derived] for generated impls

jordens · 2024-09-26T10:28:19Z

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

# Rationale for this change In order to make this rep `no_std` compatible, we need `no_std` compatible errors. `thiserror` is not `no_std` compatible, while `snafu` is. We are currently using `thiserror`. Note: While dtolnay/thiserror#304 is an open PR, it is not clear how soon this will get merged. Additionally, Substrate uses `snafu`, which would help unify dependencies for Substrate-based integrations. # What changes are included in this PR? NOTE: the individual commits may be good to review individually. - All error enum variants consisting of tuple structs are transformed into named structs. This is necessary because `snafu` does not support tuple structs. - Every `#[derive(Error)]` is substituted with the analogous `#[derive(Snafu)]`. In particular: * `#[error(...)]` attributes are substituted with equivalent `#[snafu(display(...))]` attributes * `#[error(transparent)]` attributes are substituted with equivalent `#[snafu(transparent)]` attributes (which also derive the corresponding `From` implementation) * For `ConversionError::TimestampConversionError`, the `#[snafu(context(false), display(...))]` attribute is used for deriving a `From` implementation, and at the same time maintain the custom error message - A `std` feature is introduced for the `proof-of-sql` crate, which in turns activates the `snafu/std` feature. The `std` feature is required for the `posql_db` example, because `Clap` relies on the `std::error::Error` trait. - `thiserror` still appears in the dependency tree (`cargo tree -i thiserror`), but only as a transitive dependency (via `blitzar`, and dev-dependencies) # Are these changes tested? Yes. This PR is a refactoring, and all existing tests pass.

bushrat011899

I'd really like this to land; the implementation looks good and even manages to avoid bumping MSRV as long as you include the std feature. Getting this merged would unblock some no_std support for Bevy that I'd love to get moving before the end of the year if possible.

BenjaminBrienen

This is a good change which addresses all concerns and merging this would enable other crates to continue using thiserror. It's unfortunate that the ecosystem is splitting over this problem which is already solved.

dhedey · 2024-10-10T07:55:58Z

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 · 2024-10-10T21:41:39Z

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 · 2024-10-11T10:19:17Z

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

Just a few thoughts/suggestions from a bystander.

dhedey · 2024-10-11T10:28:35Z

impl/src/expand.rs

@@ -37,7 +37,7 @@ fn fallback(input: &DeriveInput, error: syn::Error) -> TokenStream {

        #[allow(unused_qualifications)]
        #[automatically_derived]
-        impl #impl_generics std::error::Error for #ty #ty_generics #where_clause
+        impl #impl_generics ::thiserror::__private::error::Error for #ty #ty_generics #where_clause


Out of interest, why did we choose this rather than hardcoding ::std or ::core, in the compiled output (via e.g. an #error_crate variable)?

My slight concern is that use of ::thiserror::__private may leak this __private namespace out and could confuse people debugging this. But using ::std / ::core directly it's more obvious what's failing.

dhedey · 2024-10-11T10:45:57Z

impl/src/expand.rs

@@ -307,12 +309,12 @@ fn impl_enum(input: Enum) -> TokenStream {
                    let self_provide = if type_is_option(backtrace_field.ty) {
                        quote! {
                            if let ::core::option::Option::Some(backtrace) = backtrace {
-                                #request.provide_ref::<std::backtrace::Backtrace>(backtrace);
+                                #request.provide_ref::<::std::backtrace::Backtrace>(backtrace);


MINOR: It may be nice if we gave a better compiler error if a user tried to specify a backtrace when we're not in std mode, which could advise the user to enable the std feature to use this? I believe the right place for this would be in valid.rs.

... that said, it might actually cause more breakage if people are using default-features = false mode and after upgrading they get a compilation error from this "help" because they are actually running in std mode, just importing this crate incorrectly. So I think I've talked myself out of this.

So maybe safer to leave this as-is, and perhaps document that this only works if compiling with std available?

dhedey · 2024-10-11T10:47:35Z

Cargo.toml

@@ -11,6 +11,8 @@ license = "MIT OR Apache-2.0"
 repository = "https://github.com/dtolnay/thiserror"
 rust-version = "1.56"

+# without the `std` feature (i.e. `no_std`): rust-version = "1.81"


As mentioned in this comment, I feel we should attempt to widen our build compatibility to not break rustc < 1.81 builds importing thiserror with default-features = false.

dhedey · 2024-10-11T11:29:49Z

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

jdygert-spok · 2024-10-11T15:16:57Z

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 · 2024-10-11T20:53:37Z

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 · 2024-10-18T00:37:00Z

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.
servo/rust-url#831

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

clabby · 2024-10-19T15:12:49Z

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 · 2024-10-21T11:10:18Z

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

zdivelbiss · 2024-11-03T00:56:48Z

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?

dtolnay

I had a chance to work on this feature this week, and I ended up going with a different implementation that automatically enables a std dependency in compilers older than 1.81, more like my corresponding change in anyhow from earlier which has been linked above, and along the lines of what was suggested by @dhedey in #304 (comment) and #304 (comment). There were also some future compatibility drawbacks of this PR that I handled differently, which you can find in #373 and #372.

Thanks anyway for the work on this PR, and also to @dhedey for numerous substantive comments.

jordens added 11 commits May 16, 2024 15:12

support no_std with error-in-core

6085f29

exlude compiletest from no_std, fix no_std doctest

c560bcc

undo Error->MyError renames

ec51e63

use __private namespace

f7f6c77

add comment

f1d9df3

Merge branch 'master' into no-std

33b7a36

* master: Release 1.0.61 Format PR 299 with rustfmt also {std -> core}::panic::UnwindSafe use core::fmt instead of std::fmt

use explicit doc(test(attr))

3663fd1

error_in_core is stable

61b1c17

{std -> core}:error: update docs/comment

3e792e0

docs.rs: std is default

b278258

bump MSRV to 1.81

8cbdc4e

pulsastrix mentioned this pull request Jun 26, 2024

Reconsider use of error handling crates namib-project/dcaf-rs#14

Open

AkiyukiOkayasu mentioned this pull request Aug 20, 2024

Replace anyhow with thiserror AkiyukiOkayasu/pacmog#52

Merged

Systemcluster mentioned this pull request Sep 5, 2024

Support no-std #318

Closed

jordens marked this pull request as ready for review September 5, 2024 21:03

jordens added 7 commits September 6, 2024 15:11

Revert "bump MSRV to 1.81"

7fa183b

This reverts commit 8cbdc4e.

Revert "{std -> core}:error: update docs/comment"

28ce40b

This reverts commit 3e792e0.

Revert "error_in_core is stable"

0b058bc

This reverts commit 61b1c17.

minimalize changes, maintain MSRV for std

e7e9341

* let integration tests require `std` where necessary * don't use error_in_core anymore

test 1.81 and no_std

bfd7357

ci no_std: not check but test, 1.81 not necessary

706fb6a

compiletest: allow unused_attributes

a3d073b

don't warn if more than one of the `ignore` conditions hold

jordens changed the title ~~std::error -> core::error, no_std~~ core::error, no_std Sep 6, 2024

JayWhite2357 mentioned this pull request Sep 21, 2024

Feat/no std parser spaceandtimelabs/sxt-proof-of-sql#176

Closed

domenukk mentioned this pull request Sep 23, 2024

no-std support for hickory-proto hickory-dns/hickory-dns#2104

Draft

Merge remote-tracking branch 'upstream/master' into no-std

e779e1b

* upstream/master: Release 1.0.64 Mark #[automatically_derived] for generated impls

BD103 mentioned this pull request Sep 27, 2024

Tracking Issue: MVP no_std Bevy bevyengine/bevy#15460

Open

Pat-Lafon mentioned this pull request Sep 27, 2024

Support Error with std feature disabled lalrpop/lalrpop#957

Merged

bushrat011899 reviewed Sep 30, 2024

View reviewed changes

AlbertoSvg mentioned this pull request Oct 4, 2024

Allow no_std and fewer dependencies Cybersecurity-LINKS/zkryptium#9

Closed

BenjaminBrienen reviewed Oct 9, 2024

View reviewed changes

dhedey reviewed Oct 11, 2024

View reviewed changes

fjarri mentioned this pull request Oct 12, 2024

Use thiserror for errors entropyxyz/manul#17

Open

refcell mentioned this pull request Oct 19, 2024

fix: published thiserror anton-rs/kona#720

Closed

danieleades mentioned this pull request Oct 20, 2024

use 'thiserror' to define 'Error' VirxEC/dubins_paths#2

Open

juliankrieger mentioned this pull request Oct 21, 2024

With the move of the Error Trait into core, the std flag is no longer needed. google/coset#100

Open

ianthetechie mentioned this pull request Oct 22, 2024

[core] no_std support stadiamaps/ferrostar#26

Open

4 tasks

dtolnay reviewed Nov 6, 2024

View reviewed changes

dtolnay closed this Nov 6, 2024

jordens deleted the no-std branch November 6, 2024 06:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`core::error`, `no_std` #304

`core::error`, `no_std` #304

jordens commented Jun 14, 2024 •

edited

Loading

jstarks commented Jun 14, 2024

jordens commented Jul 2, 2024

umgefahren commented Aug 11, 2024

kayabaNerve commented Sep 5, 2024

jordens commented Sep 6, 2024

kayabaNerve commented Sep 6, 2024

dhedey commented Sep 24, 2024 •

edited

Loading

jordens commented Sep 26, 2024

bushrat011899 left a comment

BenjaminBrienen left a comment

dhedey commented Oct 10, 2024

jstarks commented Oct 10, 2024

dhedey commented Oct 11, 2024 •

edited

Loading

dhedey left a comment

dhedey Oct 11, 2024 •

edited

Loading

dhedey Oct 11, 2024 •

edited

Loading

dhedey Oct 11, 2024

dhedey commented Oct 11, 2024 •

edited

Loading

jdygert-spok commented Oct 11, 2024

kayabaNerve commented Oct 11, 2024

domenukk commented Oct 18, 2024

clabby commented Oct 19, 2024

juliankrieger commented Oct 21, 2024

zdivelbiss commented Nov 3, 2024

dtolnay left a comment

core::error, no_std #304

core::error, no_std #304

Conversation

jordens commented Jun 14, 2024 • edited Loading

jstarks commented Jun 14, 2024

jordens commented Jul 2, 2024

umgefahren commented Aug 11, 2024

kayabaNerve commented Sep 5, 2024

jordens commented Sep 6, 2024

kayabaNerve commented Sep 6, 2024

dhedey commented Sep 24, 2024 • edited Loading

jordens commented Sep 26, 2024

bushrat011899 left a comment

Choose a reason for hiding this comment

BenjaminBrienen left a comment

Choose a reason for hiding this comment

dhedey commented Oct 10, 2024

jstarks commented Oct 10, 2024

dhedey commented Oct 11, 2024 • edited Loading

dhedey left a comment

Choose a reason for hiding this comment

dhedey Oct 11, 2024 • edited Loading

Choose a reason for hiding this comment

dhedey Oct 11, 2024 • edited Loading

Choose a reason for hiding this comment

dhedey Oct 11, 2024

Choose a reason for hiding this comment

dhedey commented Oct 11, 2024 • edited Loading

jdygert-spok commented Oct 11, 2024

kayabaNerve commented Oct 11, 2024

domenukk commented Oct 18, 2024

clabby commented Oct 19, 2024

juliankrieger commented Oct 21, 2024

zdivelbiss commented Nov 3, 2024

dtolnay left a comment

Choose a reason for hiding this comment

`core::error`, `no_std` #304

`core::error`, `no_std` #304

jordens commented Jun 14, 2024 •

edited

Loading

dhedey commented Sep 24, 2024 •

edited

Loading

dhedey commented Oct 11, 2024 •

edited

Loading

dhedey Oct 11, 2024 •

edited

Loading

dhedey Oct 11, 2024 •

edited

Loading

dhedey commented Oct 11, 2024 •

edited

Loading