From 7527409298ad88ae2fa6c5d517ed1730494ff2c0 Mon Sep 17 00:00:00 2001 From: marmeladema Date: Thu, 21 May 2020 20:22:47 +0100 Subject: [PATCH 01/49] Constify most non-trait `Duration` methods as described in #72440 --- src/libcore/lib.rs | 1 + src/libcore/time.rs | 47 ++++++++++++-------- src/test/ui/consts/duration-consts-2.rs | 58 +++++++++++++++++++++++++ 3 files changed, 87 insertions(+), 19 deletions(-) create mode 100644 src/test/ui/consts/duration-consts-2.rs diff --git a/src/libcore/lib.rs b/src/libcore/lib.rs index 7d21f9a9a66d0..0524a3028c07b 100644 --- a/src/libcore/lib.rs +++ b/src/libcore/lib.rs @@ -94,6 +94,7 @@ #![feature(custom_inner_attributes)] #![feature(decl_macro)] #![feature(doc_cfg)] +#![feature(duration_consts_2)] #![feature(extern_types)] #![feature(fundamental)] #![feature(intrinsics)] diff --git a/src/libcore/time.rs b/src/libcore/time.rs index ed1d5d46db5c4..7813d55dcaf51 100644 --- a/src/libcore/time.rs +++ b/src/libcore/time.rs @@ -130,10 +130,12 @@ impl Duration { /// ``` #[stable(feature = "duration", since = "1.3.0")] #[inline] - #[rustc_const_stable(feature = "duration_consts", since = "1.32.0")] - pub fn new(secs: u64, nanos: u32) -> Duration { - let secs = - secs.checked_add((nanos / NANOS_PER_SEC) as u64).expect("overflow in Duration::new"); + #[rustc_const_unstable(feature = "duration_consts_2", issue = "72440")] + pub const fn new(secs: u64, nanos: u32) -> Duration { + let secs = match secs.checked_add((nanos / NANOS_PER_SEC) as u64) { + Some(secs) => secs, + None => panic!("overflow in Duration::new"), + }; let nanos = nanos % NANOS_PER_SEC; Duration { secs, nanos } } @@ -393,7 +395,8 @@ impl Duration { /// ``` #[stable(feature = "duration_checked_ops", since = "1.16.0")] #[inline] - pub fn checked_add(self, rhs: Duration) -> Option { + #[rustc_const_unstable(feature = "duration_consts_2", issue = "72440")] + pub const fn checked_add(self, rhs: Duration) -> Option { if let Some(mut secs) = self.secs.checked_add(rhs.secs) { let mut nanos = self.nanos + rhs.nanos; if nanos >= NANOS_PER_SEC { @@ -428,7 +431,8 @@ impl Duration { /// ``` #[stable(feature = "duration_checked_ops", since = "1.16.0")] #[inline] - pub fn checked_sub(self, rhs: Duration) -> Option { + #[rustc_const_unstable(feature = "duration_consts_2", issue = "72440")] + pub const fn checked_sub(self, rhs: Duration) -> Option { if let Some(mut secs) = self.secs.checked_sub(rhs.secs) { let nanos = if self.nanos >= rhs.nanos { self.nanos - rhs.nanos @@ -464,19 +468,19 @@ impl Duration { /// ``` #[stable(feature = "duration_checked_ops", since = "1.16.0")] #[inline] - pub fn checked_mul(self, rhs: u32) -> Option { + #[rustc_const_unstable(feature = "duration_consts_2", issue = "72440")] + pub const fn checked_mul(self, rhs: u32) -> Option { // Multiply nanoseconds as u64, because it cannot overflow that way. let total_nanos = self.nanos as u64 * rhs as u64; let extra_secs = total_nanos / (NANOS_PER_SEC as u64); let nanos = (total_nanos % (NANOS_PER_SEC as u64)) as u32; - if let Some(secs) = - self.secs.checked_mul(rhs as u64).and_then(|s| s.checked_add(extra_secs)) - { - debug_assert!(nanos < NANOS_PER_SEC); - Some(Duration { secs, nanos }) - } else { - None + if let Some(s) = self.secs.checked_mul(rhs as u64) { + if let Some(secs) = s.checked_add(extra_secs) { + debug_assert!(nanos < NANOS_PER_SEC); + return Some(Duration { secs, nanos }); + } } + None } /// Checked `Duration` division. Computes `self / other`, returning [`None`] @@ -497,7 +501,8 @@ impl Duration { /// ``` #[stable(feature = "duration_checked_ops", since = "1.16.0")] #[inline] - pub fn checked_div(self, rhs: u32) -> Option { + #[rustc_const_unstable(feature = "duration_consts_2", issue = "72440")] + pub const fn checked_div(self, rhs: u32) -> Option { if rhs != 0 { let secs = self.secs / (rhs as u64); let carry = self.secs - secs * (rhs as u64); @@ -523,7 +528,8 @@ impl Duration { /// ``` #[stable(feature = "duration_float", since = "1.38.0")] #[inline] - pub fn as_secs_f64(&self) -> f64 { + #[rustc_const_unstable(feature = "duration_consts_2", issue = "72440")] + pub const fn as_secs_f64(&self) -> f64 { (self.secs as f64) + (self.nanos as f64) / (NANOS_PER_SEC as f64) } @@ -540,7 +546,8 @@ impl Duration { /// ``` #[stable(feature = "duration_float", since = "1.38.0")] #[inline] - pub fn as_secs_f32(&self) -> f32 { + #[rustc_const_unstable(feature = "duration_consts_2", issue = "72440")] + pub const fn as_secs_f32(&self) -> f32 { (self.secs as f32) + (self.nanos as f32) / (NANOS_PER_SEC as f32) } @@ -707,7 +714,8 @@ impl Duration { /// ``` #[unstable(feature = "div_duration", issue = "63139")] #[inline] - pub fn div_duration_f64(self, rhs: Duration) -> f64 { + #[rustc_const_unstable(feature = "duration_consts_2", issue = "72440")] + pub const fn div_duration_f64(self, rhs: Duration) -> f64 { self.as_secs_f64() / rhs.as_secs_f64() } @@ -724,7 +732,8 @@ impl Duration { /// ``` #[unstable(feature = "div_duration", issue = "63139")] #[inline] - pub fn div_duration_f32(self, rhs: Duration) -> f32 { + #[rustc_const_unstable(feature = "duration_consts_2", issue = "72440")] + pub const fn div_duration_f32(self, rhs: Duration) -> f32 { self.as_secs_f32() / rhs.as_secs_f32() } } diff --git a/src/test/ui/consts/duration-consts-2.rs b/src/test/ui/consts/duration-consts-2.rs new file mode 100644 index 0000000000000..e111cf1582bb6 --- /dev/null +++ b/src/test/ui/consts/duration-consts-2.rs @@ -0,0 +1,58 @@ +// run-pass + +#![feature(const_panic)] +#![feature(const_if_match)] +#![feature(duration_consts_2)] +#![feature(div_duration)] + +use std::time::Duration; + +fn duration() { + const ZERO : Duration = Duration::new(0, 0); + assert_eq!(ZERO, Duration::from_secs(0)); + + const ONE : Duration = Duration::new(0, 1); + assert_eq!(ONE, Duration::from_nanos(1)); + + const MAX : Duration = Duration::new(u64::MAX, 1_000_000_000 - 1); + + const MAX_ADD_ZERO : Option = MAX.checked_add(ZERO); + assert_eq!(MAX_ADD_ZERO, Some(MAX)); + + const MAX_ADD_ONE : Option = MAX.checked_add(ONE); + assert_eq!(MAX_ADD_ONE, None); + + const ONE_SUB_ONE : Option = ONE.checked_sub(ONE); + assert_eq!(ONE_SUB_ONE, Some(ZERO)); + + const ZERO_SUB_ONE : Option = ZERO.checked_sub(ONE); + assert_eq!(ZERO_SUB_ONE, None); + + const ONE_MUL_ONE : Option = ONE.checked_mul(1); + assert_eq!(ONE_MUL_ONE, Some(ONE)); + + const MAX_MUL_TWO : Option = MAX.checked_mul(2); + assert_eq!(MAX_MUL_TWO, None); + + const ONE_DIV_ONE : Option = ONE.checked_div(1); + assert_eq!(ONE_DIV_ONE, Some(ONE)); + + const ONE_DIV_ZERO : Option = ONE.checked_div(0); + assert_eq!(ONE_DIV_ZERO, None); + + const MAX_AS_F32 : f32 = MAX.as_secs_f32(); + assert_eq!(MAX_AS_F32, u64::MAX as f32 + 0.000_000_000_1); + + const MAX_AS_F64 : f64 = MAX.as_secs_f64(); + assert_eq!(MAX_AS_F64, u64::MAX as f64 + 0.000_000_000_1); + + const ONE_AS_F32 : f32 = ONE.div_duration_f32(ONE); + assert_eq!(ONE_AS_F32, 1.0_f32); + + const ONE_AS_F64 : f64 = ONE.div_duration_f64(ONE); + assert_eq!(ONE_AS_F64, 1.0_f64); +} + +fn main() { + duration(); +} From 37570e82baf12951f8bb408884e31640b0d29dae Mon Sep 17 00:00:00 2001 From: Nathan West Date: Thu, 28 May 2020 22:07:13 -0400 Subject: [PATCH 02/49] Substantial refactor to the design of LineWriter This commit redesigns LineWriter to work more directly on the internals of BufWriter. This interface change is to enable a future Pull Request in which Stdout can be switched between Line and Block buffered mode. --- src/libstd/io/buffered.rs | 352 ++++++++++++++++++++++++++++---------- 1 file changed, 259 insertions(+), 93 deletions(-) diff --git a/src/libstd/io/buffered.rs b/src/libstd/io/buffered.rs index 046b1a6888024..fff4d0df7e769 100644 --- a/src/libstd/io/buffered.rs +++ b/src/libstd/io/buffered.rs @@ -449,6 +449,12 @@ impl Seek for BufReader { #[stable(feature = "rust1", since = "1.0.0")] pub struct BufWriter { inner: Option, + // FIXME: Replace this with a VecDeque. Because VecDeque is a Ring buffer, + // this would enable BufWriter to operate without any interior copies. + // It was also allow a much simpler implementation of flush_buf. The main + // blocker here is that VecDeque doesn't currently have the same + // slice-specific specializations (extend_from_slice, `Extend` + // specializations) buf: Vec, // #30888: If the inner writer panics in a call to write, we don't want to // write the buffered data a second time in BufWriter's destructor. This @@ -519,6 +525,13 @@ impl BufWriter { BufWriter { inner: Some(inner), buf: Vec::with_capacity(capacity), panicked: false } } + /// Send data in our local buffer into the inner writer, looping as + /// necessary until either it's all been sent or an error occurs. + /// + /// Because all the data in the buffer has been reported to our owner as + /// "successfully written" (by returning nonzero success values from + /// `write`), any 0-length writes from `inner` must be reported as i/o + /// errors from this method. fn flush_buf(&mut self) -> io::Result<()> { let mut written = 0; let len = self.buf.len(); @@ -548,6 +561,17 @@ impl BufWriter { ret } + /// Buffer some data without flushing it, regardless of the size of the + /// data. Writes as much as possible without exceeding capacity. Returns + /// the number of bytes written. + #[inline] + fn write_to_buffer(&mut self, buf: &[u8]) -> usize { + let available = self.buf.capacity() - self.buf.len(); + let amt_to_buffer = available.min(buf.len()); + self.buf.extend_from_slice(&buf[..amt_to_buffer]); + amt_to_buffer + } + /// Gets a reference to the underlying writer. /// /// # Examples @@ -665,7 +689,22 @@ impl Write for BufWriter { self.panicked = false; r } else { - self.buf.write(buf) + Ok(self.write_to_buffer(buf)) + } + } + + fn write_all(&mut self, buf: &[u8]) -> io::Result<()> { + if self.buf.len() + buf.len() > self.buf.capacity() { + self.flush_buf()?; + } + if buf.len() >= self.buf.capacity() { + self.panicked = true; + let r = self.get_mut().write_all(buf); + self.panicked = false; + r + } else { + self.write_to_buffer(buf); + Ok(()) } } @@ -818,6 +857,200 @@ impl fmt::Display for IntoInnerError { } } +/// Private helper struct for implementing the line-buffered writing logic. +/// This shim temporarily wraps a BufWriter, and uses its internals to +/// implement a line-buffered writer (specifically by using the internal +/// methods like write_to_buffer and flush_buffer). In this way, a more +/// efficient abstraction can be created than one that only had access to +/// `write` and `flush`, without needlessly duplicating a lot of the +/// implementation details of BufWriter. This also allows existing +/// `BufWriters` to be temporarily given line-buffering logic; this is what +/// enables Stdout to be alternately in line-buffered or block-buffered mode. +#[derive(Debug)] +pub(super) struct LineWriterShim<'a, W: Write> { + inner: &'a mut BufWriter, +} + +impl<'a, W: Write> LineWriterShim<'a, W> { + pub fn new(inner: &'a mut BufWriter) -> Self { + Self { inner } + } +} + +impl<'a, W: Write> Write for LineWriterShim<'a, W> { + /// Write some data into this BufReader with line buffering. This means + /// that, if any newlines are present in the data, the data up to the last + /// newline is sent directly to the underlying writer, and data after it + /// is buffered. Returns the number of bytes written. + /// + /// This function operates on a "best effort basis"; in keeping with the + /// convention of `Write::write`, it makes at most one attempt to write + /// new data to the underlying writer. If that write only reports a partial + /// success, the remaining data will be buffered. + /// + /// Because this function attempts to send completed lines to the underlying + /// writer, it will also flush the existing buffer if it contains any + /// newlines, even if the incoming data does not contain any newlines. + fn write(&mut self, buf: &[u8]) -> io::Result { + // If there are no new newlines (that is, if this write is less than + // one line), just do a regular buffered write + let newline_idx = match memchr::memrchr(b'\n', buf) { + None => { + // Check for prior partial line writes that need to be retried + if memchr::memchr(b'\n', &self.inner.buffer()).is_some() { + self.inner.flush_buf()?; + } + return self.inner.write(buf); + } + Some(i) => i, + }; + + // Flush existing content to prepare for our write + self.inner.flush_buf()?; + + // This is what we're going to try to write directly to the inner + // writer. The rest will be buffered, if nothing goes wrong. + let lines = &buf[..newline_idx + 1]; + + // Write `lines` directly to the inner writer. In keeping with the + // `write` convention, make at most one attempt to add new (unbuffered) + // data. Because this write doesn't touch the BufWriter state directly, + // and the buffer is known to be empty, we don't need to worry about + // self.panicked here. + let flushed = self.inner.get_mut().write(lines)?; + + // Now that the write has succeeded, buffer the rest (or as much of + // the rest as possible). If there were any unwritten newlines, we + // only buffer out to the last unwritten newline; this helps prevent + // flushing partial lines on subsequent calls to write_buffered_lines. + let tail = &buf[flushed..]; + let buffered = match memchr::memrchr(b'\n', tail) { + None => self.inner.write_to_buffer(tail), + Some(i) => self.inner.write_to_buffer(&tail[..i + 1]), + }; + Ok(flushed + buffered) + } + + fn flush(&mut self) -> io::Result<()> { + self.inner.flush() + } + + /// Write some vectored data into this BufReader with line buffering. This + /// means that, if any newlines are present in the data, the data up to + /// and including the buffer containing the last newline is sent directly + /// to the inner writer, and the data after it is buffered. Returns the + /// number of bytes written. + /// + /// This function operates on a "best effort basis"; in keeping with the + /// convention of `Write::write`, it makes at most one attempt to write + /// new data to the underlying writer. + /// + /// Because this function attempts to send completed lines to the underlying + /// writer, it will also flush the existing buffer if it contains any + /// newlines. + /// + /// Because sorting through an array of `IoSlice` can be a bit convoluted, + /// This method differs from write in the following ways: + /// + /// - It attempts to write all the buffers up to and including the one + /// containing the last newline. This means that it may attempt to + /// write a partial line. + /// - If the write only reports partial success, it does not attempt to + /// find the precise location of the written bytes and buffer the rest. + fn write_vectored(&mut self, bufs: &[IoSlice<'_>]) -> io::Result { + // Find the buffer containing the last newline + let last_newline_buf_idx = bufs + .iter() + .enumerate() + .rev() + .find_map(|(i, buf)| memchr::memchr(b'\n', buf).map(|_| i)); + + // If there are no new newlines (that is, if this write is less than + // one line), just do a regular buffered write + let last_newline_buf_idx = match last_newline_buf_idx { + // No newlines; just do a normal buffered write + None => { + // Check for prior partial line writes that need to be retried + if memchr::memchr(b'\n', &self.inner.buffer()).is_some() { + self.inner.flush_buf()?; + } + return self.inner.write_vectored(bufs); + } + Some(i) => i, + }; + + // Flush existing content to prepare for our write + self.inner.flush_buf()?; + + // This is what we're going to try to write directly to the inner + // writer. The rest will be buffered, if nothing goes wrong. + let (lines, tail) = bufs.split_at(last_newline_buf_idx + 1); + + // Write `lines` directly to the inner writer. In keeping with the + // `write` convention, make at most one attempt to add new (unbuffered) + // data. Because this write doesn't touch the BufWriter state directly, + // and the buffer is known to be empty, we don't need to worry about + // self.panicked here. + let flushed = self.inner.write_vectored(lines)?; + + // Don't try to reconstruct the exact amount written; just bail + // in the event of a partial write + let lines_len = lines.iter().map(|buf| buf.len()).sum(); + if flushed < lines_len { + return Ok(flushed); + } + + // Now that the write has succeeded, buffer the rest (or as much of the + // rest as possible) + let buffered: usize = + tail.iter().map(|buf| self.inner.write_to_buffer(buf)).take_while(|&n| n > 0).sum(); + + Ok(flushed + buffered) + } + + fn is_write_vectored(&self) -> bool { + self.inner.is_write_vectored() + } + + /// Write some data into this BufReader with line buffering. This means + /// that, if any newlines are present in the data, the data up to the last + /// newline is sent directly to the underlying writer, and data after it + /// is buffered. + /// + /// Because this function attempts to send completed lines to the underlying + /// writer, it will also flush the existing buffer if it contains any + /// newlines, even if the incoming data does not contain any newlines. + fn write_all(&mut self, buf: &[u8]) -> io::Result<()> { + // If there are no new newlines (that is, if this write is less than + // one line), just do a regular buffered write + let newline_idx = match memchr::memrchr(b'\n', buf) { + None => { + // Check for prior partial line writes that need to be retried + if memchr::memchr(b'\n', &self.inner.buffer()).is_some() { + self.inner.flush_buf()?; + } + return self.inner.write_all(buf); + } + Some(i) => i, + }; + + // Flush existing content to prepare for our write + self.inner.flush_buf()?; + + // This is what we're going to try to write directly to the inner + // writer. The rest will be buffered, if nothing goes wrong. + let (lines, tail) = buf.split_at(newline_idx + 1); + + // Write `lines` directly to the inner writer, bypassing the buffer. + self.inner.get_mut().write_all(lines)?; + + // Now that the write has succeeded, buffer the rest with BufWriter::write_all. + // This will buffer as much as possible, but continue flushing as + // necessary if our tail is huge. + self.inner.write_all(tail) + } +} + /// Wraps a writer and buffers output to it, flushing whenever a newline /// (`0x0a`, `'\n'`) is detected. /// @@ -885,7 +1118,6 @@ impl fmt::Display for IntoInnerError { #[stable(feature = "rust1", since = "1.0.0")] pub struct LineWriter { inner: BufWriter, - need_flush: bool, } impl LineWriter { @@ -926,7 +1158,7 @@ impl LineWriter { /// ``` #[stable(feature = "rust1", since = "1.0.0")] pub fn with_capacity(capacity: usize, inner: W) -> LineWriter { - LineWriter { inner: BufWriter::with_capacity(capacity, inner), need_flush: false } + LineWriter { inner: BufWriter::with_capacity(capacity, inner) } } /// Gets a reference to the underlying writer. @@ -1000,110 +1232,40 @@ impl LineWriter { /// ``` #[stable(feature = "rust1", since = "1.0.0")] pub fn into_inner(self) -> Result>> { - self.inner.into_inner().map_err(|IntoInnerError(buf, e)| { - IntoInnerError(LineWriter { inner: buf, need_flush: false }, e) - }) + self.inner + .into_inner() + .map_err(|IntoInnerError(buf, e)| IntoInnerError(LineWriter { inner: buf }, e)) } } #[stable(feature = "rust1", since = "1.0.0")] impl Write for LineWriter { fn write(&mut self, buf: &[u8]) -> io::Result { - if self.need_flush { - self.flush()?; - } - - // Find the last newline character in the buffer provided. If found then - // we're going to write all the data up to that point and then flush, - // otherwise we just write the whole block to the underlying writer. - let i = match memchr::memrchr(b'\n', buf) { - Some(i) => i, - None => return self.inner.write(buf), - }; - - // Ok, we're going to write a partial amount of the data given first - // followed by flushing the newline. After we've successfully written - // some data then we *must* report that we wrote that data, so future - // errors are ignored. We set our internal `need_flush` flag, though, in - // case flushing fails and we need to try it first next time. - let n = self.inner.write(&buf[..=i])?; - self.need_flush = true; - if self.flush().is_err() || n != i + 1 { - return Ok(n); - } + LineWriterShim::new(&mut self.inner).write(buf) + } - // At this point we successfully wrote `i + 1` bytes and flushed it out, - // meaning that the entire line is now flushed out on the screen. While - // we can attempt to finish writing the rest of the data provided. - // Remember though that we ignore errors here as we've successfully - // written data, so we need to report that. - match self.inner.write(&buf[i + 1..]) { - Ok(i) => Ok(n + i), - Err(_) => Ok(n), - } + fn flush(&mut self) -> io::Result<()> { + self.inner.flush() } - // Vectored writes are very similar to the writes above, but adjusted for - // the list of buffers that we have to write. fn write_vectored(&mut self, bufs: &[IoSlice<'_>]) -> io::Result { - if self.need_flush { - self.flush()?; - } + LineWriterShim::new(&mut self.inner).write_vectored(bufs) + } - // Find the last newline, and failing that write the whole buffer - let last_newline = bufs.iter().enumerate().rev().find_map(|(i, buf)| { - let pos = memchr::memrchr(b'\n', buf)?; - Some((i, pos)) - }); - let (i, j) = match last_newline { - Some(pair) => pair, - None => return self.inner.write_vectored(bufs), - }; - let (prefix, suffix) = bufs.split_at(i); - let (buf, suffix) = suffix.split_at(1); - let buf = &buf[0]; - - // Write everything up to the last newline, flushing afterwards. Note - // that only if we finished our entire `write_vectored` do we try the - // subsequent - // `write` - let mut n = 0; - let prefix_amt = prefix.iter().map(|i| i.len()).sum(); - if prefix_amt > 0 { - n += self.inner.write_vectored(prefix)?; - self.need_flush = true; - } - if n == prefix_amt { - match self.inner.write(&buf[..=j]) { - Ok(m) => n += m, - Err(e) if n == 0 => return Err(e), - Err(_) => return Ok(n), - } - self.need_flush = true; - } - if self.flush().is_err() || n != j + 1 + prefix_amt { - return Ok(n); - } + fn is_write_vectored(&self) -> bool { + self.inner.is_write_vectored() + } - // ... and now write out everything remaining - match self.inner.write(&buf[j + 1..]) { - Ok(i) => n += i, - Err(_) => return Ok(n), - } + fn write_all(&mut self, buf: &[u8]) -> io::Result<()> { + LineWriterShim::new(&mut self.inner).write_all(buf) + } - if suffix.iter().map(|s| s.len()).sum::() == 0 { - return Ok(n); - } - match self.inner.write_vectored(suffix) { - Ok(i) => Ok(n + i), - Err(_) => Ok(n), - } + fn write_all_vectored(&mut self, bufs: &mut [IoSlice<'_>]) -> io::Result<()> { + LineWriterShim::new(&mut self.inner).write_all_vectored(bufs) } - fn flush(&mut self) -> io::Result<()> { - self.inner.flush()?; - self.need_flush = false; - Ok(()) + fn write_fmt(&mut self, fmt: fmt::Arguments<'_>) -> io::Result<()> { + LineWriterShim::new(&mut self.inner).write_fmt(fmt) } } @@ -1137,7 +1299,11 @@ mod tests { impl Read for ShortReader { fn read(&mut self, _: &mut [u8]) -> io::Result { - if self.lengths.is_empty() { Ok(0) } else { Ok(self.lengths.remove(0)) } + if self.lengths.is_empty() { + Ok(0) + } else { + Ok(self.lengths.remove(0)) + } } } From 0f3815886a0d7aaefd222b4592c3ef2ccfd0ebc2 Mon Sep 17 00:00:00 2001 From: Nathan West Date: Sun, 31 May 2020 01:29:48 -0400 Subject: [PATCH 03/49] Updated comments; only pre-flush newline terminated buffers --- src/libstd/io/buffered.rs | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/src/libstd/io/buffered.rs b/src/libstd/io/buffered.rs index fff4d0df7e769..0ae5db0df1778 100644 --- a/src/libstd/io/buffered.rs +++ b/src/libstd/io/buffered.rs @@ -896,8 +896,10 @@ impl<'a, W: Write> Write for LineWriterShim<'a, W> { // one line), just do a regular buffered write let newline_idx = match memchr::memrchr(b'\n', buf) { None => { - // Check for prior partial line writes that need to be retried - if memchr::memchr(b'\n', &self.inner.buffer()).is_some() { + // Check for prior partial line writes that need to be retried. + // Only retry if the buffer contains a completed line, to + // avoid flushing partial lines. + if let Some(b'\n') = self.inner.buffer().last().copied() { self.inner.flush_buf()?; } return self.inner.write(buf); @@ -916,13 +918,13 @@ impl<'a, W: Write> Write for LineWriterShim<'a, W> { // `write` convention, make at most one attempt to add new (unbuffered) // data. Because this write doesn't touch the BufWriter state directly, // and the buffer is known to be empty, we don't need to worry about - // self.panicked here. + // self.inner.panicked here. let flushed = self.inner.get_mut().write(lines)?; // Now that the write has succeeded, buffer the rest (or as much of // the rest as possible). If there were any unwritten newlines, we // only buffer out to the last unwritten newline; this helps prevent - // flushing partial lines on subsequent calls to write_buffered_lines. + // flushing partial lines on subsequent calls to LineWriterShim::write. let tail = &buf[flushed..]; let buffered = match memchr::memrchr(b'\n', tail) { None => self.inner.write_to_buffer(tail), @@ -970,8 +972,10 @@ impl<'a, W: Write> Write for LineWriterShim<'a, W> { let last_newline_buf_idx = match last_newline_buf_idx { // No newlines; just do a normal buffered write None => { - // Check for prior partial line writes that need to be retried - if memchr::memchr(b'\n', &self.inner.buffer()).is_some() { + // Check for prior partial line writes that need to be retried. + // Only retry if the buffer contains a completed line, to + // avoid flushing partial lines. + if let Some(b'\n') = self.inner.buffer().last().copied() { self.inner.flush_buf()?; } return self.inner.write_vectored(bufs); @@ -1025,8 +1029,10 @@ impl<'a, W: Write> Write for LineWriterShim<'a, W> { // one line), just do a regular buffered write let newline_idx = match memchr::memrchr(b'\n', buf) { None => { - // Check for prior partial line writes that need to be retried - if memchr::memchr(b'\n', &self.inner.buffer()).is_some() { + // Check for prior partial line writes that need to be retried. + // Only retry if the buffer contains a completed line, to + // avoid flushing partial lines. + if let Some(b'\n') = self.inner.buffer().last().copied() { self.inner.flush_buf()?; } return self.inner.write_all(buf); From 4a1597f7a77b17e42217d02750af2ae8012430af Mon Sep 17 00:00:00 2001 From: Nathan West Date: Sun, 31 May 2020 02:37:36 -0400 Subject: [PATCH 04/49] Expressionify `LineWriterShim::write` --- src/libstd/io/buffered.rs | 62 ++++++++++++++++++++------------------- 1 file changed, 32 insertions(+), 30 deletions(-) diff --git a/src/libstd/io/buffered.rs b/src/libstd/io/buffered.rs index 0ae5db0df1778..6098416df19b2 100644 --- a/src/libstd/io/buffered.rs +++ b/src/libstd/io/buffered.rs @@ -892,9 +892,9 @@ impl<'a, W: Write> Write for LineWriterShim<'a, W> { /// writer, it will also flush the existing buffer if it contains any /// newlines, even if the incoming data does not contain any newlines. fn write(&mut self, buf: &[u8]) -> io::Result { - // If there are no new newlines (that is, if this write is less than - // one line), just do a regular buffered write - let newline_idx = match memchr::memrchr(b'\n', buf) { + match memchr::memrchr(b'\n', buf) { + // If there are no new newlines (that is, if this write is less than + // one line), just do a regular buffered write None => { // Check for prior partial line writes that need to be retried. // Only retry if the buffer contains a completed line, to @@ -902,35 +902,37 @@ impl<'a, W: Write> Write for LineWriterShim<'a, W> { if let Some(b'\n') = self.inner.buffer().last().copied() { self.inner.flush_buf()?; } - return self.inner.write(buf); + self.inner.write(buf) + } + // Otherwise, arrange for the lines to be written directly to the + // inner writer. + Some(newline_idx) => { + // Flush existing content to prepare for our write + self.inner.flush_buf()?; + + // This is what we're going to try to write directly to the inner + // writer. The rest will be buffered, if nothing goes wrong. + let lines = &buf[..newline_idx + 1]; + + // Write `lines` directly to the inner writer. In keeping with the + // `write` convention, make at most one attempt to add new (unbuffered) + // data. Because this write doesn't touch the BufWriter state directly, + // and the buffer is known to be empty, we don't need to worry about + // self.inner.panicked here. + let flushed = self.inner.get_mut().write(lines)?; + + // Now that the write has succeeded, buffer the rest (or as much of + // the rest as possible). If there were any unwritten newlines, we + // only buffer out to the last unwritten newline; this helps prevent + // flushing partial lines on subsequent calls to LineWriterShim::write. + let tail = &buf[flushed..]; + let buffered = match memchr::memrchr(b'\n', tail) { + None => self.inner.write_to_buffer(tail), + Some(i) => self.inner.write_to_buffer(&tail[..i + 1]), + }; + Ok(flushed + buffered) } - Some(i) => i, - }; - - // Flush existing content to prepare for our write - self.inner.flush_buf()?; - - // This is what we're going to try to write directly to the inner - // writer. The rest will be buffered, if nothing goes wrong. - let lines = &buf[..newline_idx + 1]; - - // Write `lines` directly to the inner writer. In keeping with the - // `write` convention, make at most one attempt to add new (unbuffered) - // data. Because this write doesn't touch the BufWriter state directly, - // and the buffer is known to be empty, we don't need to worry about - // self.inner.panicked here. - let flushed = self.inner.get_mut().write(lines)?; - - // Now that the write has succeeded, buffer the rest (or as much of - // the rest as possible). If there were any unwritten newlines, we - // only buffer out to the last unwritten newline; this helps prevent - // flushing partial lines on subsequent calls to LineWriterShim::write. - let tail = &buf[flushed..]; - let buffered = match memchr::memrchr(b'\n', tail) { - None => self.inner.write_to_buffer(tail), - Some(i) => self.inner.write_to_buffer(&tail[..i + 1]), }; - Ok(flushed + buffered) } fn flush(&mut self) -> io::Result<()> { From 5edad37b04d4daac62b01946142589e0f69aa22c Mon Sep 17 00:00:00 2001 From: Nathan West Date: Sun, 31 May 2020 03:21:51 -0400 Subject: [PATCH 05/49] Expressionify write_all --- src/libstd/io/buffered.rs | 40 ++++++++++++++++++++------------------- 1 file changed, 21 insertions(+), 19 deletions(-) diff --git a/src/libstd/io/buffered.rs b/src/libstd/io/buffered.rs index 6098416df19b2..de2a32345f1c9 100644 --- a/src/libstd/io/buffered.rs +++ b/src/libstd/io/buffered.rs @@ -932,7 +932,7 @@ impl<'a, W: Write> Write for LineWriterShim<'a, W> { }; Ok(flushed + buffered) } - }; + } } fn flush(&mut self) -> io::Result<()> { @@ -1027,9 +1027,9 @@ impl<'a, W: Write> Write for LineWriterShim<'a, W> { /// writer, it will also flush the existing buffer if it contains any /// newlines, even if the incoming data does not contain any newlines. fn write_all(&mut self, buf: &[u8]) -> io::Result<()> { - // If there are no new newlines (that is, if this write is less than - // one line), just do a regular buffered write - let newline_idx = match memchr::memrchr(b'\n', buf) { + match memchr::memrchr(b'\n', buf) { + // If there are no new newlines (that is, if this write is less than + // one line), just do a regular buffered write None => { // Check for prior partial line writes that need to be retried. // Only retry if the buffer contains a completed line, to @@ -1037,25 +1037,27 @@ impl<'a, W: Write> Write for LineWriterShim<'a, W> { if let Some(b'\n') = self.inner.buffer().last().copied() { self.inner.flush_buf()?; } - return self.inner.write_all(buf); + self.inner.write_all(buf) } - Some(i) => i, - }; - - // Flush existing content to prepare for our write - self.inner.flush_buf()?; + // Otherwise, arrange for the lines to be written directly to the + // inner writer. + Some(newline_idx) => { + // Flush existing content to prepare for our write + self.inner.flush_buf()?; - // This is what we're going to try to write directly to the inner - // writer. The rest will be buffered, if nothing goes wrong. - let (lines, tail) = buf.split_at(newline_idx + 1); + // This is what we're going to try to write directly to the inner + // writer. The rest will be buffered, if nothing goes wrong. + let (lines, tail) = buf.split_at(newline_idx + 1); - // Write `lines` directly to the inner writer, bypassing the buffer. - self.inner.get_mut().write_all(lines)?; + // Write `lines` directly to the inner writer, bypassing the buffer. + self.inner.get_mut().write_all(lines)?; - // Now that the write has succeeded, buffer the rest with BufWriter::write_all. - // This will buffer as much as possible, but continue flushing as - // necessary if our tail is huge. - self.inner.write_all(tail) + // Now that the write has succeeded, buffer the rest with BufWriter::write_all. + // This will buffer as much as possible, but continue flushing as + // necessary if our tail is huge. + self.inner.write_all(tail) + } + } } } From 1bf8ba3546aace5a16852d5f58fd9206934cc829 Mon Sep 17 00:00:00 2001 From: Nathan West Date: Sun, 31 May 2020 03:28:07 -0400 Subject: [PATCH 06/49] x.py fmt --- src/libstd/io/buffered.rs | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/src/libstd/io/buffered.rs b/src/libstd/io/buffered.rs index de2a32345f1c9..35434979b85ad 100644 --- a/src/libstd/io/buffered.rs +++ b/src/libstd/io/buffered.rs @@ -1309,11 +1309,7 @@ mod tests { impl Read for ShortReader { fn read(&mut self, _: &mut [u8]) -> io::Result { - if self.lengths.is_empty() { - Ok(0) - } else { - Ok(self.lengths.remove(0)) - } + if self.lengths.is_empty() { Ok(0) } else { Ok(self.lengths.remove(0)) } } } From e0dfdc683def9573f28d7f2f077790bc3c52132b Mon Sep 17 00:00:00 2001 From: Nathan West Date: Mon, 1 Jun 2020 00:26:48 -0400 Subject: [PATCH 07/49] Added check for `is_write_vectored` --- src/libstd/io/buffered.rs | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/src/libstd/io/buffered.rs b/src/libstd/io/buffered.rs index 35434979b85ad..4ae2a5eaf95a9 100644 --- a/src/libstd/io/buffered.rs +++ b/src/libstd/io/buffered.rs @@ -961,7 +961,19 @@ impl<'a, W: Write> Write for LineWriterShim<'a, W> { /// write a partial line. /// - If the write only reports partial success, it does not attempt to /// find the precise location of the written bytes and buffer the rest. + /// + /// If the underlying vector doesn't support vectored writing, we instead + /// simply write the first non-empty buffer with `write`. This way, we + /// get the benefits of more granular partial-line handling without losing + /// anything in efficiency fn write_vectored(&mut self, bufs: &[IoSlice<'_>]) -> io::Result { + if !self.is_write_vectored() { + return match bufs.iter().find(|buf| !buf.is_empty()) { + Some(buf) => self.write(buf), + None => Ok(0), + } + } + // Find the buffer containing the last newline let last_newline_buf_idx = bufs .iter() From 2c3024b368cb2810e390f980c3f9820cc07389e8 Mon Sep 17 00:00:00 2001 From: Nathan West Date: Mon, 1 Jun 2020 00:36:31 -0400 Subject: [PATCH 08/49] Add comment describing erroneous_flush_retried --- src/libstd/io/buffered.rs | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/libstd/io/buffered.rs b/src/libstd/io/buffered.rs index 4ae2a5eaf95a9..04c1cbf5a17d3 100644 --- a/src/libstd/io/buffered.rs +++ b/src/libstd/io/buffered.rs @@ -1766,6 +1766,15 @@ mod tests { } } + /// Previously the `LineWriter` could successfully write some bytes but + /// then fail to report that it has done so. Additionally, an erroneous + /// flush after a successful write was permanently ignored. + /// + /// Test that a line writer correctly reports the number of written bytes, + /// and that it attempts to flush buffered lines from previous writes + /// before processing new data + /// + /// Regression test for #37807 #[test] fn erroneous_flush_retried() { let a = AcceptOneThenFail { written: false, flushed: false }; From e89e2e42f9269fe401b1eef0288ff12c7941544f Mon Sep 17 00:00:00 2001 From: Nathan West Date: Mon, 1 Jun 2020 01:04:33 -0400 Subject: [PATCH 09/49] Added test stubs --- src/libstd/io/buffered.rs | 109 ++++++++++++++++++++++++++++++-------- 1 file changed, 86 insertions(+), 23 deletions(-) diff --git a/src/libstd/io/buffered.rs b/src/libstd/io/buffered.rs index 04c1cbf5a17d3..09b24d810c526 100644 --- a/src/libstd/io/buffered.rs +++ b/src/libstd/io/buffered.rs @@ -971,7 +971,7 @@ impl<'a, W: Write> Write for LineWriterShim<'a, W> { return match bufs.iter().find(|buf| !buf.is_empty()) { Some(buf) => self.write(buf), None => Ok(0), - } + }; } // Find the buffer containing the last newline @@ -1321,7 +1321,11 @@ mod tests { impl Read for ShortReader { fn read(&mut self, _: &mut [u8]) -> io::Result { - if self.lengths.is_empty() { Ok(0) } else { Ok(self.lengths.remove(0)) } + if self.lengths.is_empty() { + Ok(0) + } else { + Ok(self.lengths.remove(0)) + } } } @@ -1742,27 +1746,48 @@ mod tests { b.iter(|| BufWriter::new(io::sink())); } - struct AcceptOneThenFail { - written: bool, + #[derive(Default, Clone)] + struct ProgrammableSink { + // Writes append to this slice + buffer: Vec, + + // Flushes set this flag flushed: bool, + + // If true, writes & flushes will always be an error + return_error: bool, + + // If set, only up to this number of bytes will be written in a single + // call to `write` + accept_prefix: Option, + + // If set, counts down with each write, and writes return an error + // when it hits 0 + max_writes: Option, } - impl Write for AcceptOneThenFail { + impl Write for ProgrammableSink { fn write(&mut self, data: &[u8]) -> io::Result { - if !self.written { - assert_eq!(data, b"a\nb\n"); - self.written = true; - Ok(data.len()) + if self.return_error { + Err(io::Error::new(io::ErrorKind::Other, "test")) } else { - Err(io::Error::new(io::ErrorKind::NotFound, "test")) + let len = match self.accept_prefix { + None => data.len(), + Some(prefix) => prefix.min(prefix), + }; + let data = &data[..len]; + self.buffer.extend_from_slice(data); + Ok(len) } } fn flush(&mut self) -> io::Result<()> { - assert!(self.written); - assert!(!self.flushed); - self.flushed = true; - Err(io::Error::new(io::ErrorKind::Other, "test")) + if self.return_error { + Err(io::Error::new(io::ErrorKind::Other, "test")) + } else { + self.flushed = true; + Ok(()) + } } } @@ -1777,15 +1802,7 @@ mod tests { /// Regression test for #37807 #[test] fn erroneous_flush_retried() { - let a = AcceptOneThenFail { written: false, flushed: false }; - - let mut l = LineWriter::new(a); - assert_eq!(l.write(b"a\nb\na").unwrap(), 4); - assert!(l.get_ref().written); - assert!(l.get_ref().flushed); - l.get_mut().flushed = false; - - assert_eq!(l.write(b"a").unwrap_err().kind(), io::ErrorKind::Other) + todo!() } #[test] @@ -1895,4 +1912,50 @@ mod tests { io::Error::new(io::ErrorKind::Other, "x") } } + + /// Test that, given this input: + /// + /// Line 1\n + /// Line 2\n + /// Line 3\n + /// Line 4 + /// + /// And given a result that only writes to midway through Line 2 + /// + /// That only up to the end of Line 3 is buffered + /// + /// This behavior is desirable because it prevents flushing partial lines + #[test] + fn test_partial_write_buffers_line() { + todo!() + } + + /// Test that, given this input: + /// + /// Line 1\n + /// Line 2\n + /// Line 3 + /// + /// And given that the full write of lines 1 and 2 was successful + /// That data up to Line 3 is buffered + #[test] + fn test_partial_line_buffered_after_line_write() { + todo!() + } + + /// Test that, given a partial line that exceeds the length of + /// LineBuffer's buffer (that is, without a trailing newline), that that + /// line is written to the inner writer + #[test] + fn test_long_line_flushed() { + todo!() + } + + /// Test that, given a very long partial line *after* successfully + /// flushing a complete line, that that line is buffered unconditionally, + /// and no additional writes take place + #[test] + fn test_long_tail_not_flushed() { + todo!() + } } From f0a08073e47cc4a832cea66a7ef6bd9506061a72 Mon Sep 17 00:00:00 2001 From: Nathan West Date: Mon, 1 Jun 2020 17:36:55 -0400 Subject: [PATCH 10/49] Various testing & implementation updates: - Added a bunch of new unit tests - Removed test_line_buffer_fail_flush - Updated erroneous_flush_retried - Added helper methods to LineWriterShim for code clarity, to distinguish "self.buffer" (the BufWriter) from self.inner (the thing wrapped by the BufWriter) - Un-expressionized write & write_all - Added clause to bail early on Ok(0) --- src/libstd/io/buffered.rs | 344 +++++++++++++++++++++++++------------- 1 file changed, 232 insertions(+), 112 deletions(-) diff --git a/src/libstd/io/buffered.rs b/src/libstd/io/buffered.rs index 09b24d810c526..730ae8a0ee471 100644 --- a/src/libstd/io/buffered.rs +++ b/src/libstd/io/buffered.rs @@ -868,12 +868,30 @@ impl fmt::Display for IntoInnerError { /// enables Stdout to be alternately in line-buffered or block-buffered mode. #[derive(Debug)] pub(super) struct LineWriterShim<'a, W: Write> { - inner: &'a mut BufWriter, + buffer: &'a mut BufWriter, } impl<'a, W: Write> LineWriterShim<'a, W> { - pub fn new(inner: &'a mut BufWriter) -> Self { - Self { inner } + pub fn new(buffer: &'a mut BufWriter) -> Self { + Self { buffer } + } + + /// Get a reference to the inner writer (that is, the writer wrapped by + /// the BufWriter) + fn inner(&self) -> &W { + self.buffer.get_ref() + } + + /// Get a mutable reference to the inner writer (that is, the writer + /// wrapped by the BufWriter). Be careful with this writer, as writes to + /// it will bypass the buffer. + fn inner_mut(&mut self) -> &mut W { + self.buffer.get_mut() + } + + /// Get the content currently buffered in self.buffer + fn buffered(&self) -> &[u8] { + self.buffer.buffer() } } @@ -892,51 +910,58 @@ impl<'a, W: Write> Write for LineWriterShim<'a, W> { /// writer, it will also flush the existing buffer if it contains any /// newlines, even if the incoming data does not contain any newlines. fn write(&mut self, buf: &[u8]) -> io::Result { - match memchr::memrchr(b'\n', buf) { + let newline_idx = match memchr::memrchr(b'\n', buf) { // If there are no new newlines (that is, if this write is less than // one line), just do a regular buffered write None => { // Check for prior partial line writes that need to be retried. // Only retry if the buffer contains a completed line, to // avoid flushing partial lines. - if let Some(b'\n') = self.inner.buffer().last().copied() { - self.inner.flush_buf()?; + if let Some(b'\n') = self.buffered().last().copied() { + self.buffer.flush_buf()?; } - self.inner.write(buf) + return self.buffer.write(buf); } // Otherwise, arrange for the lines to be written directly to the // inner writer. - Some(newline_idx) => { - // Flush existing content to prepare for our write - self.inner.flush_buf()?; - - // This is what we're going to try to write directly to the inner - // writer. The rest will be buffered, if nothing goes wrong. - let lines = &buf[..newline_idx + 1]; - - // Write `lines` directly to the inner writer. In keeping with the - // `write` convention, make at most one attempt to add new (unbuffered) - // data. Because this write doesn't touch the BufWriter state directly, - // and the buffer is known to be empty, we don't need to worry about - // self.inner.panicked here. - let flushed = self.inner.get_mut().write(lines)?; - - // Now that the write has succeeded, buffer the rest (or as much of - // the rest as possible). If there were any unwritten newlines, we - // only buffer out to the last unwritten newline; this helps prevent - // flushing partial lines on subsequent calls to LineWriterShim::write. - let tail = &buf[flushed..]; - let buffered = match memchr::memrchr(b'\n', tail) { - None => self.inner.write_to_buffer(tail), - Some(i) => self.inner.write_to_buffer(&tail[..i + 1]), - }; - Ok(flushed + buffered) - } + Some(newline_idx) => newline_idx, + }; + + // Flush existing content to prepare for our write + self.buffer.flush_buf()?; + + // This is what we're going to try to write directly to the inner + // writer. The rest will be buffered, if nothing goes wrong. + let lines = &buf[..newline_idx + 1]; + + // Write `lines` directly to the inner writer. In keeping with the + // `write` convention, make at most one attempt to add new (unbuffered) + // data. Because this write doesn't touch the BufWriter state directly, + // and the buffer is known to be empty, we don't need to worry about + // self.buffer.panicked here. + let flushed = self.inner_mut().write(lines)?; + + // If buffer returns Ok(0), propagate that to the caller without + // doing additional buffering; otherwise we're just guaranteeing + // an "ErrorKind::WriteZero" later. + if flushed == 0 { + return Ok(0); } + + // Now that the write has succeeded, buffer the rest (or as much of + // the rest as possible). If there were any unwritten newlines, we + // only buffer out to the last unwritten newline; this helps prevent + // flushing partial lines on subsequent calls to LineWriterShim::write. + let tail = &buf[flushed..]; + let buffered = match memchr::memrchr(b'\n', tail) { + None => self.buffer.write_to_buffer(tail), + Some(i) => self.buffer.write_to_buffer(&tail[..i + 1]), + }; + Ok(flushed + buffered) } fn flush(&mut self) -> io::Result<()> { - self.inner.flush() + self.buffer.flush() } /// Write some vectored data into this BufReader with line buffering. This @@ -967,6 +992,8 @@ impl<'a, W: Write> Write for LineWriterShim<'a, W> { /// get the benefits of more granular partial-line handling without losing /// anything in efficiency fn write_vectored(&mut self, bufs: &[IoSlice<'_>]) -> io::Result { + // If there's no specialized behavior for write_vectored, just use + // write. This has the benefit of more granular partial-line handling. if !self.is_write_vectored() { return match bufs.iter().find(|buf| !buf.is_empty()) { Some(buf) => self.write(buf), @@ -989,16 +1016,16 @@ impl<'a, W: Write> Write for LineWriterShim<'a, W> { // Check for prior partial line writes that need to be retried. // Only retry if the buffer contains a completed line, to // avoid flushing partial lines. - if let Some(b'\n') = self.inner.buffer().last().copied() { - self.inner.flush_buf()?; + if let Some(b'\n') = self.buffered().last().copied() { + self.buffer.flush_buf()?; } - return self.inner.write_vectored(bufs); + return self.buffer.write_vectored(bufs); } Some(i) => i, }; // Flush existing content to prepare for our write - self.inner.flush_buf()?; + self.buffer.flush_buf()?; // This is what we're going to try to write directly to the inner // writer. The rest will be buffered, if nothing goes wrong. @@ -1009,7 +1036,14 @@ impl<'a, W: Write> Write for LineWriterShim<'a, W> { // data. Because this write doesn't touch the BufWriter state directly, // and the buffer is known to be empty, we don't need to worry about // self.panicked here. - let flushed = self.inner.write_vectored(lines)?; + let flushed = self.inner_mut().write_vectored(lines)?; + + // If inner returns Ok(0), propagate that to the caller without + // doing additional buffering; otherwise we're just guaranteeing + // an "ErrorKind::WriteZero" later. + if flushed == 0 { + return Ok(0); + } // Don't try to reconstruct the exact amount written; just bail // in the event of a partial write @@ -1021,13 +1055,15 @@ impl<'a, W: Write> Write for LineWriterShim<'a, W> { // Now that the write has succeeded, buffer the rest (or as much of the // rest as possible) let buffered: usize = - tail.iter().map(|buf| self.inner.write_to_buffer(buf)).take_while(|&n| n > 0).sum(); + tail.iter().map(|buf| self.buffer.write_to_buffer(buf)).take_while(|&n| n > 0).sum(); Ok(flushed + buffered) } fn is_write_vectored(&self) -> bool { - self.inner.is_write_vectored() + // It's hard to imagine these diverging, but it's worth checking + // just in case, because we call `write_vectored` on both. + self.buffer.is_write_vectored() && self.inner().is_write_vectored() } /// Write some data into this BufReader with line buffering. This means @@ -1039,37 +1075,37 @@ impl<'a, W: Write> Write for LineWriterShim<'a, W> { /// writer, it will also flush the existing buffer if it contains any /// newlines, even if the incoming data does not contain any newlines. fn write_all(&mut self, buf: &[u8]) -> io::Result<()> { - match memchr::memrchr(b'\n', buf) { + let newline_idx = match memchr::memrchr(b'\n', buf) { // If there are no new newlines (that is, if this write is less than // one line), just do a regular buffered write None => { // Check for prior partial line writes that need to be retried. // Only retry if the buffer contains a completed line, to // avoid flushing partial lines. - if let Some(b'\n') = self.inner.buffer().last().copied() { - self.inner.flush_buf()?; + if let Some(b'\n') = self.buffered().last().copied() { + self.buffer.flush_buf()?; } - self.inner.write_all(buf) + return self.buffer.write_all(buf); } // Otherwise, arrange for the lines to be written directly to the // inner writer. - Some(newline_idx) => { - // Flush existing content to prepare for our write - self.inner.flush_buf()?; + Some(newline_idx) => newline_idx, + }; - // This is what we're going to try to write directly to the inner - // writer. The rest will be buffered, if nothing goes wrong. - let (lines, tail) = buf.split_at(newline_idx + 1); + // Flush existing content to prepare for our write + self.buffer.flush_buf()?; - // Write `lines` directly to the inner writer, bypassing the buffer. - self.inner.get_mut().write_all(lines)?; + // This is what we're going to try to write directly to the inner + // writer. The rest will be buffered, if nothing goes wrong. + let (lines, tail) = buf.split_at(newline_idx + 1); - // Now that the write has succeeded, buffer the rest with BufWriter::write_all. - // This will buffer as much as possible, but continue flushing as - // necessary if our tail is huge. - self.inner.write_all(tail) - } - } + // Write `lines` directly to the inner writer, bypassing the buffer. + self.inner_mut().write_all(lines)?; + + // Now that the write has succeeded, buffer the rest with + // BufWriter::write_all. This will buffer as much as possible, but + // continue flushing as necessary if our tail is huge. + self.buffer.write_all(tail) } } @@ -1310,7 +1346,7 @@ where #[cfg(test)] mod tests { use crate::io::prelude::*; - use crate::io::{self, BufReader, BufWriter, IoSlice, LineWriter, SeekFrom}; + use crate::io::{self, BufReader, BufWriter, ErrorKind, IoSlice, LineWriter, SeekFrom}; use crate::sync::atomic::{AtomicUsize, Ordering}; use crate::thread; @@ -1598,34 +1634,6 @@ mod tests { assert_eq!(v, []); } - #[test] - fn test_line_buffer_fail_flush() { - // Issue #32085 - struct FailFlushWriter<'a>(&'a mut Vec); - - impl Write for FailFlushWriter<'_> { - fn write(&mut self, buf: &[u8]) -> io::Result { - self.0.extend_from_slice(buf); - Ok(buf.len()) - } - fn flush(&mut self) -> io::Result<()> { - Err(io::Error::new(io::ErrorKind::Other, "flush failed")) - } - } - - let mut buf = Vec::new(); - { - let mut writer = LineWriter::new(FailFlushWriter(&mut buf)); - let to_write = b"abc\ndef"; - if let Ok(written) = writer.write(to_write) { - assert!(written < to_write.len(), "didn't flush on new line"); - // PASS - return; - } - } - assert!(buf.is_empty(), "write returned an error but wrote data"); - } - #[test] fn test_line_buffer() { let mut writer = LineWriter::new(Vec::new()); @@ -1746,44 +1754,62 @@ mod tests { b.iter(|| BufWriter::new(io::sink())); } + /// A simple `Write` target, designed to be wrapped by `LineWriter` / + /// `BufWriter` / etc, that can have its `write` & `flush` behavior + /// configured #[derive(Default, Clone)] struct ProgrammableSink { // Writes append to this slice - buffer: Vec, + pub buffer: Vec, - // Flushes set this flag - flushed: bool, + // Flush sets this flag + pub flushed: bool, // If true, writes & flushes will always be an error - return_error: bool, + pub always_error: bool, // If set, only up to this number of bytes will be written in a single // call to `write` - accept_prefix: Option, + pub accept_prefix: Option, // If set, counts down with each write, and writes return an error // when it hits 0 - max_writes: Option, + pub max_writes: Option, + + // If set, attempting to write when max_writes == Some(0) will be an + // error; otherwise, it will return Ok(0). + pub error_after_max_writes: bool, } impl Write for ProgrammableSink { fn write(&mut self, data: &[u8]) -> io::Result { - if self.return_error { - Err(io::Error::new(io::ErrorKind::Other, "test")) - } else { - let len = match self.accept_prefix { - None => data.len(), - Some(prefix) => prefix.min(prefix), - }; - let data = &data[..len]; - self.buffer.extend_from_slice(data); - Ok(len) + if self.always_error { + return Err(io::Error::new(io::ErrorKind::Other, "test - write always_error")); + } + + match self.max_writes { + Some(0) if self.error_after_max_writes => { + return Err(io::Error::new(io::ErrorKind::Other, "test - max_writes")) + } + Some(0) => return Ok(0), + Some(ref mut count) => *count -= 1, + None => {} } + + let len = match self.accept_prefix { + None => data.len(), + Some(prefix) => data.len().min(prefix), + }; + + let data = &data[..len]; + self.buffer.extend_from_slice(data); + + Ok(len) } fn flush(&mut self) -> io::Result<()> { - if self.return_error { - Err(io::Error::new(io::ErrorKind::Other, "test")) + if self.always_error { + Err(io::Error::new(io::ErrorKind::Other, "test - flush always_error")) } else { self.flushed = true; Ok(()) @@ -1802,7 +1828,27 @@ mod tests { /// Regression test for #37807 #[test] fn erroneous_flush_retried() { - todo!() + let writer = ProgrammableSink { + // Only write up to 4 bytes at a time + accept_prefix: Some(4), + + // Accept the first two writes, then error the others + max_writes: Some(2), + error_after_max_writes: true, + + ..Default::default() + }; + + // This should write the first 4 bytes. The rest will be buffered, out + // to the last newline. + let mut writer = LineWriter::new(writer); + assert_eq!(writer.write(b"a\nb\nc\nd\ne").unwrap(), 8); + + // This write should attempt to flush "c\nd\n", then buffer "e". No + // errors should happen here because no further writes should be + // attempted against `writer`. + assert_eq!(writer.write(b"e").unwrap(), 1); + assert_eq!(&writer.get_ref().buffer, b"a\nb\nc\nd\n"); } #[test] @@ -1927,7 +1973,14 @@ mod tests { /// This behavior is desirable because it prevents flushing partial lines #[test] fn test_partial_write_buffers_line() { - todo!() + let writer = ProgrammableSink { accept_prefix: Some(13), ..Default::default() }; + let mut writer = LineWriter::new(writer); + + assert_eq!(writer.write(b"Line 1\nLine 2\nLine 3\nLine4").unwrap(), 21); + assert_eq!(&writer.get_ref().buffer, b"Line 1\nLine 2"); + + assert_eq!(writer.write(b"Line 4").unwrap(), 6); + assert_eq!(&writer.get_ref().buffer, b"Line 1\nLine 2\nLine 3\n"); } /// Test that, given this input: @@ -1940,7 +1993,14 @@ mod tests { /// That data up to Line 3 is buffered #[test] fn test_partial_line_buffered_after_line_write() { - todo!() + let writer = ProgrammableSink::default(); + let mut writer = LineWriter::new(writer); + + assert_eq!(writer.write(b"Line 1\nLine 2\nLine 3").unwrap(), 20); + assert_eq!(&writer.get_ref().buffer, b"Line 1\nLine 2\n"); + + assert!(writer.flush().is_ok()); + assert_eq!(&writer.get_ref().buffer, b"Line 1\nLine 2\nLine 3"); } /// Test that, given a partial line that exceeds the length of @@ -1948,14 +2008,74 @@ mod tests { /// line is written to the inner writer #[test] fn test_long_line_flushed() { - todo!() + let writer = ProgrammableSink::default(); + let mut writer = LineWriter::with_capacity(5, writer); + + assert_eq!(writer.write(b"0123456789").unwrap(), 10); + assert_eq!(&writer.get_ref().buffer, b"0123456789"); } /// Test that, given a very long partial line *after* successfully /// flushing a complete line, that that line is buffered unconditionally, - /// and no additional writes take place + /// and no additional writes take place. This assures the property that + /// `write` should make at-most-one attempt to write new data. #[test] fn test_long_tail_not_flushed() { - todo!() + let writer = ProgrammableSink::default(); + let mut writer = LineWriter::with_capacity(5, writer); + + // Assert that Line 1\n is flushed, and 01234 is buffered + assert_eq!(writer.write(b"Line 1\n0123456789").unwrap(), 12); + assert_eq!(&writer.get_ref().buffer, b"Line 1\n"); + + // Because the buffer is full, this subsequent write will flush it + assert_eq!(writer.write(b"5").unwrap(), 1); + assert_eq!(&writer.get_ref().buffer, b"Line 1\n01234"); + } + + /// Test that, if an attempt to pre-flush buffered data returns Ok(0), + /// this is propagated as an error. + #[test] + fn test_line_buffer_write0_error() { + let writer = ProgrammableSink { + // Accept one write, then return Ok(0) on subsequent ones + max_writes: Some(1), + + ..Default::default() + }; + let mut writer = LineWriter::new(writer); + + // This should write "Line 1\n" and buffer "Partial" + assert_eq!(writer.write(b"Line 1\nPartial").unwrap(), 14); + assert_eq!(&writer.get_ref().buffer, b"Line 1\n"); + + // This will attempt to flush "partial", which will return Ok(0), which + // needs to be an error, because we've already informed the client + // that we accepted the write. + let err = writer.write(b" Line End\n").unwrap_err(); + assert_eq!(err.kind(), ErrorKind::WriteZero); + assert_eq!(&writer.get_ref().buffer, b"Line 1\n"); + } + + /// Test that, if a write returns Ok(0) after a successful pre-flush, this + /// is propogated as Ok(0) + #[test] + fn test_line_buffer_write0_normal() { + let writer = ProgrammableSink { + // Accept two writes, then return Ok(0) on subsequent ones + max_writes: Some(2), + + ..Default::default() + }; + let mut writer = LineWriter::new(writer); + + // This should write "Line 1\n" and buffer "Partial" + assert_eq!(writer.write(b"Line 1\nPartial").unwrap(), 14); + assert_eq!(&writer.get_ref().buffer, b"Line 1\n"); + + // This will flush partial, which will succeed, but then return Ok(0) + // when flushing " Line End\n" + assert_eq!(writer.write(b" Line End\n").unwrap(), 0); + assert_eq!(&writer.get_ref().buffer, b"Line 1\nPartial"); } } From b6296e88f0176255e67672a70c9761729e20f33f Mon Sep 17 00:00:00 2001 From: Nathan West Date: Tue, 2 Jun 2020 00:43:06 -0400 Subject: [PATCH 11/49] Tons of testing updates, other minor changes - Cleaned up BufWriter::seek - Updated line_vectored test - Updated line_vectored_partial_and_errors test - Added several new tests --- src/libstd/io/buffered.rs | 130 +++++++++++++++++++++++++++++++------- 1 file changed, 106 insertions(+), 24 deletions(-) diff --git a/src/libstd/io/buffered.rs b/src/libstd/io/buffered.rs index 730ae8a0ee471..ced84b777ce77 100644 --- a/src/libstd/io/buffered.rs +++ b/src/libstd/io/buffered.rs @@ -751,7 +751,8 @@ impl Seek for BufWriter { /// /// Seeking always writes out the internal buffer before seeking. fn seek(&mut self, pos: SeekFrom) -> io::Result { - self.flush_buf().and_then(|_| self.get_mut().seek(pos)) + self.flush_buf()?; + self.get_mut().seek(pos) } } @@ -1862,12 +1863,13 @@ mod tests { IoSlice::new(b"a"), ]) .unwrap(), - 2, + 1, ); assert_eq!(a.get_ref(), b"\n"); assert_eq!( a.write_vectored(&[ + IoSlice::new(b"a"), IoSlice::new(&[]), IoSlice::new(b"b"), IoSlice::new(&[]), @@ -1876,7 +1878,7 @@ mod tests { IoSlice::new(b"c"), ]) .unwrap(), - 3, + 4, ); assert_eq!(a.get_ref(), b"\n"); a.flush().unwrap(); @@ -1893,17 +1895,21 @@ mod tests { 0, ); assert_eq!(a.write_vectored(&[IoSlice::new(b"a\nb"),]).unwrap(), 3); - assert_eq!(a.get_ref(), b"\nabaca\n"); + assert_eq!(a.get_ref(), b"\nabaca\nb"); } #[test] fn line_vectored_partial_and_errors() { + use crate::collections::VecDeque; + enum Call { Write { inputs: Vec<&'static [u8]>, output: io::Result }, Flush { output: io::Result<()> }, } + + #[derive(Default)] struct Writer { - calls: Vec, + calls: VecDeque, } impl Write for Writer { @@ -1912,19 +1918,23 @@ mod tests { } fn write_vectored(&mut self, buf: &[IoSlice<'_>]) -> io::Result { - match self.calls.pop().unwrap() { + match self.calls.pop_front().expect("unexpected call to write") { Call::Write { inputs, output } => { assert_eq!(inputs, buf.iter().map(|b| &**b).collect::>()); output } - _ => panic!("unexpected call to write"), + Call::Flush { .. } => panic!("unexpected call to write; expected a flush"), } } + fn is_write_vectored(&self) -> bool { + true + } + fn flush(&mut self) -> io::Result<()> { - match self.calls.pop().unwrap() { + match self.calls.pop_front().expect("Unexpected call to flush") { Call::Flush { output } => output, - _ => panic!("unexpected call to flush"), + Call::Write { .. } => panic!("unexpected call to flush; expected a write"), } } } @@ -1938,20 +1948,22 @@ mod tests { } // partial writes keep going - let mut a = LineWriter::new(Writer { calls: Vec::new() }); + let mut a = LineWriter::new(Writer::default()); a.write_vectored(&[IoSlice::new(&[]), IoSlice::new(b"abc")]).unwrap(); - a.get_mut().calls.push(Call::Flush { output: Ok(()) }); - a.get_mut().calls.push(Call::Write { inputs: vec![b"bcx\n"], output: Ok(4) }); - a.get_mut().calls.push(Call::Write { inputs: vec![b"abcx\n"], output: Ok(1) }); + + a.get_mut().calls.push_back(Call::Write { inputs: vec![b"abc"], output: Ok(1) }); + a.get_mut().calls.push_back(Call::Write { inputs: vec![b"bc"], output: Ok(2) }); + a.get_mut().calls.push_back(Call::Write { inputs: vec![b"x", b"\n"], output: Ok(2) }); + a.write_vectored(&[IoSlice::new(b"x"), IoSlice::new(b"\n")]).unwrap(); - a.get_mut().calls.push(Call::Flush { output: Ok(()) }); + + a.get_mut().calls.push_back(Call::Flush { output: Ok(()) }); a.flush().unwrap(); // erroneous writes stop and don't write more - a.get_mut().calls.push(Call::Write { inputs: vec![b"x\n"], output: Err(err()) }); - assert_eq!(a.write_vectored(&[IoSlice::new(b"x"), IoSlice::new(b"\na")]).unwrap(), 2); - a.get_mut().calls.push(Call::Flush { output: Ok(()) }); - a.get_mut().calls.push(Call::Write { inputs: vec![b"x\n"], output: Ok(2) }); + a.get_mut().calls.push_back(Call::Write { inputs: vec![b"x", b"\na"], output: Err(err()) }); + a.get_mut().calls.push_back(Call::Flush { output: Ok(()) }); + assert!(a.write_vectored(&[IoSlice::new(b"x"), IoSlice::new(b"\na")]).is_err()); a.flush().unwrap(); fn err() -> io::Error { @@ -1959,6 +1971,41 @@ mod tests { } } + /// Test that, in cases where vectored writing is not enabled, the + /// LineWriter uses the normal `write` call, which more-corectly handles + /// partial lines + #[test] + fn line_vectored_ignored() { + let writer = ProgrammableSink::default(); + let mut writer = LineWriter::new(writer); + + let content = [ + IoSlice::new(b"Line 1\nLine"), + IoSlice::new(b" 2\nLine 3\nL"), + IoSlice::new(b"ine 4"), + IoSlice::new(b"\nLine 5\n"), + ]; + + let count = writer.write_vectored(&content).unwrap(); + assert_eq!(count, 11); + assert_eq!(&writer.get_ref().buffer, b"Line 1\n"); + + let count = writer.write_vectored(&content[1..]).unwrap(); + assert_eq!(count, 11); + assert_eq!(&writer.get_ref().buffer, b"Line 1\nLine 2\nLine 3\n"); + + let count = writer.write_vectored(&content[2..]).unwrap(); + assert_eq!(count, 5); + assert_eq!(&writer.get_ref().buffer, b"Line 1\nLine 2\nLine 3\n"); + + let count = writer.write_vectored(&content[3..]).unwrap(); + assert_eq!(count, 8); + assert_eq!( + writer.get_ref().buffer.as_slice(), + b"Line 1\nLine 2\nLine 3\nLine 4\n Line 5".as_ref() + ); + } + /// Test that, given this input: /// /// Line 1\n @@ -1972,7 +2019,7 @@ mod tests { /// /// This behavior is desirable because it prevents flushing partial lines #[test] - fn test_partial_write_buffers_line() { + fn partial_write_buffers_line() { let writer = ProgrammableSink { accept_prefix: Some(13), ..Default::default() }; let mut writer = LineWriter::new(writer); @@ -1992,7 +2039,7 @@ mod tests { /// And given that the full write of lines 1 and 2 was successful /// That data up to Line 3 is buffered #[test] - fn test_partial_line_buffered_after_line_write() { + fn partial_line_buffered_after_line_write() { let writer = ProgrammableSink::default(); let mut writer = LineWriter::new(writer); @@ -2007,7 +2054,7 @@ mod tests { /// LineBuffer's buffer (that is, without a trailing newline), that that /// line is written to the inner writer #[test] - fn test_long_line_flushed() { + fn long_line_flushed() { let writer = ProgrammableSink::default(); let mut writer = LineWriter::with_capacity(5, writer); @@ -2020,7 +2067,7 @@ mod tests { /// and no additional writes take place. This assures the property that /// `write` should make at-most-one attempt to write new data. #[test] - fn test_long_tail_not_flushed() { + fn line_long_tail_not_flushed() { let writer = ProgrammableSink::default(); let mut writer = LineWriter::with_capacity(5, writer); @@ -2036,7 +2083,7 @@ mod tests { /// Test that, if an attempt to pre-flush buffered data returns Ok(0), /// this is propagated as an error. #[test] - fn test_line_buffer_write0_error() { + fn line_buffer_write0_error() { let writer = ProgrammableSink { // Accept one write, then return Ok(0) on subsequent ones max_writes: Some(1), @@ -2060,7 +2107,7 @@ mod tests { /// Test that, if a write returns Ok(0) after a successful pre-flush, this /// is propogated as Ok(0) #[test] - fn test_line_buffer_write0_normal() { + fn line_buffer_write0_normal() { let writer = ProgrammableSink { // Accept two writes, then return Ok(0) on subsequent ones max_writes: Some(2), @@ -2078,4 +2125,39 @@ mod tests { assert_eq!(writer.write(b" Line End\n").unwrap(), 0); assert_eq!(&writer.get_ref().buffer, b"Line 1\nPartial"); } + + /// LineWriter has a custom `write_all`; make sure it works correctly + #[test] + fn line_write_all() { + let writer = ProgrammableSink { + // Only write 5 bytes at a time + accept_prefix: Some(5), + ..Default::default() + }; + let mut writer = LineWriter::new(writer); + + writer.write_all(b"Line 1\nLine 2\nLine 3\nLine 4\nPartial").unwrap(); + assert_eq!(&writer.get_ref().buffer, b"Line 1\nLine 2\nLine 3\nLine 4\n"); + writer.write_all(b" Line 5\n").unwrap(); + assert_eq!( + writer.get_ref().buffer.as_slice(), + b"Line 1\nLine 2\nLine 3\nLine 4\nPartial Line 5\n".as_ref(), + ); + } + + #[test] + fn line_write_all_error() { + let writer = ProgrammableSink { + // Only accept up to 3 writes of up to 5 bytes each + accept_prefix: Some(5), + max_writes: Some(3), + ..Default::default() + }; + + let mut writer = LineWriter::new(writer); + let res = writer.write_all(b"Line 1\nLine 2\nLine 3\nLine 4\nPartial"); + assert!(res.is_err()); + // An error from write_all leaves everything in an indeterminate state, + // so there's nothing else to test here + } } From e022d3452de74b5596810f0aa26193e262de725b Mon Sep 17 00:00:00 2001 From: Nathan West Date: Tue, 2 Jun 2020 00:44:16 -0400 Subject: [PATCH 12/49] Fixed typo in test --- src/libstd/io/buffered.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libstd/io/buffered.rs b/src/libstd/io/buffered.rs index ced84b777ce77..abfc2fc37c4f9 100644 --- a/src/libstd/io/buffered.rs +++ b/src/libstd/io/buffered.rs @@ -2002,7 +2002,7 @@ mod tests { assert_eq!(count, 8); assert_eq!( writer.get_ref().buffer.as_slice(), - b"Line 1\nLine 2\nLine 3\nLine 4\n Line 5".as_ref() + b"Line 1\nLine 2\nLine 3\nLine 4\nLine 5\n".as_ref() ); } From e4328ae54573aa42051246b675bf5280ea58eddf Mon Sep 17 00:00:00 2001 From: Nathan West Date: Tue, 2 Jun 2020 01:01:30 -0400 Subject: [PATCH 13/49] Code review updates: all minor style fixes - Renamed write_to_buffer to write_to_buf, for consistency - Fixed references to flush_buf - Optimized `write` to use one less `memchr` call --- src/libstd/io/buffered.rs | 26 ++++++++++++++------------ 1 file changed, 14 insertions(+), 12 deletions(-) diff --git a/src/libstd/io/buffered.rs b/src/libstd/io/buffered.rs index abfc2fc37c4f9..7b7c8c6e3177a 100644 --- a/src/libstd/io/buffered.rs +++ b/src/libstd/io/buffered.rs @@ -565,7 +565,7 @@ impl BufWriter { /// data. Writes as much as possible without exceeding capacity. Returns /// the number of bytes written. #[inline] - fn write_to_buffer(&mut self, buf: &[u8]) -> usize { + fn write_to_buf(&mut self, buf: &[u8]) -> usize { let available = self.buf.capacity() - self.buf.len(); let amt_to_buffer = available.min(buf.len()); self.buf.extend_from_slice(&buf[..amt_to_buffer]); @@ -689,7 +689,7 @@ impl Write for BufWriter { self.panicked = false; r } else { - Ok(self.write_to_buffer(buf)) + Ok(self.write_to_buf(buf)) } } @@ -703,7 +703,7 @@ impl Write for BufWriter { self.panicked = false; r } else { - self.write_to_buffer(buf); + self.write_to_buf(buf); Ok(()) } } @@ -861,7 +861,7 @@ impl fmt::Display for IntoInnerError { /// Private helper struct for implementing the line-buffered writing logic. /// This shim temporarily wraps a BufWriter, and uses its internals to /// implement a line-buffered writer (specifically by using the internal -/// methods like write_to_buffer and flush_buffer). In this way, a more +/// methods like write_to_buf and flush_buf). In this way, a more /// efficient abstraction can be created than one that only had access to /// `write` and `flush`, without needlessly duplicating a lot of the /// implementation details of BufWriter. This also allows existing @@ -925,7 +925,7 @@ impl<'a, W: Write> Write for LineWriterShim<'a, W> { } // Otherwise, arrange for the lines to be written directly to the // inner writer. - Some(newline_idx) => newline_idx, + Some(newline_idx) => newline_idx + 1, }; // Flush existing content to prepare for our write @@ -933,7 +933,7 @@ impl<'a, W: Write> Write for LineWriterShim<'a, W> { // This is what we're going to try to write directly to the inner // writer. The rest will be buffered, if nothing goes wrong. - let lines = &buf[..newline_idx + 1]; + let lines = &buf[..newline_idx]; // Write `lines` directly to the inner writer. In keeping with the // `write` convention, make at most one attempt to add new (unbuffered) @@ -953,11 +953,10 @@ impl<'a, W: Write> Write for LineWriterShim<'a, W> { // the rest as possible). If there were any unwritten newlines, we // only buffer out to the last unwritten newline; this helps prevent // flushing partial lines on subsequent calls to LineWriterShim::write. - let tail = &buf[flushed..]; - let buffered = match memchr::memrchr(b'\n', tail) { - None => self.buffer.write_to_buffer(tail), - Some(i) => self.buffer.write_to_buffer(&tail[..i + 1]), - }; + let tail = + if flushed < newline_idx { &buf[flushed..newline_idx] } else { &buf[newline_idx..] }; + + let buffered = self.buffer.write_to_buf(tail); Ok(flushed + buffered) } @@ -1056,7 +1055,7 @@ impl<'a, W: Write> Write for LineWriterShim<'a, W> { // Now that the write has succeeded, buffer the rest (or as much of the // rest as possible) let buffered: usize = - tail.iter().map(|buf| self.buffer.write_to_buffer(buf)).take_while(|&n| n > 0).sum(); + tail.iter().map(|buf| self.buffer.write_to_buf(buf)).take_while(|&n| n > 0).sum(); Ok(flushed + buffered) } @@ -1076,6 +1075,9 @@ impl<'a, W: Write> Write for LineWriterShim<'a, W> { /// writer, it will also flush the existing buffer if it contains any /// newlines, even if the incoming data does not contain any newlines. fn write_all(&mut self, buf: &[u8]) -> io::Result<()> { + // The default `write_all` calls `write` in a loop; we can do better + // by simply calling self.inner().write_all directly. This avoids + // round trips to `self.buffer` in the event of partial writes. let newline_idx = match memchr::memrchr(b'\n', buf) { // If there are no new newlines (that is, if this write is less than // one line), just do a regular buffered write From f7650fe3f955863e810ef7816edbe78f88393d87 Mon Sep 17 00:00:00 2001 From: Nathan West Date: Tue, 2 Jun 2020 01:07:34 -0400 Subject: [PATCH 14/49] Add comment --- src/libstd/io/buffered.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/libstd/io/buffered.rs b/src/libstd/io/buffered.rs index 7b7c8c6e3177a..b9839c318b75a 100644 --- a/src/libstd/io/buffered.rs +++ b/src/libstd/io/buffered.rs @@ -1358,6 +1358,9 @@ mod tests { lengths: Vec, } + // FIXME: rustfmt and tidy disagree about the correct formatting of this + // function. This leads to issues for users with editors configured to + // rustfmt-on-save. impl Read for ShortReader { fn read(&mut self, _: &mut [u8]) -> io::Result { if self.lengths.is_empty() { From 7a6a12bdf42804e2e2ef6437bed6a45a1ede51b2 Mon Sep 17 00:00:00 2001 From: Nathan West Date: Tue, 2 Jun 2020 01:09:55 -0400 Subject: [PATCH 15/49] Tidy fixes --- src/libstd/io/buffered.rs | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/src/libstd/io/buffered.rs b/src/libstd/io/buffered.rs index b9839c318b75a..560c8f2369846 100644 --- a/src/libstd/io/buffered.rs +++ b/src/libstd/io/buffered.rs @@ -1363,11 +1363,7 @@ mod tests { // rustfmt-on-save. impl Read for ShortReader { fn read(&mut self, _: &mut [u8]) -> io::Result { - if self.lengths.is_empty() { - Ok(0) - } else { - Ok(self.lengths.remove(0)) - } + if self.lengths.is_empty() { Ok(0) } else { Ok(self.lengths.remove(0)) } } } @@ -1795,7 +1791,7 @@ mod tests { match self.max_writes { Some(0) if self.error_after_max_writes => { - return Err(io::Error::new(io::ErrorKind::Other, "test - max_writes")) + return Err(io::Error::new(io::ErrorKind::Other, "test - max_writes")); } Some(0) => return Ok(0), Some(ref mut count) => *count -= 1, From 338a2c02e4b9cc5b1fae698ab019724340b26967 Mon Sep 17 00:00:00 2001 From: Nathan West Date: Tue, 2 Jun 2020 01:28:31 -0400 Subject: [PATCH 16/49] Reimplement flush_buf with a Guard. Longer, but cleaner. --- src/libstd/io/buffered.rs | 67 +++++++++++++++++++++++++++++---------- 1 file changed, 50 insertions(+), 17 deletions(-) diff --git a/src/libstd/io/buffered.rs b/src/libstd/io/buffered.rs index 560c8f2369846..b7e848b04d721 100644 --- a/src/libstd/io/buffered.rs +++ b/src/libstd/io/buffered.rs @@ -448,6 +448,9 @@ impl Seek for BufReader { /// [`flush`]: #method.flush #[stable(feature = "rust1", since = "1.0.0")] pub struct BufWriter { + // FIXME: Can this just be W, instead of Option? I don't see any code + // paths that lead to this being None, or that ever check if it IS none, + // even in drop implementations. inner: Option, // FIXME: Replace this with a VecDeque. Because VecDeque is a Ring buffer, // this would enable BufWriter to operate without any interior copies. @@ -533,32 +536,62 @@ impl BufWriter { /// `write`), any 0-length writes from `inner` must be reported as i/o /// errors from this method. fn flush_buf(&mut self) -> io::Result<()> { - let mut written = 0; - let len = self.buf.len(); - let mut ret = Ok(()); - while written < len { + /// Helper struct to ensure the buffer is updated after all the writes + /// are complete + struct BufGuard<'a> { + buffer: &'a mut Vec, + written: usize, + } + + impl<'a> BufGuard<'a> { + fn new(buffer: &'a mut Vec) -> Self { + Self { buffer, written: 0 } + } + + /// The unwritten part of the buffer + fn remaining(&self) -> &[u8] { + &self.buffer[self.written..] + } + + /// Flag some bytes as removed from the front of the buffer + fn consume(&mut self, amt: usize) { + self.written += amt; + } + + /// true if all of the bytes have been written + fn done(&self) -> bool { + self.written >= self.buffer.len() + } + } + + impl Drop for BufGuard<'_> { + fn drop(&mut self) { + if self.written > 0 { + self.buffer.drain(..self.written); + } + } + } + + let mut guard = BufGuard::new(&mut self.buf); + let inner = self.inner.as_mut().unwrap(); + while !guard.done() { self.panicked = true; - let r = self.inner.as_mut().unwrap().write(&self.buf[written..]); + let r = inner.write(guard.remaining()); self.panicked = false; match r { Ok(0) => { - ret = - Err(Error::new(ErrorKind::WriteZero, "failed to write the buffered data")); - break; + return Err(Error::new( + ErrorKind::WriteZero, + "failed to write the buffered data", + )) } - Ok(n) => written += n, + Ok(n) => guard.consume(n), Err(ref e) if e.kind() == io::ErrorKind::Interrupted => {} - Err(e) => { - ret = Err(e); - break; - } + Err(e) => return Err(e), } } - if written > 0 { - self.buf.drain(..written); - } - ret + Ok(()) } /// Buffer some data without flushing it, regardless of the size of the From c869638cba8e9ab8392016b11675706c4e514b6e Mon Sep 17 00:00:00 2001 From: Nathan West Date: Tue, 2 Jun 2020 01:30:40 -0400 Subject: [PATCH 17/49] Added comment about BufWrite::write_all --- src/libstd/io/buffered.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/libstd/io/buffered.rs b/src/libstd/io/buffered.rs index b7e848b04d721..f9bde7897e997 100644 --- a/src/libstd/io/buffered.rs +++ b/src/libstd/io/buffered.rs @@ -727,6 +727,10 @@ impl Write for BufWriter { } fn write_all(&mut self, buf: &[u8]) -> io::Result<()> { + // Normally, `write_all` just calls `write` in a loop. We can do better + // by calling `self.get_mut().write_all()` directly, which avoids + // round trips through the buffer in the event of a series of partial + // writes. if self.buf.len() + buf.len() > self.buf.capacity() { self.flush_buf()?; } From 61f591e1738845911048ff43d956bb9d39987910 Mon Sep 17 00:00:00 2001 From: Nathan West Date: Tue, 2 Jun 2020 01:36:40 -0400 Subject: [PATCH 18/49] Improved line_vectored_ignored. Added stylistic semicolon. --- src/libstd/io/buffered.rs | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/src/libstd/io/buffered.rs b/src/libstd/io/buffered.rs index f9bde7897e997..02615bffe3bd1 100644 --- a/src/libstd/io/buffered.rs +++ b/src/libstd/io/buffered.rs @@ -584,7 +584,7 @@ impl BufWriter { return Err(Error::new( ErrorKind::WriteZero, "failed to write the buffered data", - )) + )); } Ok(n) => guard.consume(n), Err(ref e) if e.kind() == io::ErrorKind::Interrupted => {} @@ -2018,8 +2018,11 @@ mod tests { let mut writer = LineWriter::new(writer); let content = [ + IoSlice::new(&[]), IoSlice::new(b"Line 1\nLine"), IoSlice::new(b" 2\nLine 3\nL"), + IoSlice::new(&[]), + IoSlice::new(&[]), IoSlice::new(b"ine 4"), IoSlice::new(b"\nLine 5\n"), ]; @@ -2028,15 +2031,15 @@ mod tests { assert_eq!(count, 11); assert_eq!(&writer.get_ref().buffer, b"Line 1\n"); - let count = writer.write_vectored(&content[1..]).unwrap(); + let count = writer.write_vectored(&content[2..]).unwrap(); assert_eq!(count, 11); assert_eq!(&writer.get_ref().buffer, b"Line 1\nLine 2\nLine 3\n"); - let count = writer.write_vectored(&content[2..]).unwrap(); + let count = writer.write_vectored(&content[5..]).unwrap(); assert_eq!(count, 5); assert_eq!(&writer.get_ref().buffer, b"Line 1\nLine 2\nLine 3\n"); - let count = writer.write_vectored(&content[3..]).unwrap(); + let count = writer.write_vectored(&content[6..]).unwrap(); assert_eq!(count, 8); assert_eq!( writer.get_ref().buffer.as_slice(), From 2d22c7741816aa391f619f31b14b1ca01cc31b61 Mon Sep 17 00:00:00 2001 From: Nathan West Date: Tue, 2 Jun 2020 01:43:10 -0400 Subject: [PATCH 19/49] Fixed bug in write_vectored & empty buffers --- src/libstd/io/buffered.rs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/libstd/io/buffered.rs b/src/libstd/io/buffered.rs index 02615bffe3bd1..5189b39aaae68 100644 --- a/src/libstd/io/buffered.rs +++ b/src/libstd/io/buffered.rs @@ -1091,8 +1091,12 @@ impl<'a, W: Write> Write for LineWriterShim<'a, W> { // Now that the write has succeeded, buffer the rest (or as much of the // rest as possible) - let buffered: usize = - tail.iter().map(|buf| self.buffer.write_to_buf(buf)).take_while(|&n| n > 0).sum(); + let buffered: usize = tail + .iter() + .filter(|buf| !buf.is_empty()) + .map(|buf| self.buffer.write_to_buf(buf)) + .take_while(|&n| n > 0) + .sum(); Ok(flushed + buffered) } From 2c23b9066f761e07f1c4db6ca8d8f0bf48e4e296 Mon Sep 17 00:00:00 2001 From: Nathan West Date: Tue, 2 Jun 2020 11:56:02 -0400 Subject: [PATCH 20/49] Comment updates --- src/libstd/io/buffered.rs | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/libstd/io/buffered.rs b/src/libstd/io/buffered.rs index 5189b39aaae68..51098f6ff6af5 100644 --- a/src/libstd/io/buffered.rs +++ b/src/libstd/io/buffered.rs @@ -730,7 +730,7 @@ impl Write for BufWriter { // Normally, `write_all` just calls `write` in a loop. We can do better // by calling `self.get_mut().write_all()` directly, which avoids // round trips through the buffer in the event of a series of partial - // writes. + // writes in some circumstances. if self.buf.len() + buf.len() > self.buf.capacity() { self.flush_buf()?; } @@ -1116,9 +1116,6 @@ impl<'a, W: Write> Write for LineWriterShim<'a, W> { /// writer, it will also flush the existing buffer if it contains any /// newlines, even if the incoming data does not contain any newlines. fn write_all(&mut self, buf: &[u8]) -> io::Result<()> { - // The default `write_all` calls `write` in a loop; we can do better - // by simply calling self.inner().write_all directly. This avoids - // round trips to `self.buffer` in the event of partial writes. let newline_idx = match memchr::memrchr(b'\n', buf) { // If there are no new newlines (that is, if this write is less than // one line), just do a regular buffered write From e999ca5ac3a21183fdc8cc99350752d0f1ef0bdd Mon Sep 17 00:00:00 2001 From: Nathan West Date: Tue, 2 Jun 2020 12:00:12 -0400 Subject: [PATCH 21/49] Remove inline from write_to_buf --- src/libstd/io/buffered.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/libstd/io/buffered.rs b/src/libstd/io/buffered.rs index 51098f6ff6af5..a66fa6ea36a28 100644 --- a/src/libstd/io/buffered.rs +++ b/src/libstd/io/buffered.rs @@ -597,7 +597,6 @@ impl BufWriter { /// Buffer some data without flushing it, regardless of the size of the /// data. Writes as much as possible without exceeding capacity. Returns /// the number of bytes written. - #[inline] fn write_to_buf(&mut self, buf: &[u8]) -> usize { let available = self.buf.capacity() - self.buf.len(); let amt_to_buffer = available.min(buf.len()); From 70ba320052669c06e2285d16b4a36af83f9c83da Mon Sep 17 00:00:00 2001 From: Nathan West Date: Tue, 2 Jun 2020 12:40:05 -0400 Subject: [PATCH 22/49] More minor changes - Fixed test after write_vectored bugfix - Some comments --- src/libstd/io/buffered.rs | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/libstd/io/buffered.rs b/src/libstd/io/buffered.rs index a66fa6ea36a28..c599bd8b55127 100644 --- a/src/libstd/io/buffered.rs +++ b/src/libstd/io/buffered.rs @@ -715,6 +715,7 @@ impl Write for BufWriter { if self.buf.len() + buf.len() > self.buf.capacity() { self.flush_buf()?; } + // FIXME: Why no len > capacity? Why not buffer len == capacity? if buf.len() >= self.buf.capacity() { self.panicked = true; let r = self.get_mut().write(buf); @@ -733,6 +734,7 @@ impl Write for BufWriter { if self.buf.len() + buf.len() > self.buf.capacity() { self.flush_buf()?; } + // FIXME: Why no len > capacity? Why not buffer len == capacity? if buf.len() >= self.buf.capacity() { self.panicked = true; let r = self.get_mut().write_all(buf); @@ -749,6 +751,7 @@ impl Write for BufWriter { if self.buf.len() + total_len > self.buf.capacity() { self.flush_buf()?; } + // FIXME: Why no len > capacity? Why not buffer len == capacity? if total_len >= self.buf.capacity() { self.panicked = true; let r = self.get_mut().write_vectored(bufs); @@ -1901,13 +1904,12 @@ mod tests { IoSlice::new(b"a"), ]) .unwrap(), - 1, + 2, ); assert_eq!(a.get_ref(), b"\n"); assert_eq!( a.write_vectored(&[ - IoSlice::new(b"a"), IoSlice::new(&[]), IoSlice::new(b"b"), IoSlice::new(&[]), @@ -1916,7 +1918,7 @@ mod tests { IoSlice::new(b"c"), ]) .unwrap(), - 4, + 3, ); assert_eq!(a.get_ref(), b"\n"); a.flush().unwrap(); From 5b1a40c18168a8049eb9f3af3178ec1846e64730 Mon Sep 17 00:00:00 2001 From: Nathan West Date: Tue, 2 Jun 2020 12:58:49 -0400 Subject: [PATCH 23/49] BufWriter::write* methods now use fewer runtime checks --- src/libstd/io/buffered.rs | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/src/libstd/io/buffered.rs b/src/libstd/io/buffered.rs index c599bd8b55127..1998cdde8d08e 100644 --- a/src/libstd/io/buffered.rs +++ b/src/libstd/io/buffered.rs @@ -722,7 +722,8 @@ impl Write for BufWriter { self.panicked = false; r } else { - Ok(self.write_to_buf(buf)) + self.buf.extend_from_slice(buf); + Ok(buf.len()) } } @@ -741,7 +742,7 @@ impl Write for BufWriter { self.panicked = false; r } else { - self.write_to_buf(buf); + self.buf.extend_from_slice(buf); Ok(()) } } @@ -758,7 +759,8 @@ impl Write for BufWriter { self.panicked = false; r } else { - self.buf.write_vectored(bufs) + bufs.iter().for_each(|b| self.buf.extend_from_slice(b)); + Ok(total_len) } } @@ -1403,7 +1405,11 @@ mod tests { // rustfmt-on-save. impl Read for ShortReader { fn read(&mut self, _: &mut [u8]) -> io::Result { - if self.lengths.is_empty() { Ok(0) } else { Ok(self.lengths.remove(0)) } + if self.lengths.is_empty() { + Ok(0) + } else { + Ok(self.lengths.remove(0)) + } } } From 8df5ae0fffc9de2884e0c916bdcd74cb69949b1c Mon Sep 17 00:00:00 2001 From: Nathan West Date: Tue, 2 Jun 2020 14:36:03 -0400 Subject: [PATCH 24/49] x.py fix AGAIN --- src/libstd/io/buffered.rs | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/src/libstd/io/buffered.rs b/src/libstd/io/buffered.rs index 1998cdde8d08e..61ad5d0c274ff 100644 --- a/src/libstd/io/buffered.rs +++ b/src/libstd/io/buffered.rs @@ -1405,11 +1405,7 @@ mod tests { // rustfmt-on-save. impl Read for ShortReader { fn read(&mut self, _: &mut [u8]) -> io::Result { - if self.lengths.is_empty() { - Ok(0) - } else { - Ok(self.lengths.remove(0)) - } + if self.lengths.is_empty() { Ok(0) } else { Ok(self.lengths.remove(0)) } } } From 60ab99f9bdcb33ea025aca6a94e34f2ae5b5b75e Mon Sep 17 00:00:00 2001 From: Nathan West Date: Tue, 2 Jun 2020 15:57:20 -0400 Subject: [PATCH 25/49] Fixed corner case related to partial-line buffering - Fixed partial-line buffering issue - Added tests Thanks @the8472 for catching! --- src/libstd/io/buffered.rs | 93 +++++++++++++++++++++++++++++++++++---- 1 file changed, 85 insertions(+), 8 deletions(-) diff --git a/src/libstd/io/buffered.rs b/src/libstd/io/buffered.rs index 61ad5d0c274ff..d094f19b5a5ff 100644 --- a/src/libstd/io/buffered.rs +++ b/src/libstd/io/buffered.rs @@ -994,8 +994,26 @@ impl<'a, W: Write> Write for LineWriterShim<'a, W> { // the rest as possible). If there were any unwritten newlines, we // only buffer out to the last unwritten newline; this helps prevent // flushing partial lines on subsequent calls to LineWriterShim::write. - let tail = - if flushed < newline_idx { &buf[flushed..newline_idx] } else { &buf[newline_idx..] }; + + // Handle the cases in order of most-common to least-common, under + // the presumption that most writes succeed in totality, and that most + // writes are smaller than the buffer. + // - Is this a partial line (ie, no newlines left in the unwritten tail) + // - If not, does the data out to the last unwritten newline fit in + // the buffer? + // - If not, scan for the last newline that *does* fit in the buffer + let tail = if flushed >= newline_idx { + &buf[flushed..] + } else if newline_idx - flushed <= self.buffer.capacity() { + &buf[flushed..newline_idx] + } else { + let scan_area = &buf[flushed..]; + let scan_area = &scan_area[..self.buffer.capacity()]; + match memchr::memrchr(b'\n', scan_area) { + Some(newline_idx) => &scan_area[..newline_idx + 1], + None => scan_area, + } + }; let buffered = self.buffer.write_to_buf(tail); Ok(flushed + buffered) @@ -1809,8 +1827,11 @@ mod tests { // Flush sets this flag pub flushed: bool, - // If true, writes & flushes will always be an error - pub always_error: bool, + // If true, writes will always be an error + pub always_write_error: bool, + + // If true, flushes will always be an error + pub always_flush_error: bool, // If set, only up to this number of bytes will be written in a single // call to `write` @@ -1827,8 +1848,8 @@ mod tests { impl Write for ProgrammableSink { fn write(&mut self, data: &[u8]) -> io::Result { - if self.always_error { - return Err(io::Error::new(io::ErrorKind::Other, "test - write always_error")); + if self.always_write_error { + return Err(io::Error::new(io::ErrorKind::Other, "test - always_write_error")); } match self.max_writes { @@ -1852,8 +1873,8 @@ mod tests { } fn flush(&mut self) -> io::Result<()> { - if self.always_error { - Err(io::Error::new(io::ErrorKind::Other, "test - flush always_error")) + if self.always_flush_error { + Err(io::Error::new(io::ErrorKind::Other, "test - always_flush_error")) } else { self.flushed = true; Ok(()) @@ -2205,4 +2226,60 @@ mod tests { // An error from write_all leaves everything in an indeterminate state, // so there's nothing else to test here } + + /// Under certain circumstances, the old implementation of LineWriter + /// would try to buffer "to the last newline" but be forced to buffer + /// less than that, leading to inappropriate partial line writes. + /// Regression test for that issue. + #[test] + fn partial_multiline_buffering() { + let writer = ProgrammableSink { + // Write only up to 5 bytes at a time + accept_prefix: Some(5), + ..Default::default() + }; + + let mut writer = LineWriter::with_capacity(10, writer); + + let content = b"AAAAABBBBB\nCCCCDDDDDD\nEEE"; + + // When content is written, LineWriter will try to write blocks A, B, + // C, and D. Only block A will succeed. Under the old behavior, LineWriter + // would then try to buffer B, C and D, but because its capacity is 10, + // it will only be able to buffer B and C. We don't want it to buffer + // partial lines if it can avoid it, so the correct behavior is to + // only buffer block B (with its newline). + assert_eq!(writer.write(content).unwrap(), 11); + assert_eq!(writer.get_ref().buffer, *b"AAAAA"); + + writer.flush().unwrap(); + assert_eq!(writer.get_ref().buffer, *b"AAAAABBBBB\n"); + } + + /// Same as test_partial_multiline_buffering, but in the event NO full lines + /// fit in the buffer, just buffer as much as possible + #[test] + fn partial_multiline_buffering_without_full_line() { + let writer = ProgrammableSink { + // Write only up to 5 bytes at a time + accept_prefix: Some(5), + ..Default::default() + }; + + let mut writer = LineWriter::with_capacity(5, writer); + + let content = b"AAAAABBBBBBBBBB\nCCCCC\nDDDDD"; + + // When content is written, LineWriter will try to write blocks A, B, + // and C. Only block A will succeed. Under the old behavior, LineWriter + // would then try to buffer B and C, but because its capacity is 5, + // it will only be able to buffer part of B. Because it's not possible + // for it to buffer any complete lines, it should buffer as much of B as + // possible + assert_eq!(writer.write(content).unwrap(), 10); + assert_eq!(writer.get_ref().buffer, *b"AAAAA"); + + writer.flush().unwrap(); + assert_eq!(writer.get_ref().buffer, *b"AAAAABBBBB"); + } } From 38017a31e7728c359487817008c913a8e461a857 Mon Sep 17 00:00:00 2001 From: Nathan West Date: Tue, 2 Jun 2020 16:17:42 -0400 Subject: [PATCH 26/49] Update comments with relevant issue numbers --- src/libstd/io/buffered.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/libstd/io/buffered.rs b/src/libstd/io/buffered.rs index d094f19b5a5ff..1048c9d078d86 100644 --- a/src/libstd/io/buffered.rs +++ b/src/libstd/io/buffered.rs @@ -450,7 +450,7 @@ impl Seek for BufReader { pub struct BufWriter { // FIXME: Can this just be W, instead of Option? I don't see any code // paths that lead to this being None, or that ever check if it IS none, - // even in drop implementations. + // even in drop implementations. #72925. inner: Option, // FIXME: Replace this with a VecDeque. Because VecDeque is a Ring buffer, // this would enable BufWriter to operate without any interior copies. @@ -715,7 +715,7 @@ impl Write for BufWriter { if self.buf.len() + buf.len() > self.buf.capacity() { self.flush_buf()?; } - // FIXME: Why no len > capacity? Why not buffer len == capacity? + // FIXME: Why no len > capacity? Why not buffer len == capacity? #72919 if buf.len() >= self.buf.capacity() { self.panicked = true; let r = self.get_mut().write(buf); @@ -735,7 +735,7 @@ impl Write for BufWriter { if self.buf.len() + buf.len() > self.buf.capacity() { self.flush_buf()?; } - // FIXME: Why no len > capacity? Why not buffer len == capacity? + // FIXME: Why no len > capacity? Why not buffer len == capacity? #72919 if buf.len() >= self.buf.capacity() { self.panicked = true; let r = self.get_mut().write_all(buf); @@ -752,7 +752,7 @@ impl Write for BufWriter { if self.buf.len() + total_len > self.buf.capacity() { self.flush_buf()?; } - // FIXME: Why no len > capacity? Why not buffer len == capacity? + // FIXME: Why no len > capacity? Why not buffer len == capacity? #72919 if total_len >= self.buf.capacity() { self.panicked = true; let r = self.get_mut().write_vectored(bufs); From 59710fb716d2810c6eada13275039274b6279065 Mon Sep 17 00:00:00 2001 From: Nathan West Date: Tue, 2 Jun 2020 18:16:02 -0400 Subject: [PATCH 27/49] Clarified comment in `partial_multiline_buffering` test --- src/libstd/io/buffered.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/libstd/io/buffered.rs b/src/libstd/io/buffered.rs index 1048c9d078d86..c8c9ee4a4b47c 100644 --- a/src/libstd/io/buffered.rs +++ b/src/libstd/io/buffered.rs @@ -2246,9 +2246,9 @@ mod tests { // When content is written, LineWriter will try to write blocks A, B, // C, and D. Only block A will succeed. Under the old behavior, LineWriter // would then try to buffer B, C and D, but because its capacity is 10, - // it will only be able to buffer B and C. We don't want it to buffer - // partial lines if it can avoid it, so the correct behavior is to - // only buffer block B (with its newline). + // it will only be able to buffer B and C. We don't want to buffer + // partial lines concurrent with whole lines, so the correct behavior + // is to buffer only block B (out to the newline) assert_eq!(writer.write(content).unwrap(), 11); assert_eq!(writer.get_ref().buffer, *b"AAAAA"); From 3e48aaea03aef6e6e7ac678d11995a2a39ba9d99 Mon Sep 17 00:00:00 2001 From: Guillaume Gomez Date: Thu, 25 Jun 2020 13:19:56 +0200 Subject: [PATCH 28/49] Clean up E0704 error explanation --- src/librustc_error_codes/error_codes/E0704.md | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/librustc_error_codes/error_codes/E0704.md b/src/librustc_error_codes/error_codes/E0704.md index cde46f52c27d7..c22b274fb223e 100644 --- a/src/librustc_error_codes/error_codes/E0704.md +++ b/src/librustc_error_codes/error_codes/E0704.md @@ -1,6 +1,6 @@ -This error indicates that a incorrect visibility restriction was specified. +An incorrect visibility restriction was specified. -Example of erroneous code: +Erroneous code example: ```compile_fail,E0704 mod foo { @@ -12,6 +12,7 @@ mod foo { To make struct `Bar` only visible in module `foo` the `in` keyword should be used: + ``` mod foo { pub(in crate::foo) struct Bar { From 98a3b07213d695668081126a880fe7ac5dd9cd4e Mon Sep 17 00:00:00 2001 From: Guillaume Gomez Date: Fri, 26 Jun 2020 13:31:36 +0200 Subject: [PATCH 29/49] Add missing Stdin and StdinLock exampels --- src/libstd/io/stdio.rs | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/src/libstd/io/stdio.rs b/src/libstd/io/stdio.rs index b65b150d2c3a1..2ad92f42690e8 100644 --- a/src/libstd/io/stdio.rs +++ b/src/libstd/io/stdio.rs @@ -218,9 +218,23 @@ fn handle_ebadf(r: io::Result, default: T) -> io::Result { /// [`BufRead`]: trait.BufRead.html /// /// ### Note: Windows Portability Consideration +/// /// When operating in a console, the Windows implementation of this stream does not support /// non-UTF-8 byte sequences. Attempting to read bytes that are not valid UTF-8 will return /// an error. +/// +/// # Examples +/// +/// ```no_run +/// use std::io::{self, Read}; +/// +/// fn main() -> io::Result<()> { +/// let mut buffer = String::new(); +/// let mut stdin = io::stdin(); // We get `Stdin` here. +/// stdin.read_to_string(&mut buffer)?; +/// Ok(()) +/// } +/// ``` #[stable(feature = "rust1", since = "1.0.0")] pub struct Stdin { inner: Arc>>>, @@ -236,9 +250,26 @@ pub struct Stdin { /// [`Stdin::lock`]: struct.Stdin.html#method.lock /// /// ### Note: Windows Portability Consideration +/// /// When operating in a console, the Windows implementation of this stream does not support /// non-UTF-8 byte sequences. Attempting to read bytes that are not valid UTF-8 will return /// an error. +/// +/// # Examples +/// +/// ```no_run +/// use std::io::{self, Read}; +/// +/// fn main() -> io::Result<()> { +/// let mut buffer = String::new(); +/// let stdin = io::stdin(); // We get `Stdin` here. +/// { +/// let mut stdin_lock = stdin.lock(); // We get `StdinLock` here. +/// stdin_lock.read_to_string(&mut buffer)?; +/// } // `StdinLock` is dropped here. +/// Ok(()) +/// } +/// ``` #[stable(feature = "rust1", since = "1.0.0")] pub struct StdinLock<'a> { inner: MutexGuard<'a, BufReader>>, From 9448ed4c1f8b586c9c90a78c12afc4c826041d5a Mon Sep 17 00:00:00 2001 From: Dario Gonzalez Date: Mon, 29 Jun 2020 09:07:33 -0700 Subject: [PATCH 30/49] Obviate #[allow(improper_ctypes_definitions)] Modifies the return type for `fn entry` so that allowing improper_ctypes_definitions is no longer necessary. This change is derived from a similar pattern in `libstd/sys/sgx/abi/usercalls/raw.rs` with `UsercallReturn`. --- src/libstd/sys/sgx/abi/mod.rs | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/libstd/sys/sgx/abi/mod.rs b/src/libstd/sys/sgx/abi/mod.rs index 5ef26d4cc4dc6..b0693b63a48fd 100644 --- a/src/libstd/sys/sgx/abi/mod.rs +++ b/src/libstd/sys/sgx/abi/mod.rs @@ -17,6 +17,9 @@ pub mod usercalls; #[cfg(not(test))] global_asm!(include_str!("entry.S")); +#[repr(C)] +struct EntryReturn(u64, u64); + #[cfg(not(test))] #[no_mangle] unsafe extern "C" fn tcs_init(secondary: bool) { @@ -56,8 +59,7 @@ unsafe extern "C" fn tcs_init(secondary: bool) { // able to specify this #[cfg(not(test))] #[no_mangle] -#[allow(improper_ctypes_definitions)] -extern "C" fn entry(p1: u64, p2: u64, p3: u64, secondary: bool, p4: u64, p5: u64) -> (u64, u64) { +extern "C" fn entry(p1: u64, p2: u64, p3: u64, secondary: bool, p4: u64, p5: u64) -> EntryReturn { // FIXME: how to support TLS in library mode? let tls = Box::new(tls::Tls::new()); let _tls_guard = unsafe { tls.activate() }; @@ -65,7 +67,7 @@ extern "C" fn entry(p1: u64, p2: u64, p3: u64, secondary: bool, p4: u64, p5: u64 if secondary { super::thread::Thread::entry(); - (0, 0) + EntryReturn(0, 0) } else { extern "C" { fn main(argc: isize, argv: *const *const u8) -> isize; From bddb266089deb0cb879caeff1bb9aa860746e062 Mon Sep 17 00:00:00 2001 From: David Wood Date: Thu, 2 Jul 2020 14:13:39 +0100 Subject: [PATCH 31/49] typeck: check for infer before type impls trait This commit checks that the target type of the cast (an error related to which is being reported) does not have types to be inferred before checking if it implements the `From` trait. Signed-off-by: David Wood --- src/librustc_typeck/check/cast.rs | 1 + src/test/ui/issues/issue-73886.rs | 6 ++++++ src/test/ui/issues/issue-73886.stderr | 15 +++++++++++++++ 3 files changed, 22 insertions(+) create mode 100644 src/test/ui/issues/issue-73886.rs create mode 100644 src/test/ui/issues/issue-73886.stderr diff --git a/src/librustc_typeck/check/cast.rs b/src/librustc_typeck/check/cast.rs index 1ea7bf25ef2ed..8948e5a3e00db 100644 --- a/src/librustc_typeck/check/cast.rs +++ b/src/librustc_typeck/check/cast.rs @@ -387,6 +387,7 @@ impl<'a, 'tcx> CastCheck<'tcx> { // Check for infer types because cases like `Option<{integer}>` would // panic otherwise. if !expr_ty.has_infer_types() + && !ty.has_infer_types() && fcx.tcx.type_implements_trait(( from_trait, ty, diff --git a/src/test/ui/issues/issue-73886.rs b/src/test/ui/issues/issue-73886.rs new file mode 100644 index 0000000000000..2f1ec8c6d6227 --- /dev/null +++ b/src/test/ui/issues/issue-73886.rs @@ -0,0 +1,6 @@ +fn main() { + let _ = &&[0] as &[_]; + //~^ ERROR non-primitive cast: `&&[i32; 1]` as `&[_]` + let _ = 7u32 as Option<_>; + //~^ ERROR non-primitive cast: `u32` as `std::option::Option<_>` +} diff --git a/src/test/ui/issues/issue-73886.stderr b/src/test/ui/issues/issue-73886.stderr new file mode 100644 index 0000000000000..e8ab7db6b82c2 --- /dev/null +++ b/src/test/ui/issues/issue-73886.stderr @@ -0,0 +1,15 @@ +error[E0605]: non-primitive cast: `&&[i32; 1]` as `&[_]` + --> $DIR/issue-73886.rs:2:13 + | +LL | let _ = &&[0] as &[_]; + | ^^^^^^^^^^^^^ an `as` expression can only be used to convert between primitive types or to coerce to a specific trait object + +error[E0605]: non-primitive cast: `u32` as `std::option::Option<_>` + --> $DIR/issue-73886.rs:4:13 + | +LL | let _ = 7u32 as Option<_>; + | ^^^^^^^^^^^^^^^^^ an `as` expression can only be used to convert between primitive types or to coerce to a specific trait object + +error: aborting due to 2 previous errors + +For more information about this error, try `rustc --explain E0605`. From 23d7b3f6f1a345ad95f0812c85613627164b6c39 Mon Sep 17 00:00:00 2001 From: Erik Desjardins Date: Fri, 10 Jul 2020 19:02:10 -0400 Subject: [PATCH 32/49] Remove an unwrap in layout computation --- src/librustc_middle/ty/layout.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/librustc_middle/ty/layout.rs b/src/librustc_middle/ty/layout.rs index 39b8566e7a873..efdb5f27afb2a 100644 --- a/src/librustc_middle/ty/layout.rs +++ b/src/librustc_middle/ty/layout.rs @@ -774,12 +774,12 @@ impl<'tcx> LayoutCx<'tcx, TyCtxt<'tcx>> { (present_variants.next(), present_variants.next()) }; let present_first = match present_first { - present_first @ Some(_) => present_first, + Some(present_first) => present_first, // Uninhabited because it has no variants, or only absent ones. None if def.is_enum() => return tcx.layout_raw(param_env.and(tcx.types.never)), // If it's a struct, still compute a layout so that we can still compute the // field offsets. - None => Some(VariantIdx::new(0)), + None => VariantIdx::new(0), }; let is_struct = !def.is_enum() || @@ -791,7 +791,7 @@ impl<'tcx> LayoutCx<'tcx, TyCtxt<'tcx>> { // Struct, or univariant enum equivalent to a struct. // (Typechecking will reject discriminant-sizing attrs.) - let v = present_first.unwrap(); + let v = present_first; let kind = if def.is_enum() || variants[v].is_empty() { StructKind::AlwaysSized } else { From b8632e15483420784e9f4b95c882d24839dbada9 Mon Sep 17 00:00:00 2001 From: Nathan West Date: Sat, 11 Jul 2020 03:37:14 -0400 Subject: [PATCH 33/49] Removed FIXME --- src/libstd/io/buffered.rs | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/libstd/io/buffered.rs b/src/libstd/io/buffered.rs index c8c9ee4a4b47c..03a9fb91e5d96 100644 --- a/src/libstd/io/buffered.rs +++ b/src/libstd/io/buffered.rs @@ -448,9 +448,6 @@ impl Seek for BufReader { /// [`flush`]: #method.flush #[stable(feature = "rust1", since = "1.0.0")] pub struct BufWriter { - // FIXME: Can this just be W, instead of Option? I don't see any code - // paths that lead to this being None, or that ever check if it IS none, - // even in drop implementations. #72925. inner: Option, // FIXME: Replace this with a VecDeque. Because VecDeque is a Ring buffer, // this would enable BufWriter to operate without any interior copies. From 140bfc58aa9d193aeca1cce905a00195f8f23f3b Mon Sep 17 00:00:00 2001 From: Nathan West Date: Sat, 11 Jul 2020 03:41:06 -0400 Subject: [PATCH 34/49] Removed another FIXME --- src/libstd/io/buffered.rs | 6 ------ 1 file changed, 6 deletions(-) diff --git a/src/libstd/io/buffered.rs b/src/libstd/io/buffered.rs index 03a9fb91e5d96..2f2a67b0be96d 100644 --- a/src/libstd/io/buffered.rs +++ b/src/libstd/io/buffered.rs @@ -449,12 +449,6 @@ impl Seek for BufReader { #[stable(feature = "rust1", since = "1.0.0")] pub struct BufWriter { inner: Option, - // FIXME: Replace this with a VecDeque. Because VecDeque is a Ring buffer, - // this would enable BufWriter to operate without any interior copies. - // It was also allow a much simpler implementation of flush_buf. The main - // blocker here is that VecDeque doesn't currently have the same - // slice-specific specializations (extend_from_slice, `Extend` - // specializations) buf: Vec, // #30888: If the inner writer panics in a call to write, we don't want to // write the buffered data a second time in BufWriter's destructor. This From 997accc214f3a915541b06f3126568d367161882 Mon Sep 17 00:00:00 2001 From: Nathan West Date: Sat, 11 Jul 2020 14:45:22 -0400 Subject: [PATCH 35/49] Remove doubled "is_write_vectored" --- src/libstd/io/buffered.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libstd/io/buffered.rs b/src/libstd/io/buffered.rs index 2f2a67b0be96d..597bad0c2eeba 100644 --- a/src/libstd/io/buffered.rs +++ b/src/libstd/io/buffered.rs @@ -1117,7 +1117,7 @@ impl<'a, W: Write> Write for LineWriterShim<'a, W> { fn is_write_vectored(&self) -> bool { // It's hard to imagine these diverging, but it's worth checking // just in case, because we call `write_vectored` on both. - self.buffer.is_write_vectored() && self.inner().is_write_vectored() + self.buffer.is_write_vectored() } /// Write some data into this BufReader with line buffering. This means From 6a7b5df55ffddd5a152ebbb865d490e52e93bec7 Mon Sep 17 00:00:00 2001 From: Nathan West Date: Sat, 11 Jul 2020 15:33:45 -0400 Subject: [PATCH 36/49] Removed unused method --- src/libstd/io/buffered.rs | 6 ------ 1 file changed, 6 deletions(-) diff --git a/src/libstd/io/buffered.rs b/src/libstd/io/buffered.rs index 597bad0c2eeba..cedc993b46147 100644 --- a/src/libstd/io/buffered.rs +++ b/src/libstd/io/buffered.rs @@ -909,12 +909,6 @@ impl<'a, W: Write> LineWriterShim<'a, W> { Self { buffer } } - /// Get a reference to the inner writer (that is, the writer wrapped by - /// the BufWriter) - fn inner(&self) -> &W { - self.buffer.get_ref() - } - /// Get a mutable reference to the inner writer (that is, the writer /// wrapped by the BufWriter). Be careful with this writer, as writes to /// it will bypass the buffer. From 606593fecec5510a32b6aa3b0bc2bd5cf81f28e2 Mon Sep 17 00:00:00 2001 From: Nathan West Date: Sun, 12 Jul 2020 01:00:22 -0400 Subject: [PATCH 37/49] Minor updates - Remove outdated comment - Refactor flush-retry behavior into its own method - Some other comment updates --- src/libstd/io/buffered.rs | 40 +++++++++++++++++---------------------- 1 file changed, 17 insertions(+), 23 deletions(-) diff --git a/src/libstd/io/buffered.rs b/src/libstd/io/buffered.rs index cedc993b46147..7d9c33582bca1 100644 --- a/src/libstd/io/buffered.rs +++ b/src/libstd/io/buffered.rs @@ -920,6 +920,16 @@ impl<'a, W: Write> LineWriterShim<'a, W> { fn buffered(&self) -> &[u8] { self.buffer.buffer() } + + /// Flush the buffer iff the last byte is a newline (indicating that an + /// earlier write only succeeded partially, and we want to retry flushing + /// the buffered line before continuing with a subsequent write) + fn flush_if_completed_line(&mut self) -> io::Result<()> { + match self.buffered().last().copied() { + Some(b'\n') => self.buffer.flush_buf(), + _ => Ok(()), + } + } } impl<'a, W: Write> Write for LineWriterShim<'a, W> { @@ -941,12 +951,7 @@ impl<'a, W: Write> Write for LineWriterShim<'a, W> { // If there are no new newlines (that is, if this write is less than // one line), just do a regular buffered write None => { - // Check for prior partial line writes that need to be retried. - // Only retry if the buffer contains a completed line, to - // avoid flushing partial lines. - if let Some(b'\n') = self.buffered().last().copied() { - self.buffer.flush_buf()?; - } + self.flush_if_completed_line()?; return self.buffer.write(buf); } // Otherwise, arrange for the lines to be written directly to the @@ -1025,9 +1030,10 @@ impl<'a, W: Write> Write for LineWriterShim<'a, W> { /// Because sorting through an array of `IoSlice` can be a bit convoluted, /// This method differs from write in the following ways: /// - /// - It attempts to write all the buffers up to and including the one - /// containing the last newline. This means that it may attempt to - /// write a partial line. + /// - It attempts to write the full content of all the buffers up to and + /// including the one containing the last newline. This means that it + /// may attempt to write a partial line, that buffer has data past the + /// newline. /// - If the write only reports partial success, it does not attempt to /// find the precise location of the written bytes and buffer the rest. /// @@ -1057,12 +1063,7 @@ impl<'a, W: Write> Write for LineWriterShim<'a, W> { let last_newline_buf_idx = match last_newline_buf_idx { // No newlines; just do a normal buffered write None => { - // Check for prior partial line writes that need to be retried. - // Only retry if the buffer contains a completed line, to - // avoid flushing partial lines. - if let Some(b'\n') = self.buffered().last().copied() { - self.buffer.flush_buf()?; - } + self.flush_if_completed_line()?; return self.buffer.write_vectored(bufs); } Some(i) => i, @@ -1109,8 +1110,6 @@ impl<'a, W: Write> Write for LineWriterShim<'a, W> { } fn is_write_vectored(&self) -> bool { - // It's hard to imagine these diverging, but it's worth checking - // just in case, because we call `write_vectored` on both. self.buffer.is_write_vectored() } @@ -1127,12 +1126,7 @@ impl<'a, W: Write> Write for LineWriterShim<'a, W> { // If there are no new newlines (that is, if this write is less than // one line), just do a regular buffered write None => { - // Check for prior partial line writes that need to be retried. - // Only retry if the buffer contains a completed line, to - // avoid flushing partial lines. - if let Some(b'\n') = self.buffered().last().copied() { - self.buffer.flush_buf()?; - } + self.flush_if_completed_line()?; return self.buffer.write_all(buf); } // Otherwise, arrange for the lines to be written directly to the From 9a8b516de046ac909f22f0280e1a8a0d87ba0d06 Mon Sep 17 00:00:00 2001 From: Lzu Tao Date: Fri, 10 Jul 2020 15:59:25 +0000 Subject: [PATCH 38/49] Sorting feature attributes in std --- src/libstd/lib.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/libstd/lib.rs b/src/libstd/lib.rs index bd585d39c242f..16a4b38a1ae33 100644 --- a/src/libstd/lib.rs +++ b/src/libstd/lib.rs @@ -242,8 +242,8 @@ #![feature(atomic_mut_ptr)] #![feature(box_syntax)] #![feature(c_variadic)] -#![feature(cfg_accessible)] #![feature(can_vector)] +#![feature(cfg_accessible)] #![feature(cfg_target_has_atomic)] #![feature(cfg_target_thread_local)] #![feature(char_error_internals)] @@ -276,8 +276,8 @@ #![feature(hashmap_internals)] #![feature(int_error_internals)] #![feature(int_error_matching)] -#![feature(into_future)] #![feature(integer_atomics)] +#![feature(into_future)] #![feature(lang_items)] #![feature(libc)] #![feature(link_args)] @@ -286,6 +286,7 @@ #![feature(log_syntax)] #![feature(maybe_uninit_ref)] #![feature(maybe_uninit_slice)] +#![feature(min_specialization)] #![feature(needs_panic_runtime)] #![feature(negative_impls)] #![feature(never_type)] @@ -305,7 +306,6 @@ #![feature(shrink_to)] #![feature(slice_concat_ext)] #![feature(slice_internals)] -#![feature(min_specialization)] #![feature(staged_api)] #![feature(std_internals)] #![feature(stdsimd)] From 1e05e09fe9df9919dc1c1c0f36255f4f1098fc24 Mon Sep 17 00:00:00 2001 From: Lzu Tao Date: Fri, 10 Jul 2020 15:59:25 +0000 Subject: [PATCH 39/49] Remove the useless indentation --- src/libstd/thread/mod.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/libstd/thread/mod.rs b/src/libstd/thread/mod.rs index d435ca6842518..d354a9b1842c2 100644 --- a/src/libstd/thread/mod.rs +++ b/src/libstd/thread/mod.rs @@ -641,9 +641,8 @@ where #[stable(feature = "rust1", since = "1.0.0")] pub fn current() -> Thread { thread_info::current_thread().expect( - "use of std::thread::current() is not \ - possible after the thread's local \ - data has been destroyed", + "use of std::thread::current() is not possible \ + after the thread's local data has been destroyed", ) } From 0ff820cb62f0b21d422440353b8def10c05ed735 Mon Sep 17 00:00:00 2001 From: Lzu Tao Date: Sun, 12 Jul 2020 10:17:22 +0000 Subject: [PATCH 40/49] Move constants to top file --- src/libstd/sys/windows/path.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/libstd/sys/windows/path.rs b/src/libstd/sys/windows/path.rs index 524f21f889bc2..e70ddce3aa579 100644 --- a/src/libstd/sys/windows/path.rs +++ b/src/libstd/sys/windows/path.rs @@ -2,6 +2,9 @@ use crate::ffi::OsStr; use crate::mem; use crate::path::Prefix; +pub const MAIN_SEP_STR: &str = "\\"; +pub const MAIN_SEP: char = '\\'; + fn os_str_as_u8_slice(s: &OsStr) -> &[u8] { unsafe { mem::transmute(s) } } @@ -90,5 +93,3 @@ pub fn parse_prefix(path: &OsStr) -> Option> { } } -pub const MAIN_SEP_STR: &str = "\\"; -pub const MAIN_SEP: char = '\\'; From 90a7d2470a7ae29c529ae2a3684c47839c57d763 Mon Sep 17 00:00:00 2001 From: Lzu Tao Date: Fri, 10 Jul 2020 15:59:25 +0000 Subject: [PATCH 41/49] Make is_valid_drive_letter function --- src/libstd/sys/windows/path.rs | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/libstd/sys/windows/path.rs b/src/libstd/sys/windows/path.rs index e70ddce3aa579..e2b48a22dcdb7 100644 --- a/src/libstd/sys/windows/path.rs +++ b/src/libstd/sys/windows/path.rs @@ -22,6 +22,12 @@ pub fn is_verbatim_sep(b: u8) -> bool { b == b'\\' } +// In most DOS systems, it is not possible to have more than 26 drive letters. +// See . +pub fn is_valid_drive_letter(disk: u8) -> bool { + disk.is_ascii_alphabetic() +} + pub fn parse_prefix(path: &OsStr) -> Option> { use crate::path::Prefix::*; unsafe { @@ -52,7 +58,7 @@ pub fn parse_prefix(path: &OsStr) -> Option> { let idx = path.iter().position(|&b| b == b'\\'); if idx == Some(2) && path[1] == b':' { let c = path[0]; - if c.is_ascii() && (c as char).is_alphabetic() { + if is_valid_drive_letter(c) { // \\?\C:\ path return Some(VerbatimDisk(c.to_ascii_uppercase())); } @@ -77,7 +83,7 @@ pub fn parse_prefix(path: &OsStr) -> Option> { } else if path.get(1) == Some(&b':') { // C: let c = path[0]; - if c.is_ascii() && (c as char).is_alphabetic() { + if is_valid_drive_letter(c) { return Some(Disk(c.to_ascii_uppercase())); } } From 27a966a149f55fe29a25a2fc07e6c8a011ae3dbf Mon Sep 17 00:00:00 2001 From: Lzu Tao Date: Fri, 10 Jul 2020 15:59:25 +0000 Subject: [PATCH 42/49] Make use of slice::strip_prefix and slice pattern --- src/libstd/lib.rs | 1 + src/libstd/sys/windows/path.rs | 51 ++++++++++++++++------------------ 2 files changed, 25 insertions(+), 27 deletions(-) diff --git a/src/libstd/lib.rs b/src/libstd/lib.rs index 16a4b38a1ae33..5215db7cdb3ce 100644 --- a/src/libstd/lib.rs +++ b/src/libstd/lib.rs @@ -306,6 +306,7 @@ #![feature(shrink_to)] #![feature(slice_concat_ext)] #![feature(slice_internals)] +#![feature(slice_strip)] #![feature(staged_api)] #![feature(std_internals)] #![feature(stdsimd)] diff --git a/src/libstd/sys/windows/path.rs b/src/libstd/sys/windows/path.rs index e2b48a22dcdb7..25ecc79abf0f1 100644 --- a/src/libstd/sys/windows/path.rs +++ b/src/libstd/sys/windows/path.rs @@ -29,23 +29,20 @@ pub fn is_valid_drive_letter(disk: u8) -> bool { } pub fn parse_prefix(path: &OsStr) -> Option> { - use crate::path::Prefix::*; + use Prefix::{DeviceNS, Disk, Verbatim, VerbatimDisk, VerbatimUNC, UNC}; unsafe { // The unsafety here stems from converting between &OsStr and &[u8] // and back. This is safe to do because (1) we only look at ASCII // contents of the encoding and (2) new &OsStr values are produced // only from ASCII-bounded slices of existing &OsStr values. - let mut path = os_str_as_u8_slice(path); + let path = os_str_as_u8_slice(path); - if path.starts_with(br"\\") { - // \\ - path = &path[2..]; - if path.starts_with(br"?\") { - // \\?\ - path = &path[2..]; - if path.starts_with(br"UNC\") { - // \\?\UNC\server\share - path = &path[4..]; + // \\ + if let Some(path) = path.strip_prefix(br"\\") { + // \\?\ + if let Some(path) = path.strip_prefix(br"?\") { + // \\?\UNC\server\share + if let Some(path) = path.strip_prefix(br"UNC\") { let (server, share) = match parse_two_comps(path, is_verbatim_sep) { Some((server, share)) => { (u8_slice_as_os_str(server), u8_slice_as_os_str(share)) @@ -55,22 +52,23 @@ pub fn parse_prefix(path: &OsStr) -> Option> { return Some(VerbatimUNC(server, share)); } else { // \\?\path - let idx = path.iter().position(|&b| b == b'\\'); - if idx == Some(2) && path[1] == b':' { - let c = path[0]; - if is_valid_drive_letter(c) { - // \\?\C:\ path + match path { + // \\?\C:\path + [c, b':', b'\\', ..] if is_valid_drive_letter(*c) => { return Some(VerbatimDisk(c.to_ascii_uppercase())); } + // \\?\cat_pics + _ => { + let idx = path.iter().position(|&b| b == b'\\').unwrap_or(path.len()); + let slice = &path[..idx]; + return Some(Verbatim(u8_slice_as_os_str(slice))); + } } - let slice = &path[..idx.unwrap_or(path.len())]; - return Some(Verbatim(u8_slice_as_os_str(slice))); } - } else if path.starts_with(b".\\") { - // \\.\path - path = &path[2..]; - let pos = path.iter().position(|&b| b == b'\\'); - let slice = &path[..pos.unwrap_or(path.len())]; + } else if let Some(path) = path.strip_prefix(b".\\") { + // \\.\COM42 + let idx = path.iter().position(|&b| b == b'\\').unwrap_or(path.len()); + let slice = &path[..idx]; return Some(DeviceNS(u8_slice_as_os_str(slice))); } match parse_two_comps(path, is_sep_byte) { @@ -78,12 +76,11 @@ pub fn parse_prefix(path: &OsStr) -> Option> { // \\server\share return Some(UNC(u8_slice_as_os_str(server), u8_slice_as_os_str(share))); } - _ => (), + _ => {} } - } else if path.get(1) == Some(&b':') { + } else if let [c, b':', ..] = path { // C: - let c = path[0]; - if is_valid_drive_letter(c) { + if is_valid_drive_letter(*c) { return Some(Disk(c.to_ascii_uppercase())); } } From b1d6798899b7830e869d06e1ddb106a75cb61cc8 Mon Sep 17 00:00:00 2001 From: Lzu Tao Date: Fri, 10 Jul 2020 15:59:25 +0000 Subject: [PATCH 43/49] Rewrite parse_two_comps --- src/libstd/sys/windows/path.rs | 30 ++++++++++++++++++---------- src/libstd/sys/windows/path/tests.rs | 21 +++++++++++++++++++ 2 files changed, 41 insertions(+), 10 deletions(-) create mode 100644 src/libstd/sys/windows/path/tests.rs diff --git a/src/libstd/sys/windows/path.rs b/src/libstd/sys/windows/path.rs index 25ecc79abf0f1..fa22d3bb7d6f4 100644 --- a/src/libstd/sys/windows/path.rs +++ b/src/libstd/sys/windows/path.rs @@ -2,6 +2,9 @@ use crate::ffi::OsStr; use crate::mem; use crate::path::Prefix; +#[cfg(test)] +mod tests; + pub const MAIN_SEP_STR: &str = "\\"; pub const MAIN_SEP: char = '\\'; @@ -43,7 +46,7 @@ pub fn parse_prefix(path: &OsStr) -> Option> { if let Some(path) = path.strip_prefix(br"?\") { // \\?\UNC\server\share if let Some(path) = path.strip_prefix(br"UNC\") { - let (server, share) = match parse_two_comps(path, is_verbatim_sep) { + let (server, share) = match get_first_two_components(path, is_verbatim_sep) { Some((server, share)) => { (u8_slice_as_os_str(server), u8_slice_as_os_str(share)) } @@ -71,7 +74,7 @@ pub fn parse_prefix(path: &OsStr) -> Option> { let slice = &path[..idx]; return Some(DeviceNS(u8_slice_as_os_str(slice))); } - match parse_two_comps(path, is_sep_byte) { + match get_first_two_components(path, is_sep_byte) { Some((server, share)) if !server.is_empty() && !share.is_empty() => { // \\server\share return Some(UNC(u8_slice_as_os_str(server), u8_slice_as_os_str(share))); @@ -86,13 +89,20 @@ pub fn parse_prefix(path: &OsStr) -> Option> { } return None; } - - fn parse_two_comps(mut path: &[u8], f: fn(u8) -> bool) -> Option<(&[u8], &[u8])> { - let first = &path[..path.iter().position(|x| f(*x))?]; - path = &path[(first.len() + 1)..]; - let idx = path.iter().position(|x| f(*x)); - let second = &path[..idx.unwrap_or(path.len())]; - Some((first, second)) - } } +/// Returns the first two path components with predicate `f`. +/// +/// The two components returned will be use by caller +/// to construct `VerbatimUNC` or `UNC` Windows path prefix. +/// +/// Returns [`None`] if there are no separators in path. +fn get_first_two_components(path: &[u8], f: fn(u8) -> bool) -> Option<(&[u8], &[u8])> { + let idx = path.iter().position(|&x| f(x))?; + // Panic safe + // The max `idx+1` is `path.len()` and `path[path.len()..]` is a valid index. + let (first, path) = (&path[..idx], &path[idx + 1..]); + let idx = path.iter().position(|&x| f(x)).unwrap_or(path.len()); + let second = &path[..idx]; + Some((first, second)) +} diff --git a/src/libstd/sys/windows/path/tests.rs b/src/libstd/sys/windows/path/tests.rs new file mode 100644 index 0000000000000..fbac1dc1ca17a --- /dev/null +++ b/src/libstd/sys/windows/path/tests.rs @@ -0,0 +1,21 @@ +use super::*; + +#[test] +fn test_get_first_two_components() { + assert_eq!( + get_first_two_components(br"server\share", is_verbatim_sep), + Some((&b"server"[..], &b"share"[..])), + ); + + assert_eq!( + get_first_two_components(br"server\", is_verbatim_sep), + Some((&b"server"[..], &b""[..])) + ); + + assert_eq!( + get_first_two_components(br"\server\", is_verbatim_sep), + Some((&b""[..], &b"server"[..])) + ); + + assert_eq!(get_first_two_components(br"there are no separators here", is_verbatim_sep), None,); +} From 0281a05f66aada7afc3e5b665b4c48a44abbc517 Mon Sep 17 00:00:00 2001 From: Lzu Tao Date: Fri, 10 Jul 2020 15:59:25 +0000 Subject: [PATCH 44/49] Prefer empty OsStr over unsafe cast from [u8] --- src/libstd/sys/windows/path.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libstd/sys/windows/path.rs b/src/libstd/sys/windows/path.rs index fa22d3bb7d6f4..899a3065038d7 100644 --- a/src/libstd/sys/windows/path.rs +++ b/src/libstd/sys/windows/path.rs @@ -50,7 +50,7 @@ pub fn parse_prefix(path: &OsStr) -> Option> { Some((server, share)) => { (u8_slice_as_os_str(server), u8_slice_as_os_str(share)) } - None => (u8_slice_as_os_str(path), u8_slice_as_os_str(&[])), + None => (u8_slice_as_os_str(path), OsStr::new("")), }; return Some(VerbatimUNC(server, share)); } else { From e31898b02409b8ce5db32761d45c78f9f74dbdc6 Mon Sep 17 00:00:00 2001 From: Lzu Tao Date: Sun, 12 Jul 2020 14:46:00 +0000 Subject: [PATCH 45/49] Reduce unsafe scope --- src/libstd/sys/windows/path.rs | 97 +++++++++++++++++----------------- 1 file changed, 48 insertions(+), 49 deletions(-) diff --git a/src/libstd/sys/windows/path.rs b/src/libstd/sys/windows/path.rs index 899a3065038d7..dda3ed68cfc95 100644 --- a/src/libstd/sys/windows/path.rs +++ b/src/libstd/sys/windows/path.rs @@ -8,6 +8,10 @@ mod tests; pub const MAIN_SEP_STR: &str = "\\"; pub const MAIN_SEP: char = '\\'; +// The unsafety here stems from converting between `&OsStr` and `&[u8]` +// and back. This is safe to do because (1) we only look at ASCII +// contents of the encoding and (2) new &OsStr values are produced +// only from ASCII-bounded slices of existing &OsStr values. fn os_str_as_u8_slice(s: &OsStr) -> &[u8] { unsafe { mem::transmute(s) } } @@ -33,62 +37,57 @@ pub fn is_valid_drive_letter(disk: u8) -> bool { pub fn parse_prefix(path: &OsStr) -> Option> { use Prefix::{DeviceNS, Disk, Verbatim, VerbatimDisk, VerbatimUNC, UNC}; - unsafe { - // The unsafety here stems from converting between &OsStr and &[u8] - // and back. This is safe to do because (1) we only look at ASCII - // contents of the encoding and (2) new &OsStr values are produced - // only from ASCII-bounded slices of existing &OsStr values. - let path = os_str_as_u8_slice(path); - // \\ - if let Some(path) = path.strip_prefix(br"\\") { - // \\?\ - if let Some(path) = path.strip_prefix(br"?\") { - // \\?\UNC\server\share - if let Some(path) = path.strip_prefix(br"UNC\") { - let (server, share) = match get_first_two_components(path, is_verbatim_sep) { - Some((server, share)) => { - (u8_slice_as_os_str(server), u8_slice_as_os_str(share)) - } - None => (u8_slice_as_os_str(path), OsStr::new("")), - }; - return Some(VerbatimUNC(server, share)); - } else { - // \\?\path - match path { - // \\?\C:\path - [c, b':', b'\\', ..] if is_valid_drive_letter(*c) => { - return Some(VerbatimDisk(c.to_ascii_uppercase())); - } - // \\?\cat_pics - _ => { - let idx = path.iter().position(|&b| b == b'\\').unwrap_or(path.len()); - let slice = &path[..idx]; - return Some(Verbatim(u8_slice_as_os_str(slice))); - } + let path = os_str_as_u8_slice(path); + + // \\ + if let Some(path) = path.strip_prefix(br"\\") { + // \\?\ + if let Some(path) = path.strip_prefix(br"?\") { + // \\?\UNC\server\share + if let Some(path) = path.strip_prefix(br"UNC\") { + let (server, share) = match get_first_two_components(path, is_verbatim_sep) { + Some((server, share)) => unsafe { + (u8_slice_as_os_str(server), u8_slice_as_os_str(share)) + }, + None => (unsafe { u8_slice_as_os_str(path) }, OsStr::new("")), + }; + return Some(VerbatimUNC(server, share)); + } else { + // \\?\path + match path { + // \\?\C:\path + [c, b':', b'\\', ..] if is_valid_drive_letter(*c) => { + return Some(VerbatimDisk(c.to_ascii_uppercase())); + } + // \\?\cat_pics + _ => { + let idx = path.iter().position(|&b| b == b'\\').unwrap_or(path.len()); + let slice = &path[..idx]; + return Some(Verbatim(unsafe { u8_slice_as_os_str(slice) })); } } - } else if let Some(path) = path.strip_prefix(b".\\") { - // \\.\COM42 - let idx = path.iter().position(|&b| b == b'\\').unwrap_or(path.len()); - let slice = &path[..idx]; - return Some(DeviceNS(u8_slice_as_os_str(slice))); - } - match get_first_two_components(path, is_sep_byte) { - Some((server, share)) if !server.is_empty() && !share.is_empty() => { - // \\server\share - return Some(UNC(u8_slice_as_os_str(server), u8_slice_as_os_str(share))); - } - _ => {} } - } else if let [c, b':', ..] = path { - // C: - if is_valid_drive_letter(*c) { - return Some(Disk(c.to_ascii_uppercase())); + } else if let Some(path) = path.strip_prefix(b".\\") { + // \\.\COM42 + let idx = path.iter().position(|&b| b == b'\\').unwrap_or(path.len()); + let slice = &path[..idx]; + return Some(DeviceNS(unsafe { u8_slice_as_os_str(slice) })); + } + match get_first_two_components(path, is_sep_byte) { + Some((server, share)) if !server.is_empty() && !share.is_empty() => { + // \\server\share + return Some(unsafe { UNC(u8_slice_as_os_str(server), u8_slice_as_os_str(share)) }); } + _ => {} + } + } else if let [c, b':', ..] = path { + // C: + if is_valid_drive_letter(*c) { + return Some(Disk(c.to_ascii_uppercase())); } - return None; } + None } /// Returns the first two path components with predicate `f`. From 083c2f6ceb82b75ec04c73f2b39a414985fd9a92 Mon Sep 17 00:00:00 2001 From: David Wood Date: Sun, 12 Jul 2020 18:22:09 +0100 Subject: [PATCH 46/49] pprust: support multiline comments within lines This commit adds support to rustc_ast_pretty for multiline comments that start and end within a line of source code. Signed-off-by: David Wood --- src/librustc_ast_pretty/pprust.rs | 15 ++++++++++++-- src/test/pretty/issue-73626.rs | 34 +++++++++++++++++++++++++++++++ 2 files changed, 47 insertions(+), 2 deletions(-) create mode 100644 src/test/pretty/issue-73626.rs diff --git a/src/librustc_ast_pretty/pprust.rs b/src/librustc_ast_pretty/pprust.rs index 2d803262f79e1..c33cae57872ac 100644 --- a/src/librustc_ast_pretty/pprust.rs +++ b/src/librustc_ast_pretty/pprust.rs @@ -450,9 +450,20 @@ pub trait PrintState<'a>: std::ops::Deref + std::ops::Dere fn print_comment(&mut self, cmnt: &comments::Comment) { match cmnt.style { comments::Mixed => { - assert_eq!(cmnt.lines.len(), 1); self.zerobreak(); - self.word(cmnt.lines[0].clone()); + if let Some((last, lines)) = cmnt.lines.split_last() { + self.ibox(0); + + for line in lines { + self.word(line.clone()); + self.hardbreak() + } + + self.word(last.clone()); + self.space(); + + self.end(); + } self.zerobreak() } comments::Isolated => { diff --git a/src/test/pretty/issue-73626.rs b/src/test/pretty/issue-73626.rs new file mode 100644 index 0000000000000..a002f09be3b4e --- /dev/null +++ b/src/test/pretty/issue-73626.rs @@ -0,0 +1,34 @@ +fn main(/* + --- +*/) { + let x /* this is one line */ = 3; + + let x /* + * this + * is + * multiple + * lines + */ = 3; + + let x = /* + * this + * is + * multiple + * lines + * after + * the + * = + */ 3; + + let x /* + * this + * is + * multiple + * lines + * including + * a + + * blank + * line + */ = 3; +} From 5daedea3dbeb8fb2639d3d142b008135f4fd2b43 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20K=C3=BCber?= Date: Wed, 8 Jul 2020 19:39:26 -0700 Subject: [PATCH 47/49] Detect tuple struct incorrectly used as struct pat --- Cargo.lock | 1 + src/librustc_error_codes/error_codes.rs | 1 + src/librustc_error_codes/error_codes/E0769.md | 39 ++++++++ src/librustc_typeck/Cargo.toml | 1 + src/librustc_typeck/check/pat.rs | 91 ++++++++++++++++--- .../missing-fields-in-struct-pattern.rs | 3 +- .../missing-fields-in-struct-pattern.stderr | 17 +--- src/test/ui/type/type-check/issue-41314.rs | 4 +- .../ui/type/type-check/issue-41314.stderr | 17 +--- src/test/ui/union/union-fields-2.stderr | 12 +-- 10 files changed, 137 insertions(+), 49 deletions(-) create mode 100644 src/librustc_error_codes/error_codes/E0769.md diff --git a/Cargo.lock b/Cargo.lock index 2096a3dfff9ea..61a0b5caebec6 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3996,6 +3996,7 @@ dependencies = [ "rustc_data_structures", "rustc_errors", "rustc_hir", + "rustc_hir_pretty", "rustc_index", "rustc_infer", "rustc_middle", diff --git a/src/librustc_error_codes/error_codes.rs b/src/librustc_error_codes/error_codes.rs index f687221d78e03..2e1b897216bb2 100644 --- a/src/librustc_error_codes/error_codes.rs +++ b/src/librustc_error_codes/error_codes.rs @@ -450,6 +450,7 @@ E0765: include_str!("./error_codes/E0765.md"), E0766: include_str!("./error_codes/E0766.md"), E0767: include_str!("./error_codes/E0767.md"), E0768: include_str!("./error_codes/E0768.md"), +E0769: include_str!("./error_codes/E0769.md"), ; // E0006, // merged with E0005 // E0008, // cannot bind by-move into a pattern guard diff --git a/src/librustc_error_codes/error_codes/E0769.md b/src/librustc_error_codes/error_codes/E0769.md new file mode 100644 index 0000000000000..d1995be9899b1 --- /dev/null +++ b/src/librustc_error_codes/error_codes/E0769.md @@ -0,0 +1,39 @@ +A tuple struct or tuple variant was used in a pattern as if it were a +struct or struct variant. + +Erroneous code example: + +```compile_fail,E0769 +enum E { + A(i32), +} +let e = E::A(42); +match e { + E::A { number } => println!("{}", x), +} +``` + +To fix this error, you can use the tuple pattern: + +``` +# enum E { +# A(i32), +# } +# let e = E::A(42); +match e { + E::A(number) => println!("{}", number), +} +``` + +Alternatively, you can also use the struct pattern by using the correct +field names and binding them to new identifiers: + +``` +# enum E { +# A(i32), +# } +# let e = E::A(42); +match e { + E::A { 0: number } => println!("{}", number), +} +``` diff --git a/src/librustc_typeck/Cargo.toml b/src/librustc_typeck/Cargo.toml index 9329069c48dd1..93b503c976be4 100644 --- a/src/librustc_typeck/Cargo.toml +++ b/src/librustc_typeck/Cargo.toml @@ -18,6 +18,7 @@ rustc_attr = { path = "../librustc_attr" } rustc_data_structures = { path = "../librustc_data_structures" } rustc_errors = { path = "../librustc_errors" } rustc_hir = { path = "../librustc_hir" } +rustc_hir_pretty = { path = "../librustc_hir_pretty" } rustc_target = { path = "../librustc_target" } rustc_session = { path = "../librustc_session" } smallvec = { version = "1.0", features = ["union", "may_dangle"] } diff --git a/src/librustc_typeck/check/pat.rs b/src/librustc_typeck/check/pat.rs index ea47ae68ce7d3..a654fc3dfc2df 100644 --- a/src/librustc_typeck/check/pat.rs +++ b/src/librustc_typeck/check/pat.rs @@ -1082,20 +1082,23 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { .filter(|ident| !used_fields.contains_key(&ident)) .collect::>(); - if !inexistent_fields.is_empty() && !variant.recovered { - self.error_inexistent_fields( + let inexistent_fields_err = if !inexistent_fields.is_empty() && !variant.recovered { + Some(self.error_inexistent_fields( adt.variant_descr(), &inexistent_fields, &mut unmentioned_fields, variant, - ); - } + )) + } else { + None + }; // Require `..` if struct has non_exhaustive attribute. if variant.is_field_list_non_exhaustive() && !adt.did.is_local() && !etc { self.error_foreign_non_exhaustive_spat(pat, adt.variant_descr(), fields.is_empty()); } + let mut unmentioned_err = None; // Report an error if incorrect number of the fields were specified. if adt.is_union() { if fields.len() != 1 { @@ -1107,7 +1110,25 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { tcx.sess.struct_span_err(pat.span, "`..` cannot be used in union patterns").emit(); } } else if !etc && !unmentioned_fields.is_empty() { - self.error_unmentioned_fields(pat.span, &unmentioned_fields, variant); + unmentioned_err = Some(self.error_unmentioned_fields(pat.span, &unmentioned_fields)); + } + match (inexistent_fields_err, unmentioned_err) { + (Some(mut i), Some(mut u)) => { + if let Some(mut e) = self.error_tuple_variant_as_struct_pat(pat, fields, variant) { + // We don't want to show the inexistent fields error when this was + // `Foo { a, b }` when it should have been `Foo(a, b)`. + i.delay_as_bug(); + u.delay_as_bug(); + e.emit(); + } else { + i.emit(); + u.emit(); + } + } + (None, Some(mut err)) | (Some(mut err), None) => { + err.emit(); + } + (None, None) => {} } no_field_errors } @@ -1154,7 +1175,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { inexistent_fields: &[Ident], unmentioned_fields: &mut Vec, variant: &ty::VariantDef, - ) { + ) -> DiagnosticBuilder<'tcx> { let tcx = self.tcx; let (field_names, t, plural) = if inexistent_fields.len() == 1 { (format!("a field named `{}`", inexistent_fields[0]), "this", "") @@ -1221,15 +1242,62 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { it explicitly.", ); } - err.emit(); + err + } + + fn error_tuple_variant_as_struct_pat( + &self, + pat: &Pat<'_>, + fields: &'tcx [hir::FieldPat<'tcx>], + variant: &ty::VariantDef, + ) -> Option> { + if let (CtorKind::Fn, PatKind::Struct(qpath, ..)) = (variant.ctor_kind, &pat.kind) { + let path = rustc_hir_pretty::to_string(rustc_hir_pretty::NO_ANN, |s| { + s.print_qpath(qpath, false) + }); + let mut err = struct_span_err!( + self.tcx.sess, + pat.span, + E0769, + "tuple variant `{}` written as struct variant", + path + ); + let (sugg, appl) = if fields.len() == variant.fields.len() { + ( + fields + .iter() + .map(|f| match self.tcx.sess.source_map().span_to_snippet(f.pat.span) { + Ok(f) => f, + Err(_) => rustc_hir_pretty::to_string(rustc_hir_pretty::NO_ANN, |s| { + s.print_pat(f.pat) + }), + }) + .collect::>() + .join(", "), + Applicability::MachineApplicable, + ) + } else { + ( + variant.fields.iter().map(|_| "_").collect::>().join(", "), + Applicability::MaybeIncorrect, + ) + }; + err.span_suggestion( + pat.span, + "use the tuple variant pattern syntax instead", + format!("{}({})", path, sugg), + appl, + ); + return Some(err); + } + None } fn error_unmentioned_fields( &self, span: Span, unmentioned_fields: &[Ident], - variant: &ty::VariantDef, - ) { + ) -> DiagnosticBuilder<'tcx> { let field_names = if unmentioned_fields.len() == 1 { format!("field `{}`", unmentioned_fields[0]) } else { @@ -1248,9 +1316,6 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { field_names ); diag.span_label(span, format!("missing {}", field_names)); - if variant.ctor_kind == CtorKind::Fn { - diag.note("trying to match a tuple variant with a struct variant pattern"); - } if self.tcx.sess.teach(&diag.get_code().unwrap()) { diag.note( "This error indicates that a pattern for a struct fails to specify a \ @@ -1259,7 +1324,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { ignore unwanted fields.", ); } - diag.emit(); + diag } fn check_pat_box( diff --git a/src/test/ui/missing/missing-fields-in-struct-pattern.rs b/src/test/ui/missing/missing-fields-in-struct-pattern.rs index 24b6b55db6692..40304a674a633 100644 --- a/src/test/ui/missing/missing-fields-in-struct-pattern.rs +++ b/src/test/ui/missing/missing-fields-in-struct-pattern.rs @@ -2,8 +2,7 @@ struct S(usize, usize, usize, usize); fn main() { if let S { a, b, c, d } = S(1, 2, 3, 4) { - //~^ ERROR struct `S` does not have fields named `a`, `b`, `c`, `d` [E0026] - //~| ERROR pattern does not mention fields `0`, `1`, `2`, `3` [E0027] + //~^ ERROR tuple variant `S` written as struct variant println!("hi"); } } diff --git a/src/test/ui/missing/missing-fields-in-struct-pattern.stderr b/src/test/ui/missing/missing-fields-in-struct-pattern.stderr index f7037468996f4..6583524aad18f 100644 --- a/src/test/ui/missing/missing-fields-in-struct-pattern.stderr +++ b/src/test/ui/missing/missing-fields-in-struct-pattern.stderr @@ -1,18 +1,9 @@ -error[E0026]: struct `S` does not have fields named `a`, `b`, `c`, `d` - --> $DIR/missing-fields-in-struct-pattern.rs:4:16 - | -LL | if let S { a, b, c, d } = S(1, 2, 3, 4) { - | ^ ^ ^ ^ struct `S` does not have these fields - -error[E0027]: pattern does not mention fields `0`, `1`, `2`, `3` +error[E0769]: tuple variant `S` written as struct variant --> $DIR/missing-fields-in-struct-pattern.rs:4:12 | LL | if let S { a, b, c, d } = S(1, 2, 3, 4) { - | ^^^^^^^^^^^^^^^^ missing fields `0`, `1`, `2`, `3` - | - = note: trying to match a tuple variant with a struct variant pattern + | ^^^^^^^^^^^^^^^^ help: use the tuple variant pattern syntax instead: `S(a, b, c, d)` -error: aborting due to 2 previous errors +error: aborting due to previous error -Some errors have detailed explanations: E0026, E0027. -For more information about an error, try `rustc --explain E0026`. +For more information about this error, try `rustc --explain E0769`. diff --git a/src/test/ui/type/type-check/issue-41314.rs b/src/test/ui/type/type-check/issue-41314.rs index 856d4ff6334bc..cbd39f5f9e6ed 100644 --- a/src/test/ui/type/type-check/issue-41314.rs +++ b/src/test/ui/type/type-check/issue-41314.rs @@ -4,7 +4,7 @@ enum X { fn main() { match X::Y(0) { - X::Y { number } => {} //~ ERROR does not have a field named `number` - //~^ ERROR pattern does not mention field `0` + X::Y { number } => {} + //~^ ERROR tuple variant `X::Y` written as struct variant } } diff --git a/src/test/ui/type/type-check/issue-41314.stderr b/src/test/ui/type/type-check/issue-41314.stderr index c2bba98d10a83..bd4d2071c2059 100644 --- a/src/test/ui/type/type-check/issue-41314.stderr +++ b/src/test/ui/type/type-check/issue-41314.stderr @@ -1,18 +1,9 @@ -error[E0026]: variant `X::Y` does not have a field named `number` - --> $DIR/issue-41314.rs:7:16 - | -LL | X::Y { number } => {} - | ^^^^^^ variant `X::Y` does not have this field - -error[E0027]: pattern does not mention field `0` +error[E0769]: tuple variant `X::Y` written as struct variant --> $DIR/issue-41314.rs:7:9 | LL | X::Y { number } => {} - | ^^^^^^^^^^^^^^^ missing field `0` - | - = note: trying to match a tuple variant with a struct variant pattern + | ^^^^^^^^^^^^^^^ help: use the tuple variant pattern syntax instead: `X::Y(number)` -error: aborting due to 2 previous errors +error: aborting due to previous error -Some errors have detailed explanations: E0026, E0027. -For more information about an error, try `rustc --explain E0026`. +For more information about this error, try `rustc --explain E0769`. diff --git a/src/test/ui/union/union-fields-2.stderr b/src/test/ui/union/union-fields-2.stderr index 68cb66d89d218..48654347285d3 100644 --- a/src/test/ui/union/union-fields-2.stderr +++ b/src/test/ui/union/union-fields-2.stderr @@ -48,18 +48,18 @@ error: union patterns should have exactly one field LL | let U { a, b } = u; | ^^^^^^^^^^ -error[E0026]: union `U` does not have a field named `c` - --> $DIR/union-fields-2.rs:18:19 - | -LL | let U { a, b, c } = u; - | ^ union `U` does not have this field - error: union patterns should have exactly one field --> $DIR/union-fields-2.rs:18:9 | LL | let U { a, b, c } = u; | ^^^^^^^^^^^^^ +error[E0026]: union `U` does not have a field named `c` + --> $DIR/union-fields-2.rs:18:19 + | +LL | let U { a, b, c } = u; + | ^ union `U` does not have this field + error: union patterns should have exactly one field --> $DIR/union-fields-2.rs:20:9 | From 0e89f50f6e937349233db1c52af2b33c683782e6 Mon Sep 17 00:00:00 2001 From: Jarek Samic Date: Sun, 12 Jul 2020 14:37:22 -0400 Subject: [PATCH 48/49] Clean up handling of style files in rustdoc Disable all themes other than `light.css` to prevent rule conflicts --- src/librustdoc/config.rs | 5 +- src/librustdoc/html/layout.rs | 25 +++++----- src/librustdoc/html/render.rs | 87 ++++++++++++++++++++++------------ src/librustdoc/html/sources.rs | 2 +- 4 files changed, 76 insertions(+), 43 deletions(-) diff --git a/src/librustdoc/config.rs b/src/librustdoc/config.rs index 14a6f3c89a3c9..39e33da44964e 100644 --- a/src/librustdoc/config.rs +++ b/src/librustdoc/config.rs @@ -20,6 +20,7 @@ use crate::core::new_handler; use crate::externalfiles::ExternalHtml; use crate::html; use crate::html::markdown::IdMap; +use crate::html::render::StylePath; use crate::html::static_files; use crate::opts; use crate::passes::{self, Condition, DefaultPassOption}; @@ -207,7 +208,7 @@ pub struct RenderOptions { pub sort_modules_alphabetically: bool, /// List of themes to extend the docs with. Original argument name is included to assist in /// displaying errors if it fails a theme check. - pub themes: Vec, + pub themes: Vec, /// If present, CSS file that contains rules to add to the default CSS. pub extension_css: Option, /// A map of crate names to the URL to use instead of querying the crate's `html_root_url`. @@ -410,7 +411,7 @@ impl Options { )) .emit(); } - themes.push(theme_file); + themes.push(StylePath { path: theme_file, disabled: true }); } } diff --git a/src/librustdoc/html/layout.rs b/src/librustdoc/html/layout.rs index ea65b3905272e..cc6b38ebcdb7f 100644 --- a/src/librustdoc/html/layout.rs +++ b/src/librustdoc/html/layout.rs @@ -3,7 +3,7 @@ use std::path::PathBuf; use crate::externalfiles::ExternalHtml; use crate::html::escape::Escape; use crate::html::format::{Buffer, Print}; -use crate::html::render::ensure_trailing_slash; +use crate::html::render::{ensure_trailing_slash, StylePath}; #[derive(Clone)] pub struct Layout { @@ -36,7 +36,7 @@ pub fn render( page: &Page<'_>, sidebar: S, t: T, - themes: &[PathBuf], + style_files: &[StylePath], ) -> String { let static_root_path = page.static_root_path.unwrap_or(page.root_path); format!( @@ -52,10 +52,7 @@ pub fn render( \ \ - {themes}\ - \ - \ + {style_files}\ \ \ {css_extension}\ @@ -172,13 +169,19 @@ pub fn render( after_content = layout.external_html.after_content, sidebar = Buffer::html().to_display(sidebar), krate = layout.krate, - themes = themes + style_files = style_files .iter() - .filter_map(|t| t.file_stem()) - .filter_map(|t| t.to_str()) + .filter_map(|t| { + if let Some(stem) = t.path.file_stem() { Some((stem, t.disabled)) } else { None } + }) + .filter_map(|t| { + if let Some(path) = t.0.to_str() { Some((path, t.1)) } else { None } + }) .map(|t| format!( - r#""#, - Escape(&format!("{}{}{}", static_root_path, t, page.resource_suffix)) + r#""#, + Escape(&format!("{}{}{}", static_root_path, t.0, page.resource_suffix)), + if t.1 { "disabled" } else { "" }, + if t.0 == "light" { "id=\"themeStyle\"" } else { "" } )) .collect::(), suffix = page.resource_suffix, diff --git a/src/librustdoc/html/render.rs b/src/librustdoc/html/render.rs index 8bba21a2e7ace..be14e686cf146 100644 --- a/src/librustdoc/html/render.rs +++ b/src/librustdoc/html/render.rs @@ -187,8 +187,8 @@ crate struct SharedContext { /// This flag indicates whether listings of modules (in the side bar and documentation itself) /// should be ordered alphabetically or in order of appearance (in the source code). pub sort_modules_alphabetically: bool, - /// Additional themes to be added to the generated docs. - pub themes: Vec, + /// Additional CSS files to be added to the generated docs. + pub style_files: Vec, /// Suffix to be added on resource files (if suffix is "-v2" then "light.css" becomes /// "light-v2.css"). pub resource_suffix: String, @@ -417,6 +417,14 @@ impl Serialize for TypeWithKind { } } +#[derive(Debug, Clone)] +pub struct StylePath { + /// The path to the theme + pub path: PathBuf, + /// What the `disabled` attribute should be set to in the HTML tag + pub disabled: bool, +} + thread_local!(static CACHE_KEY: RefCell> = Default::default()); thread_local!(pub static CURRENT_DEPTH: Cell = Cell::new(0)); @@ -460,7 +468,7 @@ pub fn run( id_map, playground_url, sort_modules_alphabetically, - themes, + themes: style_files, extension_css, extern_html_root_urls, resource_suffix, @@ -530,7 +538,7 @@ pub fn run( layout, created_dirs: Default::default(), sort_modules_alphabetically, - themes, + style_files, resource_suffix, static_root_path, fs: DocFS::new(&errors), @@ -539,6 +547,18 @@ pub fn run( playground, }; + // Add the default themes to the `Vec` of stylepaths + // + // Note that these must be added before `sources::render` is called + // so that the resulting source pages are styled + // + // `light.css` is not disabled because it is the stylesheet that stays loaded + // by the browser as the theme stylesheet. The theme system (hackily) works by + // changing the href to this stylesheet. All other themes are disabled to + // prevent rule conflicts + scx.style_files.push(StylePath { path: PathBuf::from("light.css"), disabled: false }); + scx.style_files.push(StylePath { path: PathBuf::from("dark.css"), disabled: true }); + let dst = output; scx.ensure_dir(&dst)?; krate = sources::render(&dst, &mut scx, krate)?; @@ -615,11 +635,34 @@ fn write_shared( // then we'll run over the "official" styles. let mut themes: FxHashSet = FxHashSet::default(); - for entry in &cx.shared.themes { - let content = try_err!(fs::read(&entry), &entry); - let theme = try_none!(try_none!(entry.file_stem(), &entry).to_str(), &entry); - let extension = try_none!(try_none!(entry.extension(), &entry).to_str(), &entry); - cx.shared.fs.write(cx.path(&format!("{}.{}", theme, extension)), content.as_slice())?; + for entry in &cx.shared.style_files { + let theme = try_none!(try_none!(entry.path.file_stem(), &entry.path).to_str(), &entry.path); + let extension = + try_none!(try_none!(entry.path.extension(), &entry.path).to_str(), &entry.path); + + // Handle the official themes + match theme { + "light" => write_minify( + &cx.shared.fs, + cx.path("light.css"), + static_files::themes::LIGHT, + options.enable_minification, + )?, + "dark" => write_minify( + &cx.shared.fs, + cx.path("dark.css"), + static_files::themes::DARK, + options.enable_minification, + )?, + _ => { + // Handle added third-party themes + let content = try_err!(fs::read(&entry.path), &entry.path); + cx.shared + .fs + .write(cx.path(&format!("{}.{}", theme, extension)), content.as_slice())?; + } + }; + themes.insert(theme.to_owned()); } @@ -633,20 +676,6 @@ fn write_shared( write(cx.path("brush.svg"), static_files::BRUSH_SVG)?; write(cx.path("wheel.svg"), static_files::WHEEL_SVG)?; write(cx.path("down-arrow.svg"), static_files::DOWN_ARROW_SVG)?; - write_minify( - &cx.shared.fs, - cx.path("light.css"), - static_files::themes::LIGHT, - options.enable_minification, - )?; - themes.insert("light".to_owned()); - write_minify( - &cx.shared.fs, - cx.path("dark.css"), - static_files::themes::DARK, - options.enable_minification, - )?; - themes.insert("dark".to_owned()); let mut themes: Vec<&String> = themes.iter().collect(); themes.sort(); @@ -957,7 +986,7 @@ themePicker.onblur = handleThemeButtonsBlur; }) .collect::() ); - let v = layout::render(&cx.shared.layout, &page, "", content, &cx.shared.themes); + let v = layout::render(&cx.shared.layout, &page, "", content, &cx.shared.style_files); cx.shared.fs.write(&dst, v.as_bytes())?; } } @@ -1375,7 +1404,7 @@ impl Context { &page, sidebar, |buf: &mut Buffer| all.print(buf), - &self.shared.themes, + &self.shared.style_files, ); self.shared.fs.write(&final_file, v.as_bytes())?; @@ -1384,9 +1413,9 @@ impl Context { page.description = "Settings of Rustdoc"; page.root_path = "./"; - let mut themes = self.shared.themes.clone(); + let mut style_files = self.shared.style_files.clone(); let sidebar = "

Settings

"; - themes.push(PathBuf::from("settings.css")); + style_files.push(StylePath { path: PathBuf::from("settings.css"), disabled: false }); let v = layout::render( &self.shared.layout, &page, @@ -1395,7 +1424,7 @@ impl Context { self.shared.static_root_path.as_deref().unwrap_or("./"), &self.shared.resource_suffix, ), - &themes, + &style_files, ); self.shared.fs.write(&settings_file, v.as_bytes())?; @@ -1457,7 +1486,7 @@ impl Context { &page, |buf: &mut _| print_sidebar(self, it, buf), |buf: &mut _| print_item(self, it, buf), - &self.shared.themes, + &self.shared.style_files, ) } else { let mut url = self.root_path(); diff --git a/src/librustdoc/html/sources.rs b/src/librustdoc/html/sources.rs index f0900c34a4ba3..03f79b931868b 100644 --- a/src/librustdoc/html/sources.rs +++ b/src/librustdoc/html/sources.rs @@ -123,7 +123,7 @@ impl<'a> SourceCollector<'a> { &page, "", |buf: &mut _| print_src(buf, &contents), - &self.scx.themes, + &self.scx.style_files, ); self.scx.fs.write(&cur, v.as_bytes())?; self.scx.local_sources.insert(p, href); From 8c45cf8e60960698262363cf15aa650437f49a56 Mon Sep 17 00:00:00 2001 From: Jarek Samic Date: Sun, 12 Jul 2020 14:48:13 -0400 Subject: [PATCH 49/49] Add Ayu theme to rustdoc --- src/librustdoc/html/render.rs | 7 + src/librustdoc/html/static/themes/ayu.css | 561 ++++++++++++++++++++++ src/librustdoc/html/static_files.rs | 3 + 3 files changed, 571 insertions(+) create mode 100644 src/librustdoc/html/static/themes/ayu.css diff --git a/src/librustdoc/html/render.rs b/src/librustdoc/html/render.rs index be14e686cf146..456eb9fcbc41a 100644 --- a/src/librustdoc/html/render.rs +++ b/src/librustdoc/html/render.rs @@ -558,6 +558,7 @@ pub fn run( // prevent rule conflicts scx.style_files.push(StylePath { path: PathBuf::from("light.css"), disabled: false }); scx.style_files.push(StylePath { path: PathBuf::from("dark.css"), disabled: true }); + scx.style_files.push(StylePath { path: PathBuf::from("ayu.css"), disabled: true }); let dst = output; scx.ensure_dir(&dst)?; @@ -654,6 +655,12 @@ fn write_shared( static_files::themes::DARK, options.enable_minification, )?, + "ayu" => write_minify( + &cx.shared.fs, + cx.path("ayu.css"), + static_files::themes::AYU, + options.enable_minification, + )?, _ => { // Handle added third-party themes let content = try_err!(fs::read(&entry.path), &entry.path); diff --git a/src/librustdoc/html/static/themes/ayu.css b/src/librustdoc/html/static/themes/ayu.css new file mode 100644 index 0000000000000..bc21c28750fd8 --- /dev/null +++ b/src/librustdoc/html/static/themes/ayu.css @@ -0,0 +1,561 @@ +/* +Based off of the Ayu theme +Original by Dempfi (https://github.com/dempfi/ayu) +*/ + +/* General structure and fonts */ + +body { + background-color: #0f1419; + color: #c5c5c5; +} + +h1, h2, h3:not(.impl):not(.method):not(.type):not(.tymethod), h4:not(.method):not(.type):not(.tymethod) { + color: white; +} +h1.fqn { + border-bottom-color: #5c6773; +} +h1.fqn a { + color: #fff; +} +h2, h3:not(.impl):not(.method):not(.type):not(.tymethod) { + border-bottom-color: #5c6773; +} +h4:not(.method):not(.type):not(.tymethod):not(.associatedconstant) { + border: none; +} + +.in-band { + background-color: #0f1419; +} + +.invisible { + background: rgba(0, 0, 0, 0); +} + +code { + color: #ffb454; +} +h3 > code, h4 > code, h5 > code { + color: #e6e1cf; +} +pre > code { + color: #e6e1cf; +} +span code { + color: #e6e1cf; +} +.docblock a > code { + color: #39AFD7 !important; +} +.docblock code, .docblock-short code { + background-color: #191f26; +} +pre { + color: #e6e1cf; + background-color: #191f26; +} + +.sidebar { + background-color: #14191f; +} + +/* Improve the scrollbar display on firefox */ +* { + scrollbar-color: #5c6773 transparent; +} + +.sidebar { + scrollbar-color: #5c6773 transparent; +} + +/* Improve the scrollbar display on webkit-based browsers */ +::-webkit-scrollbar-track { + background-color: transparent; +} +::-webkit-scrollbar-thumb { + background-color: #5c6773; +} +.sidebar::-webkit-scrollbar-track { + background-color: transparent; +} +.sidebar::-webkit-scrollbar-thumb { + background-color: #5c6773; +} + +.sidebar .current { + background-color: transparent; + color: #ffb44c; +} + +.source .sidebar { + background-color: #0f1419; +} + +.sidebar .location { + border-color: #000; + background-color: #0f1419; + color: #fff; +} + +.sidebar-elems .location { + color: #ff7733; +} + +.sidebar-elems .location a { + color: #fff; +} + +.sidebar .version { + border-bottom-color: #DDD; +} + +.sidebar-title { + border-top-color: #5c6773; + border-bottom-color: #5c6773; +} + +.block a:hover { + background: transparent; + color: #ffb44c; +} + +.line-numbers span { color: #5c6773ab; } +.line-numbers .line-highlighted { + background-color: rgba(255, 236, 164, 0.06) !important; + padding-right: 4px; + border-right: 1px solid #ffb44c; +} + +.docblock h1, .docblock h2, .docblock h3, .docblock h4, .docblock h5 { + border-bottom-color: #5c6773; +} + +.docblock table, .docblock table td, .docblock table th { + border-color: #5c6773; +} + +.content .method .where, +.content .fn .where, +.content .where.fmt-newline { + color: #c5c5c5; +} + +.content .highlighted { + color: #000 !important; + background-color: #c6afb3; +} +.content .highlighted a, .content .highlighted span { color: #000 !important; } +.content .highlighted { + background-color: #c6afb3; +} +.search-results a { + color: #0096cf; +} +.search-results a span.desc { + color: #c5c5c5; +} + +.content .stability::before { color: #ccc; } + +.content span.foreigntype, .content a.foreigntype { color: #ef57ff; } +.content span.union, .content a.union { color: #98a01c; } +.content span.constant, .content a.constant, +.content span.static, .content a.static { color: #6380a0; } +.content span.primitive, .content a.primitive { color: #32889b; } +.content span.traitalias, .content a.traitalias { color: #57d399; } +.content span.keyword, .content a.keyword { color: #de5249; } + +.content span.externcrate, .content span.mod, .content a.mod { + color: #acccf9; +} +.content span.struct, .content a.struct { + color: #ffa0a5; +} +.content span.enum, .content a.enum { + color: #99e0c9; +} +.content span.trait, .content a.trait { + color: #39AFD7; +} +.content span.type, .content a.type { + color: #cfbcf5; +} +.content span.fn, .content a.fn, .content span.method, +.content a.method, .content span.tymethod, +.content a.tymethod, .content .fnname { + color: #fdd687; +} +.content span.attr, .content a.attr, .content span.derive, +.content a.derive, .content span.macro, .content a.macro { + color: #a37acc; +} + +pre.rust .comment, pre.rust .doccomment { + color: #788797; + font-style: italic; +} + +nav:not(.sidebar) { + border-bottom-color: #e0e0e0; +} +nav.main .current { + border-top-color: #5c6773; + border-bottom-color: #5c6773; +} +nav.main .separator { + border: 1px solid #5c6773; +} +a { + color: #c5c5c5; +} + +.docblock:not(.type-decl) a:not(.srclink):not(.test-arrow), +.docblock-short a:not(.srclink):not(.test-arrow), .stability a { + color: #39AFD7; +} + +.stab.internal a { + color: #304FFE; +} + +.collapse-toggle { + color: #999; +} + +#crate-search { + color: #c5c5c5; + background-color: #141920; + border-radius: 4px; + box-shadow: none; + border-color: #5c6773; +} + +.search-input { + color: #ffffff; + background-color: #141920; + box-shadow: none; + transition: box-shadow 150ms ease-in-out; + border-radius: 4px; + margin-left: 8px; +} + +#crate-search+.search-input:focus { + box-shadow: 0px 6px 20px 0px black; +} + +.search-focus:disabled { + color: #929292; +} + +.module-item .stab { + color: #000; +} + +.stab.unstable, +.stab.internal, +.stab.deprecated, +.stab.portability { + color: #c5c5c5; + background: #314559 !important; + border-style: none !important; + border-radius: 4px; + padding: 3px 6px 3px 6px; +} + +.stab.portability > code { + color: #e6e1cf; + background-color: transparent; +} + +#help > div { + background: #14191f; + box-shadow: 0px 6px 20px 0px black; + border: none; + border-radius: 4px; +} + +.since { + color: grey; +} + +tr.result span.primitive::after, tr.result span.keyword::after { + color: #788797; +} + +.line-numbers :target { background-color: transparent; } + +/* Code highlighting */ +pre.rust .number, pre.rust .string { color: #b8cc52; } +pre.rust .kw, pre.rust .kw-2, pre.rust .prelude-ty, +pre.rust .bool-val, pre.rust .prelude-val, +pre.rust .op, pre.rust .lifetime { color: #ff7733; } +pre.rust .macro, pre.rust .macro-nonterminal { color: #a37acc; } +pre.rust .question-mark { + color: #ff9011; +} +pre.rust .self { + color: #36a3d9; + font-style: italic; +} +pre.rust .attribute { + color: #e6e1cf; +} +pre.rust .attribute .ident, pre.rust .attribute .op { + color: #e6e1cf; +} + +.example-wrap > pre.line-number { + color: #5c67736e; + border: none; +} + +a.test-arrow { + font-size: 100%; + color: #788797; + border-radius: 4px; + background-color: rgba(255, 255, 255, 0); +} + +a.test-arrow:hover { + background-color: rgba(242, 151, 24, 0.05); + color: #ffb44c; +} + +.toggle-label { + color: #999; +} + +:target > code, :target > .in-band { + background: rgba(255, 236, 164, 0.06); + border-right: 3px solid #ffb44c; +} + +pre.compile_fail { + border-left: 2px solid rgba(255,0,0,.4); +} + +pre.compile_fail:hover, .information:hover + pre.compile_fail { + border-left: 2px solid #f00; +} + +pre.should_panic { + border-left: 2px solid rgba(255,0,0,.4); +} + +pre.should_panic:hover, .information:hover + pre.should_panic { + border-left: 2px solid #f00; +} + +pre.ignore { + border-left: 2px solid rgba(255,142,0,.6); +} + +pre.ignore:hover, .information:hover + pre.ignore { + border-left: 2px solid #ff9200; +} + +.tooltip.compile_fail { + color: rgba(255,0,0,.5); +} + +.information > .compile_fail:hover { + color: #f00; +} + +.tooltip.should_panic { + color: rgba(255,0,0,.5); +} + +.information > .should_panic:hover { + color: #f00; +} + +.tooltip.ignore { + color: rgba(255,142,0,.6); +} + +.information > .ignore:hover { + color: #ff9200; +} + +.search-failed a { + color: #39AFD7; +} + +.tooltip .tooltiptext { + background-color: #314559; + color: #c5c5c5; + border: 1px solid #5c6773; +} + +.tooltip .tooltiptext::after { + border-color: transparent #314559 transparent transparent; +} + +#titles > div.selected { + background-color: #141920 !important; + border-bottom: 1px solid #ffb44c !important; + border-top: none; +} + +#titles > div:not(.selected) { + background-color: transparent !important; + border: none; +} + +#titles > div:hover { + border-bottom: 1px solid rgba(242, 151, 24, 0.3); +} + +#titles > div > div.count { + color: #888; +} + +/* rules that this theme does not need to set, here to satisfy the rule checker */ +/* note that a lot of these are partially set in some way (meaning they are set +individually rather than as a group) */ +/* TODO: these rules should be at the bottom of the file but currently must be +above the `@media (max-width: 700px)` rules due to a bug in the css checker */ +/* see https://github.com/rust-lang/rust/pull/71237#issuecomment-618170143 */ +.content .highlighted.mod, .content .highlighted.externcrate {} +.search-input:focus {} +.content span.attr,.content a.attr,.block a.current.attr,.content span.derive,.content a.derive,.block a.current.derive,.content span.macro,.content a.macro,.block a.current.macro {} +.content .highlighted.trait {} +.content span.struct,.content a.struct,.block a.current.struct {} +#titles>div:hover,#titles>div.selected {} +.content .highlighted.traitalias {} +.content span.type,.content a.type,.block a.current.type {} +.content span.union,.content a.union,.block a.current.union {} +.content .highlighted.foreigntype {} +pre.rust .lifetime {} +.content .highlighted.primitive {} +.content .highlighted.constant,.content .highlighted.static {} +.stab.unstable {} +.content .highlighted.fn,.content .highlighted.method,.content .highlighted.tymethod {} +h2,h3:not(.impl):not(.method):not(.type):not(.tymethod),h4:not(.method):not(.type):not(.tymethod) {} +.content span.enum,.content a.enum,.block a.current.enum {} +.content span.constant,.content a.constant,.block a.current.constant,.content span.static,.content a.static,.block a.current.static {} +.content span.keyword,.content a.keyword,.block a.current.keyword {} +pre.rust .comment {} +.content .highlighted.enum {} +.content .highlighted.struct {} +.content .highlighted.keyword {} +.content span.traitalias,.content a.traitalias,.block a.current.traitalias {} +.content span.fn,.content a.fn,.block a.current.fn,.content span.method,.content a.method,.block a.current.method,.content span.tymethod,.content a.tymethod,.block a.current.tymethod,.content .fnname {} +pre.rust .kw {} +pre.rust .self,pre.rust .bool-val,pre.rust .prelude-val,pre.rust .attribute,pre.rust .attribute .ident {} +.content span.foreigntype,.content a.foreigntype,.block a.current.foreigntype {} +pre.rust .doccomment {} +.stab.deprecated {} +.content .highlighted.attr,.content .highlighted.derive,.content .highlighted.macro {} +.stab.portability {} +.content .highlighted.union {} +.content span.primitive,.content a.primitive,.block a.current.primitive {} +.content span.externcrate,.content span.mod,.content a.mod,.block a.current.mod {} +.content .highlighted.type {} +pre.rust .kw-2,pre.rust .prelude-ty {} +.content span.trait,.content a.trait,.block a.current.trait {} +.stab.internal {} + +@media (max-width: 700px) { + .sidebar-menu { + background-color: #14191f; + border-bottom-color: #5c6773; + border-right-color: #5c6773; + } + + .sidebar-elems { + background-color: #14191f; + border-right-color: #5c6773; + } + + #sidebar-filler { + background-color: #14191f; + border-bottom-color: #5c6773; + } +} + +kbd { + color: #c5c5c5; + background-color: #314559; + border-color: #5c6773; + border-bottom-color: #5c6773; + box-shadow-color: #c6cbd1; +} + +#theme-picker, #settings-menu { + border-color: #5c6773; + background-color: #0f1419; +} + +#theme-picker > img, #settings-menu > img { + filter: invert(100); +} + +#theme-picker:hover, #theme-picker:focus, +#settings-menu:hover, #settings-menu:focus { + border-color: #e0e0e0; +} + +#theme-choices { + border-color: #5c6773; + background-color: #0f1419; +} + +#theme-choices > button:not(:first-child) { + border-top-color: #c5c5c5; +} + +#theme-choices > button:hover, #theme-choices > button:focus { + background-color: rgba(70, 70, 70, 0.33); +} + +@media (max-width: 700px) { + #theme-picker { + background: #0f1419; + } +} + +#all-types { + background-color: #14191f; +} +#all-types:hover { + background-color: rgba(70, 70, 70, 0.33); +} + +.search-results td span.alias { + color: #c5c5c5; +} +.search-results td span.grey { + color: #999; +} + +#sidebar-toggle { + background-color: #14191f; +} +#sidebar-toggle:hover { + background-color: rgba(70, 70, 70, 0.33); +} +#source-sidebar { + background-color: #14191f; +} +#source-sidebar > .title { + color: #fff; + border-bottom-color: #5c6773; +} +div.files > a:hover, div.name:hover { + background-color: #14191f; + color: #ffb44c; +} +div.files > .selected { + background-color: #14191f; + color: #ffb44c; +} +.setting-line > .title { + border-bottom-color: #5c6773; +} +input:checked + .slider { + background-color: #ffb454 !important; +} diff --git a/src/librustdoc/html/static_files.rs b/src/librustdoc/html/static_files.rs index 6790f3bd5d0b1..6bd7e53cdfbe2 100644 --- a/src/librustdoc/html/static_files.rs +++ b/src/librustdoc/html/static_files.rs @@ -64,6 +64,9 @@ pub mod themes { /// The "dark" theme. pub static DARK: &str = include_str!("static/themes/dark.css"); + + /// The "ayu" theme. + pub static AYU: &str = include_str!("static/themes/ayu.css"); } /// Files related to the Fira Sans font.