mut borrow checker seems to be too conservative in matches

[erickt]

I've been working on a new version of the serialization framework, and I've run into an edge case with the borrow checker that is a little painful. Here's my reduced code:

pub enum Value<'self> {
    A(&'self str),
    B,
    C,
}

pub trait Decoder<'self> {
    fn read<'a>(&'a mut self) -> Value<'a>;
}

pub trait Decodable<'self, D: Decoder<'self>> {
    fn decode(d: &mut D) -> Self;
}

impl<'self, D: Decoder<'self>> Decodable<'self, D> for () {
    fn decode(d: &mut D) -> () {
        match d.read() {
            A(x) => { let _ = x; () }
            B => Decodable::decode(d),
            C => Decodable::decode(d),
        }
    }
}

This errors with:

foo.rs:19:35: 19:36 error: cannot borrow `*d` as mutable more than once at a time
foo.rs:19             B => Decodable::decode(d),
                                             ^
foo.rs:17:14: 17:15 note: second borrow of `*d` as mutable occurs here
foo.rs:17         match d.read() {
                        ^
foo.rs:20:35: 20:36 error: cannot borrow `*d` as mutable more than once at a time
foo.rs:20             C => Decodable::decode(d),
                                             ^
foo.rs:17:14: 17:15 note: second borrow of `*d` as mutable occurs here
foo.rs:17         match d.read() {
                        ^
error: aborting due to 2 previous errors

I'm not sure if this should be an error because the error is being caused by the A variant capturing the value, not the B and C variant. This is clear with this impl, which compiles fine:

impl<'self, D: Decoder<'self>> Decodable<'self, D> for () {
    fn decode(d: &mut D) -> () {
        match d.read() {
            A(*) => (),
            B => Decodable::decode(d),
            C => Decodable::decode(d),
        }
    }
}

The only way currently that I can get this to work is if I use a mini-state machine, as in:

impl<'self, D: Decoder<'self>> Decodable<'self, D> for () {
    fn decode(d: &mut D) -> () {
        enum State {
            BState,
            CState,
        }

        let state = match d.read() {
            A(x) => { let _ = x; return (); }
            B => BState,
            C => CState,
        };

        match state {
            BState => Decodable::decode(d),
            CState => Decodable::decode(d),
        }
    }
}

Which is a bit obnoxious to write when we have a large amount of value variants.

[nikomatsakis]

Dup of #6393 -- fixing this is not so easy

[erickt]

@nikomatsakis: You mentioned on IRC this might not actually get fixed once #6393 is implemented. Should this be reopened?

[nikomatsakis]

We can reopen, but we have to be more precise what you are asking for. This is kind of a subtle problem with many dimensions. I think the problem, specifically, is that returning Value<'a> says that the lifetime of the returned value is linked to the lifetime of the &mut self parameter, right? But this isn't really what you wanted, because it implies that you cannot use the self object so long as the Value is live.

If I had to guess, I'd guess that what you wanted is more like "this Value object will be freed when self goes away, but they are otherwise independent". Does that sound right?

I have encountered a similar scenario with the Arena type (described in #10444). The basic fix that I want to do there is to move the lifetime in the result into a parameter on the Arena type ('pool in that case). This is more or self what you have with 'self -- currently 'self is not used in this trait, so I don't really know what it's supposed to represent, I would have thought it represents the lifetime of the returned value. Or is it just a holdover?

[pnkfelix]

cc me

[steveklabnik]

Updated code:

pub enum Value<'a> {
    A(&'a str),
    B,
    C,
}

pub trait Decoder {
    fn read<'a>(&'a mut self) -> Value<'a>;
}

pub trait Decodable<D: Decoder> {
    fn decode(d: &mut D) -> Self;
}

impl<D: Decoder> Decodable<D> for () {
    fn decode(d: &mut D) -> () {
        match d.read() {
            Value::A(x) => { let _ = x; () }
            Value::B => Decodable::decode(d),
            Value::C => Decodable::decode(d),
        }
    }
}

fn main() {}

Is this ticket still worthwhile? I would agree with what @nikomatsakis is saying, with regards to the lifetime here.

[steveklabnik]

(Also, I'm not sure what to tag this as)

[nikomatsakis]

I am inclined to close this ticket. There are various ways we could improve the borrow checker and I don't know that having this ticket open will help us. It's not a bug in any case, more like an enhancement.