From 2083eae2c5b480ccad672fb2fbb1b2bb5774a606 Mon Sep 17 00:00:00 2001 From: Jesse Luehrs Date: Tue, 14 Dec 2021 03:49:13 -0500 Subject: replace all uses of unwrap(), expect(), and indexing with unreachable!() and also document why they are unreachable --- src/cell.rs | 16 +++-- src/grid.rs | 79 +++++++++++++++++++++---- src/lib.rs | 3 + src/row.rs | 23 ++++---- src/screen.rs | 185 ++++++++++++++++++++++++++++++++++++++++++++++++++-------- src/term.rs | 14 +++-- 6 files changed, 264 insertions(+), 56 deletions(-) diff --git a/src/cell.rs b/src/cell.rs index 1b44461..5f09ffb 100644 --- a/src/cell.rs +++ b/src/cell.rs @@ -19,7 +19,9 @@ impl PartialEq for Cell { return false; } let len = self.len(); - self.contents[..len] == other.contents[..len] + // self.len() always returns a valid value + self.contents.get(..len).unwrap_or_else(|| unreachable!()) + == other.contents.get(..len).unwrap_or_else(|| unreachable!()) } } @@ -40,15 +42,19 @@ impl Cell { } pub(crate) fn append(&mut self, c: char) { - if self.len() >= CODEPOINTS_IN_CELL { + let len = self.len(); + if len >= CODEPOINTS_IN_CELL { return; } - if self.len() == 0 { - self.contents[self.len()] = ' '; + if len == 0 { + // 0 is always less than 6 + *self.contents.get_mut(0).unwrap_or_else(|| unreachable!()) = ' '; self.len += 1; } - self.contents[self.len()] = c; + let len = self.len(); + // we already checked that len < CODEPOINTS_IN_CELL + *self.contents.get_mut(len).unwrap_or_else(|| unreachable!()) = c; self.len += 1; } diff --git a/src/grid.rs b/src/grid.rs index 085e744..21fb03c 100644 --- a/src/grid.rs +++ b/src/grid.rs @@ -152,7 +152,8 @@ impl Grid { pub fn current_row_mut(&mut self) -> &mut crate::row::Row { self.drawing_row_mut(self.pos.row) - .expect("cursor not pointing to a cell") + // we assume self.pos.row is always valid + .unwrap_or_else(|| unreachable!()) } pub fn visible_cell(&self, pos: Pos) -> Option<&crate::cell::Cell> { @@ -209,7 +210,9 @@ impl Grid { let mut prev_pos = Pos::default(); let mut wrapping = false; for (i, row) in self.visible_rows().enumerate() { - let i = i.try_into().unwrap(); + // we limit the number of cols to a u16 (see Size), so + // visible_rows() can never return more rows than will fit + let i = i.try_into().unwrap_or_else(|_| unreachable!()); let (new_pos, new_attrs) = row.write_contents_formatted( contents, 0, @@ -245,7 +248,9 @@ impl Grid { for (i, (row, prev_row)) in self.visible_rows().zip(prev.visible_rows()).enumerate() { - let i = i.try_into().unwrap(); + // we limit the number of cols to a u16 (see Size), so + // visible_rows() can never return more rows than will fit + let i = i.try_into().unwrap_or_else(|_| unreachable!()); let (new_pos, new_attrs) = row.write_contents_diff( contents, prev_row, @@ -289,10 +294,23 @@ impl Grid { row: self.pos.row, col: self.size.cols - 1, }; - if self.drawing_cell(pos).unwrap().is_wide_continuation() { + if self + .drawing_cell(pos) + // we assume self.pos.row is always valid, and self.size.cols + // - 1 is always a valid column + .unwrap_or_else(|| unreachable!()) + .is_wide_continuation() + { pos.col = self.size.cols - 2; } - let cell = self.drawing_cell(pos).unwrap(); + let cell = + // we assume self.pos.row is always valid, and self.size.cols + // - 2 must be a valid column because self.size.cols - 1 is + // always valid and we just checked that the cell at + // self.size.cols - 1 is a wide continuation character, which + // means that the first half of the wide character must be + // before it + self.drawing_cell(pos).unwrap_or_else(|| unreachable!()); if cell.has_contents() { if let Some(prev_pos) = prev_pos { crate::term::MoveFromTo::new(prev_pos, pos) @@ -316,11 +334,27 @@ impl Grid { for i in (0..self.pos.row).rev() { pos.row = i; pos.col = self.size.cols - 1; - if self.drawing_cell(pos).unwrap().is_wide_continuation() + if self + .drawing_cell(pos) + // i is always less than self.pos.row, which we assume + // to be always valid, so it must also be valid. + // self.size.cols - 1 is always a valid col. + .unwrap_or_else(|| unreachable!()) + .is_wide_continuation() { pos.col = self.size.cols - 2; } - let cell = self.drawing_cell(pos).unwrap(); + let cell = self + .drawing_cell(pos) + // i is always less than self.pos.row, which we assume + // to be always valid, so it must also be valid. + // self.size.cols - 2 is valid because self.size.cols + // - 1 is always valid, and col gets set to + // self.size.cols - 2 when the cell at self.size.cols + // - 1 is a wide continuation character, meaning that + // the first half of the wide character must be before + // it + .unwrap_or_else(|| unreachable!()); if cell.has_contents() { if let Some(prev_pos) = prev_pos { if prev_pos.row != i @@ -379,7 +413,11 @@ impl Grid { contents.push(b' '); // we know that the cell has no contents, but it still may // have drawing attributes (background color, etc) - let end_cell = self.drawing_cell(pos).unwrap(); + let end_cell = self + .drawing_cell(pos) + // we assume self.pos.row is always valid, and + // self.size.cols - 1 is always a valid column + .unwrap_or_else(|| unreachable!()); end_cell .attrs() .write_escape_code_diff(contents, &prev_attrs); @@ -449,7 +487,13 @@ impl Grid { let size = self.size; let pos = self.pos; let wide = pos.col < size.cols - && self.drawing_cell(pos).unwrap().is_wide_continuation(); + && self + .drawing_cell(pos) + // we assume self.pos.row is always valid, and we know we are + // not off the end of a row because we just checked pos.col < + // size.cols + .unwrap_or_else(|| unreachable!()) + .is_wide_continuation(); let row = self.current_row_mut(); for _ in 0..count { if wide { @@ -486,7 +530,11 @@ impl Grid { for _ in 0..count { self.rows.remove(self.scroll_bottom as usize); self.rows.insert(self.pos.row as usize, self.new_row()); - self.rows[self.scroll_bottom as usize].wrap(false); + self.rows + .get_mut(self.scroll_bottom as usize) + // self.scroll_bottom is maintained to always be a valid row + .unwrap_or_else(|| unreachable!()) + .wrap(false); } } @@ -520,7 +568,11 @@ impl Grid { for _ in 0..count { self.rows.remove(self.scroll_bottom as usize); self.rows.insert(self.scroll_top as usize, self.new_row()); - self.rows[self.scroll_bottom as usize].wrap(false); + self.rows + .get_mut(self.scroll_bottom as usize) + // self.scroll_bottom is maintained to always be a valid row + .unwrap_or_else(|| unreachable!()) + .wrap(false); } } @@ -625,7 +677,10 @@ impl Grid { prev_pos.row -= scrolled; let new_pos = self.pos; self.drawing_row_mut(prev_pos.row) - .unwrap() + // we assume self.pos.row is always valid, and so prev_pos.row + // must be valid because it is always less than or equal to + // self.pos.row + .unwrap_or_else(|| unreachable!()) .wrap(wrap && prev_pos.row + 1 == new_pos.row); } } diff --git a/src/lib.rs b/src/lib.rs index bfc977b..0567140 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -36,6 +36,9 @@ #![warn(clippy::cargo)] #![warn(clippy::pedantic)] #![warn(clippy::nursery)] +#![warn(clippy::unwrap_used)] +#![warn(clippy::expect_used)] +#![warn(clippy::indexing_slicing)] #![allow(clippy::cognitive_complexity)] #![allow(clippy::missing_const_for_fn)] #![allow(clippy::missing_panics_doc)] diff --git a/src/row.rs b/src/row.rs index 4b0b76e..64b981b 100644 --- a/src/row.rs +++ b/src/row.rs @@ -15,7 +15,11 @@ impl Row { } fn cols(&self) -> u16 { - self.cells.len().try_into().unwrap() + self.cells + .len() + .try_into() + // we limit the number of cols to a u16 (see Size) + .unwrap_or_else(|_| unreachable!()) } pub fn clear(&mut self, attrs: crate::attrs::Attrs) { @@ -113,7 +117,8 @@ impl Row { } prev_was_wide = cell.is_wide(); - let col: u16 = col.try_into().unwrap(); + // we limit the number of cols to a u16 (see Size) + let col: u16 = col.try_into().unwrap_or_else(|_| unreachable!()); if cell.has_contents() { for _ in 0..(col - prev_col) { contents.push(' '); @@ -180,10 +185,9 @@ impl Row { } prev_was_wide = cell.is_wide(); - let pos = crate::grid::Pos { - row, - col: col.try_into().unwrap(), - }; + // we limit the number of cols to a u16 (see Size) + let col: u16 = col.try_into().unwrap_or_else(|_| unreachable!()); + let pos = crate::grid::Pos { row, col }; if let Some((prev_col, attrs)) = erase { if cell.has_contents() || cell.attrs() != attrs { @@ -338,10 +342,9 @@ impl Row { } prev_was_wide = cell.is_wide(); - let pos = crate::grid::Pos { - row, - col: col.try_into().unwrap(), - }; + // we limit the number of cols to a u16 (see Size) + let col: u16 = col.try_into().unwrap_or_else(|_| unreachable!()); + let pos = crate::grid::Pos { row, col }; if let Some((prev_col, attrs)) = erase { if cell.has_contents() || cell.attrs() != attrs { diff --git a/src/screen.rs b/src/screen.rs index 9e81ee2..95951d9 100644 --- a/src/screen.rs +++ b/src/screen.rs @@ -292,7 +292,9 @@ impl Screen { ) -> impl Iterator> + '_ { let mut wrapping = false; self.grid().visible_rows().enumerate().map(move |(i, row)| { - let i = i.try_into().unwrap(); + // number of rows in a grid is stored in a u16 (see Size), so + // visible_rows can never return enough rows to overflow here + let i = i.try_into().unwrap_or_else(|_| unreachable!()); let mut contents = vec![]; row.write_contents_formatted( &mut contents, @@ -359,7 +361,9 @@ impl Screen { .zip(prev.grid().visible_rows()) .enumerate() .map(move |(i, (row, prev_row))| { - let i = i.try_into().unwrap(); + // number of rows in a grid is stored in a u16 (see Size), so + // visible_rows can never return enough rows to overflow here + let i = i.try_into().unwrap_or_else(|_| unreachable!()); let mut contents = vec![]; row.write_contents_diff( &mut contents, @@ -811,7 +815,11 @@ impl Screen { // don't even try to draw control characters return; } - let width = width.unwrap_or(1).try_into().unwrap(); + let width = width + .unwrap_or(1) + .try_into() + // width() can only return 0, 1, or 2 + .unwrap_or_else(|_| unreachable!()); // it doesn't make any sense to wrap if the last column in a row // didn't already have contents. don't try to handle the case where a @@ -829,7 +837,10 @@ impl Screen { row: pos.row, col: size.cols - 1, }) - .unwrap(); + // pos.row is valid, since it comes directly from + // self.grid().pos() which we assume to always have a valid + // row value. size.cols - 1 is also always a valid column. + .unwrap_or_else(|| unreachable!()); if last_cell.has_contents() || last_cell.is_wide_continuation() { wrap = true; } @@ -845,7 +856,11 @@ impl Screen { row: pos.row, col: pos.col - 1, }) - .unwrap(); + // pos.row is valid, since it comes directly from + // self.grid().pos() which we assume to always have a + // valid row value. pos.col - 1 is valid because we just + // checked for pos.col > 0. + .unwrap_or_else(|| unreachable!()); if prev_cell.is_wide_continuation() { prev_cell = self .grid_mut() @@ -853,11 +868,24 @@ impl Screen { row: pos.row, col: pos.col - 2, }) - .unwrap(); + // pos.row is valid, since it comes directly from + // self.grid().pos() which we assume to always have a + // valid row value. we know pos.col - 2 is valid + // because the cell at pos.col - 1 is a wide + // continuation character, which means there must be + // the first half of the wide character before it. + .unwrap_or_else(|| unreachable!()); } prev_cell.append(c); } else if pos.row > 0 { - let prev_row = self.grid().drawing_row(pos.row - 1).unwrap(); + let prev_row = self + .grid() + .drawing_row(pos.row - 1) + // pos.row is valid, since it comes directly from + // self.grid().pos() which we assume to always have a + // valid row value. pos.row - 1 is valid because we just + // checked for pos.row > 0. + .unwrap_or_else(|| unreachable!()); if prev_row.wrapped() { let mut prev_cell = self .grid_mut() @@ -865,7 +893,12 @@ impl Screen { row: pos.row - 1, col: size.cols - 1, }) - .unwrap(); + // pos.row is valid, since it comes directly from + // self.grid().pos() which we assume to always have a + // valid row value. pos.row - 1 is valid because we + // just checked for pos.row > 0. col of size.cols - 1 + // is always valid. + .unwrap_or_else(|| unreachable!()); if prev_cell.is_wide_continuation() { prev_cell = self .grid_mut() @@ -873,7 +906,15 @@ impl Screen { row: pos.row - 1, col: size.cols - 2, }) - .unwrap(); + // pos.row is valid, since it comes directly from + // self.grid().pos() which we assume to always + // have a valid row value. pos.row - 1 is valid + // because we just checked for pos.row > 0. col of + // size.cols - 2 is valid because the cell at + // size.cols - 1 is a wide continuation character, + // so it must have the first half of the wide + // character before it. + .unwrap_or_else(|| unreachable!()); } prev_cell.append(c); } @@ -882,7 +923,11 @@ impl Screen { if self .grid() .drawing_cell(pos) - .unwrap() + // pos.row is valid because we assume self.grid().pos() to + // always have a valid row value. pos.col is valid because we + // called col_wrap() immediately before this, which ensures + // that self.grid().pos().col has a valid value. + .unwrap_or_else(|| unreachable!()) .is_wide_continuation() { let prev_cell = self @@ -891,27 +936,69 @@ impl Screen { row: pos.row, col: pos.col - 1, }) - .unwrap(); + // pos.row is valid because we assume self.grid().pos() to + // always have a valid row value. pos.col is valid because + // we called col_wrap() immediately before this, which + // ensures that self.grid().pos().col has a valid value. + // pos.col - 1 is valid because the cell at pos.col is a + // wide continuation character, so it must have the first + // half of the wide character before it. + .unwrap_or_else(|| unreachable!()); prev_cell.clear(attrs); } - if self.grid().drawing_cell(pos).unwrap().is_wide() { + if self + .grid() + .drawing_cell(pos) + // pos.row is valid because we assume self.grid().pos() to + // always have a valid row value. pos.col is valid because we + // called col_wrap() immediately before this, which ensures + // that self.grid().pos().col has a valid value. + .unwrap_or_else(|| unreachable!()) + .is_wide() + { let next_cell = self .grid_mut() .drawing_cell_mut(crate::grid::Pos { row: pos.row, col: pos.col + 1, }) - .unwrap(); + // pos.row is valid because we assume self.grid().pos() to + // always have a valid row value. pos.col is valid because + // we called col_wrap() immediately before this, which + // ensures that self.grid().pos().col has a valid value. + // pos.col + 1 is valid because the cell at pos.col is a + // wide character, so it must have the second half of the + // wide character after it. + .unwrap_or_else(|| unreachable!()); next_cell.set(' ', attrs); } - let cell = self.grid_mut().drawing_cell_mut(pos).unwrap(); + let cell = self + .grid_mut() + .drawing_cell_mut(pos) + // pos.row is valid because we assume self.grid().pos() to + // always have a valid row value. pos.col is valid because we + // called col_wrap() immediately before this, which ensures + // that self.grid().pos().col has a valid value. + .unwrap_or_else(|| unreachable!()); cell.set(c, attrs); self.grid_mut().col_inc(1); if width > 1 { let pos = self.grid().pos(); - if self.grid().drawing_cell(pos).unwrap().is_wide() { + if self + .grid() + .drawing_cell(pos) + // pos.row is valid because we assume self.grid().pos() to + // always have a valid row value. pos.col is valid because + // we called col_wrap() earlier, which ensures that + // self.grid().pos().col has a valid value. this is true + // even though we just called col_inc, because this branch + // only happens if width > 1, and col_wrap takes width + // into account. + .unwrap_or_else(|| unreachable!()) + .is_wide() + { let next_next_pos = crate::grid::Pos { row: pos.row, col: pos.col + 1, @@ -919,17 +1006,37 @@ impl Screen { let next_next_cell = self .grid_mut() .drawing_cell_mut(next_next_pos) - .unwrap(); + // pos.row is valid because we assume + // self.grid().pos() to always have a valid row value. + // pos.col is valid because we called col_wrap() + // earlier, which ensures that self.grid().pos().col + // has a valid value. this is true even though we just + // called col_inc, because this branch only happens if + // width > 1, and col_wrap takes width into account. + // pos.col + 1 is valid because the cell at pos.col is + // wide, and so it must have the second half of the + // wide character after it. + .unwrap_or_else(|| unreachable!()); next_next_cell.clear(attrs); if next_next_pos.col == size.cols - 1 { self.grid_mut() .drawing_row_mut(pos.row) - .unwrap() + // we assume self.grid().pos().row is always valid + .unwrap_or_else(|| unreachable!()) .wrap(false); } } - let next_cell = - self.grid_mut().drawing_cell_mut(pos).unwrap(); + let next_cell = self + .grid_mut() + .drawing_cell_mut(pos) + // pos.row is valid because we assume self.grid().pos() to + // always have a valid row value. pos.col is valid because + // we called col_wrap() earlier, which ensures that + // self.grid().pos().col has a valid value. this is true + // even though we just called col_inc, because this branch + // only happens if width > 1, and col_wrap takes width + // into account. + .unwrap_or_else(|| unreachable!()); next_cell.clear(crate::attrs::Attrs::default()); next_cell.set_wide_continuation(true); self.grid_mut().col_inc(1); @@ -1168,7 +1275,12 @@ impl Screen { ns => { if log::log_enabled!(log::Level::Debug) { let n = if ns.len() == 1 { - format!("{}", ns[0]) + format!( + "{}", + // we just checked that ns.len() == 1, so 0 + // must be valid + ns.get(0).unwrap_or_else(|| unreachable!()) + ) } else { format!("{:?}", ns) }; @@ -1222,7 +1334,12 @@ impl Screen { ns => { if log::log_enabled!(log::Level::Debug) { let n = if ns.len() == 1 { - format!("{}", ns[0]) + format!( + "{}", + // we just checked that ns.len() == 1, so 0 + // must be valid + ns.get(0).unwrap_or_else(|| unreachable!()) + ) } else { format!("{:?}", ns) }; @@ -1314,7 +1431,13 @@ impl Screen { ns => { if log::log_enabled!(log::Level::Debug) { let n = if ns.len() == 1 { - format!("{}", ns[0]) + format!( + "{}", + // we just checked that ns.len() == 1, so + // 0 must be valid + ns.get(0) + .unwrap_or_else(|| unreachable!()) + ) } else { format!("{:?}", ns) }; @@ -1355,7 +1478,13 @@ impl Screen { ns => { if log::log_enabled!(log::Level::Debug) { let n = if ns.len() == 1 { - format!("{}", ns[0]) + format!( + "{}", + // we just checked that ns.len() == 1, so + // 0 must be valid + ns.get(0) + .unwrap_or_else(|| unreachable!()) + ) } else { format!("{:?}", ns) }; @@ -1378,7 +1507,12 @@ impl Screen { ns => { if log::log_enabled!(log::Level::Debug) { let n = if ns.len() == 1 { - format!("{}", ns[0]) + format!( + "{}", + // we just checked that ns.len() == 1, so 0 + // must be valid + ns.get(0).unwrap_or_else(|| unreachable!()) + ) } else { format!("{:?}", ns) }; @@ -1614,7 +1748,8 @@ fn u16_to_u8(i: u16) -> Option { if i > u16::from(u8::max_value()) { None } else { - Some(i.try_into().unwrap()) + // safe because we just ensured that the value fits in a u8 + Some(i.try_into().unwrap_or_else(|_| unreachable!())) } } diff --git a/src/term.rs b/src/term.rs index 4719d41..b6a0576 100644 --- a/src/term.rs +++ b/src/term.rs @@ -86,9 +86,13 @@ impl BufWrite for MoveTo { buf.extend_from_slice(b"\x1b[H"); } else { buf.extend_from_slice(b"\x1b["); - itoa::write(&mut *buf, self.row + 1).unwrap(); + itoa::write(&mut *buf, self.row + 1) + // write to a vec can never fail + .unwrap_or_else(|_| unreachable!()); buf.push(b';'); - itoa::write(&mut *buf, self.col + 1).unwrap(); + itoa::write(&mut *buf, self.col + 1) + // write to a vec can never fail + .unwrap_or_else(|_| unreachable!()); buf.push(b'H'); } } @@ -288,7 +292,8 @@ impl BufWrite for MoveRight { 1 => buf.extend_from_slice(b"\x1b[C"), n => { buf.extend_from_slice(b"\x1b["); - itoa::write(&mut *buf, n).unwrap(); + // write to a vec can never fail + itoa::write(&mut *buf, n).unwrap_or_else(|_| unreachable!()); buf.push(b'C'); } } @@ -320,7 +325,8 @@ impl BufWrite for EraseChar { 1 => buf.extend_from_slice(b"\x1b[X"), n => { buf.extend_from_slice(b"\x1b["); - itoa::write(&mut *buf, n).unwrap(); + // write to a vec can never fail + itoa::write(&mut *buf, n).unwrap_or_else(|_| unreachable!()); buf.push(b'X'); } } -- cgit v1.2.3-54-g00ecf