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

Deserializing to std::borrow::Cow<str> always allocates #1852

Closed
Vlad-Shcherbina opened this issue Nov 22, 2019 · 10 comments
Closed

Deserializing to std::borrow::Cow<str> always allocates #1852

Vlad-Shcherbina opened this issue Nov 22, 2019 · 10 comments

Comments

@Vlad-Shcherbina
Copy link

// [dependencies]
// serde_json = "1.0.41"
fn main() {
    let v = "hello";
    let s = serde_json::to_string(&v).unwrap();
    println!("{}", s);
    let v2: std::borrow::Cow<str> = serde_json::from_str(&s).unwrap();
    match v2 {
        std::borrow::Cow::Borrowed(..) => println!("borrowed"),
        std::borrow::Cow::Owned(..) => println!("owned"),
    }
}

Expected result

"hello"
borrowed

Because string hello appears in the data as is, with no escaping or reader buffer boundaries.

Actual result

"hello"
owned
@Vlad-Shcherbina
Copy link
Author

This happens because serde has a blanket impl for Cow that doesn't care about Cow::Borrowed:

serde/serde/src/de/impls.rs

Lines 1726 to 1739 in d540e72

#[cfg(any(feature = "std", feature = "alloc"))]
impl<'de, 'a, T: ?Sized> Deserialize<'de> for Cow<'a, T>
where
T: ToOwned,
T::Owned: Deserialize<'de>,
{
#[inline]
fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
where
D: Deserializer<'de>,
{
T::Owned::deserialize(deserializer).map(Cow::Owned)
}
}

If specialization was in stable it would be possible to create an impl for Cow<str> that understands borrowed strs.

@cormacrelf
Copy link

You can work around this with deserialize_with.

https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=418dd6b98dfa62d43c4cc7fa8b7ea0d6

See (and run) the test for usage.

@pickfire
Copy link

pickfire commented Feb 12, 2020

Would adding specialization for this helps? Such as for T: Sized.

@dtolnay dtolnay transferred this issue from serde-rs/json Jul 5, 2020
@chpio
Copy link

chpio commented Aug 24, 2021

Seem to work ok by putting the Cow in a newtype wrapper and adding the borrow attribute to the Cow field: https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=9ba7a398755c35aa9ad24baa7c745371

@mqudsi
Copy link

mqudsi commented Nov 18, 2022

Is the demonstration from above the only way to use deserialize_with to support both borrowed and owned deserialization?

I realized that I can ship a library w/ its standardized json format and struct definitions with #[deserialize_with = ...] annotated everywhere correctly, include tests that cover json deserialization using serde_json::from_str() that pass perfectly, then realize later that a simple deserialize_with() target that doesn't delegate work to a Visitor impl but rather just deserializes on the spot can end up failing every time if a consumer of the library tries to deserialize to the same struct using serde_json::from_reader() instead of serde_json::from_string().

@Lucretiel
Copy link

Lucretiel commented Nov 18, 2022

Why would it fail? from_reader would use visit_str instead of visit_borrowed_str, so it should still work (you'd just get all owned strings)

@mqudsi
Copy link

mqudsi commented Nov 18, 2022

That's only if you specifically implement and go through the Deserialize/Visitor trait. As I mentioned, I was asking about the simpler/less boilerplate self-contained approach of attempting to decode directly in the function named as the deserialize_with target.

@dtolnay
Copy link
Member

dtolnay commented Jul 9, 2023

I think this is the intended behavior. For better or worse, there only gets to be 1 Deserialize<'de> trait impl for Cow<'a, str>. Either that impl has a 'de: 'a bound, or it doesn't, and different common usage patterns need a deserialize_with and/or borrow attribute either way. I think the way this is currently implemented is the best tradeoff.

@dtolnay dtolnay closed this as completed Jul 9, 2023
@fogti
Copy link

fogti commented Jul 10, 2023

Can the other alternative be implemented, perhaps via a wrapper? has someone done so already? (similar to the serde-bytes stuff?; perhaps even put it into exactly that crate?)

@dtolnay
Copy link
Member

dtolnay commented Jul 10, 2023

Yeah, if it would be helpful someone could implement BorrowCow that mirrors the API of Cow but with the opposite lifetime bound for the Deserialize impl.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

8 participants