Skip to content

Commit

Permalink
fix: different approach to fix off-by-one without affecting display
Browse files Browse the repository at this point in the history
This approach removes the additional empty line that would cause
the display logic to disagree in line count.

The major difference is that this new approach does not change the
output of the display implementation, therefore, no test files need to
be updated, and no additional line is printed to the end-user.

As for the cause of the index out-of-bounds, this is simply a
disagreement between line sources, such as the `line-numbers`
crate and Tree Sitter, which all reports the correct number of lines,
with the `str::lines` iterator which does not iterate over the last
new line character if the last line is empty.
  • Loading branch information
JonathanxD committed Jul 8, 2024
1 parent 27f9e87 commit 017381e
Show file tree
Hide file tree
Showing 5 changed files with 36 additions and 54 deletions.
6 changes: 3 additions & 3 deletions src/display/inline.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ use crate::{
display::context::{calculate_after_context, calculate_before_context, opposite_positions},
display::hunks::Hunk,
display::style::{self, apply_colors, apply_line_number_color},
lines::{format_line_num, lines_raw, MaxLine},
lines::{format_line_num, MaxLine},
options::DisplayOptions,
parse::syntax::MatchedPos,
summary::FileFormat,
Expand Down Expand Up @@ -43,8 +43,8 @@ pub(crate) fn print(
)
} else {
(
lines_raw(lhs_src).map(|s| format!("{}\n", s)).collect(),
lines_raw(rhs_src).map(|s| format!("{}\n", s)).collect(),
lhs_src.lines().map(|s| format!("{}\n", s)).collect(),
rhs_src.lines().map(|s| format!("{}\n", s)).collect(),
)
};

Expand Down
28 changes: 15 additions & 13 deletions src/display/side_by_side.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ use crate::{
split_and_apply, BackgroundColor,
},
hash::DftHashMap,
lines::{format_line_num, lines_raw},
lines::format_line_num,
options::{DisplayMode, DisplayOptions},
parse::syntax::{zip_pad_shorter, MatchedPos},
summary::FileFormat,
Expand Down Expand Up @@ -338,8 +338,8 @@ pub(crate) fn print(
)
} else {
(
lines_raw(&lhs_src).map(|s| format!("{}\n", s)).collect(),
lines_raw(&rhs_src).map(|s| format!("{}\n", s)).collect(),
lhs_src.lines().map(|s| format!("{}\n", s)).collect(),
rhs_src.lines().map(|s| format!("{}\n", s)).collect(),
)
};

Expand Down Expand Up @@ -401,8 +401,8 @@ pub(crate) fn print(
let mut prev_lhs_line_num = None;
let mut prev_rhs_line_num = None;

let lhs_lines = lines_raw(lhs_src).collect::<Vec<_>>();
let rhs_lines = lines_raw(rhs_src).collect::<Vec<_>>();
let lhs_lines = lhs_src.lines().collect::<Vec<_>>();
let rhs_lines = rhs_src.lines().collect::<Vec<_>>();
let matched_lines = all_matched_lines_filled(lhs_mps, rhs_mps, &lhs_lines, &rhs_lines);
let mut matched_lines_to_print = &matched_lines[..];

Expand Down Expand Up @@ -515,14 +515,16 @@ pub(crate) fn print(
None => vec![" ".repeat(source_dims.content_width)],
};
let rhs_line = match rhs_line_num {
Some(rhs_line_num) => split_and_apply(
rhs_lines[rhs_line_num.as_usize()],
source_dims.content_width,
display_options.tab_width,
rhs_highlights.get(rhs_line_num).unwrap_or(&vec![]),
Side::Right,
),
None => vec!["".into()],
Some(rhs_line_num) if rhs_line_num.as_usize() <= rhs_lines.len() => {
split_and_apply(
rhs_lines[rhs_line_num.as_usize()],
source_dims.content_width,
display_options.tab_width,
rhs_highlights.get(rhs_line_num).unwrap_or(&vec![]),
Side::Right,
)
}
_ => vec!["".into()],
};

for (i, (lhs_line, rhs_line)) in zip_pad_shorter(&lhs_line, &rhs_line)
Expand Down
4 changes: 2 additions & 2 deletions src/display/style.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ use crate::parse::syntax::StringKind;
use crate::{
constants::Side,
hash::DftHashMap,
lines::{byte_len, lines_raw},
lines::byte_len,
options::DisplayOptions,
parse::syntax::{AtomKind, MatchKind, MatchedPos, TokenKind},
summary::FileFormat,
Expand Down Expand Up @@ -401,7 +401,7 @@ pub(crate) fn apply_colors(
positions: &[MatchedPos],
) -> Vec<String> {
let styles = color_positions(side, background, syntax_highlight, file_format, positions);
let lines = lines_raw(s).collect::<Vec<_>>();
let lines = s.lines().collect::<Vec<_>>();
style_lines(&lines, &styles)
}

Expand Down
17 changes: 16 additions & 1 deletion src/line_parser.rs
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,22 @@ pub(crate) fn change_positions(lhs_src: &str, rhs_src: &str) -> Vec<MatchedPos>
// have a very large number of words, don't diff
// individual words.
if lhs_words.len() > MAX_WORDS_IN_LINE || rhs_words.len() > MAX_WORDS_IN_LINE {
for lhs_pos in lhs_lp.from_region(lhs_offset, lhs_offset + lhs_part.len()) {
// If the last line is empty, `lhs_lines` will have one line less
// than other sources, such as `LinePositions::from`.
// This is a workaround for the off-by-one error caused by this disagreement
// between `str::lines` and other sources used to calculate ranges.
let lp_from_region = {
let mut lp_from_region =
lhs_lp.from_region(lhs_offset, lhs_offset + lhs_part.len());
if let Some(last) = lp_from_region.last() {
if last.start_col == 0 && last.end_col == 0 {
lp_from_region.pop();
}
}
lp_from_region
};

for lhs_pos in lp_from_region {
mps.push(MatchedPos {
kind: MatchKind::NovelWord {
highlight: TokenKind::Atom(AtomKind::Normal),
Expand Down
35 changes: 0 additions & 35 deletions src/lines.rs
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
//! Manipulate lines of text and groups of lines.
use std::iter::{self, Chain, Once};
use std::ops::Sub;
use std::str::Lines;

use line_numbers::LineNumber;

Expand Down Expand Up @@ -38,30 +36,6 @@ pub(crate) fn is_all_whitespace(s: &str) -> bool {
s.chars().all(|c| c.is_whitespace())
}

pub(crate) fn lines_raw(s: &str) -> impl Iterator<Item = &str> {
if s.ends_with('\n') {
LinesIter::Chained(s.lines().chain(iter::once("")))
} else {
LinesIter::Lines(s.lines())
}
}

enum LinesIter<'a> {
Chained(Chain<Lines<'a>, Once<&'a str>>),
Lines(Lines<'a>),
}

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

fn next(&mut self) -> Option<Self::Item> {
match self {
Self::Chained(iter) => iter.next(),
Self::Lines(iter) => iter.next(),
}
}
}

#[cfg(test)]
mod tests {
use pretty_assertions::assert_eq;
Expand Down Expand Up @@ -96,13 +70,4 @@ mod tests {
fn test_is_all_whitespace() {
assert!(is_all_whitespace(" \n\t"));
}

#[test]
fn test_last_new_line_preserved() {
assert_eq!(lines_raw("").count(), 0);
assert_eq!(lines_raw("a").count(), 1);
assert_eq!(lines_raw("\n").count(), 2);
assert_eq!(lines_raw("a\n").count(), 2);
assert_eq!(lines_raw("a\n\n").count(), 3);
}
}

0 comments on commit 017381e

Please sign in to comment.