From 8826ad5ddd9dd86f48729334cf569b33a7c6e7c6 Mon Sep 17 00:00:00 2001 From: Antonio Scandurra Date: Mon, 6 Jun 2022 15:15:50 +0200 Subject: [PATCH 1/3] Make `Buffer::edit` and `MultiBuffer::edit` resilient to inverted ranges Previously, we would accept edits containing out-of-order ranges. When generating such ranges in our randomized tests, many invariants started breaking causing e.g. undo/redo to misbehave and operation application to panic. In theory, we should never pass inverted ranges, but this commit changes the above functions to swap the start and the end when that occurs to avoid breaking the entire system and panicking. --- crates/editor/src/display_map.rs | 2 +- crates/editor/src/multi_buffer.rs | 67 +++++++++++++++++++++---------- crates/language/src/buffer.rs | 11 ++++- 3 files changed, 55 insertions(+), 25 deletions(-) diff --git a/crates/editor/src/display_map.rs b/crates/editor/src/display_map.rs index 4378db540700dfd142551347832e10668b6f11e7..3e740f7b75deb7c9c3c0b8be1923cb0739306e6a 100644 --- a/crates/editor/src/display_map.rs +++ b/crates/editor/src/display_map.rs @@ -693,7 +693,7 @@ pub mod tests { } } _ => { - buffer.update(cx, |buffer, cx| buffer.randomly_edit(&mut rng, 5, cx)); + buffer.update(cx, |buffer, cx| buffer.randomly_mutate(&mut rng, 5, cx)); } } diff --git a/crates/editor/src/multi_buffer.rs b/crates/editor/src/multi_buffer.rs index 6dd1b0685b675a96780e8017becfb5bcac240c3c..32e2021fd2b2fc52d9b87a8e021fe28f95dab1a8 100644 --- a/crates/editor/src/multi_buffer.rs +++ b/crates/editor/src/multi_buffer.rs @@ -17,6 +17,7 @@ use std::{ cell::{Ref, RefCell}, cmp, fmt, io, iter::{self, FromIterator}, + mem, ops::{Range, RangeBounds, Sub}, str, sync::Arc, @@ -298,7 +299,7 @@ impl MultiBuffer { pub fn edit_internal( &mut self, - edits_iter: I, + edits: I, autoindent: bool, cx: &mut ModelContext, ) where @@ -310,14 +311,16 @@ impl MultiBuffer { return; } + let snapshot = self.read(cx); + let edits = edits.into_iter().map(|(range, new_text)| { + let mut range = range.start.to_offset(&snapshot)..range.end.to_offset(&snapshot); + if range.start > range.end { + mem::swap(&mut range.start, &mut range.end); + } + (range, new_text) + }); + if let Some(buffer) = self.as_singleton() { - let snapshot = self.read(cx); - let edits = edits_iter.into_iter().map(|(range, new_text)| { - ( - range.start.to_offset(&snapshot)..range.end.to_offset(&snapshot), - new_text, - ) - }); return buffer.update(cx, |buffer, cx| { let language_name = buffer.language().map(|language| language.name()); let indent_size = cx.global::().tab_size(language_name.as_deref()); @@ -329,29 +332,26 @@ impl MultiBuffer { }); } - let snapshot = self.read(cx); let mut buffer_edits: HashMap, Arc, bool)>> = Default::default(); let mut cursor = snapshot.excerpts.cursor::(); - for (range, new_text) in edits_iter { + for (range, new_text) in edits { let new_text: Arc = new_text.into(); - let start = range.start.to_offset(&snapshot); - let end = range.end.to_offset(&snapshot); - cursor.seek(&start, Bias::Right, &()); - if cursor.item().is_none() && start == *cursor.start() { + cursor.seek(&range.start, Bias::Right, &()); + if cursor.item().is_none() && range.start == *cursor.start() { cursor.prev(&()); } let start_excerpt = cursor.item().expect("start offset out of bounds"); - let start_overshoot = start - cursor.start(); + let start_overshoot = range.start - cursor.start(); let buffer_start = start_excerpt.range.start.to_offset(&start_excerpt.buffer) + start_overshoot; - cursor.seek(&end, Bias::Right, &()); - if cursor.item().is_none() && end == *cursor.start() { + cursor.seek(&range.end, Bias::Right, &()); + if cursor.item().is_none() && range.end == *cursor.start() { cursor.prev(&()); } let end_excerpt = cursor.item().expect("end offset out of bounds"); - let end_overshoot = end - cursor.start(); + let end_overshoot = range.end - cursor.start(); let buffer_end = end_excerpt.range.start.to_offset(&end_excerpt.buffer) + end_overshoot; if start_excerpt.id == end_excerpt.id { @@ -373,7 +373,7 @@ impl MultiBuffer { .or_insert(Vec::new()) .push((end_excerpt_range, new_text.clone(), false)); - cursor.seek(&start, Bias::Right, &()); + cursor.seek(&range.start, Bias::Right, &()); cursor.next(&()); while let Some(excerpt) = cursor.item() { if excerpt.id == end_excerpt.id { @@ -1310,7 +1310,11 @@ impl MultiBuffer { let end = snapshot.clip_offset(rng.gen_range(new_start..=snapshot.len()), Bias::Right); let start = snapshot.clip_offset(rng.gen_range(new_start..=end), Bias::Right); last_end = Some(end); - let range = start..end; + + let mut range = start..end; + if rng.gen_bool(0.2) { + mem::swap(&mut range.start, &mut range.end); + } let new_text_len = rng.gen_range(0..10); let new_text: String = RandomCharIter::new(&mut *rng).take(new_text_len).collect(); @@ -1414,8 +1418,27 @@ impl MultiBuffer { mutation_count: usize, cx: &mut ModelContext, ) { + use rand::prelude::*; + if rng.gen_bool(0.7) || self.singleton { - self.randomly_edit(rng, mutation_count, cx); + let buffer = self + .buffers + .borrow() + .values() + .choose(rng) + .map(|state| state.buffer.clone()); + + if rng.gen() && buffer.is_some() { + buffer.unwrap().update(cx, |buffer, cx| { + if rng.gen() { + buffer.randomly_edit(rng, mutation_count, cx); + } else { + buffer.randomly_undo_redo(rng, cx); + } + }); + } else { + self.randomly_edit(rng, mutation_count, cx); + } } else { self.randomly_edit_excerpts(rng, mutation_count, cx); } @@ -3330,7 +3353,7 @@ mod tests { }); buffer_2.update(cx, |buffer, cx| { buffer.edit([(0..0, "Y")], cx); - buffer.edit([(6..0, "Z")], cx); + buffer.edit([(6..6, "Z")], cx); }); let new_snapshot = multibuffer.read(cx).snapshot(cx); diff --git a/crates/language/src/buffer.rs b/crates/language/src/buffer.rs index ccb3d382ea89c865cee61d942f475f496c68ed1a..38dbaa29fde63e8a9b42e0cdc6b0a39e9ce06d2d 100644 --- a/crates/language/src/buffer.rs +++ b/crates/language/src/buffer.rs @@ -23,6 +23,7 @@ use std::{ ffi::OsString, future::Future, iter::{Iterator, Peekable}, + mem, ops::{Deref, DerefMut, Range}, path::{Path, PathBuf}, str, @@ -1108,7 +1109,10 @@ impl Buffer { // Skip invalid edits and coalesce contiguous ones. let mut edits: Vec<(Range, Arc)> = Vec::new(); for (range, new_text) in edits_iter { - let range = range.start.to_offset(self)..range.end.to_offset(self); + let mut range = range.start.to_offset(self)..range.end.to_offset(self); + if range.start > range.end { + mem::swap(&mut range.start, &mut range.end); + } let new_text = new_text.into(); if !new_text.is_empty() || !range.is_empty() { if let Some((prev_range, prev_text)) = edits.last_mut() { @@ -1452,7 +1456,10 @@ impl Buffer { } let new_start = last_end.map_or(0, |last_end| last_end + 1); - let range = self.random_byte_range(new_start, rng); + let mut range = self.random_byte_range(new_start, rng); + if rng.gen_bool(0.2) { + mem::swap(&mut range.start, &mut range.end); + } last_end = Some(range.end); let new_text_len = rng.gen_range(0..10); From 70afc06666329da3543d169df339135674d26b2b Mon Sep 17 00:00:00 2001 From: Antonio Scandurra Date: Mon, 6 Jun 2022 16:15:11 +0200 Subject: [PATCH 2/3] Handle out-of-order edits coming from LSP --- crates/language/src/language.rs | 8 ++- crates/project/src/project.rs | 121 +++++++++++++++++++++++++++++++- 2 files changed, 126 insertions(+), 3 deletions(-) diff --git a/crates/language/src/language.rs b/crates/language/src/language.rs index 00f86a2488cd1b1a3b85623056e9a6f45777eb0e..f13088f99750c52f82ace0de7a58210bd1f81f23 100644 --- a/crates/language/src/language.rs +++ b/crates/language/src/language.rs @@ -22,6 +22,7 @@ use serde_json::Value; use std::{ any::Any, cell::RefCell, + mem, ops::Range, path::{Path, PathBuf}, str, @@ -692,5 +693,10 @@ pub fn range_to_lsp(range: Range) -> lsp::Range { } pub fn range_from_lsp(range: lsp::Range) -> Range { - point_from_lsp(range.start)..point_from_lsp(range.end) + let mut start = point_from_lsp(range.start); + let mut end = point_from_lsp(range.end); + if start > end { + mem::swap(&mut start, &mut end); + } + start..end } diff --git a/crates/project/src/project.rs b/crates/project/src/project.rs index 3e09436b4bc80ad3df67f9c54123869f2c4e4fed..dc21e5c253e54b8f84c8e924c4230aedc02c7983 100644 --- a/crates/project/src/project.rs +++ b/crates/project/src/project.rs @@ -36,7 +36,7 @@ use sha2::{Digest, Sha256}; use similar::{ChangeTag, TextDiff}; use std::{ cell::RefCell, - cmp::{self, Ordering}, + cmp::{self, Ordering, Reverse}, convert::TryInto, ffi::OsString, hash::Hash, @@ -5164,8 +5164,10 @@ impl Project { let mut lsp_edits = lsp_edits .into_iter() .map(|edit| (range_from_lsp(edit.range), edit.new_text)) - .peekable(); + .collect::>(); + lsp_edits.sort_by_key(|(range, _)| range.start); + let mut lsp_edits = lsp_edits.into_iter().peekable(); let mut edits = Vec::new(); while let Some((mut range, mut new_text)) = lsp_edits.next() { // Combine any LSP edits that are adjacent. @@ -6959,6 +6961,121 @@ mod tests { }); } + #[gpui::test] + async fn test_invalid_edits_from_lsp(cx: &mut gpui::TestAppContext) { + cx.foreground().forbid_parking(); + + let text = " + use a::b; + use a::c; + + fn f() { + b(); + c(); + } + " + .unindent(); + + let fs = FakeFs::new(cx.background()); + fs.insert_tree( + "/dir", + json!({ + "a.rs": text.clone(), + }), + ) + .await; + + let project = Project::test(fs, ["/dir".as_ref()], cx).await; + let buffer = project + .update(cx, |project, cx| project.open_local_buffer("/dir/a.rs", cx)) + .await + .unwrap(); + + // Simulate the language server sending us edits in a non-ordered fashion, + // with ranges sometimes being inverted. + let edits = project + .update(cx, |project, cx| { + project.edits_from_lsp( + &buffer, + [ + lsp::TextEdit { + range: lsp::Range::new( + lsp::Position::new(0, 9), + lsp::Position::new(0, 9), + ), + new_text: "\n\n".into(), + }, + lsp::TextEdit { + range: lsp::Range::new( + lsp::Position::new(0, 8), + lsp::Position::new(0, 4), + ), + new_text: "a::{b, c}".into(), + }, + lsp::TextEdit { + range: lsp::Range::new( + lsp::Position::new(1, 0), + lsp::Position::new(7, 0), + ), + new_text: "".into(), + }, + lsp::TextEdit { + range: lsp::Range::new( + lsp::Position::new(0, 9), + lsp::Position::new(0, 9), + ), + new_text: " + fn f() { + b(); + c(); + }" + .unindent(), + }, + ], + None, + cx, + ) + }) + .await + .unwrap(); + + buffer.update(cx, |buffer, cx| { + let edits = edits + .into_iter() + .map(|(range, text)| { + ( + range.start.to_point(&buffer)..range.end.to_point(&buffer), + text, + ) + }) + .collect::>(); + + assert_eq!( + edits, + [ + (Point::new(0, 4)..Point::new(0, 8), "a::{b, c}".into()), + (Point::new(1, 0)..Point::new(2, 0), "".into()) + ] + ); + + for (range, new_text) in edits { + buffer.edit([(range, new_text)], cx); + } + assert_eq!( + buffer.text(), + " + use a::{b, c}; + + fn f() { + b(); + c(); + } + " + .unindent() + ); + }); + } + fn chunks_with_diagnostics( buffer: &Buffer, range: Range, From 1ecc51f035e56f3412eba0174ed78ab4cef3e90f Mon Sep 17 00:00:00 2001 From: Antonio Scandurra Date: Mon, 6 Jun 2022 16:23:49 +0200 Subject: [PATCH 3/3] Fix warnings --- crates/project/src/project.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/project/src/project.rs b/crates/project/src/project.rs index dc21e5c253e54b8f84c8e924c4230aedc02c7983..7575b67f252c640a33811491c214d4a4d4db177f 100644 --- a/crates/project/src/project.rs +++ b/crates/project/src/project.rs @@ -36,7 +36,7 @@ use sha2::{Digest, Sha256}; use similar::{ChangeTag, TextDiff}; use std::{ cell::RefCell, - cmp::{self, Ordering, Reverse}, + cmp::{self, Ordering}, convert::TryInto, ffi::OsString, hash::Hash,