Consider taking writers by value

[HadrienG2]

&mut T where T: Write implements Write, so there is no reason to ask for a borrowed writer in the API. But this is unlikely to be a problem in practice because owned writers are rarely used.

[frankmcsherry]

This seems very plausible! I'll check it out.

[frankmcsherry]

Investigation suggests that while this is fine for encode, it could be a bit gross for the internals. The reason being if we accept a mut write: W and need to use it multiple times, we would need to invoke recursive calls with &mut write to avoid transferring ownership. This results in a recursive call with the write implementor &mut W, and each bit of structural recursion layers on a new &mut.

This seems like it would confound structural recursion through tree-structured data, e.g.

struct Tree {
    children: Vec<Tree>,
}

[HadrienG2]

Let's not do this then, as I said it's not a big issue ;)

[HadrienG2]

Wait. Given that &mut W implements Write since W implements Write, what prevents you from sending around an &mut writer while traversing a tree ?

[frankmcsherry]

I think that is what happens now, isn't it? We could allow encode to take a mut writer: W but we probably (?) want entomb to continue to take a writer: &mut W. Maybe I mis-understand what you are proposing though.

[HadrienG2]

To clarify, I meant that because &mut W implements Write just like owned writers, this code is legal:

pub fn takes_write_impl(_: impl Write) {}

pub fn also_takes_write_impl(mut w: impl Write) {
    takes_write_impl(&mut w);
    takes_write_impl(w);
}

Therefore, there is no problem with accepting "owned" writers, and using them multiple times.

[frankmcsherry]

Yup, I agree. However,

pub fn takes_write_impl(mut w: impl std::io::Write) {
    takes_write_impl(&mut w)
}

fn main() {
    let mut bytes = Vec::new();
    takes_write_impl(&mut bytes);
}

trips a recursion limit

error: reached the recursion limit while instantiating `takes_write_impl::<&mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut &mut std::vec::Vec<u8>>`
 --> src/main.rs:1:1
  |
1 | / pub fn takes_write_impl(mut w: impl std::io::Write) {
2 | |     takes_write_impl(&mut w)
3 | | }
  | |_^

error: aborting due to previous error

This is the problem I think exists, as we seem to need to use &mut writer in the body of many of the writing methods (edit: which may recursively call themselves)

[HadrienG2]

Oh, I see. This could cause problems with deep object trees indeed, especially as it also implies that the compiler is generating O(N) copies of take_write_impl when using this pattern, which is a compilation time liability.

Then I agree with you: encode can take a writer by value, but entomb shouldn't.