write_message now takes ownership of the Write argument

[eliaslevy]

A recent commit changed write_message in serialize.rs from

pub fn write_message<W, A>(write: &mut W, message: &message::Builder<A>) -> ::std::io::Result<()>

to

pub fn write_message<W, A>(mut write: W, message: &message::Builder<A>) -> Result<()>

I am wondering why this changed.

It means you can no longer call write_message in a loop with a reference to a Write. E.g.

pub fn transcode<I,O>(input: I, output: &mut O, packed: bool)
    -> Result<(),Box<dyn Error>>
    where I: Read, O: Write
{
    let write_message = if packed { serialize_packed::write_message } else { serialize::write_message };

    let input = BufReader::new(input);
    for line in input.lines() {
        let line = line?;
        let e: Event = serde_json::from_str(&line)?;
        let bin = e.to_capnp();
        write_message(output, &bin)?
    }
    Ok(())
}

Now gives an error:

17 | pub fn transcode<I,O>(input: I, output: &mut O, packed: bool)
   |                                             ------ move occurs because `output` has type `&mut O`, which does not implement the `Copy` trait
...
28 |         write_message(output, &bin)?
   |                       ^^^^^^ value moved here, in previous iteration of loop

[dwrensha]

There is a blanket impl like this

impl<W: Write + ?Sized> Write for &mut W { ... }

so it should work for you to do write_message(&mut output, &bin)?

[eliaslevy]

Alas, no.

If you just change the call to write_message to write_message(&mut output, &bin) you get the error:

17 | pub fn transcode<I,O>(input: I, output: &mut O, packed: bool)
   |                                 ------ help: consider changing this to be mutable: `mut output`
...
28 |         write_message(&mut output, &bin)?
   |                       ^^^^^^^^^^^ cannot borrow as mutable

If transcode is changed to transcode<I,O>(input: I, mut output: O, packed: bool) the error becomes:

28 |         write_message(&mut output, &bin)?
   |                       ^^^^^^^^^^^ mutable borrow starts here in previous iteration of loop

[dwrensha]

Wow, interesting.

I was able to replicate the error.

The error goes away if I directly use serialized_packed::write_message or serialize::write_message. There is also no error if I move this assignment into the loop:

let write_message = if packed { serialize_packed::write_message } else { serialize::write_message };

I tried manually annotating the type of the local write_message variable, but then I get errors like "one type is more general than the other".

[dwrensha]

I recommend working around this problem by doing this in the inner loop:

if packed {
   serialize_packed::write_message(output, &bin)?
} else {
   serialize::write_message(output, &bin)?
}

I'm not sure if there's a better way.

[eliaslevy]

Moving the selection into the inner loop and calling the right function directly doesn't work for similar reasons:

pub fn transcode<I,O>(input: I, output: &mut O, packed: bool)
    -> Result<(),Box<dyn Error>>
    where I: Read, O: Write
{
    let input = BufReader::new(input);
    for line in input.lines() {
        let line = line?;
        let e: Event = serde_json::from_str(&line)?;
        let bin = e.to_capnp();
        if packed { 
            serialize_packed::write_message(output, &bin)?;
        } else {
            serialize::write_message(output, &bin)?;
        };
    }
    Ok(())
}

results in

17 | pub fn transcode<I,O>(input: I, output: &mut O, packed: bool)
   |                                 ------ move occurs because `output` has type `&mut O`, which does not implement the `Copy` trait
...
27 |             serialize_packed::write_message(output, &bin)?;
   |                                             ^^^^^^ value moved here, in previous iteration of loop
28 |         } else {
29 |             serialize::write_message(output, &bin)?;
   |                                      ------ value moved here, in previous iteration of loop

The write_message takes ownership of the mutable reference, which can't be reused in the next loop. If write_message explicitly accepted a reference like it did before, it would not take ownership of the reference and it would not be a problem.

Changing the function to take ownership of the Write argument instead of passing in a reference and calling write_message via reference at the call site does work, because the references are not reused:

pub fn transcode<I,O>(input: I, mut output: O, packed: bool)
    -> Result<(),Box<dyn Error>>
    where I: Read, O: Write
{
    let input = BufReader::new(input);
    for line in input.lines() {
        let line = line?;
        let e: Event = serde_json::from_str(&line)?;
        let bin = e.to_capnp();
        if packed {
            serialize_packed::write_message(&mut output, &bin)?;
        } else {
            serialize::write_message(&mut output, &bin)?;
        };
    }
    Ok(())
}

But now the function can't be used if the caller is not willing to give up ownership of the Write argument.

Seem like a step back in usability and should probably be considered a breaking change.

[dwrensha]

Does it work to call the write functions with &mut output and to make the initial binding of output to be mut, like this?

pub fn transcode<I,O>(input: I, mut output: &mut O, packed: bool)
    -> Result<(),Box<dyn Error>>
    where I: Read, O: Write
{
    let input = BufReader::new(input);
    for line in input.lines() {
        let line = line?;
        let e: Event = serde_json::from_str(&line)?;
        let bin = e.to_capnp();
        if packed { 
            serialize_packed::write_message(&mut output, &bin)?;
        } else {
            serialize::write_message(&mut output, &bin)?;
        };
    }
    Ok(())
}

[eliaslevy]

It does! I'll admit, I am not entirely sure what is going on there.

[eliaslevy]

I suppose making output mutable allows borrowing the mutable reference a the write_message call site, so you end up with a &mut &mut O: Write and Rust automatically dereferences the chain of references.

[dwrensha]

Another thing you could do is write your own little wrapper functions, like this:

fn write_message_wrapper<W, A>(write: &mut W, message: &capnp::message::Builder<A>) -> capnp::Result<()>
 where W: capnp::Write, A: capnp::message::Allocator {
  capnp::serialize::write_message(write, message)
}

fn write_message_packed_wrapper<W, A>(write: &mut W, message: &capnp::message::Builder<A>) -> capnp::Result<()>
 where W: capnp::Write, A: capnp::message::Allocator {
  capnp::serialize_packed::write_message(write, message)
}

Then your transcode function can stay very similar to how you initially had it:

pub fn transcode<I,O>(input: I, output: &mut O, packed: bool)
    -> Result<(),Box<dyn Error>>
    where I: Read, O: Write
{
    let write_message = if packed { write_message_packed_wrapper } else { write_message_wrapper };
    let input = BufReader::new(input);
    for line in input.lines() {
        let line = line?;
        let e: Event = serde_json::from_str(&line)?;
        let bin = e.to_capnp();
        write_message(output, &bin)?;
    }
    Ok(())
}

That being said, it's unfortunate that we need to resort to this kind of workaround. When I made the change to the signature of capnp::serialize::write_message() (as part of the 0.13 release), I was under the impression that it would only make write_message() more convenient to use. Your example proves me wrong!

I wonder if this should be considered a bug in the borrow checker.

[dwrensha]

I asked about this on the rust-lang zulip: https://rust-lang.zulipchat.com/#narrow/stream/122651-general/topic/surprising.20borrow.20checker.20behavior/near/203602445

Reportedly, the reason that the new capnp::serialize::write_message() fails is that it forces the lifetime to be "early-bound", while the old version allowed it to be be "late-bound".

[dwrensha]

On Zulip, eddyb suggested this way to avoid defining wrapper functions:

pub fn transcode<I,O>(input: I, output: &mut O, packed: bool)
    -> Result<(),Box<dyn Error>>
    where I: Read, O: Write
{
    let write_message : fn(&mut O, &_) -> capnp::Result<()> =
        if packed {
            |w, m| serialize_packed::write_message(w, m)
        } else {
            |w, m| serialize::write_message(w, m)
        };
    let input = BufReader::new(input);
    for line in input.lines() {
        let line = line?;
        let e: Event = serde_json::from_str(&line)?;
        let bin = e.to_capnp();
        write_message(output, &bin)?;
    }
    Ok(())
}

[eliaslevy]

Thanks for the suggestion.

[fogti]

I think this issue is fixed and can thus be closed, right?

[eliaslevy]

I suppose? There is a work around, although the original issue still stands.

[fogti]

I would think #195 (comment) is basically the way I would recommend to solve this if it appears elsewhere (e.g. as this is the way it is handled with std::io::Write, etc. elsewhere).

[dwrensha]

I considered going back to the old type signatures in the 0.14 release, but it wasn't quite as simple as I had hoped (because of the no-std stuff added in the 0.13 release). I think it might still make sense to go back to the old signatures in a future release, if we can find a simple and minimally disruptive way to do so.