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.

Sign up for GitHub

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

feat: support deserializing types that borrow data in from_str #505

Closed
wants to merge 5 commits into from
Closed

feat: support deserializing types that borrow data in from_str #505

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
86f251c
feat: support deserializing types that borrow data in from_str
AlexTMjugador Jan 31, 2023
44563c2
test: add test for borrowed data
AlexTMjugador Jan 31, 2023
8bfba74
chore: run cargo fmt
AlexTMjugador Jan 31, 2023
d742abe
chore: reflect bound relaxation changes on toml_edit as well
AlexTMjugador Jan 31, 2023
5b9b6d8
chore(test): add #[serde(borrow)] attribute to borrowed_data test str…
AlexTMjugador Jan 31, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions crates/toml/src/de.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,9 +38,9 @@
/// assert_eq!(config.owner.name, "Lisa");
/// ```
#[cfg(feature = "parse")]
pub fn from_str<T>(s: &'_ str) -> Result<T, Error>
pub fn from_str<'a, T>(s: &'a str) -> Result<T, Error>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a test for borrowed data to ensure this works.

If it does work, then more investigation is needed because it shouldn't according to the serde docs. &str gets parsed into owned types and then we deserialize that.

Copy link
Author

@AlexTMjugador AlexTMjugador Jan 31, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've just added the test in 44563c2, thanks!

I think that the tested code works because Strings, being an owned type, have a implicit 'static lifetime bound (they can live indefinitely long until dropped). Cows support storing owned data, so a valid lifetime for a Cow that either stores owned data or static strings is Cow<'static, str>. Due to lifetime coercion, a 'static lifetime can be used whenever a shorter lifetime is expected, such as 'de in this case: the lifetime of the serialized data source.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is that sufficient if the user explicitly declared the field as borrow?

What about if the user isn't using Cow?

Copy link
Author

@AlexTMjugador AlexTMjugador Jan 31, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is that sufficient if the user explicitly declared the field as borrow?

Yes, it is. I've pushed a commit that adds that attribute to the field to show that it works too.

What about if the user isn't using Cow?

When using &'a str in the new borrowed_data test, a TomlError is returned with message invalid type: string "bar", expected a borrowed string, which makes sense, because the deserializer does not borrow strings, and it can't move the owned string it deserializes to a reference in the struct.

However, this is not a new error condition: it can already be reproduced by using T::deserialize(toml::de::Deserializer::new(s)).

Morally, I see some value in accepting these kind of structs from users: TOML may just be one of several formats they want to (de)serialize it from, and other formats properly support zero-copy deserialization. Newcomers to serde are very unlikely to mess around with zero-copy deserialization anyway.

Edit: I think it may be useful to point out that serde's author approved these "magical" Cow properties on several obscure issues on its repo: serde-rs/serde-rs.github.io#46, serde-rs/serde-rs.github.io#57. Logically, if Cows are meant to support maybe owned, maybe borrowed data, their lifetime must not be 'static, but somehow derived from the deserializer lifetime, so structs that contain them and want to take advantage of this cannot implement DeserializeOwned.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First, this isn't a morality discussion.

Second, we are shifting a compile time error into a runtime error for the easy path while, as you pointed out, a workaround is available for those that want to borrow. In API design, we should be emphasizing the safe path with the easy path while ideally still supporting the unsafe route. The current solutions fits that to a T. I don't know why the serde docs make the recommendation they do but in this case, I see it being useful.

I'm going to go ahead and close this and ask any further discussion to happen in the appropriate issue (#490).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change should be reflected in toml_edit as well

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

where
T: serde::de::DeserializeOwned,
T: serde::de::Deserialize<'a>,
Comment on lines +41 to +43
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change should be semver-compatible according to my best interpretation of the API evolution RFC.

This changes the trait bounds from a more general one to a more specific one which is a breaking change

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the review! 😄 Isn't it the reverse, though? DeserializeOwned is syntax sugar for the higher-ranked trait bound for<'de> Deserialize<'de>, so I'd argue that DeserializeOwned is more specific. The HRTB's RFC seems to support this, stating that:

[...] if I have a trait reference like for<'a> FnMut(&'a int), that would be usable wherever a trait reference with a concrete lifetime, like FnMut(&'static int), is expected.

{
T::deserialize(Deserializer::new(s))
}
Expand Down
4 changes: 2 additions & 2 deletions crates/toml/tests/testsuite/enum_external_deserialize.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,9 @@ struct Multi {
enums: Vec<TheEnum>,
}

fn value_from_str<T>(s: &'_ str) -> Result<T, toml::de::Error>
fn value_from_str<'a, T>(s: &'a str) -> Result<T, toml::de::Error>
where
T: serde::de::DeserializeOwned,
T: serde::de::Deserialize<'a>,
{
T::deserialize(toml::de::ValueDeserializer::new(s))
}
Expand Down
4 changes: 2 additions & 2 deletions crates/toml/tests/testsuite/spanned.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,9 +37,9 @@ fn test_spanned_field() {
foo: T,
}

fn good<T>(s: &str, expected: &str, end: Option<usize>)
fn good<'a, T>(s: &'a str, expected: &str, end: Option<usize>)
where
T: serde::de::DeserializeOwned + Debug + PartialEq,
T: serde::de::Deserialize<'a> + Debug + PartialEq,
{
let foo: Foo<T> = toml::from_str(s).unwrap();

Expand Down