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

Derive for borrowed fields #837

Merged
merged 4 commits into from
Apr 4, 2017
Merged

Derive for borrowed fields #837

merged 4 commits into from
Apr 4, 2017

Conversation

dtolnay
Copy link
Member

@dtolnay dtolnay commented Apr 3, 2017

I am still working on polishing and documenting and testing, but I got this working and measured some 10% improvements in realistic use cases.

struct S<'a, 'b, 'c, 'd, 'e> {
    // copied from the input
    w: String,

    // &str and &[u8] are always implicitly borrowed from the input
    x: &'a str,

    // other types require an attribute to opt into borrowing
    #[serde(borrow)]
    y: Cow<'b, str>,

    // can narrow down which lifetimes should be borrowed
    #[serde(borrow = "'c + 'd")]
    z: Z<'c, 'd, 'static, 'e>,
}

The union of all borrowed lifetimes becomes the bound on the 'de lifetime of the Deserialize impl.

impl<'de: 'a+'b+'c+'d, 'a, 'b, 'c, 'd, 'e> Deserialize<'de> for S<'a, 'b, 'c, 'd, 'e>

Fixes #492.

@dtolnay dtolnay added the wip label Apr 3, 2017
@dtolnay dtolnay requested a review from oli-obk April 3, 2017 07:48
@oli-obk
Copy link
Member

oli-obk commented Apr 3, 2017

I don't get why we need the is_cow function. The user opts in with the attribute. Can't we just trust them? Or call into and let the error reporting do its thing?

@dtolnay
Copy link
Member Author

dtolnay commented Apr 3, 2017

There are effectively two different Cow<str> impls:

// the real impl
impl<'de, 'a, T: ?Sized> Deserialize<'de> for Cow<'a, T>

// implemented by borrowed_cow_str
impl<'de: 'a, 'a> Deserialize<'de> for Cow<'a, str>

They are different impls, contain different logic etc. "Trusting the user" has nothing to do with selecting which of these impls is used.

The behavior we want from a Cow<str> impl is:

  1. visit_str and visit_string copy the data
  2. visit_borrowed_str borrows the data if 'de outlives 'a
  3. visit_borrowed_str copies the data if 'de does not outlive 'a

In Rust this is impossible to achieve with a single impl because there is no way to do lifetime dispatch: "if 'de outlives 'a then this, else this." Even with specialization this is broken, like in rust-lang/rust#40582.

The alternative I considered and ruled out was to have an exact duplicate of the Deserialize trait called DeserializeBorrowed so that we can have two different impls for every type, and serde(borrow) would imply deserialize_with=DeserializeBorrowed::deserialize. I ruled this out because Cow is the only type for which you commonly want two behaviors and another trait would increase complexity much more than this approach.

@dtolnay dtolnay merged commit 7c27e10 into 1.0 Apr 4, 2017
@dtolnay dtolnay deleted the borrow branch April 4, 2017 17:51
@dtolnay dtolnay removed the wip label Apr 28, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants