Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support no_std #64

Closed
wants to merge 2 commits into from
Closed

Support no_std #64

wants to merge 2 commits into from

Conversation

IcyDefiance
Copy link

This PR adds an "std" feature that is enabled by default, which allows users to disable the Error trait when needed, while keeping the Display and From traits.

This is useful for libraries that would like to support no_std while using thiserror to reduce boilerplate. For example: https://github.com/bytecodealliance/cranelift/issues/1261#issuecomment-589475011

@joshlf
Copy link

joshlf commented Feb 25, 2020

Thanks for doing this, @IcyDefiance ! This will unblock some work for us in Fuchsia.


[dev-dependencies]
anyhow = "1.0"
ref-cast = "1.0"
rustversion = "1.0"
trybuild = { version = "1.0.19", features = ["diff"] }

[features]
default = ["std"]
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note, this is technically a breaking change if anyone was already using thiserror with default-features = false, as they will now have reduced functionality.

I got burned by this when I added no_std to num crates -- rust-num/num#297. I've seen a lot of crates brush that off though, whether deliberate or not.

@Nemo157
Copy link

Nemo157 commented Feb 26, 2020

Would it be possible to introduce an alloc feature at the same time to avoid a second technically breaking change? Would there be enough functionality left without it active to make sense?

@tarcieri
Copy link

@Nemo157 an alloc feature would be nice if std::error::Error ever got moved to liballoc. Some discussion about that:

https://internals.rust-lang.org/t/can-we-move-std-error-to-core/11887/1

@IcyDefiance
Copy link
Author

IcyDefiance commented Feb 26, 2020

There was a PR to do that, but it was closed because it's incompatible with RFC 2504, which adds a backtrace thing. It's still possible that it'll be moved to alloc, but probably not anytime soon.

Aside from that, an alloc feature may have some use because it would still leave the From traits when it's disabled, but it would be even more complicated to support and avoid breaking in the future. I don't feel like I should make that decision, but I can implement it if dtolnay is okay with it.

I think I'll introduce no-std-compat tonight, though. I'll be able to eliminate the use of core that way, which will be a little easier to maintain.

@tarcieri
Copy link

@IcyDefiance if you're talking about this thread, the concern was what to do about backtrace in no_std environments, but... it seems like backtrace has no_std support?

rust-lang/rust#62502 (comment)

@shepmaster
Copy link

shepmaster/snafu#85 (comment)

At one point it “had” support but not really.

@shepmaster
Copy link

Consider using a shared alternate trait. See shepmaster/snafu#184

@Nemo157
Copy link

Nemo157 commented Feb 26, 2020

I wasn't even considering any eventual alloc::error::Error. I was just thinking about the fact that I have a no_std + optional-alloc + optional-std library so adding thiserror to reduce the error boilerplate would not be possible even after this PR; the alloc feature doesn't currently affect my errors at all, but the fact that thiserror links against alloc even with no-default-features makes it not fully no_std (so I would be satisfied if you could just remove the extern crate alloc, but I haven't spent the time to look through and see why it is there).

@Nemo157
Copy link

Nemo157 commented Feb 26, 2020

Re

an alloc feature [...] would be even more complicated to support and avoid breaking in the future

I haven't found that at all the case, you need to annotate some more things #[cfg(feature = "alloc")] but that's just more of the same as the step you take when introducing #[cfg(feature = "std")], there's nothing overly complicated about it. (And I personally find it much easier to just avoid anything like no_std_compat or as std hacks, just treat the standard library as a set of 3 crates and use appropriate paths for each item).

The biggest thing is to just ensure you have CI covering all three variations.

Copy link
Owner

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR. I am concerned that this approach is brittle from a feature unification perspective. That is, if some no-std crate (particularly a library supporting std as well as no-std) has:

use thiserror::Error;

#[derive(Error, Debug)]
#[error("...")]
pub struct MyError;

impl core_error::Error for MyError {}

and anything else in the dependency graph enables thiserror/std and core-error/std then it causes breakage in the above correct code.

Same problem for:

#[cfg(feature = "std")]
impl std::error::Error for MyError {}

Thoughts? Is there a different way to do no-std support in a way that doesn't cause breakage when only part of the dependency graph has no-std in mind?

@dtolnay dtolnay mentioned this pull request Mar 27, 2020
@joshlf
Copy link

joshlf commented Mar 30, 2020

@dtolnay With respect to your first scenario, perhaps thiserror could derive core_error::Error when std is disabled so that the user would not need to write that impl manually?

With respect to your second scenario, I would think that it would be easier to have the hypothetical crate's std feature depend on thiserror/std instead of writing that code? We could presumably document that deriving Error AND having a manual impl of std::error::Error at the same time is unsupported?

@dtolnay
Copy link
Owner

dtolnay commented Apr 12, 2020

Would it be workable for people if we emit an impl for "std::error::Error" unconditionally, and rely on downstream to have a "std" in scope that has whatever Error trait they want implemented?

// crate root
#![no_std]

mod std {
    pub mod error {
        pub use core_error::Error;

        // or, if you're not using sources and backtraces, maybe even:
        pub trait Error {}
    }
}


// module
use crate::std;
use thiserror::Error;

#[derive(Error, Debug)]
enum MyError {...}

I think this cleanly eliminates all bad interactions relating to feature unification.

@joshlf
Copy link

joshlf commented Apr 12, 2020

@dtolnay I agree that that would probably work, although I feel like it's pretty unidiomatic. So long as we're going to ask no_std users to do extra work, I feel like asking them to rely on thiserror/std if they provide a std feature themselves is more idiomatic. But I think your proposed solution would probably get the job done.

@tarcieri
Copy link

Personally I really dislike "fake std" approaches (e.g. use core as std) in truly (i.e. heapless) no_std environments.

For more featured environments, I'm a big fan of building out parts of std (and putting a mini/semi-std into the sysroot), but in truly heapless environments where developers really want to depend on libcore alone, I find approaches which pretend some minimal subset of std is available confusing and hard-to-debug (e.g. from a consumer-of-a-crate perspective, it calls into question whether std is actually needed or not)

@dtolnay
Copy link
Owner

dtolnay commented Apr 12, 2020

@tarcieri that's a surprising distinction that I haven't heard before. I would be interested to dig into why it's okay for more featured environments to piece together a custom minimal std subset, but not okay in heapless environments.

Isn't using a derive(Error) inside a no-std crate precisely about "pretending some minimal subset of std is available"? It's possible this is arguing thiserror shouldn't support no-std in the first place.

@dtolnay
Copy link
Owner

dtolnay commented Apr 12, 2020

I feel like asking them to rely on thiserror/std if they provide a std feature themselves is more idiomatic.

This only addresses the non-problematic direction, enabling thiserror/std if downstream/std is enabled. The problems come from inability to enable downstream/std if thiserror/std is enabled.

@tarcieri
Copy link

tarcieri commented Apr 12, 2020

I would be interested to dig into why it's okay for more featured environments to piece together a custom minimal std subset, but not okay in heapless environments.

The distinction is more like as part of your project you intend to build out some meaningful subset of std functionality, versus other environments where literally everything the std facade provides beyond what's in libcore (or potentially liballoc too) is irrelevant to your use case.

std::error::Error is a notable oddity that IMO belongs in liballoc

@cuviper
Copy link

cuviper commented Apr 13, 2020

Would it be workable for people if we emit an impl for "std::error::Error" unconditionally, and rely on downstream to have a "std" in scope that has whatever Error trait they want implemented?

If it were just about the derived impl, I'd suggest having an explicitly separate derive that does everything but Error. However, you'll also have to reconsider thiserror::private::AsDynError -- did you already have something in mind for that in the downstream-"std" idea?

@quasiyoke

This comment has been minimized.

@tarcieri
Copy link

a bit off topic, but:

If you'd like a crate which provides a way to derive Display similar to thiserror which works on no_std, I'd recommend displaydoc:

https://github.com/yaahc/displaydoc

You'll still need to add something like:

#[cfg(feature = "std")]
impl std::error::Error for MyError {}

...but otherwise it will still allow you to derive Display in a no_std-friendly way.

mpapierski pushed a commit to mpapierski/casper-node that referenced this pull request Mar 5, 2021
Currently thiserror crate does not support no_std so while wait for upstream PR[1]
to get merged I've used "thiserror" for std case, and for no_std
thiserror is hidden under cfg_attr.

[1]: dtolnay/thiserror#64
bors bot added a commit to casper-network/casper-node that referenced this pull request Mar 5, 2021
1077: NDRS-974: Get rid of failure r=mpapierski a=mpapierski

Ref: https://casperlabs.atlassian.net/browse/NDRS-974

Removes failure and uses optional thiserror for std case, and for no_std (i.e. wasm32 target) is hidden behind cfg_attr. This complication happens because thiserror does not support no_std upstream but a PR is pending dtolnay/thiserror#64

Co-authored-by: Michał Papierski <michal@casperlabs.io>
Co-authored-by: Michał Papierski <michal@papierski.net>
@vivekvpandya

This comment has been minimized.

@danieleades

This comment has been minimized.

@shepmaster
Copy link

Another alternative crate that supports managing no_std compatible errors at this point in time is SNAFU

This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants