Return HRTB type

code review
1

#hrtb #polonius

I tried to fix this issue.
But I'm not familiar with HRTB, I really don't know whether use HRTB in return type is nice.
I'm not sure my fix is correct.

Anyone take a code review ?

2

As I understand it, it will be unnecessarily restrictive. Substitute 'static into hrtb and you'll see. You basically downgraded if to DeserializeOwned.

And in general, you've changed OP's code. What you provided is not equivalent to their code, thus it seems more like off topic.

By the way, if you want to contribute, please don't just paste the code without explanations and even a syntax highlight. This is a basic respect.

1 Like
3

Is it possible to use Refcell to deal with this issue ?

4

If it's acceptable to change Foo and its methods, etc, yes.

Here's some discussion on what's going on.

This bound:

    fn read<'c, P>(&'c mut self) -> P
    where
        P: serde::Deserialize<'c>,

says that P reborrows *self for the duration 'c. During that duration, the &mut Self (and the Self) cannot be used. This is because &mut _ borrows are exclusive borrows. It's not possible to pass some smaller borrow duration to serde because you need to return a P, and P only meets a Deserialize<'c> bound.

read gets called from read_non_empty, which has a &'c mut self and needs to return P, just like read does. It can only call read with the full borrow duration 'c, which is longer than the function body.

So after one call to read in read_non_empty, self becomes unusable. Thus it's impossible to call it in a loop.

By changing the methods to take &self instead of &mut self, we're moving from exclusive borrows to shared borrows. You still have to pass in a &'c Self with the full outer lifetime due to the Deserialize<'c> bound, but now it's okay to do that as many times as you want, because your &'c Self is a shared borrow and it implements Copy too.

Depending on the details, though, I might instead try to wrap P in a type that returns a deserialize error when it would be empty and deserialize that instead. Unfortunately I don't have time to give it a shot right now.

1 Like
5

This topic was automatically closed 90 days after the last reply. We invite you to open a new topic if you have further questions or comments.