new literacy::{Read, Write} traits to handle std/no_std

[RCasatta]

This follows the discussion in #120

introduce traits literacy::{Read, Write} that are:

• by default with std enabled automatically implemented for std::io::{Read, Write}
• in no_std with use-core2 feature implemented for core2::io::{Read, Write}
• without neither std and nor core2 user must provide implementation

This is the first proposal, an alternative one is following along:

• this implementation use literacy::Error with variants and provides the write_all blanket methods. The downside of this impl is that custom implementation can't have custom errors
• The alternative new literacy::{Read, Write} traits to handle std/no_std (alternative) #127 use associated types as error so users could provide custom errors, however the write_all implementation doesn't cover all path

Not yet sure which one is better or can be improved.
Also not sure if the approach followed in rust-bitcoin/rust-bitcoin#603 is better and make this useless, but I had already partially done this so here it is.

[devrandom]

The advantage of this is that the library user can supply their own implementation of the literacy traits, which seems desirable for no_std if they don't like depending on core2 / bare-io.

[devrandom]

Won't there be an issue compiling with the rust-bitcoin MSRV, which is 1.29? The core2 dependency has an edition = 2018 in the Cargo.toml.

[RCasatta]

my understanding is with no_std we are already not respecting MSRV 1.29, for example rust-bech32 use extern crate alloc which is available from 1.36

[devrandom]

oh, of course

[sgeisler]

Feature-gated enums are really bad because cargo uses the union set of all features in the dependency tree. E.g. if there are two deps with a rust-bitcoin dep and one rust-bitcoin has use-core and another std, cargo will compile it once with both features. That means a non-breaking change (just changing the feature set of rust-bitcoin without exposing it) in one of the deps could break exhaustive matches.

Would Box<dyn SomeNonStdErrorTrait> work here?

[RCasatta]

I thought of Box<dyn SomeNonStdErrorTrait> for the Other variant, but I am not sure what SomeNonStdErrorTrait could be, Debug? Or something created ad-hoc?

[sgeisler]

It could be a new trait defined as trait OurErrorTrait: Dsiplay + Debug + … {}, essentially just a hack to make multiple trait bounds object safe.

[RCasatta]

I think we can't avoid the feature gate on the error variant on 1.29, because you can't use Box taken from alloc on 1.29

[devrandom]

With 1.29, you must turn on std and must not turn on use-core2 anyway, so there seems to be no issue. Box only needs to be imported from alloc conditional on not(feature = "std")), which will be skipped by 1.29.

[RCasatta]

Yeah, right, I had to update the CI for that (skipping no_std for 1.29) 4727b63

[devrandom]

I'm a bit confused as to how CI was passing before, given that 1.29 can't compile core2.

[RCasatta]

It was not compiling it, features in contrib/test.sh were not updated (there were also 1 feature not existing anymore)

[thomaseizinger]

The two features are mutually exclusive right? (which by itself also doesn't play well with cargo's way of handling features)

Instead of having conditionally defined variants, you could have a single variant and conditionally define its private, inner field?

I think that should address @sgeisler's concern.

[sgeisler]

I like where this is going, I think the Any change would be super nice to have in here if possible, so not a full ACK yet. And someone should have a thorough look at the CI changes so we don't break it again (I'm not in the mood for bash right now though).

[sgeisler]

We should definitely include Any here if possible. This would allow a user that knows which error they expect (typical use case: you use either std or core2) to downcast to that error for further introspection. Maybe we'd even want to provide a helper method for that.

Also: I hope we can come up with a better name, it's not really a marker trait imo. Maybe call it LiteracyError?

[devrandom]

What is the best place for this, given that it could be used by both rust-bitcoin (for consensus encoding) and bitcoin_hashes? Should it be a new repo?

[TheBlueMatt]

What is the best place for this

Good question! I don't have any strong opinion here, just practically it could stay here as rust-bitcoin already depends on bitcoin_hashes. If we have enough to put in it a new crate isn't terrible, but given its pretty small I kinda figure just leave it here?

[devrandom]

But if it's here, then bitcoin_hashes can't use it.

[TheBlueMatt]

here, then bitcoin_hashes can't use it.

here is bitcoin_hashes?

[devrandom]

Sorry, misread that. That's fine for the time being.

[RCasatta]

What is the best place for this

I think it would be a problem having this here if libraries at the same level as bitcoin_hashes would need this trait, but at the moment I don't think we have any. For example rust-bech32 doesn't use io::{Read,Write}

[RCasatta]

someone should have a thorough look at the CI changes so we don't break it again

I think moving as much logic as possible inside github action small job instead of the contrib/test.sh files in another PR would help a lot. Having one job for each combination of rust version / feature would explode jobs but make outputs more visible (it would make warning emitted available in the web interface and also not-existing feature more immediate to see)

The downside of this it that is surely less friendly to launch in local env

[RCasatta]

closing in favor of #127