Create RefReader/RefWriter for io traits

[DaGenix]

I created RefReader and RefWriter structs that wrap a mutable reference to a Reader or Writer value. This works exactly like the ByRef struct in the iter module and allows passing a reference to a Reader or Writer to function expecting a Reader or Writer by value with the caller retaining ownership to the original value.

I also modified LimitReader to take the wrapped Reader by value instead of by reference.

@sfackler

[lilyball]

Within the &'a mut on R, there's no more need for the lifetime parameter.

[sfackler]

They aren't derived automatically because it's not possible in general (e.g. methods that take self by value). It'd also probably cause weirdness with the auto-deref method lookup.

[alexcrichton]

I'm curious if we could take the Iterator-style approach with a by_ref() method here. That would take any reader and then loan it out to a wrapped Reader, essentially implementing what you have here but through a wrapper struct instead of a direct impl.

Would that solve your use case?

[sfackler]

I would prefer an adapter type as well:

pub struct RefReader<'a, R>(&'a mut R);

impl <'a, R: Reader> Reader for RefReader<'a, R> { ... }

I think having a Reader impl for both R and &mut R will end up being confusing.

[alexcrichton]

In fact, if we added an adapter type, I would love to remove the impl of Reader for &mut R because it's kinda a weird hack.

[DaGenix]

An adaptor type sounds good to me. Should there be a new method on Reader
named by_ref() to construct an instance of it (like Iterator) or should be
be constructed via a new function? I'd prefer the former, I think, since
its less verbose.

-Palmer Cox

On Thu, Feb 13, 2014 at 3:49 PM, Alex Crichton [email protected]:

In fact, if we added an adapter type, I would love to remove the impl of
Reader for &mut R because it's kinda a weird hack.

Reply to this email directly or view it on GitHubhttps://github.com//pull/12230#issuecomment-35024192
.

[alexcrichton]

I like a method:

fn by_ref<'a>(&'a mut self) -> ByRefReader<'a> { ... }
fn by_ref<'a>(&'a mut self) -> ByRefWriter<'a> { ... }

[bill-myers]

An adapter type seems worse, since it means that it doesn't "just work", and that the codebase would be littered by adapter types for every trait where this is useful.

In fact, I'd argue that it should be derived automatically for all traits where all methods take self in a way that makes the derivation possible, and that this pull request should merely serve as a stopgap until that is implemented.

This way, anything that takes a Reader can be automatically used either by giving it one or by having it keep a reference, which seems the most intuitive course of action.

[huonw]

Having an impl for &mut R would require mut r: &mut R everywhere, instead of just r: &mut R.

[DaGenix]

@huonw: I don't quite follow your point here. If we had impl <'a, R: Reader> Reader for &'a mut R, when would additional mutability be required?

[DaGenix]

@alexcrichton Do you mean you'd like to remove impl<'a> Reader for &'a mut Reader? The impl<'a, R: Reader> Reader for &'mut R is what this pull request currently adds. Or am I confused about something.

[DaGenix]

I'll update this PR to create adapter types instead of the new impls.

[DaGenix]

I rebased the PR to create adapter structs.

However, I'm not a big fan of this solution, at least not as the basis for a more general pattern in Rust programming. When creating a decorator, I think it makes the most sense that the decorator takes the value to decorate by value. Decorators that do this have the nice property of being Send if the value that they are wrapping is also Send. Decorators that take the value to wrap by reference, however, I believe, are never Send which may be a bit limiting to their use cases. So, as the author of a decorator, I'd prefer to enable the widest possible use cases, so, I want to take the thing to wrap by value. As the user of a decorator, I'd like to be able to decide if I want the decorator to take ownership of the value that I want to wrap or if I want it to take ownership of a reference to it - thus giving me the most flexibility. Creating the adapter structs does solve these problems. However, I think it has some drawbacks:

• Its a fair amount of boilerplate. As a library author, I have to create a new struct and then add a method to my trait to instantiate that struct. And the method that needs to be added isn't really related to what my trait does, its more related to how someone might use that trait. So, the method feels a little bit out of place. One potential issue is that different library authors might end up implementing this in slightly different ways - by_ref(), byref(), RefThing::new(), etc which isn't great for consistency. The 2nd issue is that many library authors might just decide to wrap reference types exclusively - this works for many common cases and is less code, but it limits utility by making the instances non-Send.
• The syntax feels a bit weird. If I want to create a reference to something, I do: let x = &y. However, I can't pass x to a generic method that expects a type that implements one of y's traits. If I want to do that, I need to do: let x = y.by_ref(). This feels like 2nd-class syntax for doing semantically the same thing as the first. I could be wrong, but, I believe its only possible to implement by_ref() in cases where all trait method take self by reference - ie, the same set where Rust could just derive it for us and produce an error if that wasn't possible. This feels like bit of a wart for newcomers and it also feels like it will be fairly difficult to write up a set of simple guidelines for when a library author should create a by_ref() method.

So, my opinion is that this PR provides a feature that I think could be useful in the short term (its useful for me), however, it doesn't feel like a great long term solution. Having Rust derive the relevant impls automatically feels more natural.

I've tried taking a look into what would be required to have Rust derive those impls automatically, but, I don't really know where to start. If someone can point me in the right direction, and if it something that's not too intensely difficult, I'd be happy to take a stab at it.

[DaGenix]

Oh, I also addressed the comments by @kballard and removed the unnecessary lifetime parameters. Thanks for the review!

[alexcrichton]

I personally like how this turned out, but @DaGenix brings up some valid points. What do others think about this? Some thoughts I have are:

• I agree that this is some boilerplate, but it doesn't seem much more unreasonable than the boilerplate of impl<'a, T: Reader> Reader for &'a mut T { ... }.
• I don't think we have to worry about construction forms once the standard library has settled on one. It's the same worry for creating iterators, do you call it .each(), .iter(), or .iterator()? It seems that community conventions normally win-out and guide design.
• I think you're right in that the compiler can only automatically derive this with traits that have all methods as &mut and &. I also would find it very odd for the compiler to automatically derive traits in just this specific use case.

[bill-myers]

It's more unreasonable, since it introduces a redundant type and a redundant method.

Regarding automatic derivation, one could always add a #[deriving] variant.

Something to think about is whether it might make sense to forbid having traits with multiple forms of self parameters: this way, all traits would use a single form of self, and of course the by-value self version would inherit the &mut version, which would inherit the & version (and yes, it would also be illegal to inherit from traits with "stronger" self parameter methods).

Automatic derivation would then no longer have the discontinuity that traits suddenly stop being derived when you add a by-value self method to a trait made only of &self and &mut self, because adding such a method would be an error.

An equivalent alternative is to automatically construct the subtraits, so that for instance Trait[&] would refer to the methods in trait that take &self, Trait[&mut] to the & and &mut self methods, etc. (this adds more syntax but reduces redundancy).

An even more radical approach is to remove all forms of self parameter except by-value, and support the other forms by implementing the trait on &Struct, &mut Struct, ~Struct, etc.; however, this would probably be harder to learn and adds complexity in trait bounds (since you need to express "T such that &T implements Trait"), so I think it's a worse choice.

[DaGenix]

@bill-myers I really like the Trait[&], Trait[&mut] idea. It feels like it solves the problem at hand while also being minimally impactful to the language as a whole. It would be nice if Trait[&] were the default and it required extra syntax to use the version of the trait that included the by-value self methods, but that would be more impactful to existing code. Maybe it would be best to open another ticket to explore that idea (or one of your other ideas) more fully?

@alexcrichton Regarding this implementation vs introducing a new impl, I agree that its roughly the same cost regarding boilerplate. I liked the idea of having Rust automatically create the impl, but after thinking about it some more, I think I'm starting to see why that's probably not a great idea. So, if this looks good to you and it looks like a good addition to Rust, please pull this.

Thanks everyone for the reviews and comments!

[sfackler]

The lifetime on the left can be removed.

[DaGenix]

Whoops. Thanks for noticing that. I've fixed it up and rebased.

On Sat, Feb 15, 2014 at 12:36 AM, Steven Fackler
[email protected]:

In src/libstd/io/util.rs:

}

-impl<'a, R: Reader> Reader for LimitReader<'a, R> {
+impl<'a, R: Reader> Reader for LimitReader {

The lifetime on the left can be removed.

Reply to this email directly or view it on GitHubhttps://github.com//pull/12230/files#r9771560
.

[sfackler]

Thanks!

[MicahChalmer]

Short of changing the language, it's possible to create a generic ByRef traits in rust as it currently stands. Their usage isn't all that friendly, but I think it's at least interesting and relevant to the discussion here.

The generic ByRef looks like this:

mod by_ref {
    pub trait ByRef<T> {
        fn get_ref<'a>(&'a self) -> &'a T;
    }

    // If we had a "Deref" kind, then rather than having to separately
    // impl for each pointer type, then we might have something like this:
    // impl<'b,T,U:Deref<T>> ByRef<T> for U {
    //     fn by_ref<'a>(&'a self) -> &'a T { &**self }
    // }
    impl<'b,T> ByRef<T> for &'b T {
        fn get_ref<'a>(&'a self) -> &'a T { &**self }
    }
    impl<T> ByRef<T> for ~T {
        fn get_ref<'a>(&'a self) -> &'a T { &**self }
    }

    // We can't make bare objects all implement this--so we have to wrap it:
    pub fn by_val<T>(t:T) -> ByRefBare<T> { ByRefBare(t) }
    struct ByRefBare<T>(T);
    impl<T> ByRef<T> for ByRefBare<T> {
        fn get_ref<'a>(&'a self) -> &'a T { match self { &ByRefBare(ref x) => x } }
    }
}

This gives you a ByRef<T> that is implemented by pointers to T (borrowed and otherwise) as well as a wrapper around T itself that hopefully optimizes out.

Let's see how it's used--say we have this trait, and want to make a decorator for it:

trait Foo {
    fn foo(&self);
}

Let's further assume that both Foo and the by_ref module come other crates, and so we can't add impl<T:Foo> Foo for &T or impl<T:Foo> Foo for ByRef<T>. We can still write a decorator that takes either a value or a reference, without having to create trait-specific wrapper types such as RefFoo:

struct FooDecorator<R> {
    inner:R
}
impl<T:Foo,R:ByRef<T>> Foo for FooDecorator<R> {
  fn foo(&self) {
    /* do something... */
    inner.get_ref().foo();
    /* and do something else... */
  }
}

This isn't pretty, and unfortunately it uglifies the usage, not just the declaration: while you can call FooDecorator::new directly on a borrowed pointer or owned box, if you want to decorate an object by value you have to use FooDecorator::new(by_val(my_value)) rather than FooDecorator::new(my_value). Bleh. In exchange for this, you get to write the decorator without altering the decorated type, and without having to repeat all of the decorated type's methods a second time (as you have to to create a trait-specific wrapper like RefReader.)

You could also create a ByMutRef which would basically look the same as ByRef with a few muts in the right places.

For my tastes, the fact that you have to be aware of this mechanism when using the decorator, rather than just when writing it, make it seem like it's not worth it. I couldn't come up with a way to eliminate that--there's no way to tell the compiler that any bare T automatically implements ByRef. (You can write impl<T> ByRef<T> for T, but that can't coexist with the definitions for &T and ~T, and without those there's no point.) But if that problem could be solved, it might be worth considering whether something similar to this would be good to have in std and used, rather than having to write these kinds of wrappers for trait after trait.

[huonw]

FWIW, #12381 is what I was referring to when I said

Having an impl for &mut R would require mut r: &mut R everywhere, instead of just r: &mut R.