From 8c60003c239d95bcaff497351ebd9e82011b12dd Mon Sep 17 00:00:00 2001 From: l3ops Date: Tue, 3 Jan 2023 17:33:22 +0100 Subject: [PATCH 1/2] feat(rome_lsp): add a `replace_range` method to `Document` --- Cargo.lock | 124 +++++++++++- crates/rome_lsp/Cargo.toml | 1 + crates/rome_lsp/src/documents.rs | 15 +- crates/rome_lsp/src/handlers/text_document.rs | 16 +- crates/rome_lsp/src/line_index.rs | 185 +++++++++++++++++- 5 files changed, 321 insertions(+), 20 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index bbf20fcefca..a04e156a9e5 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -123,6 +123,21 @@ version = "0.13.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "904dfeac50f3cdaba28fc6f57fdcddb75f49ed61346676a78c4ffe55877802fd" +[[package]] +name = "bit-set" +version = "0.5.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "0700ddab506f33b20a03b13996eccd309a48e5ff77d0d95926aa0210fb4e95f1" +dependencies = [ + "bit-vec", +] + +[[package]] +name = "bit-vec" +version = "0.6.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "349f9b6a179ed607305526ca489b34ad0a41aed5f7980fa90eb03160b69598fb" + [[package]] name = "bitflags" version = "1.3.2" @@ -511,6 +526,15 @@ version = "0.1.2" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "d922f481ae01f2a3f1fff7b9e0e789f18f0c755a38ec983a3e6f37762cdcc2a2" +[[package]] +name = "fastrand" +version = "1.8.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "a7a407cfaa3385c4ae6b23e84623d48c2798d06e3e6a1878f7f59f17b3f86499" +dependencies = [ + "instant", +] + [[package]] name = "filetime" version = "0.2.17" @@ -1284,6 +1308,26 @@ dependencies = [ "unicode-ident", ] +[[package]] +name = "proptest" +version = "1.0.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "1e0d9cc07f18492d879586c92b485def06bc850da3118075cd45d50e9c95b0e5" +dependencies = [ + "bit-set", + "bitflags", + "byteorder", + "lazy_static", + "num-traits", + "quick-error 2.0.1", + "rand 0.8.5", + "rand_chacha 0.3.1", + "rand_xorshift", + "regex-syntax", + "rusty-fork", + "tempfile", +] + [[package]] name = "pulldown-cmark" version = "0.9.2" @@ -1305,6 +1349,18 @@ dependencies = [ "unreachable", ] +[[package]] +name = "quick-error" +version = "1.2.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "a1d01941d82fa2ab50be1e79e6714289dd7cde78eba4c074bc5a4374f650dfe0" + +[[package]] +name = "quick-error" +version = "2.0.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "a993555f31e5a609f617c12db6250dedcac1b0a85076912c436e6fc9b2c8e6a3" + [[package]] name = "quickcheck" version = "1.0.3" @@ -1344,7 +1400,7 @@ checksum = "6a6b1679d49b24bbfe0c803429aa1874472f50d9b363131f0e89fc356b544d03" dependencies = [ "getrandom 0.1.16", "libc", - "rand_chacha", + "rand_chacha 0.2.2", "rand_core 0.5.1", "rand_hc", ] @@ -1355,6 +1411,8 @@ version = "0.8.5" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "34af8d1a0e25924bc5b7c43c079c942339d8f0a8b57c39049bef581b46327404" dependencies = [ + "libc", + "rand_chacha 0.3.1", "rand_core 0.6.3", ] @@ -1368,6 +1426,16 @@ dependencies = [ "rand_core 0.5.1", ] +[[package]] +name = "rand_chacha" +version = "0.3.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "e6c10a63a0fa32252be49d21e7709d4d4baf8d231c2dbce1eaa8141b9b127d88" +dependencies = [ + "ppv-lite86", + "rand_core 0.6.3", +] + [[package]] name = "rand_core" version = "0.5.1" @@ -1395,6 +1463,15 @@ dependencies = [ "rand_core 0.5.1", ] +[[package]] +name = "rand_xorshift" +version = "0.3.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "d25bf25ec5ae4a3f1b92f929810509a2f53d7dca2f50b794ff57e3face536c8f" +dependencies = [ + "rand_core 0.6.3", +] + [[package]] name = "rayon" version = "1.5.3" @@ -1454,6 +1531,15 @@ version = "0.6.27" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "a3f87b73ce11b1619a3c6332f45341e0047173771e8b8b73f87bfeefb7b56244" +[[package]] +name = "remove_dir_all" +version = "0.5.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "3acd125665422973a33ac9d3dd2df85edad0f4ae9b00dafb1a05e43a9f5ef8e7" +dependencies = [ + "winapi", +] + [[package]] name = "retain_mut" version = "0.1.9" @@ -1870,6 +1956,7 @@ dependencies = [ "anyhow", "futures", "indexmap", + "proptest", "rome_analyze", "rome_console", "rome_diagnostics", @@ -2028,6 +2115,18 @@ dependencies = [ "webpki", ] +[[package]] +name = "rusty-fork" +version = "0.3.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "cb3dcc6e454c328bb824492db107ab7c0ae8fcffe4ad210136ef014458c1bc4f" +dependencies = [ + "fnv", + "quick-error 1.2.3", + "tempfile", + "wait-timeout", +] + [[package]] name = "ryu" version = "1.0.11" @@ -2254,6 +2353,20 @@ dependencies = [ "unicode-ident", ] +[[package]] +name = "tempfile" +version = "3.3.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "5cdb1ef4eaeeaddc8fbd371e5017057064af0911902ef36b39801f67cc6d79e4" +dependencies = [ + "cfg-if", + "fastrand", + "libc", + "redox_syscall", + "remove_dir_all", + "winapi", +] + [[package]] name = "termcolor" version = "1.1.3" @@ -2718,6 +2831,15 @@ version = "1.0.2" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "6a02e4885ed3bc0f2de90ea6dd45ebcbb66dacffe03547fadbb0eeae2770887d" +[[package]] +name = "wait-timeout" +version = "0.2.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "9f200f5b12eb75f8c1ed65abd4b2db8a6e1b138a20de009dacee265a2498f3f6" +dependencies = [ + "libc", +] + [[package]] name = "walkdir" version = "2.3.2" diff --git a/crates/rome_lsp/Cargo.toml b/crates/rome_lsp/Cargo.toml index 560e7c6dc59..d6c0fcf69a4 100644 --- a/crates/rome_lsp/Cargo.toml +++ b/crates/rome_lsp/Cargo.toml @@ -30,3 +30,4 @@ futures = "0.3" [dev-dependencies] tower = { version = "0.4.12", features = ["timeout"] } tokio = { workspace = true, features = ["rt", "rt-multi-thread", "macros"] } +proptest = "1.0.0" diff --git a/crates/rome_lsp/src/documents.rs b/crates/rome_lsp/src/documents.rs index a60af874486..084726352b4 100644 --- a/crates/rome_lsp/src/documents.rs +++ b/crates/rome_lsp/src/documents.rs @@ -1,3 +1,6 @@ +use anyhow::Result; +use rome_rowan::TextRange; + use crate::line_index::LineIndex; /// Represents an open [`textDocument`]. Can be cheaply cloned. @@ -6,16 +9,22 @@ use crate::line_index::LineIndex; #[derive(Clone)] pub(crate) struct Document { pub(crate) version: i32, - pub(crate) content: String, pub(crate) line_index: LineIndex, } impl Document { - pub(crate) fn new(version: i32, text: &str) -> Self { + pub(crate) fn new(version: i32, text: impl Into) -> Self { Self { version, - content: text.into(), line_index: LineIndex::new(text), } } + + pub(crate) fn text(&self) -> &str { + self.line_index.text() + } + + pub(crate) fn replace_range(&mut self, range: TextRange, replace_with: &str) -> Result<()> { + self.line_index.replace_range(range, replace_with) + } } diff --git a/crates/rome_lsp/src/handlers/text_document.rs b/crates/rome_lsp/src/handlers/text_document.rs index 9b6e1f72e95..7a531dc0382 100644 --- a/crates/rome_lsp/src/handlers/text_document.rs +++ b/crates/rome_lsp/src/handlers/text_document.rs @@ -47,10 +47,9 @@ pub(crate) async fn did_change( let version = params.text_document.version; let rome_path = session.file_path(&url)?; - let doc = session.document(&url)?; + let mut doc = session.document(&url)?; - let mut content = doc.content; - tracing::trace!("old document: {content:?}"); + tracing::trace!("old document: {:?}", doc.text()); for change in params.content_changes { match change.range { @@ -58,25 +57,24 @@ pub(crate) async fn did_change( let text_range = utils::text_range(&doc.line_index, range)?; let range = Range::::from(text_range); tracing::trace!("replace range {range:?} with {:?}", change.text); - content.replace_range(range, &change.text); + doc.replace_range(text_range, &change.text)?; } None => { tracing::trace!("replace content {:?}", change.text); - content = change.text; + doc = Document::new(version, change.text); } } } - tracing::trace!("new document: {content:?}"); - - let doc = Document::new(version, &content); + tracing::trace!("new document: {:?}", doc.text()); session.workspace.change_file(ChangeFileParams { path: rome_path, version, - content, + content: doc.text().into(), })?; + doc.version = version; session.insert_document(url.clone(), doc); if let Err(err) = session.update_diagnostics(url).await { diff --git a/crates/rome_lsp/src/line_index.rs b/crates/rome_lsp/src/line_index.rs index 0ae61bf6f62..4478f76dd09 100644 --- a/crates/rome_lsp/src/line_index.rs +++ b/crates/rome_lsp/src/line_index.rs @@ -3,8 +3,9 @@ //! //! Copied from rust-analyzer -use std::cmp::Ordering; +use std::{cmp::Ordering, ops::Range}; +use anyhow::{Context, Result}; use rome_rowan::{TextRange, TextSize}; #[derive(Clone, Debug, PartialEq, Eq)] @@ -23,7 +24,9 @@ pub(crate) struct LineCol { } impl LineIndex { - pub(crate) fn new(text: &str) -> LineIndex { + pub(crate) fn new(text: impl Into) -> LineIndex { + let text = text.into(); + let mut newlines = vec![0.into()]; for (offset, c) in text.char_indices() { @@ -33,10 +36,7 @@ impl LineIndex { } } - LineIndex { - text: text.into(), - newlines, - } + LineIndex { text, newlines } } pub(crate) fn line_col(&self, offset: TextSize) -> Option { @@ -141,11 +141,84 @@ impl LineIndex { pub(crate) fn len(&self) -> u32 { self.newlines.len().try_into().unwrap_or(u32::MAX) } + + /// Modify this [LineIndex] in place to remove the specified range, and replace it with the given string + pub(crate) fn replace_range(&mut self, range: TextRange, replace_with: &str) -> Result<()> { + let start = self.line_col(range.start()).with_context(|| { + format!( + "byte offset {:?} is larger than the document text length of {}", + range.start(), + self.text.len() + ) + })?; + + let end = self.line_col(range.end()).with_context(|| { + format!( + "byte offset {:?} is larger than the document text length of {}", + range.end(), + self.text.len() + ) + })?; + + let mut line_index = usize::try_from(start.line)? + 1; + let mut prev_end = usize::try_from(end.line)? + 1; + + let mut text = replace_with; + let mut position = range.start(); + + while let Some(offset) = text.find('\n') { + let offset = offset + 1; + text = &text[offset..]; + + let offset = TextSize::try_from(offset)?; + position += offset; + + if line_index < prev_end { + self.newlines[line_index] = position; + } else { + self.newlines.insert(line_index, position); + } + + line_index += 1; + } + + while line_index < prev_end { + self.newlines.remove(line_index); + prev_end -= 1; + } + + let prev_len = range.len(); + let next_len = TextSize::of(replace_with); + + match prev_len.cmp(&next_len) { + Ordering::Less => { + while line_index < self.newlines.len() { + self.newlines[line_index] += next_len - prev_len; + line_index += 1; + } + } + Ordering::Equal => {} + Ordering::Greater => { + while line_index < self.newlines.len() { + self.newlines[line_index] -= prev_len - next_len; + line_index += 1; + } + } + } + + let range: Range = range.into(); + self.text.replace_range(range, replace_with); + + Ok(()) + } } #[cfg(test)] mod tests { - use rome_rowan::TextSize; + use std::ops::Range; + + use proptest::prelude::*; + use rome_rowan::{TextRange, TextSize}; use super::{LineCol, LineIndex}; @@ -214,4 +287,102 @@ mod tests { check_conversion!(line_index: LineCol { line: 0, col: 26 } => TextSize::from(32)); check_conversion!(line_index: LineCol { line: 0, col: 27 } => TextSize::from(33)); } + + #[test] + fn replace_range_insert_line_1() { + let mut line_index = LineIndex::new("line 0\nline 1\nline2"); + + line_index + .replace_range( + TextRange::new(TextSize::from(7), TextSize::from(13)), + "line 1.1\nline 1.2", + ) + .unwrap(); + + assert_eq!( + line_index, + LineIndex::new("line 0\nline 1.1\nline 1.2\nline2") + ); + } + + #[test] + fn replace_range_insert_line_2() { + let mut line_index = LineIndex::new("line 0\nline 1\nline2\nline 3"); + + line_index + .replace_range( + TextRange::new(TextSize::from(7), TextSize::from(19)), + "line 1.1\nline 1.2\nline 2.1\nline 2.2", + ) + .unwrap(); + + assert_eq!( + line_index, + LineIndex::new("line 0\nline 1.1\nline 1.2\nline 2.1\nline 2.2\nline 3") + ); + } + + #[test] + fn replace_range_remove_line_1() { + let mut line_index = LineIndex::new("line 0\nline 1\nline2"); + + line_index + .replace_range(TextRange::new(TextSize::from(6), TextSize::from(13)), "") + .unwrap(); + + assert_eq!(line_index, LineIndex::new("line 0\nline2")); + } + + #[test] + fn replace_range_remove_line_2() { + let mut line_index = LineIndex::new("line 0\nline 1\nline2"); + + line_index + .replace_range(TextRange::new(TextSize::from(7), TextSize::from(14)), "") + .unwrap(); + + assert_eq!(line_index, LineIndex::new("line 0\nline2")); + } + + /// Property testing strategy that generates an arbitrary string, along with a valid text range within that string + fn text_with_range() -> impl Strategy { + any::() + .prop_flat_map(|text| { + let len = text.len().max(1); + (Just(text), 0..len) + }) + .prop_flat_map(|(text, start)| { + let len = text.len().max(1); + (Just(text), Just(start), start..len) + }) + .prop_filter_map( + "start and end are valid char indices", + |(text, start, end)| { + if !text.is_char_boundary(start) { + return None; + } + + if !text.is_char_boundary(end) { + return None; + } + + let start = TextSize::try_from(start).ok()?; + let end = TextSize::try_from(end).ok()?; + Some((text, TextRange::new(start, end))) + }, + ) + } + + proptest! { + #[test] + fn property_test((mut text, range) in text_with_range(), replace_with in any::()) { + let mut actual = LineIndex::new(&text); + actual.replace_range(range, &replace_with).unwrap(); + + text.replace_range(Range::::from(range), &replace_with); + + let expected = LineIndex::new(text); + prop_assert_eq!(actual, expected); + } + } } From 50d2422b98e931de3c782709bf10bec8b9b94e70 Mon Sep 17 00:00:00 2001 From: l3ops Date: Wed, 4 Jan 2023 18:05:37 +0100 Subject: [PATCH 2/2] improve the safety of arithmetics and indexed access --- crates/rome_lsp/src/line_index.rs | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/crates/rome_lsp/src/line_index.rs b/crates/rome_lsp/src/line_index.rs index 4478f76dd09..d41ba9fd0f5 100644 --- a/crates/rome_lsp/src/line_index.rs +++ b/crates/rome_lsp/src/line_index.rs @@ -174,6 +174,11 @@ impl LineIndex { position += offset; if line_index < prev_end { + // SAFETY: `line_index` is checked to be less than `prev_end`. + // Since `prev_end` is at most `end.line + 1`, and the value of + // `end.line` returned by `.line_col()` is at most + // `self.newlines.len() - 1`, the index is guaranteed to never + // be out-of-bounds self.newlines[line_index] = position; } else { self.newlines.insert(line_index, position); @@ -193,14 +198,18 @@ impl LineIndex { match prev_len.cmp(&next_len) { Ordering::Less => { while line_index < self.newlines.len() { - self.newlines[line_index] += next_len - prev_len; + self.newlines[line_index] = self.newlines[line_index] + .checked_add(next_len - prev_len) + .context("arithmetics overflow")?; line_index += 1; } } Ordering::Equal => {} Ordering::Greater => { while line_index < self.newlines.len() { - self.newlines[line_index] -= prev_len - next_len; + self.newlines[line_index] = self.newlines[line_index] + .checked_sub(prev_len - next_len) + .context("arithmetics overflow")?; line_index += 1; } }