Use core::error instead of std::error to increase compatibility in no_std environments

[OverOrion]

There is a thiserror fork called thiserror-core which allows it to be used in no_std environments.

I added a new feature flag core-error which enables the currently not yet stabilized error_in_core feature.
This enables the thiserror-core crate to work fine.

[apoelstra]

So, as written this is definitely going to fail CI because the error_in_core feature is nightly-only.

I can understand the desire to have this, but adding a feature flag that requires a nightly compiler is a tough pill for us to swallow ... it means that we have to explicitly list all the non-nightly features rather than using --all-features in our CI runs, and it will mean continual CI breakage as this unstable API changes out from under us.

[OverOrion]

Thank you for the quick and open feedback.

I understand your concerns regarding nightly only features. Is there anything we could do to help with it?

it means that we have to explicitly list all the non-nightly features rather than using --all-features

I am more than happy to rewrite the feature gate like this:

-#![cfg_attr(feature = "core-error", feature(error_in_core))]
+#![cfg_attr(all(feature = "core-error", not(feature = "std")), feature(error_in_core))]

So this would only be used if explicitly enabled without the std feature. Is it something that would helpful?

The error_in_core PR seems to be pretty stable, or at least there is not much complaints on its issue page, so in theory it should not mean continual CI breakage.

[apoelstra]

So this would only be used if explicitly enabled without the std feature. Is it something that would helpful?

This is a cool idea, but unfortunately it would then break additivity of features -- meaning that if somebody included secp256k1 with the core-error feature and then another dependency in the tree included it with std, Cargo would enable both and cause the first party's code to break.

[Kixunil]

If we're going to use this, I'd want the feature to be named unstable-nightly-core-error and document the limitations.

[Kixunil]

Why duplicate this? You could just modify the condition above. Oh, brainfart, it's obvious. You could just cfg use std::error::Error as StdError/use core::error::Error as StdError and change impl to say StdError.

[Kixunil]

The error_in_core PR seems to be pretty stable

I don't think so. There's whole new provider API that's being discussed and I'm not sure core::error::Error can be stabilized without it.

@apoelstra I'm not sure it'd break additivity since if the feature is enabled presumably the trait in std is the same as in core so they should be interoperable. But someone would have to test that.

[OverOrion]

Thanks for the feedback @Kixunil.

Just for clarification: are you willing to merge it if the requested changes has been made?

[Kixunil]

I wouldn't be opposed to it but it's not my call. Once it stabilizes we can conditionally enable it from build script as we do for other things, which would be nicer.

[apoelstra]

I wonder if we can conditionally enable it now somehow ... in that case I'd be fine with it (assuming also that we did the StdError thing to make sure that we aren't duplicating code, since duplicated code I worry will come out of sync and cause additivity problems).

[Kixunil]

In principle we could detect nightly but we shouldn't because it'll break if they decide to change the API or something. Opting into instability should be explicit. (That's why I asked to rename the feature.)

[apoelstra]

@Kixunil could we do both (require a feature, but disable/override it on non-nightly rather than running into compiler errors)?

[Kixunil]

If someone actually depended on it they'd get error much later in compilation which is not nice. I'd prefer we just avoid using --all-features in that case.