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.
This commit is contained in:
Ian Manske 2024-05-20 13:10:36 +00:00 committed by GitHub
parent c61075e20e
commit c98960d053
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
3 changed files with 36 additions and 46 deletions

View File

@ -508,7 +508,7 @@ fn get_files(
}
fn stream_to_file(
mut source: impl Read,
source: impl Read,
known_size: Option<u64>,
ctrlc: Option<Arc<AtomicBool>>,
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(())
}
}

View File

@ -403,8 +403,8 @@ struct StreamInfo {
metadata: Option<PipelineMetadata>,
}
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<JoinHandle<Result<(), ShellError>>, 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())

View File

@ -541,8 +541,8 @@ impl ByteStream {
/// then the [`ExitStatus`] of the [`ChildProcess`] is returned.
pub fn drain(self) -> Result<Option<ExitStatus>, 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<Option<ExitStatus>, ShellError> {
pub fn write_to(self, dest: impl Write) -> Result<Option<ExitStatus>, 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<T: From<OwnedHandle>>(file: impl Into<OwnedHandle>) -
const DEFAULT_BUF_SIZE: usize = 8192;
pub fn copy_with_interrupt<R: ?Sized, W: ?Sized>(
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<u64, ShellError>
where
R: Read,
W: Write,
{
) -> Result<u64, ShellError> {
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<R: ?Sized, W: ?Sized>(
reader: &mut R,
writer: &mut W,
fn generic_copy(
mut reader: impl Read,
mut writer: impl Write,
span: Span,
interrupt: &AtomicBool,
) -> Result<u64, ShellError>
where
R: Read,
W: Write,
{
) -> Result<u64, ShellError> {
let buf = &mut [0; DEFAULT_BUF_SIZE];
let mut len = 0;
loop {