Implement Support for no_std #138

CryZe · 2019-08-06T09:04:20Z

This brings no_std support to snafu. It now has a std feature which is activated by default. To make no_std support as frictionless as possible, snafu now internally either uses the std::error::Error trait when the feature is activated and defines its own API compatible trait instead when it's disabled, but it is never exposed publicly for now.

Resolves #85

Cargo.toml

shepmaster · 2019-08-06T13:23:19Z

src/no_std_error.rs

+/// debugging via [`source`] chains.
+///
+/// [`source`]: trait.Error.html#method.source
+pub trait Error: Debug + Display {


I'm very hesitant to copy the standard libraries Error like this, especially the implementations on arbitrary types. One thing I've been negative about is how failure created a new trait and then told everyone that they needed to switch to it to be compatible. This felt like it was a bad "team player" in the broader ecosystem.

I suppose we'd quickly run into coherence problems if we make this a marker trait and provided a blanket implementation?

pub trait Error: Debug + Display {} impl<T: Debug + Display> Error for T {}

Another avenue is to completely drop Error and just implement Debug + Display; any idea what that world might look like?

I've considered that when I initially tried implementing this, but you quickly lose out on most of snafu's feature such as the ResultExt and OptionExt traits. We'd at least need some trait that we implement in addition to Display and Debug so we can somehow make ResultExt and OptionExt work too. Also not having source chaining is probably pretty bad too. So that's why in my initial prototyping it evolved from not having an Error trait at all to eventually just a Copy of the full thing (although I did consider dropping the deprecated methods).

The Error trait introduced here is not really meant to be used anywhere really. In livesplit-core where I've been using this no-std snafu branch the Error trait never is publicly visible to anyone, nor am I even using it directly (it's only indirectly used through ResultExt and OptionExt). It is mostly just a convenience trait to not lose out on any of the features of snafu. So in a way this is not meant to be "a bad team player" that everyone should switch to. It's in a way very similar to the Backtrace shim. Maybe it can be hidden from the documentation even?

Actually the blanket implementation Error trait could maybe work. I'll see if it causes any problems.

Although I don't think there even is any harm to having a copy of the Error trait in snafu. You can still treat core as if snafu didn't have an Error trait at all. But if you want to use an Error trait snafu optionally has one available for you to use. And if someone activates the std feature (either directly through a feature of your crate or indirectly), your code automatically uses the std Error trait and there is no "bad team playing" going on.

Although one problem I can see is that errors from other core crates might not implement an Error trait at all on core, so you can't use ResultExt and co. regardless. That sounds like maybe the bounds on ResultExt and co. should maybe be lifted a bit instead then though. And it's honestly not that bad, a little newtype easily solves the problem and it's much less of a problem than not having an error type in general, which is much more painful.

It feels like we will need to release something and get feedback from people as to the usefulness. I think it's best we call the no-std support / exact implementation experimental.

For example, one thing that concerns me is what happens if one crate using no-std SNAFU exports an error type. Will other crates be able to make use of it, or will they need to wrap it in that newtype?

I'm not sure I understand what you mean with that. Why would they need to wrap it in a newtype? If the error type is from snafu in a no_std context, then there simply is no Error trait, just like usual. Or are you saying what the story is with two crates using snafu in a no_std context? That should work seamlessly too, unless they are using different snafu versions.

CryZe · 2019-08-07T14:38:34Z

1.18 can't be supported unless we also modify the macro such that it conditionally emits paths to std instead of core (well it can be supported if we put extern crate core; in the test, but that is a breaking change for everyone using 1.18). However this PR is breaking for everyone anyway with default-features = false, so depending on how we want to handle this, this may need to be part of a snafu 0.5 instead and 1.18 support could be removed at the same time (or at least it would be just an extern crate core; away for them), so we don't need to special case everything in the macro too.

CryZe · 2019-08-08T07:25:36Z

Okay, so here's a proposal: We conservatively go for doc(hidden) such that there is no publicly visible Error trait, but snafu still retains all of its features to reduce friction when a user is trying to support both std and no_std. At a later point the trait can either transparently be switched out by the real core::error::Error or be made publicly visible without causing any breaking changes.

CryZe · 2019-08-08T11:08:22Z

This is basically done from my side. The CI server seems to not be responding, but it should turn green eventually and everything is implemented.

CryZe · 2019-08-16T07:26:25Z

I rebased this onto the latest changes to resolve the conflicts. This is ready for review again :)

TL;DR of everything above:

Both std futures and futures 0.1 support is now in.
Backtrace can't be supported just yet because they don't support the alloc crate yet and thus have only very minimal support for core.
The error trait is not publicly exposed anymore.
This is a slight breaking change for Rust 2015. It is still supported, but needs extern crate core; This can be worked around though from snafu's side, if we want to do that (it's kind of hacky, but the futures crate does the same trick).
An alternative design with the Error trait would significantly worsen the maintenance of snafu and everyone using the library (or would not work out at all because it would break code when activating the std feature, such as an Error trait that is always implemented for T: Display + Debug).
The way the Error trait works now allows for a smooth (non-breaking) upgrade to core::error::Error if / once that happens.
Snafu loses no feature other than support for the backtrace crate when switching to core, which allows for a smooth transition to no_std for users of snafu.

shepmaster · 2019-08-17T13:43:07Z

compatibility-tests/v1_18/src/lib.rs

@@ -1,5 +1,6 @@
 #![cfg(test)]

+extern crate core;


I saw you said

it can be supported if we put extern crate core; in the test, but that is a breaking change for everyone using 1.18

I wanted to see which versions would be affected, so I removed this line and built in 1.18, but I didn't get an error; any idea what might have changed or that I might be doing wrong?

It's actually Rust 2015 that's affected. The proc macro targets core, but that's not in the prelude in Rust 2015. There is a workaround though that I could implement.

src/no_std_error.rs

CryZe · 2019-08-30T14:21:03Z

Good news, looks like the Error trait is moving into the alloc crate: rust-lang/rust#64017

roblabla · 2019-09-02T14:39:50Z

Bad news, the error trait can't move into the alloc crate because of RFC 2504

CryZe · 2019-09-02T16:09:41Z

Yeah while not the best news, I still think the approach in this PR works fairly well and can easily be extended to alloc::Error whenever someone figures out how to make it compatible with the backtraces. I think you want the "core::Error" introduced here anyway as a fallback for when you don't even want alloc.

roblabla · 2019-09-25T07:54:20Z

With fehler released, and failure still around, I was wondering if maybe it would be a good idea to move the Error trait in a separate crate, tentatively called core-error, that would be co-maintained by the different stakeholders of error libraries?

My hope is that it would help with the interoperability, and get no_std libraries that don't want to buy into a specific error handling crate (snafu/failure...) to still interoperate with them.

CC withoutboats/fehler#12 (comment)

CryZe · 2019-09-25T08:02:04Z

Yeah I was considering that when we had the discussion about the error trait here. There already is a crate, but it seems barely used / maintained. But maybe it can be "revived"? It should definitely have a std feature and auto upgrade to std's Error if it exists, so you automatically get the proper Error trait for the specific use case.

roblabla · 2019-09-25T09:30:08Z

Do you have a link to said crate? A quick google didn't turn up anything.

While discussing this with @leo60228 , we figured such a crate would work like this:

without any features, it'd be exactly the same as the current core error trait in snafu.
with the alloc feature, it would support downcasting (and support alloc errors)
with the std feature, it'd just re-export std::error::Error.
it would auto-detect the rustc version (with a build script) to figure which errors to implement the trait on.

If there's buy-in on the idea, I'd love to get such a crate rolling.

CryZe · 2019-09-25T11:58:37Z

Here: https://crates.io/crates/error_core

shepmaster

This looks great, and I'm down for merging it once the one question is answered. Thanks for your patience!

shepmaster · 2019-10-08T01:03:27Z

src/no_std_error.rs

+    core::fmt::Error,             // 1.11
+    core::cell::BorrowMutError,   // 1.13
+    core::cell::BorrowError,      // 1.13
+    core::char::ParseCharError    // 1.20


Since we bumped our minimum version to 1.31, are there any errors missing from this list?

Not that I'm aware of. When I created the PR these were all the Error impls in std. These used to be three macro calls where one had the rust_1_30 feature check, but I condensed that into the unconditional macro call, so I don't think I missed any (other than potential ones in 1.38+ or so).

However what is missing is all the alloc based error types. We'd need an alloc feature for those.

shepmaster · 2019-10-08T01:08:51Z

@roblabla I think that's a great idea! I'd be much happier knowing that we weren't making some completely custom trait that was tied to this crate. Ideally, some magical solution would come from libcore, but until that happens, a shared community crate seems like a great step.

Since this PR is likely to be resolved before that crate can completely be usable for our cases, would you mind opening a new issue to track the idea?

This brings no_std support to SNAFU. It now has a `std` feature which is activated by default. To make no_std support as frictionless as possible, SNAFU now reexports the `std::error::Error` trait as `snafu::Error` when the feature is activated and defines its own API compatible trait instead when it's disabled. In order to not commit to having a publicly visible copy of the `Error` trait in SNAFU, hiding it from the documentation allows for SNAFU to still support all of its features, but the user never gets to use the trait. This way SNAFU can switch out the trait at any point with `core::error::Error` if that becomes a thing. Alternatively the trait here can also be made visible at some point. Both of these possibilities are not breaking, so the conservative approach with `doc(hidden)` allows for the smoothest experience going forward. Resolves shepmaster#85

shepmaster · 2019-10-08T13:01:08Z

.cirrus.yml

+    - rustup target add thumbv6m-none-eabi
+  primary_test_script:
+    - rustc --version
+    - cargo build --no-default-features --target thumbv6m-none-eabi


Completely unrelated, but thanks for showing me how to actually make some kind of test for a no-std crate. I had some really nasty hacks to try and do this before.

shepmaster · 2019-10-08T13:15:19Z

Thanks!

shepmaster reviewed Aug 6, 2019

View reviewed changes

CryZe force-pushed the no-std branch 2 times, most recently from 1250205 to ee2bcc1 Compare August 7, 2019 14:45

CryZe mentioned this pull request Aug 8, 2019

Support no_std use cases by using alloc LiveSplit/livesplit-core#223

Closed

26 tasks

CryZe force-pushed the no-std branch 2 times, most recently from d38ace0 to 5c03a31 Compare August 16, 2019 07:25

shepmaster reviewed Aug 17, 2019

View reviewed changes

src/no_std_error.rs Outdated Show resolved Hide resolved

shepmaster reviewed Aug 17, 2019

View reviewed changes

src/no_std_error.rs Show resolved Hide resolved

shepmaster reviewed Aug 17, 2019

View reviewed changes

src/no_std_error.rs Outdated Show resolved Hide resolved

shepmaster reviewed Aug 17, 2019

View reviewed changes

src/no_std_error.rs Outdated Show resolved Hide resolved

shepmaster mentioned this pull request Aug 26, 2019

The path to 0.5 #145

Closed

7 tasks

CryZe force-pushed the no-std branch 2 times, most recently from 2aff131 to d73b54d Compare August 26, 2019 15:08

CryZe force-pushed the no-std branch from d73b54d to b633123 Compare August 29, 2019 14:51

roblabla mentioned this pull request Sep 21, 2019

no_std support? withoutboats/fehler#12

Closed

CryZe force-pushed the no-std branch 2 times, most recently from 98a3202 to 86c7bec Compare September 25, 2019 07:45

CryZe force-pushed the no-std branch from 86c7bec to 28f1591 Compare September 30, 2019 12:10

CryZe mentioned this pull request Sep 30, 2019

Update compiler error text for Rust 1.38 #182

Merged

CryZe force-pushed the no-std branch from 28f1591 to f81a098 Compare October 1, 2019 07:03

shepmaster approved these changes Oct 8, 2019

View reviewed changes

shepmaster force-pushed the no-std branch from f81a098 to 7fbf5e3 Compare October 8, 2019 12:55

shepmaster reviewed Oct 8, 2019

View reviewed changes

shepmaster approved these changes Oct 8, 2019

View reviewed changes

shepmaster merged commit 8f85b80 into shepmaster:master Oct 8, 2019

shepmaster added enhancement New feature or request breaking change Likely requires a SemVer version bump labels Nov 6, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement Support for no_std #138

Implement Support for no_std #138

CryZe commented Aug 6, 2019 •

edited

Loading

shepmaster Aug 6, 2019

CryZe Aug 6, 2019

CryZe Aug 6, 2019 •

edited

Loading

CryZe Aug 6, 2019

CryZe Aug 6, 2019 •

edited

Loading

CryZe Aug 6, 2019 •

edited

Loading

shepmaster Aug 17, 2019

CryZe Aug 17, 2019

CryZe commented Aug 7, 2019 •

edited

Loading

CryZe commented Aug 8, 2019

CryZe commented Aug 8, 2019

CryZe commented Aug 16, 2019 •

edited

Loading

shepmaster Aug 17, 2019

CryZe Aug 17, 2019 •

edited

Loading

CryZe commented Aug 30, 2019

roblabla commented Sep 2, 2019 •

edited

Loading

CryZe commented Sep 2, 2019 •

edited

Loading

roblabla commented Sep 25, 2019

CryZe commented Sep 25, 2019 •

edited

Loading

roblabla commented Sep 25, 2019 •

edited

Loading

CryZe commented Sep 25, 2019

shepmaster left a comment

shepmaster Oct 8, 2019

CryZe Oct 8, 2019 •

edited

Loading

shepmaster commented Oct 8, 2019

shepmaster Oct 8, 2019

shepmaster commented Oct 8, 2019

Implement Support for no_std #138

Implement Support for no_std #138

Conversation

CryZe commented Aug 6, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CryZe Aug 6, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CryZe Aug 6, 2019 • edited Loading

Choose a reason for hiding this comment

CryZe Aug 6, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CryZe commented Aug 7, 2019 • edited Loading

CryZe commented Aug 8, 2019

CryZe commented Aug 8, 2019

CryZe commented Aug 16, 2019 • edited Loading

Choose a reason for hiding this comment

CryZe Aug 17, 2019 • edited Loading

Choose a reason for hiding this comment

CryZe commented Aug 30, 2019

roblabla commented Sep 2, 2019 • edited Loading

CryZe commented Sep 2, 2019 • edited Loading

roblabla commented Sep 25, 2019

CryZe commented Sep 25, 2019 • edited Loading

roblabla commented Sep 25, 2019 • edited Loading

CryZe commented Sep 25, 2019

shepmaster left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CryZe Oct 8, 2019 • edited Loading

Choose a reason for hiding this comment

shepmaster commented Oct 8, 2019

Choose a reason for hiding this comment

shepmaster commented Oct 8, 2019

CryZe commented Aug 6, 2019 •

edited

Loading

CryZe Aug 6, 2019 •

edited

Loading

CryZe Aug 6, 2019 •

edited

Loading

CryZe Aug 6, 2019 •

edited

Loading

CryZe commented Aug 7, 2019 •

edited

Loading

CryZe commented Aug 16, 2019 •

edited

Loading

CryZe Aug 17, 2019 •

edited

Loading

roblabla commented Sep 2, 2019 •

edited

Loading

CryZe commented Sep 2, 2019 •

edited

Loading

CryZe commented Sep 25, 2019 •

edited

Loading

roblabla commented Sep 25, 2019 •

edited

Loading

CryZe Oct 8, 2019 •

edited

Loading