Skip to content
Merged
Show file tree
Hide file tree
Changes from 19 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 43 additions & 1 deletion src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,10 @@
use std::ops::Range;
use std::slice;

use crate::util::{strip_common_postfix, strip_common_prefix};
use crate::{
sources::words,
util::{strip_common_postfix, strip_common_prefix},
};

pub use crate::slider_heuristic::{
IndentHeuristic, IndentLevel, NoSliderHeuristic, SliderHeuristic,
Expand Down Expand Up @@ -416,6 +419,45 @@ impl Hunk {
pub fn is_pure_removal(&self) -> bool {
self.after.is_empty()
}

/// Performs a word-diff of the hunk
pub fn word_diff<'a>(
&self,
input: &InternedInput<&'a str>,
diff_input: &mut InternedInput<&'a str>,
diff: &mut Diff,
) {

Copilot AI Dec 20, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The word_diff API requires callers to pass mutable references to diff_input and diff that are populated by this function. This design is not intuitive and error-prone because:

  1. Callers must create and maintain these objects themselves
  2. The function's return type is (), making it unclear that it populates the diff parameter
  3. The relationship between the parameters is not obvious from the signature

Consider either:

  • Returning the Diff as the function's result
  • Using a builder pattern with a dedicated WordDiff type
  • Adding documentation that clearly explains the expected usage pattern and the relationship between parameters

Copilot uses AI. Check for mistakes.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While copilot is right in principle, I think compete documentation on parameters could explain why they need to be mutable.
Docs are definitely needed here.

let Hunk { before, after } = self.clone();
Comment thread
Byron marked this conversation as resolved.
diff_input.update_before(
before
.map(|index| input.before[index as usize])
.map(|token| input.interner[token])
.flat_map(|line| words(line)),
);
diff_input.update_after(
after
.map(|index| input.after[index as usize])
.map(|token| input.interner[token])
.flat_map(|line| words(line)),
);
diff.removed.clear();
diff.removed.resize(diff_input.before.len(), false);
diff.added.clear();
diff.added.resize(diff_input.after.len(), false);
if self.is_pure_removal() {
diff.removed.fill(true);
} else if self.is_pure_insertion() {
diff.added.fill(true);
} else {
diff.compute_with(
Algorithm::Myers,
Comment thread
KnorpelSenf marked this conversation as resolved.
&diff_input.before,
&diff_input.after,
diff_input.interner.num_tokens(),
);
diff.postprocess_no_heuristic(diff_input);
}
}

Copilot AI Dec 20, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The word_diff method doesn't clear the interner between calls, which could lead to memory accumulation if called repeatedly. According to the documentation of update_before and update_after, "this does not erase any tokens from the interner and might therefore be considered a memory leak."

While the comment mentions clearing the interner for long-running processes, for a public API like word_diff, it would be better to either:

  1. Clear the interner at the start of this method
  2. Document this behavior clearly in the word_diff documentation
  3. Provide a way for callers to manage this explicitly

Copilot uses AI. Check for mistakes.
}

/// Yields all [`Hunk`]s in a file in monotonically increasing order.
Expand Down
55 changes: 55 additions & 0 deletions src/sources.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,14 @@ pub fn lines(data: &str) -> Lines<'_> {
Lines(ByteLines(data.as_bytes()))
}

/// Returns a [`TokenSource`] that uses the words in `data` as Tokens. A word is
/// a sequence of alphanumeric characters as determined by
/// `char::is_alphanumeric`, or a sequence of just the space character ' '. Any
/// other characters are their own word.
Comment on lines +22 to +23

Copilot AI Dec 20, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The documentation states that "a sequence of just the space character ' '" is treated as a word, but this is incomplete. The implementation on line 104-108 treats any sequence of consecutive spaces as a single token. The documentation should clarify that multiple consecutive spaces are grouped together as one token.

Suggested change
/// `char::is_alphanumeric`, or a sequence of just the space character ' '. Any
/// other characters are their own word.
/// `char::is_alphanumeric`, or a sequence of one or more consecutive space
/// characters (' '). Any other characters are their own word.

Copilot uses AI. Check for mistakes.
Comment on lines +21 to +23

Copilot AI Dec 20, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The documentation mentions "a sequence of alphanumeric characters as determined by char::is_alphanumeric" but the implementation on lines 110-113 also includes underscores (_) as part of alphanumeric words. This is inconsistent with the documentation. Either update the documentation to mention that underscores are included in alphanumeric sequences, or remove the special handling of underscores if they should be treated as separate tokens.

Suggested change
/// a sequence of alphanumeric characters as determined by
/// `char::is_alphanumeric`, or a sequence of just the space character ' '. Any
/// other characters are their own word.
/// a sequence of "word" characters (those for which `char::is_alphanumeric`
/// returns `true`, plus the underscore character '_'), or a sequence of just
/// the space character ' '. Any other characters are their own word.

Copilot uses AI. Check for mistakes.
pub fn words(data: &str) -> Words<'_> {
Words(data)
}

/// Returns a [`TokenSource`] that uses the lines in `data` as Tokens. The newline
/// separator (`\r\n` or `\n`) is included in the emitted tokens. This means that changing
/// the newline separator from `\r\n` to `\n` (or omitting it fully on the last line) is
Expand Down Expand Up @@ -84,6 +92,53 @@ impl<'a> TokenSource for Lines<'a> {
}
}

/// A [`TokenSource`] that returns the words of a string as tokens. See
/// [`words`] for details.
#[derive(Clone, Copy, PartialEq, Eq)]
pub struct Words<'a>(&'a str);

impl<'a> Iterator for Words<'a> {
type Item = &'a str;

fn next(&mut self) -> Option<Self::Item> {
if self.0.is_empty() {
return None;
}

let initial = self.0.chars().next().unwrap();
let word_len = if initial == ' ' {
self.0
.char_indices()
.find(|(_, c)| *c != ' ')
.map_or(self.0.len(), |(index, _)| index)
} else if initial.is_alphanumeric() {
Comment thread
KnorpelSenf marked this conversation as resolved.
self.0
.char_indices()
.find(|(_, c)| !c.is_alphanumeric() && *c != '_')
.map_or(self.0.len(), |(index, _)| index)
} else {
initial.len_utf8()
};

let (word, rem) = self.0.split_at(word_len);
self.0 = rem;
Some(word)
}
}
impl<'a> TokenSource for Words<'a> {
type Token = &'a str;

type Tokenizer = Self;

fn tokenize(&self) -> Self::Tokenizer {
*self
}

fn estimate_tokens(&self) -> u32 {
(self.0.len() / 3) as u32

Copilot AI Dec 20, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The token estimation heuristic divides string length by 3 (assuming average word length of 3 characters). This could result in poor allocation sizing:

  1. For typical English text, average word length is closer to 4-5 characters when including spaces and punctuation
  2. For code with long identifiers, the average could be much higher
  3. The estimate doesn't account for the fact that each punctuation character becomes its own token

Consider using a more conservative estimate like (self.0.len() / 5) or implementing a sampling approach similar to ByteLines::estimate_tokens() that examines the first portion of the text to calculate the actual average.

Suggested change
(self.0.len() / 3) as u32
(self.0.len() / 5) as u32

Copilot uses AI. Check for mistakes.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know about this one, but it seems that heuristics aren't always right and maybe there is a way to make it configurable?
Maybe this word-diff is also so tuned to Latin text that we might say it in the function, i.e. word_diff to latin_word_diff.

}
}

/// A [`TokenSource`] that returns the lines of a byte slice as tokens. See [`byte_lines`]
/// for details.
#[derive(Clone, Copy, PartialEq, Eq)]
Expand Down
140 changes: 140 additions & 0 deletions src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,20 @@ use expect_test::{expect, expect_file};
// use git_repository as git;

use crate::intern::InternedInput;
use crate::sources::words;
use crate::unified_diff::BasicLineDiffPrinter;
use crate::{Algorithm, Diff, UnifiedDiffConfig};

#[test]
fn words_tokenizer() {
let text = "Hello, imara!\n (foo-bar_baz)";
let tokens = words(text).collect::<Vec<_>>();
assert_eq!(
tokens,
vec!["Hello", ",", " ", "imara", "!", "\n", " ", "(", "foo", "-", "bar_baz", ")"]
);
}

#[test]
fn postprocess() {
let before = r#"
Expand Down Expand Up @@ -320,6 +331,135 @@ i
}
}

#[test]
fn hunk_word_diff_pure() {
let before = r#"fn foo() -> Bar{
let mut foo = 2.0;
foo *= 100 / 2;
}"#;
let after = r#"fn foo() -> Bar{
let mut foo = 2.0;
foo *= 100 / 2;
println("hello world")
}"#;
let mut input = InternedInput::new(before, after);
for algorithm in Algorithm::ALL {
let mut diff_input = InternedInput::default();
let mut d = Diff::default();

Copilot AI Dec 20, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The variable name 'd' is too short and unclear. Consider using a more descriptive name like 'word_diff' or 'inner_diff' to make the test more readable and to clarify that this is a separate diff object from the line-level diff.

Copilot uses AI. Check for mistakes.

Copilot AI Dec 20, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test variables diff_input and d are declared once outside the algorithm loop but are reused across all algorithm iterations. While word_diff appears to clear and resize the vectors, this pattern could hide bugs if word_diff doesn't properly reset all state. Consider either:

  1. Moving these declarations inside the loop to ensure a fresh state for each algorithm
  2. Adding an explicit reset/clear call between iterations to make the intent clear

Copilot uses AI. Check for mistakes.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it's a feature to let state accumulate without resetting it to ensure the call can handle it.


println!("{algorithm:?}");

let mut diff = Diff::compute(algorithm, &input);
diff.postprocess_lines(&input);

let mut hunks = diff.hunks();
let hunk = hunks.next().expect("missing first hunk");
hunk.word_diff(&input, &mut diff_input, &mut d);
let mut h = d.hunks();
let first = h.next().expect("missing first inner hunk");
assert!(first.is_pure_insertion());
assert_eq!(first.before, 0..0);
assert_eq!(
first.after,
0..words(" println(\"hello world\")\n").count() as u32
);
assert_eq!(h.next(), None);
assert_eq!(hunks.next(), None);

swap(&mut input.before, &mut input.after);

let mut diff = Diff::compute(algorithm, &input);
diff.postprocess_lines(&input);

let mut hunks = diff.hunks();
let hunk = hunks.next().expect("missing first hunk");
hunk.word_diff(&input, &mut diff_input, &mut d);
let mut h = d.hunks();
let first = h.next().expect("missing first inner hunk");
assert!(first.is_pure_removal());
assert_eq!(
first.before,
0..words(" println(\"hello world\")\n").count() as u32
);
assert_eq!(first.after, 0..0);
assert_eq!(h.next(), None);
assert_eq!(hunks.next(), None);

swap(&mut input.before, &mut input.after);
}
}

#[test]
fn hunk_word_diff_modify() {
let before = r#"fn foo() -> Bar {
let mut foo = 2.0;
foo *= 100 / 2;
}"#;
let after = r#"fn foo() -> Bar {
let mut foo = 3.0 * 2.0;
foo += 100 / 2;
}"#;
let mut input = InternedInput::new(before, after);
for algorithm in Algorithm::ALL {
let mut diff_input = InternedInput::default();
let mut d = Diff::default();

Copilot AI Dec 20, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The variable name 'd' is too short and unclear. Consider using a more descriptive name like 'word_diff' or 'inner_diff' to make the test more readable and to clarify that this is a separate diff object from the line-level diff.

Copilot uses AI. Check for mistakes.

Copilot AI Dec 20, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test variables diff_input and d are declared once outside the algorithm loop but are reused across all algorithm iterations. While word_diff appears to clear and resize the vectors, this pattern could hide bugs if word_diff doesn't properly reset all state. Consider either:

  1. Moving these declarations inside the loop to ensure a fresh state for each algorithm
  2. Adding an explicit reset/clear call between iterations to make the intent clear

Copilot uses AI. Check for mistakes.

println!("{algorithm:?}");

let mut diff = Diff::compute(algorithm, &input);
diff.postprocess_lines(&input);

let mut hunks = diff.hunks();
let hunk = hunks.next().expect("missing first hunk");
hunk.word_diff(&input, &mut diff_input, &mut d);
let mut h = d.hunks();
let first = h.next().expect("missing first inner hunk");
assert!(first.is_pure_insertion());
let off = words(" let mut foo = ").count() as u32;
assert_eq!(first.before, off..off);
let ins = words("3.0 * ").count() as u32;
assert_eq!(first.after, off..ins + off);
let second = h.next().expect("missing second inner hunk");
let off = words(
r#" let mut foo = 2.0;
foo "#,
)
.count() as u32;
assert_eq!(second.before, off..1 + off);
assert_eq!(second.after, ins + off..1 + ins + off);
assert_eq!(h.next(), None);
assert_eq!(hunks.next(), None);

swap(&mut input.before, &mut input.after);

let mut diff = Diff::compute(algorithm, &input);
diff.postprocess_lines(&input);

let mut hunks = diff.hunks();
let hunk = hunks.next().expect("missing first hunk");
hunk.word_diff(&input, &mut diff_input, &mut d);
let mut h = d.hunks();
let first = h.next().expect("missing first inner hunk");
assert!(first.is_pure_removal());
let off = words(" let mut foo = ").count() as u32;
let rem = words("3.0 * ").count() as u32;
assert_eq!(first.before, off..rem + off);
assert_eq!(first.after, off..off);
let second = h.next().expect("missing second inner hunk");
let off = words(
r#" let mut foo = 2.0;
foo "#,
)
.count() as u32;
assert_eq!(second.before, rem + off..1 + rem + off);
assert_eq!(second.after, off..1 + off);
assert_eq!(h.next(), None);
assert_eq!(hunks.next(), None);

swap(&mut input.before, &mut input.after);
}
}

pub fn project_root() -> PathBuf {
let dir = env!("CARGO_MANIFEST_DIR");
let mut res = PathBuf::from(dir);
Expand Down