Modify use of rope crate to avoid methods that can panic

src/core/buffer/mod.rs

@@ -1,14 +1,14 @@
 pub(crate) mod gutter;
 pub(crate) mod overlay;
 mod settings;
-mod text;
+pub(crate) mod text;
 
 use settings::BufferSettings;
 
 use std::io::Write;
 use std::path::{Path, PathBuf};
 
-use tracing::info;
+use tracing::{info, warn};
 
 use self::overlay::{Overlay, OverlayLifetime};
 use self::text::TextLines;
@@ -81,7 +81,6 @@ pub(crate) struct Buffer {
     overlays: Vec<Overlay>,
 }
 
-//TODO consider try_ versions of rope methods to avoid panic
 impl Buffer {
     pub(crate) fn new() -> Self {
         Self::with_properties(BufferProperties::document())
@@ -165,11 +164,10 @@ impl Buffer {
 
     /// Extract the first line of text as a String (useful for single-line buffers like command).
     pub(super) fn first_line_text(&self) -> String {
-        if self.text.inner.line_len() == 0 {
+        let Ok(line) = self.text.try_line(0) else {
             return String::new();
-        }
-        let line = self.text.inner.line(0).to_string();
-        line.trim_end_matches('\n').to_string()
+        };
+        line.to_string().trim_end_matches('\n').to_string()
     }
 
     pub(crate) fn title(&self) -> Option<&str> {
@@ -205,7 +203,7 @@ impl Buffer {
         //TODO cf. https://docs.rs/crop/latest/crop/index.html example, do this write
         //asynchronously
         let mut file = std::fs::File::create(filename)?;
-        for chunk in self.text.inner.chunks() {
+        for chunk in self.text.chunks() {
             file.write_all(chunk.as_bytes())?;
         }
 
@@ -214,22 +212,13 @@ impl Buffer {
 
     //#[tracing::instrument()]
     pub(crate) fn visible_lines(&self, start_line: usize, end_line: usize) -> Vec<String> {
-        if self.text.inner.line_len() == 0 {
-            return vec![];
-        }
-        let end_line = if end_line > self.text.inner.line_len() {
-            self.text.inner.line_len()
-        } else {
-            end_line
-        };
+        let end_line = end_line.min(self.text.line_len());
+        let start_line = start_line.min(end_line);
 
-        let mut lines: Vec<String> = self
-            .text
-            .inner
-            .line_slice(start_line..end_line)
-            .lines()
-            .map(|line| line.to_string())
-            .collect();
+        let Ok(slice) = self.text.try_line_slice(start_line..end_line) else {
+            return vec![];
+        };
+        let mut lines: Vec<String> = slice.lines().map(|line| line.to_string()).collect();
         if self.final_newline() {
             lines.push(String::new());
         }
@@ -238,12 +227,7 @@ impl Buffer {
 
     fn final_newline(&self) -> bool {
         //TODO needs to handle multiple types of newline
-        self.text
-            .inner
-            .chars()
-            .next_back()
-            .map(|ch| ch == '\n')
-            .unwrap_or(false)
+        self.text.last_char().map(|ch| ch == '\n').unwrap_or(false)
     }
 
     #[tracing::instrument(ret)]
@@ -251,38 +235,41 @@ impl Buffer {
         let y = self.cursor_pos.y() as usize;
         let x = self.cursor_pos.x() as usize;
 
-        if self.text.inner.byte_len() == 0 {
+        let byte_len = self.text.byte_len();
+        if byte_len == 0 {
             return 0;
         }
-        let line_idx = self.text.inner.byte_of_line(y);
-        let line_len = self.text.inner.line_len();
+        let line_len = self.text.line_len();
+        let Ok(line_idx) = self.text.try_byte_of_line(y.min(line_len)) else {
+            return byte_len;
+        };
         info!(%line_idx, %line_len);
 
-        // We're at the current end of the Rope
-        if y == line_len {
-            line_idx
-        } else {
-            let line = self.text.inner.line(y);
-            //TODO this should happen at the grapheme level, controllably
-            let char_idx = line
-                .chars()
-                .take(x)
-                .fold(0, |acc, elem| acc + elem.len_utf8());
-            line_idx + char_idx
+        match self.text.try_line(y) {
+            Ok(line) => {
+                //TODO this should happen at the grapheme level, controllably
+                let char_idx = line
+                    .chars()
+                    .take(x)
+                    .fold(0, |acc, elem| acc + elem.len_utf8());
+                line_idx + char_idx
+            }
+            // We're at the current end of the text
+            Err(_) => line_idx,
         }
     }
 
     /// Convert an (x, y) position to a byte offset in the rope.
     fn position_to_byte_offset(&self, x: usize, y: usize) -> usize {
-        if self.text.inner.byte_len() == 0 {
+        let byte_len = self.text.byte_len();
+        if byte_len == 0 {
             return 0;
         }
-        let line_count = self.text.inner.line_len();
-        if y >= line_count {
-            return self.text.inner.byte_len();
-        }
-        let line_start = self.text.inner.byte_of_line(y);
-        let line = self.text.inner.line(y);
+        let (Ok(line_start), Ok(line)) = (self.text.try_byte_of_line(y), self.text.try_line(y))
+        else {
+            // Past the last line of the text
+            return byte_len;
+        };
         let char_offset = line.chars().take(x).fold(0, |acc, ch| acc + ch.len_utf8());
         line_start + char_offset
     }
@@ -291,7 +278,10 @@ impl Buffer {
     /// Handles single chars, newlines, and multi-char strings.
     pub(super) fn insert_text(&mut self, text: &str) {
         let char_idx = self.get_current_char_idx();
-        self.text.inner.insert(char_idx, text);
+        if let Err(err) = self.text.try_insert(char_idx, text) {
+            warn!(%err, %char_idx, "Refusing to insert text at an invalid offset");
+            return;
+        }
         for ch in text.chars() {
             self.cursor_pos.respond_to_new_char(ch);
         }
@@ -305,7 +295,10 @@ impl Buffer {
         if from_byte >= to_byte {
             return;
         }
-        self.text.inner.delete(from_byte..to_byte);
+        if let Err(err) = self.text.try_delete(from_byte..to_byte) {
+            warn!(%err, %from_byte, %to_byte, "Refusing to delete an invalid range");
+            return;
+        }
 
         // Place cursor at `from`, clamped to valid bounds
         let line_count = self.line_count();
@@ -319,29 +312,31 @@ impl Buffer {
     // is zero-sized
     #[tracing::instrument(ret)]
     fn get_previous_char_idx(&self) -> Option<(usize, usize)> {
-        if self.text.inner.byte_len() == 0 {
+        if self.text.byte_len() == 0 {
             return None;
         }
         let y = self.cursor_pos.y() as usize;
         let x = self.cursor_pos.x() as usize;
         //TODO this should happen at the grapheme level, controllably
-        let line_idx = self.text.inner.byte_of_line(y);
+        let line_idx = self.text.try_byte_of_line(y).ok()?;
         if x == 0 {
-            let last_line_idx = self.text.inner.byte_of_line(y - 1);
-            let last_line_len = self.text.inner.line(y - 1).byte_len();
+            // Nothing precedes the very start of the text
+            let previous_line = y.checked_sub(1)?;
+            let last_line_idx = self.text.try_byte_of_line(previous_line).ok()?;
+            let last_line_len = self.text.try_line(previous_line).ok()?.byte_len();
 
             let char_idx = last_line_idx + last_line_len;
-            let size = line_idx - char_idx;
+            let size = line_idx.checked_sub(char_idx)?;
             Some((char_idx, size))
         } else {
-            let line = self.text.inner.line(y);
+            let line = self.text.try_line(y).ok()?;
             let char_idx = line
                 .chars()
                 .take(x - 1)
                 .fold(0, |acc, elem| acc + elem.len_utf8());
             info!(%line_idx, %char_idx);
             let offset = line_idx + char_idx;
-            let len_utf8 = line.chars().nth(x - 1).unwrap().len_utf8();
+            let len_utf8 = line.chars().nth(x - 1)?.len_utf8();
             Some((offset, len_utf8))
         }
     }
@@ -349,8 +344,11 @@ impl Buffer {
     // Needs to handle conversion between utf-8 represenatation within rope, and char/grapheme representation within Cursor
     pub(super) fn delete_char(&mut self) {
         if let Some((char_idx, len_utf8)) = self.get_previous_char_idx() {
-            self.text.inner.delete(char_idx..(char_idx + len_utf8));
-            self.cursor_pos.respond_to_delete(&self.text.inner);
+            if let Err(err) = self.text.try_delete(char_idx..(char_idx + len_utf8)) {
+                warn!(%err, %char_idx, "Refusing to delete an invalid range");
+                return;
+            }
+            self.cursor_pos.respond_to_delete(&self.text);
         }
     }
 
@@ -366,7 +364,7 @@ impl BufferQuery for Buffer {
     }
 
     fn line_count(&self) -> usize {
-        let len = self.text.inner.line_len();
+        let len = self.text.line_len();
         // Ensure we always have at least 1 line for empty buffers
         if len == 0 {
             1
@@ -376,16 +374,53 @@ impl BufferQuery for Buffer {
     }
 
     fn line_len(&self, y: usize) -> usize {
-        if y >= self.text.inner.line_len() {
-            return 0;
-        }
-        self.text.inner.line(y).chars().count()
+        self.text
+            .try_line(y)
+            .map(|line| line.chars().count())
+            .unwrap_or(0)
     }
 
     fn char_at(&self, x: usize, y: usize) -> Option<char> {
-        if y >= self.text.inner.line_len() {
-            return None;
-        }
-        self.text.inner.line(y).chars().nth(x)
+        self.text.try_line(y).ok()?.chars().nth(x)
+    }
+}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[test]
+    fn backspace_at_the_start_of_the_buffer_is_a_no_op() {
+        let mut buffer = Buffer::from_text("abc");
+        buffer.set_cursor(0, 0);
+        buffer.delete_char();
+        assert_eq!(buffer.first_line_text(), "abc");
+        assert_eq!(BufferQuery::cursor(&buffer), (0, 0));
+    }
+
+    #[test]
+    fn backspace_over_the_only_newline_empties_the_buffer() {
+        let mut buffer = Buffer::from_text("\n");
+        buffer.set_cursor(0, 1);
+        buffer.delete_char();
+        assert_eq!(buffer.text.byte_len(), 0);
+        assert_eq!(BufferQuery::cursor(&buffer), (0, 0));
+    }
+
+    #[test]
+    fn queries_past_the_end_of_the_text_are_empty() {
+        let buffer = Buffer::from_text("foo\nbar");
+        assert_eq!(buffer.line_len(9), 0);
+        assert_eq!(buffer.char_at(0, 9), None);
+        assert_eq!(buffer.visible_lines(9, 12), Vec::<String>::new());
+        assert_eq!(buffer.visible_lines(2, 0), Vec::<String>::new());
+    }
+
+    #[test]
+    fn inserting_with_a_cursor_past_the_end_appends() {
+        let mut buffer = Buffer::from_text("foo");
+        buffer.set_cursor(0, 9);
+        buffer.insert_text("!");
+        assert_eq!(buffer.first_line_text(), "foo!");
     }
 }

src/core/buffer/text.rs

@@ -1,11 +1,49 @@
-use crop::Rope;
+use std::fmt::Display;
+use std::ops::Range;
+
+use crop::{Rope, RopeSlice};
 
 /// Represents the lines of text in a buffer.
 ///
 /// Currently this wraps a crop::Rope, but this will probably change at some point
 #[derive(Debug)]
 pub(crate) struct TextLines {
-    pub(super) inner: Rope,
+    inner: Rope,
+}
+
+/// Ways an offset handed to [`TextLines`] can fail to name a position in the text.
+#[derive(Debug, Clone, Copy, PartialEq, Eq)]
+pub(crate) enum TextError {
+    /// A line offset at or past the end of the text
+    LineOutOfBounds { line: usize, line_len: usize },
+    /// A byte offset past the end of the text
+    ByteOutOfBounds { offset: usize, byte_len: usize },
+    /// A byte offset falling inside a code point
+    NotCharBoundary { offset: usize },
+    /// A range whose start comes after its end
+    InvertedRange { start: usize, end: usize },
+}
+
+impl Display for TextError {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        match self {
+            TextError::LineOutOfBounds { line, line_len } => {
+                write!(f, "line {line} out of bounds, text has {line_len} lines")
+            }
+            TextError::ByteOutOfBounds { offset, byte_len } => {
+                write!(
+                    f,
+                    "byte offset {offset} out of bounds, text has {byte_len} bytes"
+                )
+            }
+            TextError::NotCharBoundary { offset } => {
+                write!(f, "byte offset {offset} is not a code point boundary")
+            }
+            TextError::InvertedRange { start, end } => {
+                write!(f, "range start {start} is greater than end {end}")
+            }
+        }
+    }
 }
 
 impl TextLines {
@@ -19,4 +57,145 @@ impl TextLines {
         inner.insert(0, text);
         Self { inner }
     }
+
+    /// Total size of the text in bytes
+    pub(crate) fn byte_len(&self) -> usize {
+        self.inner.byte_len()
+    }
+
+    /// Number of lines in the text. Note that this is 0 for empty text.
+    pub(crate) fn line_len(&self) -> usize {
+        self.inner.line_len()
+    }
+
+    /// The contiguous string slices making up the text, in order
+    pub(crate) fn chunks(&self) -> impl Iterator<Item = &'_ str> {
+        self.inner.chunks()
+    }
+
+    /// The last `char` in the text, if any
+    pub(crate) fn last_char(&self) -> Option<char> {
+        self.inner.chars().next_back()
+    }
+
+    /// Non-panicking [`Rope::line`]: the contents of `line`, without its line break.
+    pub(crate) fn try_line(&self, line: usize) -> Result<RopeSlice<'_>, TextError> {
+        let line_len = self.inner.line_len();
+        if line >= line_len {
+            return Err(TextError::LineOutOfBounds { line, line_len });
+        }
+        Ok(self.inner.line(line))
+    }
+
+    /// Non-panicking [`Rope::byte_of_line`]: the byte offset the given line starts at.
+    /// A line offset one past the last line is valid, and yields the end of the text.
+    pub(crate) fn try_byte_of_line(&self, line: usize) -> Result<usize, TextError> {
+        let line_len = self.inner.line_len();
+        if line > line_len {
+            return Err(TextError::LineOutOfBounds { line, line_len });
+        }
+        Ok(self.inner.byte_of_line(line))
+    }
+
+    /// Non-panicking [`Rope::line_slice`]: the given range of lines, line breaks included.
+    pub(crate) fn try_line_slice(&self, lines: Range<usize>) -> Result<RopeSlice<'_>, TextError> {
+        let line_len = self.inner.line_len();
+        if lines.start > lines.end {
+            return Err(TextError::InvertedRange {
+                start: lines.start,
+                end: lines.end,
+            });
+        }
+        if lines.end > line_len {
+            return Err(TextError::LineOutOfBounds {
+                line: lines.end,
+                line_len,
+            });
+        }
+        Ok(self.inner.line_slice(lines))
+    }
+
+    /// Non-panicking [`Rope::insert`]
+    pub(crate) fn try_insert(&mut self, byte_offset: usize, text: &str) -> Result<(), TextError> {
+        self.check_byte_offset(byte_offset)?;
+        self.inner.insert(byte_offset, text);
+        Ok(())
+    }
+
+    /// Non-panicking [`Rope::delete`]
+    pub(crate) fn try_delete(&mut self, bytes: Range<usize>) -> Result<(), TextError> {
+        if bytes.start > bytes.end {
+            return Err(TextError::InvertedRange {
+                start: bytes.start,
+                end: bytes.end,
+            });
+        }
+        self.check_byte_offset(bytes.start)?;
+        self.check_byte_offset(bytes.end)?;
+        self.inner.delete(bytes);
+        Ok(())
+    }
+
+    /// Verify that a byte offset is one the rope will accept
+    fn check_byte_offset(&self, offset: usize) -> Result<(), TextError> {
+        let byte_len = self.inner.byte_len();
+        if offset > byte_len {
+            return Err(TextError::ByteOutOfBounds { offset, byte_len });
+        }
+        if !self.inner.is_char_boundary(offset) {
+            return Err(TextError::NotCharBoundary { offset });
+        }
+        Ok(())
+    }
+}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[test]
+    fn line_access_is_bounds_checked() {
+        let text = TextLines::from_text("foo\nbar");
+        assert_eq!(text.try_line(1).unwrap().to_string(), "bar");
+        assert_eq!(
+            text.try_line(2),
+            Err(TextError::LineOutOfBounds {
+                line: 2,
+                line_len: 2
+            })
+        );
+        assert!(TextLines::empty().try_line(0).is_err());
+    }
+
+    #[test]
+    fn byte_of_line_allows_one_past_the_end() {
+        let text = TextLines::from_text("foo\nbar");
+        assert_eq!(text.try_byte_of_line(2), Ok(7));
+        assert!(text.try_byte_of_line(3).is_err());
+    }
+
+    #[test]
+    fn line_slice_rejects_inverted_and_oob_ranges() {
+        let text = TextLines::from_text("foo\nbar\n");
+        assert_eq!(text.try_line_slice(0..1).unwrap().to_string(), "foo\n");
+        let inverted = Range { start: 1, end: 0 };
+        assert!(text.try_line_slice(inverted).is_err());
+        assert!(text.try_line_slice(0..9).is_err());
+    }
+
+    #[test]
+    fn edits_reject_offsets_inside_a_code_point() {
+        let mut text = TextLines::from_text("🌎");
+        assert_eq!(
+            text.try_insert(1, "x"),
+            Err(TextError::NotCharBoundary { offset: 1 })
+        );
+        assert_eq!(
+            text.try_delete(0..1),
+            Err(TextError::NotCharBoundary { offset: 1 })
+        );
+        assert!(text.try_insert(5, "x").is_err());
+        assert_eq!(text.try_delete(0..4), Ok(()));
+        assert_eq!(text.byte_len(), 0);
+    }
 }

src/core/cursor.rs

@@ -1,6 +1,6 @@
 use std::fmt;
 
-use crop::Rope;
+use super::buffer::text::TextLines;
 
 /// Cursor is a simple position holder. It stores (x, y) coordinates and provides methods
 /// for updating position in response to text edits. All movement logic lives in the
@@ -47,15 +47,17 @@ impl Cursor {
         }
     }
 
-    pub(super) fn respond_to_delete(&mut self, text: &Rope) {
+    pub(super) fn respond_to_delete(&mut self, text: &TextLines) {
         if self.x == 0 {
-            if self.y == 0 {
+            let Some(previous_line) = self.y.checked_sub(1) else {
                 return;
-            }
-            self.y -= 1;
-            let new_cur_line = text.line(self.y as usize);
+            };
+            self.y = previous_line;
             // TODO this and other occurences should really be something grapheme-based
-            let char_len = new_cur_line.chars().count();
+            let char_len = text
+                .try_line(previous_line as usize)
+                .map(|line| line.chars().count())
+                .unwrap_or(0);
             self.x = char_len as u16;
         } else {
             self.x -= 1;