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

Implement Encode/Decode for CString #24

Open
udoprog opened this issue Mar 26, 2024 · 6 comments
Open

Implement Encode/Decode for CString #24

udoprog opened this issue Mar 26, 2024 · 6 comments

Comments

@udoprog
Copy link

udoprog commented Mar 26, 2024

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 `<Allocated as Encode>::Encoder: Encoder<Allocated>`
    |
    = 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 `<Allocated as Encode>::Encoder` to implement `Encoder<Allocated>`
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<Self>;
    |                   ^^^^^^^^^^^^^ 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<T: ?Sized>: 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 
`<Allocated as Decode<'__de>>::Decoder: Decoder<'__de, Allocated>`
    |
    = help: the following other types implement trait `Decode<'a>`:
              <bool as Decode<'a>>
              <char as Decode<'a>>
              <isize as Decode<'a>>
              <i8 as Decode<'a>>
              <i16 as Decode<'a>>
              <i32 as Decode<'a>>
              <i64 as Decode<'a>>
              <i128 as 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 `<Allocated as Decode<'__de>>::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>`:
              <bool as Decode<'a>>
              <char as Decode<'a>>
              <isize as Decode<'a>>
              <i8 as Decode<'a>>
              <i16 as Decode<'a>>
              <i32 as Decode<'a>>
              <i64 as Decode<'a>>
              <i128 as 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>`:
              <bool as Decode<'a>>
              <char as Decode<'a>>
              <isize as Decode<'a>>
              <i8 as Decode<'a>>
              <i16 as Decode<'a>>
              <i32 as Decode<'a>>
              <i64 as Decode<'a>>
              <i128 as 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>`:
              <bool as Decode<'a>>
              <char as Decode<'a>>
              <isize as Decode<'a>>
              <i8 as Decode<'a>>
              <i16 as Decode<'a>>
              <i32 as Decode<'a>>
              <i64 as Decode<'a>>
              <i128 as 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
Copy link
Member

caibear commented Mar 26, 2024

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
Copy link
Author

udoprog commented Mar 26, 2024

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
Copy link
Member

caibear commented Mar 26, 2024

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
Copy link
Author

udoprog commented Mar 26, 2024

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
Copy link
Member

caibear commented Mar 26, 2024

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
Copy link
Member

caibear commented Mar 28, 2024

This is blocked on determining the serialized representation.

  • lengths + bytes without nul: fast to deserialize (only have check that bytes doesn't contain any nuls).
  • bytes with nul: &'de CStr can be deserialized, but hard to deserialize quickly.

@caibear caibear changed the title CString is supported through serde, but not bitcode's own derives Implement Encode/Decode for CString Mar 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants