Fix replacement edit range computation (#12171)

## Summary

This PR fixes various bugs for computing the replacement range between
the original and modified source for the language server.

1. When finding the end offset of the source and modified range, we
should apply `zip` on the reversed iterator. The bug was that it was
reversing the already zipped iterator. The problem here is that the
length of both slices aren't going to be the same unless the source
wasn't modified at all. Refer to the [Rust
playground](https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=44f860d31bd26456f3586b6ab530c22f)
where you can see this in action.
2. Skip the first line when computing the start offset because the first
line start value will always be 0 and the default value of the source /
modified range start is also 0. So, comparing 0 and 0 is not useful
which means we can skip the first value.
3. While iterating in the reverse direction, we should only stop if the
line start is strictly less than the source start i.e., we should use
`<` instead of `<=`.

fixes: #12128 

## Test Plan

Add test cases where the text is being inserted, deleted, and replaced
between the original and new source code, validate the replacement
ranges.
This commit is contained in:
Dhruv Manilawala 2024-07-04 09:24:07 +05:30 committed by GitHub
parent 8210c1ed5b
commit d870720841
No known key found for this signature in database
GPG key ID: B5690EEEBB952194

View file

@ -1,5 +1,6 @@
use ruff_text_size::{TextLen, TextRange, TextSize}; use ruff_text_size::{TextLen, TextRange, TextSize};
#[derive(Debug)]
pub(crate) struct Replacement { pub(crate) struct Replacement {
pub(crate) source_range: TextRange, pub(crate) source_range: TextRange,
pub(crate) modified_range: TextRange, pub(crate) modified_range: TextRange,
@ -15,41 +16,46 @@ impl Replacement {
modified_line_starts: &[TextSize], modified_line_starts: &[TextSize],
) -> Self { ) -> Self {
let mut source_start = TextSize::default(); let mut source_start = TextSize::default();
let mut replaced_start = TextSize::default(); let mut modified_start = TextSize::default();
let mut source_end = source.text_len();
let mut replaced_end = modified.text_len(); for (source_line_start, modified_line_start) in source_line_starts
let mut line_iter = source_line_starts
.iter() .iter()
.copied() .copied()
.zip(modified_line_starts.iter().copied()); .zip(modified_line_starts.iter().copied())
for (source_line_start, modified_line_start) in line_iter.by_ref() { .skip(1)
if source_line_start != modified_line_start {
|| source[TextRange::new(source_start, source_line_start)] if source[TextRange::new(source_start, source_line_start)]
!= modified[TextRange::new(replaced_start, modified_line_start)] != modified[TextRange::new(modified_start, modified_line_start)]
{ {
break; break;
} }
source_start = source_line_start; source_start = source_line_start;
replaced_start = modified_line_start; modified_start = modified_line_start;
} }
let mut line_iter = line_iter.rev(); let mut source_end = source.text_len();
let mut modified_end = modified.text_len();
for (old_line_start, new_line_start) in line_iter.by_ref() { for (source_line_start, modified_line_start) in source_line_starts
if old_line_start <= source_start .iter()
|| new_line_start <= replaced_start .rev()
|| source[TextRange::new(old_line_start, source_end)] .copied()
!= modified[TextRange::new(new_line_start, replaced_end)] .zip(modified_line_starts.iter().rev().copied())
{
if source_line_start < source_start
|| modified_line_start < modified_start
|| source[TextRange::new(source_line_start, source_end)]
!= modified[TextRange::new(modified_line_start, modified_end)]
{ {
break; break;
} }
source_end = old_line_start; source_end = source_line_start;
replaced_end = new_line_start; modified_end = modified_line_start;
} }
Replacement { Replacement {
source_range: TextRange::new(source_start, source_end), source_range: TextRange::new(source_start, source_end),
modified_range: TextRange::new(replaced_start, replaced_end), modified_range: TextRange::new(modified_start, modified_end),
} }
} }
} }
@ -57,42 +63,166 @@ impl Replacement {
#[cfg(test)] #[cfg(test)]
mod tests { mod tests {
use ruff_source_file::LineIndex; use ruff_source_file::LineIndex;
use ruff_text_size::TextRange;
use super::Replacement; use super::Replacement;
fn compute_replacement(source: &str, modified: &str) -> (Replacement, String) {
let source_index = LineIndex::from_source_text(source);
let modified_index = LineIndex::from_source_text(modified);
let replacement = Replacement::between(
source,
source_index.line_starts(),
modified,
modified_index.line_starts(),
);
let mut expected = source.to_string();
expected.replace_range(
replacement.source_range.start().to_usize()..replacement.source_range.end().to_usize(),
&modified[replacement.modified_range],
);
(replacement, expected)
}
#[test] #[test]
fn find_replacement_range_works() { fn delete_first_line() {
let original = r#" let source = "aaaa
aaaa bbbb
cccc
";
let modified = "bbbb
cccc
";
let (replacement, expected) = compute_replacement(source, modified);
assert_eq!(replacement.source_range, TextRange::new(0.into(), 5.into()));
assert_eq!(replacement.modified_range, TextRange::empty(0.into()));
assert_eq!(modified, &expected);
}
#[test]
fn delete_middle_line() {
let source = "aaaa
bbbb
cccc
dddd
";
let modified = "aaaa
bbbb
dddd
";
let (replacement, expected) = compute_replacement(source, modified);
assert_eq!(
replacement.source_range,
TextRange::new(10.into(), 15.into())
);
assert_eq!(replacement.modified_range, TextRange::empty(10.into()));
assert_eq!(modified, &expected);
}
#[test]
fn delete_multiple_lines() {
let source = "aaaa
bbbb bbbb
cccc cccc
dddd dddd
eeee eeee
"#; ffff
let original_index = LineIndex::from_source_text(original); ";
let new = r#" let modified = "aaaa
bb
cccc cccc
dd dddd
"#; ffff
let new_index = LineIndex::from_source_text(new); ";
let expected = r#" let (replacement, expected) = compute_replacement(source, modified);
bb assert_eq!(
cccc replacement.source_range,
dd TextRange::new(5.into(), 25.into())
"#;
let replacement = Replacement::between(
original,
original_index.line_starts(),
new,
new_index.line_starts(),
); );
let mut test = original.to_string(); assert_eq!(
test.replace_range( replacement.modified_range,
replacement.source_range.start().to_usize()..replacement.source_range.end().to_usize(), TextRange::new(5.into(), 15.into())
&new[replacement.modified_range],
); );
assert_eq!(modified, &expected);
}
assert_eq!(expected, &test); #[test]
fn insert_first_line() {
let source = "bbbb
cccc
";
let modified = "aaaa
bbbb
cccc
";
let (replacement, expected) = compute_replacement(source, modified);
assert_eq!(replacement.source_range, TextRange::empty(0.into()));
assert_eq!(
replacement.modified_range,
TextRange::new(0.into(), 5.into())
);
assert_eq!(modified, &expected);
}
#[test]
fn insert_middle_line() {
let source = "aaaa
cccc
";
let modified = "aaaa
bbbb
cccc
";
let (replacement, expected) = compute_replacement(source, modified);
assert_eq!(replacement.source_range, TextRange::empty(5.into()));
assert_eq!(
replacement.modified_range,
TextRange::new(5.into(), 10.into())
);
assert_eq!(modified, &expected);
}
#[test]
fn insert_multiple_lines() {
let source = "aaaa
cccc
eeee
";
let modified = "aaaa
bbbb
cccc
dddd
";
let (replacement, expected) = compute_replacement(source, modified);
assert_eq!(
replacement.source_range,
TextRange::new(5.into(), 15.into())
);
assert_eq!(
replacement.modified_range,
TextRange::new(5.into(), 20.into())
);
assert_eq!(modified, &expected);
}
#[test]
fn replace_lines() {
let source = "aaaa
bbbb
cccc
";
let modified = "aaaa
bbcb
cccc
";
let (replacement, expected) = compute_replacement(source, modified);
assert_eq!(
replacement.source_range,
TextRange::new(5.into(), 10.into())
);
assert_eq!(
replacement.modified_range,
TextRange::new(5.into(), 10.into())
);
assert_eq!(modified, &expected);
} }
} }