diff --git a/examples/website/src/index.ts b/examples/website/src/index.ts index 2057e0e..722728b 100644 --- a/examples/website/src/index.ts +++ b/examples/website/src/index.ts @@ -102,10 +102,10 @@ function updateMergedText(): void { const selection = results.cursors?.length === 2 ? { - start: results.cursors[0].position, - end: results.cursors[1].position, - side: leftCursors ? 'left' : 'right', - } + start: results.cursors[0].position, + end: results.cursors[1].position, + side: leftCursors ? 'left' : 'right', + } : undefined; const fragment = document.createDocumentFragment(); @@ -127,10 +127,7 @@ function updateMergedText(): void { } fragment.appendChild( - createSelectionOverlay( - selection.side === 'left', - selection.start !== selection.end - ) + createSelectionOverlay(selection.side === 'left', selection.start !== selection.end) ); } diff --git a/reconcile-js/.gitignore b/reconcile-js/.gitignore index 6b1d0bf..95db102 100644 --- a/reconcile-js/.gitignore +++ b/reconcile-js/.gitignore @@ -1 +1,2 @@ +README.md LICENSE diff --git a/src/operation_transformation/diff_error.rs b/src/operation_transformation/diff_error.rs index 22e717f..4d5ee90 100644 --- a/src/operation_transformation/diff_error.rs +++ b/src/operation_transformation/diff_error.rs @@ -17,6 +17,13 @@ pub enum DiffError { available: usize, }, + /// A diff character count was too large to represent as usize + #[error("Invalid diff: character count {value} cannot be represented as usize")] + LengthOverflow { + /// The absolute character count from the diff + value: u64, + }, + /// A character count was too large to represent as i64 #[error("Integer overflow: value {value} cannot be represented as i64")] IntegerOverflow { diff --git a/src/operation_transformation/edited_text.rs b/src/operation_transformation/edited_text.rs index 2484036..fa41968 100644 --- a/src/operation_transformation/edited_text.rs +++ b/src/operation_transformation/edited_text.rs @@ -505,11 +505,8 @@ where /// # Errors /// /// Returns `DiffError::LengthExceedsOriginal` if the diff references a - /// range that exceeds the original text length. - /// - /// # Panics - /// - /// Panics if a character count cannot be represented as usize. + /// range that exceeds the original text length, or `DiffError::LengthOverflow` + /// if a character count cannot be represented as usize. pub fn from_diff( original_text: &'a str, diff: Vec, @@ -522,8 +519,11 @@ where for item in diff { match item { NumberOrText::Number(length) => { - let count = - usize::try_from(length.unsigned_abs()).expect("length must fit in usize"); + let count = usize::try_from(length.unsigned_abs()).map_err(|_| { + DiffError::LengthOverflow { + value: length.unsigned_abs(), + } + })?; let available = text_length - order; if count > available { return Err(DiffError::LengthExceedsOriginal { diff --git a/src/operation_transformation/operation.rs b/src/operation_transformation/operation.rs index 9c950c9..afe6936 100644 --- a/src/operation_transformation/operation.rs +++ b/src/operation_transformation/operation.rs @@ -202,25 +202,25 @@ where /// Applies the operation to the given `StringBuilder`, returning the /// modified `StringBuilder`. /// - /// When compiled in debug mode, panics if a delete operation is attempted - /// on a range of text that does not match the text to be deleted. + /// When compiled in debug mode, panics if a retain or delete operation is + /// out of order, out of bounds, or does not match its expected text. pub fn apply<'a>(&self, mut builder: StringBuilder<'a>) -> StringBuilder<'a> { match self { Operation::Equal { #[cfg(debug_assertions)] text, + #[cfg(debug_assertions)] + order, length, .. } => { #[cfg(debug_assertions)] - debug_assert!( - text.as_ref() - .is_none_or(|text| builder.get_slice_from_remaining(self.len()) == *text), - "Text (`{}`) which is supposed to be equal does not match the text in the \ - range: `{}`", - text.as_ref().unwrap_or(&String::new()), - builder.get_slice_from_remaining(self.len()) - ); + { + builder.assert_range(*order, *length); + if let Some(text) = text { + builder.assert_text(*length, text); + } + } builder.retain(*length); } @@ -230,18 +230,18 @@ where Operation::Delete { #[cfg(debug_assertions)] deleted_text, + #[cfg(debug_assertions)] + order, deleted_character_count, .. } => { #[cfg(debug_assertions)] - debug_assert!( - deleted_text - .as_ref() - .is_none_or(|text| builder.get_slice_from_remaining(self.len()) == *text), - "Text to-be-deleted `{}` does not match the text in the range: `{}`", - deleted_text.as_ref().unwrap_or(&String::new()), - builder.get_slice_from_remaining(self.len()) - ); + { + builder.assert_range(*order, *deleted_character_count); + if let Some(text) = deleted_text { + builder.assert_text(*deleted_character_count, text); + } + } builder.delete(*deleted_character_count); } diff --git a/src/utils/string_builder.rs b/src/utils/string_builder.rs index eec565d..72a1625 100644 --- a/src/utils/string_builder.rs +++ b/src/utils/string_builder.rs @@ -8,7 +8,7 @@ pub struct StringBuilder<'a> { buffer: String, #[cfg(debug_assertions)] - remaining: String, + position: usize, } impl fmt::Debug for StringBuilder<'_> { @@ -17,7 +17,7 @@ impl fmt::Debug for StringBuilder<'_> { debug_struct.field("buffer", &self.buffer); #[cfg(debug_assertions)] - debug_struct.field("remaining", &self.remaining); + debug_struct.field("remaining", &self.original.as_str()); debug_struct.finish_non_exhaustive() } @@ -30,7 +30,7 @@ impl StringBuilder<'_> { buffer: String::with_capacity(original.len()), #[cfg(debug_assertions)] - remaining: original.to_owned(), + position: 0, } } @@ -50,7 +50,7 @@ impl StringBuilder<'_> { #[cfg(debug_assertions)] { - self.remaining = self.remaining.chars().skip(length).collect(); + self.position += length; } } @@ -60,7 +60,7 @@ impl StringBuilder<'_> { #[cfg(debug_assertions)] { - self.remaining = self.remaining.chars().skip(length).collect(); + self.position += length; } } @@ -70,16 +70,24 @@ impl StringBuilder<'_> { std::mem::take(&mut self.buffer) } - /// Get a slice of the remaining original string. The slice starts from - /// where the next delete/retain operation would start and is of length - /// `length`. + /// Check the original-text position and range without copying the text. #[cfg(debug_assertions)] - pub fn get_slice_from_remaining(&self, length: usize) -> String { - let result = self.remaining.chars().take(length).collect::(); + pub fn assert_range(&self, order: usize, length: usize) { + debug_assert_eq!(self.position, order, "Operation out of order"); + debug_assert!( + length == 0 || self.original.clone().nth(length - 1).is_some(), + "Range out of bounds" + ); + } - debug_assert_eq!(result.chars().count(), length, "Range out of bounds"); - - result + /// Check the next original characters without allocating a substring. + #[cfg(debug_assertions)] + pub fn assert_text(&self, length: usize, text: &str) { + debug_assert!( + self.original.clone().take(length).eq(text.chars()), + "Text `{text}` does not match the next {length} characters of `{}`", + self.original.as_str() + ); } } @@ -132,21 +140,6 @@ mod tests { assert_eq!(builder.take(), "こんに世界, ちは"); } - #[cfg(debug_assertions)] - #[test] - fn test_get_slice() { - let original = "abcdef"; - let builder = StringBuilder::new(original); - - // Test getting a slice of the original string - assert_eq!(builder.get_slice_from_remaining(3), "abc"); - - // Test getting a slice that includes both buffer and remaining original - let mut builder = StringBuilder::new(original); - builder.retain(2); // "ab" in buffer - assert_eq!(builder.get_slice_from_remaining(2), "cd"); - } - #[test] fn test_retain_all() { let original = "Hello, world!"; diff --git a/tests/example_document.rs b/tests/example_document.rs index b16cd44..cb71046 100644 --- a/tests/example_document.rs +++ b/tests/example_document.rs @@ -40,10 +40,9 @@ impl ExampleDocument { /// If the result string does not match the expected string, the program /// will panic. pub fn assert_eq(&self, result: &EditedText<'_, String>) { - let result_str = ExampleDocument::text_with_cursors_to_string(result); assert_eq!( - self.expected, result_str, - "Left (expected) isn't equal to right (actual). Actual: ```\n{result_str}```", + Self::string_to_text_with_cursors(&self.expected), + result.apply() ); } @@ -62,30 +61,6 @@ impl ExampleDocument { ); } - fn text_with_cursors_to_string(document: &EditedText<'_, String>) -> String { - let merged = document.apply(); - let mut result = merged.text(); - for (i, cursor) in merged.cursors().iter().enumerate() { - assert!( - cursor.char_index() <= result.len(), // equals in case of insert at the end - "Cursor index out of bounds: {} > {} when testing for '{}.'", - cursor.char_index(), - result.len(), - result - ); - - result.insert( - result - .char_indices() - .nth(cursor.char_index() + i) - .map_or_else(|| result.len(), |(byte_index, _)| byte_index), /* find the utf8 char index of the insert - * in byte index */ - '|', - ); - } - result - } - fn string_to_text_with_cursors(text: &str) -> TextWithCursors { let cursors = Self::parse_cursors(text); let text = text.replace('|', ""); diff --git a/tests/wasm.rs b/tests/wasm.rs index 82af503..0ed65c0 100644 --- a/tests/wasm.rs +++ b/tests/wasm.rs @@ -1,6 +1,6 @@ #![cfg(feature = "wasm")] -use reconcile_text::{BuiltinTokenizer, CursorPosition, TextWithCursors, wasm::*}; +use reconcile_text::{BuiltinTokenizer, CursorPosition, EditedText, TextWithCursors, wasm::*}; use wasm_bindgen_test::*; #[wasm_bindgen_test(unsupported = test)] @@ -55,6 +55,16 @@ fn test_merge_binary() { ); } +#[wasm_bindgen_test(unsupported = test)] +fn test_from_diff_rejects_oversized_lengths() { + for length in [1_i64 << 32, -(1_i64 << 40), i64::MAX, i64::MIN] { + let error = EditedText::from_diff("abc", vec![length.into()], &*BuiltinTokenizer::Word) + .unwrap_err(); + + assert!(error.to_string().starts_with("Invalid diff:")); + } +} + #[wasm_bindgen_test] // JsValue isn't supported outside of wasm fn test_diff() { let parent = "hello ";