Reflect the half-duplex nature of pipes in their types

[jbapple]

No description provided.

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @huonw (or someone else) soon.

[rust-highfive]

Warning

• These commits modify unsafe code. Please review it carefully!

[huonw]

Nice use of the type system! I wonder if we could call the PipeImpl type Pipe and so avoid the empty struct Pipe;.

I think @aturon and/or @alexcrichton may have some thoughts about this.

[jbapple]

Thanks! I tried using Pipe as the name of the generic struct that is now named PipeImpl, but then client calls like

Pipe::open(libc::STDERR_FILENO)

have to be

Pipe::<Readable>::open(libc::STDERR_FILENO)

which means Readable and Writable have to be pub, in addition to being more verbose. It could be

PipeReader::open(libc::STDERR_FILENO)

but for issue #11047.

There are a few other ways we could avoid that empty struct.

First, we could make it a module, but then rustc warns, "module Pipe should have a snake case name such as pipe".

Second, we could promote the functions to the top level of the module (possibly renaming them to disambiguate them from other opens and from_filedescs). This doesn't seem like it's the pattern in libstd now, though.

Third, open and from_filedesc could become methods of PipePair, modulo some renaming, like open becoming open_end, open_side, or open_half.

Thoughts?

[alexcrichton]

@jbapple thanks for taking a look at these apis! They're definitely sorely in need of a redesign.

The API proposed here leaves me with many questions, and we may want to hold off this work to get a more holistic view of the entire std::io module and see what role this module plays in the grander scheme of things. Some of the questions that came to mind were:

1. The name PipeImpl is pretty nonstandard, we don't have many other instances of FooImpl in the standard library.
2. The usage of phantom types is also pretty rare, which while clever, we need to consider.
3. The reliance on this type-level distinction for I/O objects being readable or writable is pretty rare, and we would want to be sure to apply this design everywhere instead of just to this one module.
4. The questions you brought up about Pipe::open are somewhat worrying.
5. The blank struct Pipe just for a namespace is worrying.
6. The exposure of FileDesc is worrying as I don't believe it's a public type that can be constructed.

Would you be ok holding off this work for now? The std::io module is the next major blocker to tackle during stabilization, and we'd like to be sure to spearhead a design for the entire module at once instead of having each individual module go in its own direction.

[jbapple]

Yes, I don't see any need to rush into anything.

While big std::io redesign is being considered, I may make changes to address your numbered concerns in this ticket, but that should not be misinterpreted as a vote in favor of rushing into changes that might be better done holistically.

[jbapple]

This most recent change addresses the previous concerns as follows:

1. PipeImpl is still present, but not public.
2. The phantom types are gone. This leads to more duplication in the implementation, but the exposed API is simpler.
3. Not addressed. As @alexcrichton noted, this is reason to pause and reflect before moving forward.
4. open is now a method on PipeReader and PipeWriter directly.
5. The blank struct was removed.
6. The FileDesc exposure is the same as it is in the current API.

[alexcrichton]

Closing for reasons stated here (RFC coming soon)