From c98960d0536dc0849d538e24cd52038b4c26685b Mon Sep 17 00:00:00 2001 From: Ian Manske Date: Mon, 20 May 2024 13:10:36 +0000 Subject: [PATCH] Take owned `Read` and `Write` (#12909) # Description As @YizhePKU pointed out, the [Rust API guidelines](https://rust-lang.github.io/api-guidelines/interoperability.html#generic-readerwriter-functions-take-r-read-and-w-write-by-value-c-rw-value) recommend that generic functions take readers and writers by value and not by reference. This PR changes `copy_with_interupt` and few other places to take owned `Read` and `Write` instead of mutable references. --- crates/nu-command/src/filesystem/save.rs | 4 +- crates/nu-command/src/filters/tee.rs | 10 +-- .../nu-protocol/src/pipeline/byte_stream.rs | 68 ++++++++----------- 3 files changed, 36 insertions(+), 46 deletions(-) diff --git a/crates/nu-command/src/filesystem/save.rs b/crates/nu-command/src/filesystem/save.rs index 340ceb4f62..ab5257fb01 100644 --- a/crates/nu-command/src/filesystem/save.rs +++ b/crates/nu-command/src/filesystem/save.rs @@ -508,7 +508,7 @@ fn get_files( } fn stream_to_file( - mut source: impl Read, + source: impl Read, known_size: Option, ctrlc: Option>, mut file: File, @@ -555,7 +555,7 @@ fn stream_to_file( Ok(()) } } else { - copy_with_interrupt(&mut source, &mut file, span, ctrlc.as_deref())?; + copy_with_interrupt(source, file, span, ctrlc.as_deref())?; Ok(()) } } diff --git a/crates/nu-command/src/filters/tee.rs b/crates/nu-command/src/filters/tee.rs index d6decd3bc6..2251f6646a 100644 --- a/crates/nu-command/src/filters/tee.rs +++ b/crates/nu-command/src/filters/tee.rs @@ -403,8 +403,8 @@ struct StreamInfo { metadata: Option, } -fn copy(mut src: impl Read, mut dest: impl Write, info: &StreamInfo) -> Result<(), ShellError> { - copy_with_interrupt(&mut src, &mut dest, info.span, info.ctrlc.as_deref())?; +fn copy(src: impl Read, dest: impl Write, info: &StreamInfo) -> Result<(), ShellError> { + copy_with_interrupt(src, dest, info.span, info.ctrlc.as_deref())?; Ok(()) } @@ -416,8 +416,8 @@ fn copy_pipe(pipe: ChildPipe, dest: impl Write, info: &StreamInfo) -> Result<(), } fn copy_on_thread( - mut src: impl Read + Send + 'static, - mut dest: impl Write + Send + 'static, + src: impl Read + Send + 'static, + dest: impl Write + Send + 'static, info: &StreamInfo, ) -> Result>, ShellError> { let span = info.span; @@ -425,7 +425,7 @@ fn copy_on_thread( thread::Builder::new() .name("stderr copier".into()) .spawn(move || { - copy_with_interrupt(&mut src, &mut dest, span, ctrlc.as_deref())?; + copy_with_interrupt(src, dest, span, ctrlc.as_deref())?; Ok(()) }) .map_err(|e| e.into_spanned(span).into()) diff --git a/crates/nu-protocol/src/pipeline/byte_stream.rs b/crates/nu-protocol/src/pipeline/byte_stream.rs index e77c2cc855..35ce39ed31 100644 --- a/crates/nu-protocol/src/pipeline/byte_stream.rs +++ b/crates/nu-protocol/src/pipeline/byte_stream.rs @@ -541,8 +541,8 @@ impl ByteStream { /// then the [`ExitStatus`] of the [`ChildProcess`] is returned. pub fn drain(self) -> Result, ShellError> { match self.stream { - ByteStreamSource::Read(mut read) => { - copy_with_interrupt(&mut read, &mut io::sink(), self.span, self.ctrlc.as_deref())?; + ByteStreamSource::Read(read) => { + copy_with_interrupt(read, io::sink(), self.span, self.ctrlc.as_deref())?; Ok(None) } ByteStreamSource::File(_) => Ok(None), @@ -566,16 +566,16 @@ impl ByteStream { /// /// If the source of the [`ByteStream`] is [`ByteStreamSource::Child`], /// then the [`ExitStatus`] of the [`ChildProcess`] is returned. - pub fn write_to(self, dest: &mut impl Write) -> Result, ShellError> { + pub fn write_to(self, dest: impl Write) -> Result, ShellError> { let span = self.span; let ctrlc = self.ctrlc.as_deref(); match self.stream { - ByteStreamSource::Read(mut read) => { - copy_with_interrupt(&mut read, dest, span, ctrlc)?; + ByteStreamSource::Read(read) => { + copy_with_interrupt(read, dest, span, ctrlc)?; Ok(None) } - ByteStreamSource::File(mut file) => { - copy_with_interrupt(&mut file, dest, span, ctrlc)?; + ByteStreamSource::File(file) => { + copy_with_interrupt(file, dest, span, ctrlc)?; Ok(None) } ByteStreamSource::Child(mut child) => { @@ -586,11 +586,11 @@ impl ByteStream { if let Some(stdout) = child.stdout.take() { match stdout { - ChildPipe::Pipe(mut pipe) => { - copy_with_interrupt(&mut pipe, dest, span, ctrlc)?; + ChildPipe::Pipe(pipe) => { + copy_with_interrupt(pipe, dest, span, ctrlc)?; } - ChildPipe::Tee(mut tee) => { - copy_with_interrupt(&mut tee, dest, span, ctrlc)?; + ChildPipe::Tee(tee) => { + copy_with_interrupt(tee, dest, span, ctrlc)?; } } } @@ -612,14 +612,14 @@ impl ByteStream { write_to_out_dest(read, stdout, true, span, ctrlc)?; Ok(None) } - ByteStreamSource::File(mut file) => { + ByteStreamSource::File(file) => { match stdout { OutDest::Pipe | OutDest::Capture | OutDest::Null => {} OutDest::Inherit => { - copy_with_interrupt(&mut file, &mut io::stdout(), span, ctrlc)?; + copy_with_interrupt(file, io::stdout(), span, ctrlc)?; } OutDest::File(f) => { - copy_with_interrupt(&mut file, &mut f.as_ref(), span, ctrlc)?; + copy_with_interrupt(file, f.as_ref(), span, ctrlc)?; } } Ok(None) @@ -974,7 +974,7 @@ fn trim_end_newline(string: &mut String) { } fn write_to_out_dest( - mut read: impl Read, + read: impl Read, stream: &OutDest, stdout: bool, span: Span, @@ -982,12 +982,10 @@ fn write_to_out_dest( ) -> Result<(), ShellError> { match stream { OutDest::Pipe | OutDest::Capture => return Ok(()), - OutDest::Null => copy_with_interrupt(&mut read, &mut io::sink(), span, ctrlc), - OutDest::Inherit if stdout => { - copy_with_interrupt(&mut read, &mut io::stdout(), span, ctrlc) - } - OutDest::Inherit => copy_with_interrupt(&mut read, &mut io::stderr(), span, ctrlc), - OutDest::File(file) => copy_with_interrupt(&mut read, &mut file.as_ref(), span, ctrlc), + OutDest::Null => copy_with_interrupt(read, io::sink(), span, ctrlc), + OutDest::Inherit if stdout => copy_with_interrupt(read, io::stdout(), span, ctrlc), + OutDest::Inherit => copy_with_interrupt(read, io::stderr(), span, ctrlc), + OutDest::File(file) => copy_with_interrupt(read, file.as_ref(), span, ctrlc), }?; Ok(()) } @@ -1004,22 +1002,18 @@ pub(crate) fn convert_file>(file: impl Into) - const DEFAULT_BUF_SIZE: usize = 8192; -pub fn copy_with_interrupt( - reader: &mut R, - writer: &mut W, +pub fn copy_with_interrupt( + mut reader: impl Read, + mut writer: impl Write, span: Span, interrupt: Option<&AtomicBool>, -) -> Result -where - R: Read, - W: Write, -{ +) -> Result { if let Some(interrupt) = interrupt { // #[cfg(any(target_os = "linux", target_os = "android"))] // { // return crate::sys::kernel_copy::copy_spec(reader, writer); // } - match generic_copy(reader, writer, span, interrupt) { + match generic_copy(&mut reader, &mut writer, span, interrupt) { Ok(len) => { writer.flush().err_span(span)?; Ok(len) @@ -1030,7 +1024,7 @@ where } } } else { - match io::copy(reader, writer) { + match io::copy(&mut reader, &mut writer) { Ok(n) => { writer.flush().err_span(span)?; Ok(n) @@ -1044,16 +1038,12 @@ where } // Copied from [`std::io::copy`] -fn generic_copy( - reader: &mut R, - writer: &mut W, +fn generic_copy( + mut reader: impl Read, + mut writer: impl Write, span: Span, interrupt: &AtomicBool, -) -> Result -where - R: Read, - W: Write, -{ +) -> Result { let buf = &mut [0; DEFAULT_BUF_SIZE]; let mut len = 0; loop {