-
Notifications
You must be signed in to change notification settings - Fork 760
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
Cow
Deserialize
impl seems wasteful
#1497
Comments
Your suggested impl does not compile because Cow::Borrowed contains &T, not T. error[E0631]: type mismatch in function arguments
|
| T::deserialize(deserializer).map(Cow::Borrowed)
| ^^^
| |
| expected signature of `fn(T) -> _`
| found signature of `fn(&_) -> _` |
I agree that it's not ideal, but it's the best "default" for For derives, we have |
Here's an example use std::borrow::Cow;
use std::fmt;
use serde::de::{self, Deserialize, Deserializer, Visitor};
struct Borrowable<'a, T: ?Sized + ToOwned>(Cow<'a, T>);
impl<'a, 'de: 'a> Deserialize<'de> for Borrowable<'a, str> {
fn deserialize<D>(de: D) -> Result<Self, D::Error>
where
D: Deserializer<'de>,
{
struct MyVisitor;
impl<'de> Visitor<'de> for MyVisitor {
type Value = Borrowable<'de, str>;
fn expecting(&self, f: &mut fmt::Formatter) -> fmt::Result {
write!(f, "an owned or borrowed string")
}
fn visit_borrowed_str<E>(self, v: &'de str) -> Result<Self::Value, E>
where
E: de::Error,
{
Ok(Borrowable(Cow::Borrowed(v)))
}
fn visit_str<E>(self, v: &str) -> Result<Self::Value, E>
where
E: de::Error,
{
Ok(Borrowable(Cow::Owned(v.to_string())))
}
fn visit_string<E>(self, v: String) -> Result<Self::Value, E>
where
E: de::Error,
{
Ok(Borrowable(Cow::Owned(v)))
}
}
de.deserialize_str(MyVisitor)
}
} |
Just to clarify the point about generic implementations - you might try to implement it like this: impl<'a, 'de: 'a, T> Deserialize<'de> for Borrowable<'a, T>
where
T: ?Sized + ToOwned,
&'a T: Deserialize<'de>,
T::Owned: Deserialize<'de>,
{
fn deserialize<D>(de: D) -> Result<Self, D::Error>
where
D: Deserializer<'de>,
{
<&T>::deserialize(de)
.map(|x| Borrowable(Cow::Borrowed(x)))
.or_else(|_| <T::Owned>::deserialize(de)
.map(|x| Borrowable(Cow::Owned(x))))
}
} This fails with an error: |
If at some point in the future, Rust has really powerful specialization support (such that even lifetime specialization works), serde could provide an In the meantime you can use As a replacement for calling |
FYI, there's a crate with helper borrowing Cow. The difference between |
The Deserialize impl proposed above in #1497 (comment) is already present in core Serde if you opt in to it using Line 59 in 7aa4950
The problem with making it the default impl is that unfortunately when a read stream is used as the data source, this impl fails to compile. For example, I took the impl from the comment above and wrote the following test, and the test fails to compile: #[test]
fn test_borrow() {
let json_buf = "\"Hello World\"".to_string();
let data_new: Borrowable<str> = serde_json::from_reader(json_buf.as_bytes())
.ok()
.unwrap_or(Borrowable(Cow::Borrowed("fallback")));
assert!(matches!(data_new.0, Cow::Borrowed("Hello World")));
} Error message:
|
The
Cow
Deserialize
implementation seems wasteful:Deserialize
is implemented for every type instd
that implementsToOwned
. Rather than deserializing into theToOwned::Owned
type, shouldn't it just directly deserialize the borrowed type so as not to perform unnecessary clones? Something like:The text was updated successfully, but these errors were encountered: