[Bug]: v0.2.1 API is incompatible with v0.2.0 API

Pagten commented 1 year ago

Is there an existing issue for this?

[X] I have searched the existing issues

Code of Conduct

[X] I agree to follow this project's Code of Conduct

Current Behaviour

In v0.2.1 the signature of ciborium::de::from_reader() changed from

pub fn from_reader<'de, T: de::Deserialize<'de>, R: Read>(reader: R) -> Result<T, Error<R::Error>>

to

pub fn from_reader<T: de::DeserializeOwned, R: Read>(reader: R) -> Result<T, Error<R::Error>>

At first sight the new bound appears to be a looser bound than the original one, because in the serde crate we have

impl<T> DeserializeOwned for T where T: for<'de> Deserialize<'de> {}

so apparently anything that implements Deserialize also implements DeserializeOwned.

That is not a correct interpretation of the above blanket implementation, however, because it actually says that only types that implement Deserialize<'de> for any lifetime 'de also implement DeserializeOwned. Hence the signature change of ciborium::de::from_reader() is not a bound loosening and should therefore be considered a major change according to SemVer.

All this is to say that I think v0.2.1 should have actually gotten the version number 0.3.0.

Expected Behaviour

The signature change of ciborium::de::from_reader() is released under a new major version of the crate.

Environment Information

/

Steps To Reproduce

No response

thill commented 7 months ago

This bug impacts me and I stumbled upon this issue. In case it helps, here's a simple example to reproduce. This compiles on =0.2.0 but not on 0.2.1

use std::borrow::Cow;

fn main() {
    #[derive(serde::Deserialize, serde::Serialize, Debug, PartialEq)]
    pub struct MyMessage<'a> {
        #[serde(borrow)]
        pub value: Cow<'a, str>,
    }

    let string: String = "hello world!".to_string();

    let my_message = MyMessage {
        value: Cow::Borrowed(&string),
    };

    let mut cbor = Vec::new();
    ciborium::ser::into_writer(&my_message, &mut cbor).unwrap();

    let decoded = ciborium::de::from_reader(cbor.as_slice()).unwrap();
    assert_eq!(my_message, decoded);
}

error: implementation of `Deserialize` is not general enough
  --> src/main.rs:19:19
   |
19 |     let decoded = ciborium::de::from_reader(cbor.as_slice()).unwrap();
   |                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ implementation of `Deserialize` is not general enough
   |
   = note: `MyMessage<'_>` must implement `Deserialize<'0>`, for any lifetime `'0`...
   = note: ...but `MyMessage<'_>` actually implements `Deserialize<'1>`, for some specific lifetime `'1`

rjzak commented 7 months ago

This seems to be an issue with how I did the release, I should have bumped the version to 0.3. How should this be corrected?

For context, this came from this PR: https://github.com/enarx/ciborium/pull/46 related to this issue https://github.com/enarx/ciborium/issues/45.

thill commented 7 months ago

Ah, I see. So Cow in my case was always copying instead of borrowing, which means it is owned anyways. In that case, yes, DeserializeOwned is appropriate to fix #45, but it probably should have been released as 0.3. That can't be undone, so maybe y'all should just consider some explicit documentation about this limitation.

Regardless of this issue and the #45 issue, I think it's important to call out the "no-borrowing" limitation of this crate early in the docs. Many other serde implementations do support zero-copy &str deserialization, including the old serde_cbor crate. Folks coming in expecting to be able to "just use cbor" in place of their existing serde-compatible format may have troubles if their codebase is already relying on a lot of borrow semantics to save time coping strings during deserialization workflows.

thill commented 7 months ago

I'm not asking anyone to do any work here, but would y'all consider a ciborium::de::from_slice PR in the future that addresses this limitation? Or is your intention to only ever support the current reader and writer semantics?

rjzak commented 7 months ago

PRs welcome! Would this be an additional function? We're happy to accept changes which make this crate more useful, we aren't set on doing things in a specific way, I don't think.

thill commented 7 months ago

Would this be an additional function?

I believe that that would be required given the current dependence on the Read trait in from_reader. More expressive borrowing is needed.

enarx / ciborium