Rollup merge of rust-lang#58454 - pitdicker:windows_stdio, r=alexcrichton

Refactor Windows stdio and remove stdin double buffering

I was looking for something nice and small to work on, tried to tackle a few FIXME's in Windows stdio, and things grew from there.

This part of the standard library contains some tricky code, and has changed over the years to handle more corner cases. It could use some refactoring and extra comments.

Changes/fixes:
- Made `StderrRaw` `pub(crate)`, to remove the `Write` implementations on `sys::Stderr` (used unsynchronised for panic output).
- Remove the unused `Read` implementation on `sys::windows::stdin`
- The `windows::stdio::Output` enum made sense when we cached the handles, but we can use simple functions like `is_console` now that we get the handle on every read/write
- `write` can now calculate the number of written bytes as UTF-8 when we can't write all `u16`s.
- If `write` could only write one half of a surrogate pair, attempt another write for the other because user code can't reslice in any way that would allow us to write it otherwise.
- Removed the double buffering on stdin. Documentation on the unexposed `StdinRaw` says: 'This handle is not synchronized or buffered in any fashion'; which is now true.
- `sys::windows::Stdin` now always only partially fills its buffer, so we can guarantee any arbitrary UTF-16 can be re-encoded without losing any data.
- `sys::windows::STDIN_BUF_SIZE` is slightly larger to compensate. There should be no real change in the number of syscalls the buffered `Stdin` does. This buffer is a little larger, while the extra buffer on Stdin is gone.
- `sys::windows::Stdin` now attempts to handle unpaired surrogates at its buffer boundary.
- `sys::windows::Stdin` no langer allocates for its buffer, but the UTF-16 decoding still does.

### Testing
I did some manual testing of reading and writing to console. The console does support UTF-16 in some sense, but doesn't supporting displaying characters outside the BMP.
- compile stage 1 stdlib with a tiny value for `MAX_BUFFER_SIZE` to make it easier to catch corner cases
- run a simple test program that reads on stdin, and echo's to stdout
- write some lines with plenty of ASCII and emoji in a text editor
- copy and paste in console to stdin
- return with `\r\n\` or CTRL-Z
- copy and paste in text editor
- check it round-trips

-----

Fixes rust-lang#23344. All but one of the suggestions in that issue are now implemented. the missing one is:

> * When reading data, we require the entire set of input to be valid UTF-16. We should instead attempt to read as much of the input as possible as valid UTF-16, only returning an error for the actual invalid elements. For example if we read 10 elements, 5 of which are valid UTF-16, the 6th is bad, and then the remaining are all valid UTF-16, we should probably return the first 5 on a call to `read`, then return an error, then return the remaining on the next call to `read`.

Stdin in Console mode is dealing with text directly input by a user. In my opinion getting an unpaired surrogate is quite unlikely in that case, and a valid reason to error on the entire line of input (which is probably short). Dealing with it is incompatible with an unbuffered stdin, which seems the more interesting guarantee to me.

Author: Centril

src/libstd/io/mod.rs

@@ -286,6 +286,9 @@ pub use self::stdio::{_print, _eprint};
 #[doc(no_inline, hidden)]
 pub use self::stdio::{set_panic, set_print};
 
+// Used inside the standard library for panic output.
+pub(crate) use self::stdio::stderr_raw;
+
 pub mod prelude;
 mod buffered;
 mod cursor;

src/libstd/io/stdio.rs

@@ -32,7 +32,9 @@ struct StdoutRaw(stdio::Stdout);
 ///
 /// This handle is not synchronized or buffered in any fashion. Constructed via
 /// the `std::io::stdio::stderr_raw` function.
-struct StderrRaw(stdio::Stderr);
+///
+/// Not exposed, but used inside the standard library for panic output.
+pub(crate) struct StderrRaw(stdio::Stderr);
 
 /// Constructs a new raw handle to the standard input of this process.
 ///
@@ -61,7 +63,9 @@ fn stdout_raw() -> io::Result<StdoutRaw> { stdio::Stdout::new().map(StdoutRaw) }
 ///
 /// The returned handle has no external synchronization or buffering layered on
 /// top.
-fn stderr_raw() -> io::Result<StderrRaw> { stdio::Stderr::new().map(StderrRaw) }
+///
+/// Not exposed, but used inside the standard library for panic output.
+pub(crate) fn stderr_raw() -> io::Result<StderrRaw> { stdio::Stderr::new().map(StderrRaw) }
 
 impl Read for StdinRaw {
     fn read(&mut self, buf: &mut [u8]) -> io::Result<usize> { self.0.read(buf) }

src/libstd/sys/cloudabi/stdio.rs

@@ -9,8 +9,10 @@ impl Stdin {
     pub fn new() -> io::Result<Stdin> {
         Ok(Stdin(()))
     }
+}
 
-    pub fn read(&self, _: &mut [u8]) -> io::Result<usize> {
+impl io::Read for Stdin {
+    fn read(&mut self, _buf: &mut [u8]) -> io::Result<usize> {
         Ok(0)
     }
 }
@@ -19,15 +21,17 @@ impl Stdout {
     pub fn new() -> io::Result<Stdout> {
         Ok(Stdout(()))
     }
+}
 
-    pub fn write(&self, _: &[u8]) -> io::Result<usize> {
+impl io::Write for Stdout {
+    fn write(&mut self, _buf: &[u8]) -> io::Result<usize> {
         Err(io::Error::new(
             io::ErrorKind::BrokenPipe,
             "Stdout is not connected to any output in this environment",
         ))
     }
 
-    pub fn flush(&self) -> io::Result<()> {
+    fn flush(&mut self) -> io::Result<()> {
         Ok(())
     }
 }
@@ -36,29 +40,18 @@ impl Stderr {
     pub fn new() -> io::Result<Stderr> {
         Ok(Stderr(()))
     }
+}
 
-    pub fn write(&self, _: &[u8]) -> io::Result<usize> {
+impl io::Write for Stderr {
+    fn write(&mut self, _buf: &[u8]) -> io::Result<usize> {
         Err(io::Error::new(
             io::ErrorKind::BrokenPipe,
             "Stderr is not connected to any output in this environment",
         ))
     }
 
-    pub fn flush(&self) -> io::Result<()> {
-        Ok(())
-    }
-}
-
-// FIXME: right now this raw stderr handle is used in a few places because
-//        std::io::stderr_raw isn't exposed, but once that's exposed this impl
-//        should go away
-impl io::Write for Stderr {
-    fn write(&mut self, data: &[u8]) -> io::Result<usize> {
-        Stderr::write(self, data)
-    }
-
     fn flush(&mut self) -> io::Result<()> {
-        Stderr::flush(self)
+        Ok(())
     }
 }
 

src/libstd/sys/redox/stdio.rs

@@ -8,55 +8,48 @@ pub struct Stderr(());
 
 impl Stdin {
     pub fn new() -> io::Result<Stdin> { Ok(Stdin(())) }
+}
 
-    pub fn read(&self, data: &mut [u8]) -> io::Result<usize> {
+impl io::Read for Stdin {
+    fn read(&mut self, buf: &mut [u8]) -> io::Result<usize> {
         let fd = FileDesc::new(0);
-        let ret = fd.read(data);
+        let ret = fd.read(buf);
         fd.into_raw();
         ret
     }
 }
 
 impl Stdout {
     pub fn new() -> io::Result<Stdout> { Ok(Stdout(())) }
+}
 
-    pub fn write(&self, data: &[u8]) -> io::Result<usize> {
+impl io::Write for Stdout {
+    fn write(&mut self, buf: &[u8]) -> io::Result<usize> {
         let fd = FileDesc::new(1);
-        let ret = fd.write(data);
+        let ret = fd.write(buf);
         fd.into_raw();
         ret
     }
 
-    pub fn flush(&self) -> io::Result<()> {
+    fn flush(&mut self) -> io::Result<()> {
         cvt(syscall::fsync(1)).and(Ok(()))
     }
 }
 
 impl Stderr {
     pub fn new() -> io::Result<Stderr> { Ok(Stderr(())) }
+}
 
-    pub fn write(&self, data: &[u8]) -> io::Result<usize> {
+impl io::Write for Stderr {
+    fn write(&mut self, buf: &[u8]) -> io::Result<usize> {
         let fd = FileDesc::new(2);
-        let ret = fd.write(data);
+        let ret = fd.write(buf);
         fd.into_raw();
         ret
     }
 
-    pub fn flush(&self) -> io::Result<()> {
-        cvt(syscall::fsync(2)).and(Ok(()))
-    }
-}
-
-// FIXME: right now this raw stderr handle is used in a few places because
-//        std::io::stderr_raw isn't exposed, but once that's exposed this impl
-//        should go away
-impl io::Write for Stderr {
-    fn write(&mut self, data: &[u8]) -> io::Result<usize> {
-        Stderr::write(self, data)
-    }
-
     fn flush(&mut self) -> io::Result<()> {
-        Stderr::flush(self)
+        cvt(syscall::fsync(2)).and(Ok(()))
     }
 }
 
@@ -67,5 +60,5 @@ pub fn is_ebadf(err: &io::Error) -> bool {
 pub const STDIN_BUF_SIZE: usize = ::sys_common::io::DEFAULT_BUF_SIZE;
 
 pub fn panic_output() -> Option<impl io::Write> {
-    Stderr::new().ok()
+    io::stderr_raw().ok()
 }

src/libstd/sys/sgx/stdio.rs

@@ -16,46 +16,39 @@ fn with_std_fd<F: FnOnce(&FileDesc) -> R, R>(fd: abi::Fd, f: F) -> R {
 
 impl Stdin {
     pub fn new() -> io::Result<Stdin> { Ok(Stdin(())) }
+}
 
-    pub fn read(&self, data: &mut [u8]) -> io::Result<usize> {
-        with_std_fd(abi::FD_STDIN, |fd| fd.read(data))
+impl io::Read for Stdin {
+    fn read(&mut self, buf: &mut [u8]) -> io::Result<usize> {
+        with_std_fd(abi::FD_STDIN, |fd| fd.read(buf))
     }
 }
 
 impl Stdout {
     pub fn new() -> io::Result<Stdout> { Ok(Stdout(())) }
+}
 
-    pub fn write(&self, data: &[u8]) -> io::Result<usize> {
-        with_std_fd(abi::FD_STDOUT, |fd| fd.write(data))
+impl io::Write for Stdout {
+    fn write(&mut self, buf: &[u8]) -> io::Result<usize> {
+        with_std_fd(abi::FD_STDOUT, |fd| fd.write(buf))
     }
 
-    pub fn flush(&self) -> io::Result<()> {
+    fn flush(&mut self) -> io::Result<()> {
         with_std_fd(abi::FD_STDOUT, |fd| fd.flush())
     }
 }
 
 impl Stderr {
     pub fn new() -> io::Result<Stderr> { Ok(Stderr(())) }
-
-    pub fn write(&self, data: &[u8]) -> io::Result<usize> {
-        with_std_fd(abi::FD_STDERR, |fd| fd.write(data))
-    }
-
-    pub fn flush(&self) -> io::Result<()> {
-        with_std_fd(abi::FD_STDERR, |fd| fd.flush())
-    }
 }
 
-// FIXME: right now this raw stderr handle is used in a few places because
-//        std::io::stderr_raw isn't exposed, but once that's exposed this impl
-//        should go away
 impl io::Write for Stderr {
-    fn write(&mut self, data: &[u8]) -> io::Result<usize> {
-        Stderr::write(self, data)
+    fn write(&mut self, buf: &[u8]) -> io::Result<usize> {
+        with_std_fd(abi::FD_STDERR, |fd| fd.write(buf))
     }
 
     fn flush(&mut self) -> io::Result<()> {
-        Stderr::flush(self)
+        with_std_fd(abi::FD_STDERR, |fd| fd.flush())
     }
 }
 

src/libstd/sys/unix/stdio.rs

@@ -8,55 +8,48 @@ pub struct Stderr(());
 
 impl Stdin {
     pub fn new() -> io::Result<Stdin> { Ok(Stdin(())) }
+}
 
-    pub fn read(&self, data: &mut [u8]) -> io::Result<usize> {
+impl io::Read for Stdin {
+    fn read(&mut self, buf: &mut [u8]) -> io::Result<usize> {
         let fd = FileDesc::new(libc::STDIN_FILENO);
-        let ret = fd.read(data);
+        let ret = fd.read(buf);
         fd.into_raw(); // do not close this FD
         ret
     }
 }
 
 impl Stdout {
     pub fn new() -> io::Result<Stdout> { Ok(Stdout(())) }
+}
 
-    pub fn write(&self, data: &[u8]) -> io::Result<usize> {
+impl io::Write for Stdout {
+    fn write(&mut self, buf: &[u8]) -> io::Result<usize> {
         let fd = FileDesc::new(libc::STDOUT_FILENO);
-        let ret = fd.write(data);
+        let ret = fd.write(buf);
         fd.into_raw(); // do not close this FD
         ret
     }
 
-    pub fn flush(&self) -> io::Result<()> {
+    fn flush(&mut self) -> io::Result<()> {
         Ok(())
     }
 }
 
 impl Stderr {
     pub fn new() -> io::Result<Stderr> { Ok(Stderr(())) }
+}
 
-    pub fn write(&self, data: &[u8]) -> io::Result<usize> {
+impl io::Write for Stderr {
+    fn write(&mut self, buf: &[u8]) -> io::Result<usize> {
         let fd = FileDesc::new(libc::STDERR_FILENO);
-        let ret = fd.write(data);
+        let ret = fd.write(buf);
         fd.into_raw(); // do not close this FD
         ret
     }
 
-    pub fn flush(&self) -> io::Result<()> {
-        Ok(())
-    }
-}
-
-// FIXME: right now this raw stderr handle is used in a few places because
-//        std::io::stderr_raw isn't exposed, but once that's exposed this impl
-//        should go away
-impl io::Write for Stderr {
-    fn write(&mut self, data: &[u8]) -> io::Result<usize> {
-        Stderr::write(self, data)
-    }
-
     fn flush(&mut self) -> io::Result<()> {
-        Stderr::flush(self)
+        Ok(())
     }
 }
 
@@ -67,5 +60,5 @@ pub fn is_ebadf(err: &io::Error) -> bool {
 pub const STDIN_BUF_SIZE: usize = ::sys_common::io::DEFAULT_BUF_SIZE;
 
 pub fn panic_output() -> Option<impl io::Write> {
-    Stderr::new().ok()
+    io::stderr_raw().ok()
 }

src/libstd/sys/wasm/stdio.rs

@@ -9,23 +9,27 @@ impl Stdin {
     pub fn new() -> io::Result<Stdin> {
         Ok(Stdin)
     }
+}
 
-    pub fn read(&self, data: &mut [u8]) -> io::Result<usize> {
-        Ok(ReadSysCall::perform(0, data))
+impl io::Read for Stdin {
+    fn read(&mut self, buf: &mut [u8]) -> io::Result<usize> {
+        Ok(ReadSysCall::perform(0, buf))
     }
 }
 
 impl Stdout {
     pub fn new() -> io::Result<Stdout> {
         Ok(Stdout)
     }
+}
 
-    pub fn write(&self, data: &[u8]) -> io::Result<usize> {
-        WriteSysCall::perform(1, data);
-        Ok(data.len())
+impl io::Write for Stdout {
+    fn write(&mut self, buf: &[u8]) -> io::Result<usize> {
+        WriteSysCall::perform(1, buf);
+        Ok(buf.len())
     }
 
-    pub fn flush(&self) -> io::Result<()> {
+    fn flush(&mut self) -> io::Result<()> {
         Ok(())
     }
 }
@@ -34,23 +38,16 @@ impl Stderr {
     pub fn new() -> io::Result<Stderr> {
         Ok(Stderr)
     }
-
-    pub fn write(&self, data: &[u8]) -> io::Result<usize> {
-        WriteSysCall::perform(2, data);
-        Ok(data.len())
-    }
-
-    pub fn flush(&self) -> io::Result<()> {
-        Ok(())
-    }
 }
 
 impl io::Write for Stderr {
-    fn write(&mut self, data: &[u8]) -> io::Result<usize> {
-        (&*self).write(data)
+    fn write(&mut self, buf: &[u8]) -> io::Result<usize> {
+        WriteSysCall::perform(2, buf);
+        Ok(buf.len())
     }
+
     fn flush(&mut self) -> io::Result<()> {
-        (&*self).flush()
+        Ok(())
     }
 }
 
@@ -62,7 +59,7 @@ pub fn is_ebadf(_err: &io::Error) -> bool {
 
 pub fn panic_output() -> Option<impl io::Write> {
     if cfg!(feature = "wasm_syscall") {
-        Stderr::new().ok()
+        io::stderr_raw().ok()
     } else {
         None
     }

src/libstd/sys/windows/process.rs

@@ -252,9 +252,9 @@ impl Stdio {
             // should still be unavailable so propagate the
             // INVALID_HANDLE_VALUE.
             Stdio::Inherit => {
-                match stdio::get(stdio_id) {
+                match stdio::get_handle(stdio_id) {
                     Ok(io) => {
-                        let io = Handle::new(io.handle());
+                        let io = Handle::new(io);
                         let ret = io.duplicate(0, true,
                                                c::DUPLICATE_SAME_ACCESS);
                         io.into_raw();