Add forwarding impls for Read, Write, Seek to Arc, Rc

[eholk]

This adds forwarding impls for std::io::Read, std::io::Write, and std::io::Seek to alloc:sync::Arc and alloc::rc::Rc. This is relevant for types such as std::fs::File, std::net::TcpStream, and std::os::unix::UnixStream which implement Read, Write, and Seek for &T.

It is currently possible to do this manually through wrappers (See the "Implement a forwarding wrapper by hand" section for an example), but providing forwarding impls makes this pattern nicer to use. In some cases this can also be done with an extra reference, as shown below:

let stream = TcpStream::connect("localhost:8080");
let stream = Arc::new(stream);

&stream1.write(b"hello world")?; // OK: Read is available for &Arc<TcpStream>.
stream1.write(b"hello world")?;  // Error: Read is not available for Arc<TcpStream>.
                                 // (Enabled by this PR)

The reason why we want Arc<T>: Read where &T: Read is because this enables Arc<T> to be passed directly into APIs which expect T: Read:

fn operate<R: Read>(reader: R) {}

let stream = TcpStream::connect("localhost:8080");
let stream = Arc::new(stream);
operate(stream);  // Error: `Arc<TcpStream>` does not implement `Read`
                  // (Enabled by this PR)

Implement a forwarding wrapper by hand

These trait impls do not allow anything which wasn't allowed before. Dereferencing to the inner type to get Arc<T>: Read to work is already possible by creating an intermediate type, as shown here:

/// A variant of `Arc` that delegates IO traits if available on `&T`.
#[derive(Debug)]
pub struct IoArc<T>(Arc<T>);

impl<T> IoArc<T> {
    /// Create a new instance of IoArc.
    pub fn new(data: T) -> Self {
        Self(Arc::new(data))
    }
}

impl<T> Read for IoArc<T>
where
    for<'a> &'a T: Read,
{
    fn read(&mut self, buf: &mut [u8]) -> io::Result<usize> {
        (&mut &*self.0).read(buf)
    }
}

impl<T> Write for IoArc<T>
where
    for<'a> &'a T: Write,
{
    fn write(&mut self, buf: &[u8]) -> io::Result<usize> {
        (&mut &*self.0).write(buf)
    }

    fn flush(&mut self) -> io::Result<()> {
        (&mut &*self.0).flush()
    }
}

// forward `Clone`, `Seek` as well here

References

• IO trait delegation for Arc blog post
• async-dup crate
• io-arc crate

Joint work with @yoshuawuyts.

[rust-highfive]

r? @m-ou-se

(rust-highfive has picked a reviewer for you, use r? to override)

[nrc]

I think this is a good addition, but it desperately needs some documentation! I'm not sure where the best place for that is, but either on the types or the impls or somewhere. I think it needs explaining that these impls exist, what they allow, how they can be used, some examples, etc. There should also be comments (i.e., not user-facing docs) explaining why the bound on the impl is the way it is (why not T: Read, etc) and probably a comment explaining why these are on Arc and Rc, not &Arc and &Rc.

[eholk]

@nrc - thanks for the suggestion! I added some doc comments on the impls, which I think also does a reason for the bound, etc. Let me know if you think this is clear enough, and if not I can add more detail.

[nrc]

lgtm, thanks for the docs!

[m-ou-se]

@rfcbot merge

[rfcbot]

Team member @m-ou-se has proposed to merge this. The next step is review by the rest of the tagged team members:

• @Amanieu
• @BurntSushi
• @dtolnay
• @joshtriplett
• @m-ou-se
• @yaahc

Concerns:

• behavior on impls that mutate the reference (Add forwarding impls for Read, Write, Seek to Arc, Rc #93044 (comment))

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

[m-ou-se]

At first glance seems to have the same issue as pointed out here: #94744 (comment), but the implementation in this PR implicitly requires Sized, meaning that T = [u8] is not accepted.

[tbu-]

At first glance seems to have the same issue as pointed out here: #94744 (comment), but the implementation in this PR implicitly requires Sized, meaning that T = [u8] is not accepted.

Sounds like this should be explicitly documented as a pitfall as T = [u8] is probably not the only possible Read etc. implementation which might have this problem.

[dtolnay]

It's not clear to me that this PR is less problematic than #94744. The relevant distinction in #94744 was whether the impls mutate the incoming reference or not, not whether the reference is fat. Ruling out fat references as done here doesn't rule out impls which work by mutating the reference, in general.

Here is one counterexample:

use std::io::{Result, Write};

#[derive(Debug)]
enum Parity {
    Even,
    Odd,
}

impl Write for &Parity {
    fn write(&mut self, buf: &[u8]) -> Result<usize> {
        if buf.iter().map(|x| x.count_ones()).sum::<u32>() % 2 == 1 {
            match self {
                Parity::Even => *self = &Parity::Odd,
                Parity::Odd => *self = &Parity::Even,
            }
        }
        Ok(buf.len())
    }

    fn flush(&mut self) -> Result<()> {
        Ok(())
    }
}

fn main() {
    let mut parity = &Parity::Even;
    write!(parity, "###").unwrap();
    println!("{:?}", parity);  // Odd
    write!(parity, "###").unwrap();
    println!("{:?}", parity);  // Even
}

Are we saying these impls are not always correct but still useful enough in practice to include anyway?

@rfcbot concern behavior on impls that mutate the reference

[eholk]

Thanks for the example, @dtolnay. I think that's a really clear illustration of the issue. Here's what it looks like with this PR, going through an ARC:

use std::io::{Result, Write};
use std::sync::Arc;

#[derive(Debug)]
enum Parity {
    Even,
    Odd,
}

impl Write for &Parity {
    fn write(&mut self, buf: &[u8]) -> Result<usize> {
        if buf.iter().map(|x| x.count_ones()).sum::<u32>() % 2 == 1 {
            match self {
                Parity::Even => *self = &Parity::Odd,
                Parity::Odd => *self = &Parity::Even,
            }
        }
        Ok(buf.len())
    }

    fn flush(&mut self) -> Result<()> {
        Ok(())
    }
}

fn main() {
    let mut parity = Arc::new(Parity::Even);
    write!(parity, "###").unwrap();
    println!("{:?}", parity);  // Even
    write!(parity, "###").unwrap();
    println!("{:?}", parity);  // Even
}

Basically, the write! has no effect.

This feels like the sort of thing that should require unsafe to make happen, although I can see why it doesn't. It definitely looks like a footgun to me.

I'd love to find a more generic way of doing this than specializing for File, AsyncStream, and every other type that makes sense here. I guess we could add something like an InteriorMutabilityIO marker trait to make it easier to opt in, but that seems kind of ugly too.

One argument in favor of still merging this is that it doesn't do anything users can't already do on their own. That said, I think it makes sense to avoid adding known footguns to the standard library if we can avoid it.

[m-ou-se]

It definitely looks like a footgun to me.

@rfcbot abort

[dtolnay]

@rfcbot cancel

[rfcbot]

@dtolnay proposal cancelled.

[WaffleLapkin]

Since the proposal was canceled, should this PR be closed?