SoftbearStudios / bitcode

A binary encoder/decoder for Rust
https://crates.io/crates/bitcode/
MIT License
369 stars 19 forks source link

Implement Encode/Decode for CString #24

Open udoprog opened 7 months ago

udoprog commented 7 months ago

I've updated bitcode, and if you try and build the musli test suite right now like this:

cargo build -p tests --no-default-features --features no-rt,std,alloc,bitcode-derive
You get the following errors ``` error[E0277]: the trait bound `CString: Encode` is not satisfied in `AllocatedEncoder` --> crates\tests\src\models.rs:144:47 | 144 | #[cfg_attr(feature = "bitcode-derive", derive(bitcode::Encode, bitcode::Decode))] | ^^^^^^^^^^^^^^^ within `AllocatedEncoder`, the trait `Encode` is not implemented for `CString`, which is required by `::Encoder: Encoder` | = help: the following other types implement trait `Encode`: bool char isize i8 i16 i32 i64 i128 and 63 others note: required because it appears within the type `AllocatedEncoder` --> crates\tests\src\models.rs:144:47 | 144 | #[cfg_attr(feature = "bitcode-derive", derive(bitcode::Encode, bitcode::Decode))] | ^^^^^^^^^^^^^^^ = note: required for `::Encoder` to implement `Encoder` note: required by a bound in `bitcode::Encode::Encoder` --> C:\Users\udoprog\.cargo\registry\src\index.crates.io-6f17d22bba15001f\bitcode-0.6.0\src\derive\mod.rs:33:19 | 33 | type Encoder: Encoder; | ^^^^^^^^^^^^^ required by this bound in `Encode::Encoder` = note: this error originates in the derive macro `bitcode::Encode` (in Nightly builds, run with -Z macro-backtrace for more info) error[E0277]: the trait bound `CString: Encode` is not satisfied --> crates\tests\src\models.rs:144:47 | 144 | #[cfg_attr(feature = "bitcode-derive", derive(bitcode::Encode, bitcode::Decode))] | ^^^^^^^^^^^^^^^ the trait `Encode` is not implemented for `CString` | = help: the following other types implement trait `Encode`: bool char isize i8 i16 i32 i64 i128 and 63 others = note: this error originates in the derive macro `bitcode::Encode` (in Nightly builds, run with -Z macro-backtrace for more info) error[E0277]: the trait bound `CString: Encode` is not satisfied in `AllocatedEncoder` --> crates\tests\src\models.rs:144:47 | 144 | #[cfg_attr(feature = "bitcode-derive", derive(bitcode::Encode, bitcode::Decode))] | ^^^^^^^^^^^^^^^ within `AllocatedEncoder`, the trait `Encode` is not implemented for `CString`, which is required by `AllocatedEncoder: Sized` | = help: the following other types implement trait `Encode`: bool char isize i8 i16 i32 i64 i128 and 63 others note: required because it appears within the type `AllocatedEncoder` --> crates\tests\src\models.rs:144:47 | 144 | #[cfg_attr(feature = "bitcode-derive", derive(bitcode::Encode, bitcode::Decode))] | ^^^^^^^^^^^^^^^ note: required by a bound in `Default` --> C:\Users\udoprog\.rustup\toolchains\stable-x86_64-pc-windows-msvc\lib/rustlib/src/rust\library\core\src\default.rs:102:20 | 102 | pub trait Default: Sized { | ^^^^^ required by this bound in `Default` = note: this error originates in the derive macro `bitcode::Encode` (in Nightly builds, run with -Z macro-backtrace for more info) error[E0277]: the trait bound `CString: Encode` is not satisfied in `AllocatedEncoder` --> crates\tests\src\models.rs:144:47 | 144 | #[cfg_attr(feature = "bitcode-derive", derive(bitcode::Encode, bitcode::Decode))] | ^^^^^^^^^^^^^^^ within `AllocatedEncoder`, the trait `Encode` is not implemented for `CString`, which is required by `AllocatedEncoder: Sync` | = help: the following other types implement trait `Encode`: bool char isize i8 i16 i32 i64 i128 and 63 others note: required because it appears within the type `AllocatedEncoder` --> crates\tests\src\models.rs:144:47 | 144 | #[cfg_attr(feature = "bitcode-derive", derive(bitcode::Encode, bitcode::Decode))] | ^^^^^^^^^^^^^^^ note: required by a bound in `Encoder` --> C:\Users\udoprog\.cargo\registry\src\index.crates.io-6f17d22bba15001f\bitcode-0.6.0\src\coder.rs:27:57 | 27 | pub trait Encoder: Buffer + Default + Send + Sync { | ^^^^ required by this bound in `Encoder` = note: this error originates in the derive macro `bitcode::Encode` (in Nightly builds, run with -Z macro-backtrace for more info) error[E0277]: the trait bound `CString: Decode<'__de>` is not satisfied in `AllocatedDecoder<'__de>` --> crates\tests\src\models.rs:144:64 | 144 | #[cfg_attr(feature = "bitcode-derive", derive(bitcode::Encode, bitcode::Decode))] | ^^^^^^^^^^^^^^^ within `AllocatedDecoder<'__de>`, the trait `Decode<'__de>` is not implemented for `CString`, which is required by `>::Decoder: Decoder<'__de, Allocated>` | = help: the following other types implement trait `Decode<'a>`: > > > > > > > > and 66 others note: required because it appears within the type `AllocatedDecoder<'__de>` --> crates\tests\src\models.rs:144:64 | 144 | #[cfg_attr(feature = "bitcode-derive", derive(bitcode::Encode, bitcode::Decode))] | ^^^^^^^^^^^^^^^ = note: required for `>::Decoder` to implement `Decoder<'__de, Allocated>` note: required by a bound in `bitcode::Decode::Decoder` --> C:\Users\udoprog\.cargo\registry\src\index.crates.io-6f17d22bba15001f\bitcode-0.6.0\src\derive\mod.rs:41:19 | 41 | type Decoder: Decoder<'a, Self>; | ^^^^^^^^^^^^^^^^^ required by this bound in `Decode::Decoder` = note: this error originates in the derive macro `bitcode::Decode` (in Nightly builds, run with -Z macro-backtrace for more info) error[E0277]: the trait bound `CString: Decode<'__de>` is not satisfied --> crates\tests\src\models.rs:144:64 | 144 | #[cfg_attr(feature = "bitcode-derive", derive(bitcode::Encode, bitcode::Decode))] | ^^^^^^^^^^^^^^^ the trait `Decode<'__de>` is not implemented for `CString` | = help: the following other types implement trait `Decode<'a>`: > > > > > > > > and 66 others = note: this error originates in the derive macro `bitcode::Decode` (in Nightly builds, run with -Z macro-backtrace for more info) error[E0277]: the trait bound `CString: Decode<'__de>` is not satisfied in `AllocatedDecoder<'__de>` --> crates\tests\src\models.rs:144:64 | 144 | #[cfg_attr(feature = "bitcode-derive", derive(bitcode::Encode, bitcode::Decode))] | ^^^^^^^^^^^^^^^ within `AllocatedDecoder<'__de>`, the trait `Decode<'__de>` is not implemented for `CString`, which is required by `AllocatedDecoder<'__de>: Sized` | = help: the following other types implement trait `Decode<'a>`: > > > > > > > > and 66 others note: required because it appears within the type `AllocatedDecoder<'__de>` --> crates\tests\src\models.rs:144:64 | 144 | #[cfg_attr(feature = "bitcode-derive", derive(bitcode::Encode, bitcode::Decode))] | ^^^^^^^^^^^^^^^ note: required by a bound in `Default` --> C:\Users\udoprog\.rustup\toolchains\stable-x86_64-pc-windows-msvc\lib/rustlib/src/rust\library\core\src\default.rs:102:20 | 102 | pub trait Default: Sized { | ^^^^^ required by this bound in `Default` = note: this error originates in the derive macro `bitcode::Decode` (in Nightly builds, run with -Z macro-backtrace for more info) error[E0277]: the trait bound `CString: Decode<'__de>` is not satisfied in `AllocatedDecoder<'__de>` --> crates\tests\src\models.rs:144:64 | 144 | #[cfg_attr(feature = "bitcode-derive", derive(bitcode::Encode, bitcode::Decode))] | ^^^^^^^^^^^^^^^ within `AllocatedDecoder<'__de>`, the trait `Decode<'__de>` is not implemented for `CString`, which is required by `AllocatedDecoder<'__de>: Sync` | = help: the following other types implement trait `Decode<'a>`: > > > > > > > > and 66 others note: required because it appears within the type `AllocatedDecoder<'__de>` --> crates\tests\src\models.rs:144:64 | 144 | #[cfg_attr(feature = "bitcode-derive", derive(bitcode::Encode, bitcode::Decode))] | ^^^^^^^^^^^^^^^ note: required by a bound in `Decoder` --> C:\Users\udoprog\.cargo\registry\src\index.crates.io-6f17d22bba15001f\bitcode-0.6.0\src\coder.rs:69:55 | 69 | pub trait Decoder<'a, T>: View<'a> + Default + Send + Sync { | ^^^^ required by this bound in `Decoder` = note: this error originates in the derive macro `bitcode::Decode` (in Nightly builds, run with -Z macro-backtrace for more info) For more information about this error, try `rustc --explain E0277`. error: could not compile `tests` (lib) due to 8 previous errors ```

Overall the errors are a bit of a red herring, but first it seems like CString just isn't supported. Second it seems like you might be imposing a Send + Sync bound which might not be necessary (or at least I don't see why it should be!).

If you don't want to add CString support I can carve out a separate category for bitcode derive in my tests.

Thank you!

caibear commented 7 months ago

Overall the errors are a bit of a red herring, but first it seems like CString just isn't supported.

Yeah, I haven't added back all the features bitcode 0.5 supported CString being one of them.

Second it seems like you might be imposing a Send + Sync bound which might not be necessary (or at least I don't see why it should be!).

This is only imposed on the Encoder, not the actual type. I think the error message is misleading.

If you don't want to add CString support I can carve out a separate category for bitcode derive in my tests.

I eventually want to add missing types, but this seems like the best option for now.

udoprog commented 7 months ago

Thanks.

I do have one minor minorly related questions if you don't mind.

I can't find serde support for serializing with existing buffers, which makes the serde-based benchmarks much slower than they should be since they'd have to allocate.

Has this been removed?

caibear commented 7 months ago

I can't find serde support for serializing with existing buffers, which makes the serde-based benchmarks much slower than they should be since they'd have to allocate.

Has this been removed?

Yes for a rather complex reason. Bitcode 0.6 encoding converts any type to a Vec<primitive> per field and copies them to the output Vec<u8>. There is no metadata between the Vec<primitive>s. bitcode::serialize has to keep track of which order it learned about the fields since bitcode::deserialize will learn about the fields in the same order (in depth explanation). If bitcode kept Vec<primitive>s from previous calls to serialize, it wouldn't record the order it learned about them.

feature = derive can use reuse allocations since it knows all the fields up front.

udoprog commented 7 months ago

All right, so I'll put the serde-based tests in a separate category too :)

I'm guessing then this also extends to fields marked with #[bitcode(with_serde)], as in additional internal allocations and copying would happen?

caibear commented 7 months ago

I'm guessing then this also extends to fields marked with #[bitcode(with_serde)], as in additional internal allocations and copying would happen?

#[bitcode(with_serde)] hasn't been yet reimplemented in 0.6 since there isn't an easy way to propagate errors since encode/decode don't return result.

caibear commented 7 months ago

This is blocked on determining the serialized representation.