Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Account for tabs when highlighting multiline code suggestions #87976

Merged
merged 5 commits into from
Aug 23, 2021
Merged

Account for tabs when highlighting multiline code suggestions #87976

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
75fd1bf
Account for tabs when highlighting multiline code suggestions
estebank Aug 12, 2021
a29a624
wip
estebank Aug 14, 2021
5626346
Fixes to span locations
estebank Aug 17, 2021
31d07ed
remove unnecessary `info!()` logging
estebank Aug 17, 2021
955e913
review comments
estebank Aug 23, 2021
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
39 changes: 20 additions & 19 deletions compiler/rustc_errors/src/emitter.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1623,7 +1623,7 @@ impl EmitterWriter {
let line_start = sm.lookup_char_pos(parts[0].span.lo()).line;
draw_col_separator_no_space(&mut buffer, 1, max_line_num_len + 1);
let mut lines = complete.lines();
for (line_pos, (line, parts)) in
for (line_pos, (line, highlight_parts)) in
lines.by_ref().zip(highlights).take(MAX_SUGGESTION_HIGHLIGHT_LINES).enumerate()
{
// Print the span column to avoid confusion
Expand Down Expand Up @@ -1658,7 +1658,7 @@ impl EmitterWriter {
);
buffer.puts(row_num, max_line_num_len + 1, "+ ", Style::Addition);
} else if is_multiline {
match &parts[..] {
match &highlight_parts[..] {
[SubstitutionHighlight { start: 0, end }] if *end == line.len() => {
buffer.puts(row_num, max_line_num_len + 1, "+ ", Style::Addition);
}
Expand All @@ -1676,16 +1676,24 @@ impl EmitterWriter {
// print the suggestion
buffer.append(row_num, &replace_tabs(line), Style::NoStyle);

if is_multiline {
for SubstitutionHighlight { start, end } in parts {
buffer.set_style_range(
row_num,
max_line_num_len + 3 + start,
max_line_num_len + 3 + end,
Style::Addition,
true,
);
}
// Colorize addition/replacements with green.
for &SubstitutionHighlight { start, end } in highlight_parts {
// Account for tabs when highlighting (#87972).
let tabs: usize = line
.chars()
.take(start)
.map(|ch| match ch {
'\t' => 3,
_ => 0,
})
.sum();
buffer.set_style_range(
row_num,
max_line_num_len + 3 + start + tabs,
max_line_num_len + 3 + end + tabs,
Comment on lines +1692 to +1693
Copy link
Member

@m-ou-se m-ou-se Aug 19, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

This assumes the addition/replacement doesn't contain tabs itself. It rarely happens, but it can occur:

image

Here, the entire closure body up to and including the final } is supposed to be highlighted, but i put some tabs in the string literal which broke it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, yes. I assumed that we didn't have tabs in the suggestion because I didn't account for cases where snippets from the user include them :-/

Copy link
Member

Choose a reason for hiding this comment

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

I suppose all cases where user snippets are included should be replaced by multispan suggestions. (This specific one is already fixed by #87958, but I think there's quite a few places left where this happens.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could you give me the repro case? I think I fixed this, but the suggestions seem to be doing the right thing for me.

Copy link
Member

@m-ou-se m-ou-se Aug 23, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

With #87958 merged, the test case in the screenshot above doesn't trigger the problem anymore. We'll have to find another case where a bunch of user code gets put verbatim in the suggestion. Grepping for span_to_snippet now. ^^

Copy link
Member

Choose a reason for hiding this comment

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

Found one:

Screenshot 2021-08-23 at 15 59 26

mod a {
    pub const A: String = String::new();
}

fn main() {
    a
    ::	A;
    //^ TAB here
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The current code handles this correctly:

Style::Addition,
true,
);
}
row_num += 1;
}
Expand Down Expand Up @@ -1723,13 +1731,6 @@ impl EmitterWriter {
assert!(underline_start >= 0 && underline_end >= 0);
let padding: usize = max_line_num_len + 3;
for p in underline_start..underline_end {
// Colorize addition/replacements with green.
buffer.set_style(
row_num - 1,
(padding as isize + p) as usize,
Style::Addition,
true,
);
if !show_diff {
// If this is a replacement, underline with `^`, if this is an addition
// underline with `+`.
Expand Down
44 changes: 37 additions & 7 deletions compiler/rustc_errors/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -283,16 +283,21 @@ impl CodeSuggestion {
let mut buf = String::new();

let mut line_highlight = vec![];
// We need to keep track of the difference between the existing code and the added
// or deleted code in order to point at the correct column *after* substitution.
let mut acc = 0;
for part in &substitution.parts {
let cur_lo = sm.lookup_char_pos(part.span.lo());
if prev_hi.line == cur_lo.line {
let mut count =
push_trailing(&mut buf, prev_line.as_ref(), &prev_hi, Some(&cur_lo));
while count > 0 {
highlights.push(std::mem::take(&mut line_highlight));
acc = 0;
count -= 1;
}
} else {
acc = 0;
highlights.push(std::mem::take(&mut line_highlight));
let mut count = push_trailing(&mut buf, prev_line.as_ref(), &prev_hi, None);
while count > 0 {
Expand All @@ -316,18 +321,43 @@ impl CodeSuggestion {
}
}
// Add a whole line highlight per line in the snippet.
let len: isize = part
.snippet
.split('\n')
.next()
.unwrap_or(&part.snippet)
.chars()
.map(|c| match c {
'\t' => 4,
_ => 1,
})
.sum();
line_highlight.push(SubstitutionHighlight {
start: cur_lo.col.0,
end: cur_lo.col.0
+ part.snippet.split('\n').next().unwrap_or(&part.snippet).len(),
start: (cur_lo.col.0 as isize + acc) as usize,
end: (cur_lo.col.0 as isize + acc + len) as usize,
});
buf.push_str(&part.snippet);
let cur_hi = sm.lookup_char_pos(part.span.hi());
if prev_hi.line == cur_lo.line {
// Account for the difference between the width of the current code and the
// snippet being suggested, so that the *later* suggestions are correctly
// aligned on the screen.
acc += len as isize - (cur_hi.col.0 - cur_lo.col.0) as isize;
}
prev_hi = cur_hi;
prev_line = sf.get_line(prev_hi.line - 1);
for line in part.snippet.split('\n').skip(1) {
acc = 0;
highlights.push(std::mem::take(&mut line_highlight));
line_highlight.push(SubstitutionHighlight { start: 0, end: line.len() });
let end: usize = line
.chars()
.map(|c| match c {
'\t' => 4,
_ => 1,
})
.sum();
line_highlight.push(SubstitutionHighlight { start: 0, end });
}
buf.push_str(&part.snippet);
prev_hi = sm.lookup_char_pos(part.span.hi());
prev_line = sf.get_line(prev_hi.line - 1);
}
highlights.push(std::mem::take(&mut line_highlight));
let only_capitalization = is_case_difference(sm, &buf, bounding_span);
Expand Down