consensus_decode should take &mut D?

[dpc]

After taking a stab at #1019 , it seems to me that consensus_decode should take d: &mut D and not as it currently does d: D. The reason is - it is basically a wrapper around Read::read which takes &mut self.

Instead of passing &mut d when descending to sub-decoding, the code could just pass d and it would work. Internally it would avoid creating &mut &mut &mut &mut T which seems kind of undesirable, even if compiler probably can get rid of these needless references.

[apoelstra]

I could be convinced. I dislake &mut D because it's a little ugly/unergonomic and strictly speaking it's less general since &mut D is Read wheneve D is.

But you may be correct that the tendency to create giant &mut &mut ... &mut chains is even worse. In rust-miniscript we were forced to use &mut D in a lot of places where we had recursive calls, because otherwise the compiler would generate an infinite &mut chain.

In summary, I have no strong opinions here.

[dpc]

it's less general since &mut D is Read wheneve D is

Indeed d: D is more general that d : &mut D. But since the trait implementations are always based on Read::read, I would argue that it's too-general, because what we really need is &mut Read in the first place.

[dpc]

Actually, no. Scratch that. Wrong way to think about it.

We need to think separately about trait bond and function signature.

A trait bond D: Read is most general, yes. And we will keep it.

However

fn foo_ref(f: &mut Foo);

is more general from the caller perspective than:

fn foo_owned(f: Foo);

It is sometimes impossible to use (call) foo_owned, while being able to call foo_ref. And it is reversed from the implementation perspective: It is sometimes impossible to implement foo_ref, while it might be possible to implement foo_owned. This is a tradeoff between freedom/flexibility between caller and implementation.

And we know that we only ever going to need &mut D to implement it, so it's best for us to give that freedom back to the caller, because when doing recursive decoding we are also the caller. :D

Because of the current needlessly strict calling convention, we are sometimes unable to call D::consensus_decode because we need to give up ownership, while we we want to use the d again, on another field. Also, note that our escape hatch is calling with &mut d, which is changing the type of the D : Read bound on the trait to &mut D.

The above point might be very important, because even if the compiler can optimize the runtime cost of &mut &mut &mut D it probably can not easily deduplicate monorphizations of all these needless types. So we end up with fn consensus_decode<X>
, fn consensus_decode<&mut X> , fn consensus_decode<&mut &mut X> , fn consensus_decode<&mut &mut X> etc. where X are the real reader types users give us . This is increasing compilation time and/or blowing up the binary size.

As for the looks and amount of noise, apart from death and taxes, the one thing that's certain in this life is appeasing Rust compiler, so we just have to accept it. :D

[Kixunil]

I remember some Rust devs saying d: D is better but the arguments about monomorphisation sound very strong to me and I'm now in favor of d: &mut D. If someone wants to disprove it, bring it on!

[apoelstra]

I'm also very convinced by the monomorphism argument.

[dpc]

Doesn't hurt to get a second opinion: https://users.rust-lang.org/t/taking-r-mut-r-vs-mut-r-r-where-r-io-read/76780