From 75fd1bf1e60dd140f2ba46a4252195ea2a9b9c89 Mon Sep 17 00:00:00 2001 From: Esteban Kuber Date: Thu, 12 Aug 2021 19:33:19 +0000 Subject: [PATCH 1/5] Account for tabs when highlighting multiline code suggestions --- compiler/rustc_errors/src/emitter.rs | 48 +++++++++++++++++----------- compiler/rustc_errors/src/lib.rs | 22 +++++++++---- 2 files changed, 45 insertions(+), 25 deletions(-) diff --git a/compiler/rustc_errors/src/emitter.rs b/compiler/rustc_errors/src/emitter.rs index 07c864c93a1..645b81b9540 100644 --- a/compiler/rustc_errors/src/emitter.rs +++ b/compiler/rustc_errors/src/emitter.rs @@ -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 @@ -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); } @@ -1676,16 +1676,33 @@ 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 start: usize = line + .chars() + .take(start) + .map(|ch| match ch { + '\t' => 4, + _ => 1, + }) + .sum(); + + let end: usize = line + .chars() + .take(end) + .map(|ch| match ch { + '\t' => 4, + _ => 1, + }) + .sum(); + buffer.set_style_range( + row_num, + max_line_num_len + 3 + start, + max_line_num_len + 3 + end, + Style::Addition, + true, + ); } row_num += 1; } @@ -1723,13 +1740,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 `+`. diff --git a/compiler/rustc_errors/src/lib.rs b/compiler/rustc_errors/src/lib.rs index ec29d8016dd..cae4a6b4723 100644 --- a/compiler/rustc_errors/src/lib.rs +++ b/compiler/rustc_errors/src/lib.rs @@ -283,6 +283,9 @@ 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 { @@ -290,6 +293,7 @@ impl CodeSuggestion { 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 { @@ -297,6 +301,7 @@ impl CodeSuggestion { let mut count = push_trailing(&mut buf, prev_line.as_ref(), &prev_hi, None); while count > 0 { highlights.push(std::mem::take(&mut line_highlight)); + acc = 0; count -= 1; } // push lines between the previous and current span (if any) @@ -305,6 +310,7 @@ impl CodeSuggestion { buf.push_str(line.as_ref()); buf.push('\n'); highlights.push(std::mem::take(&mut line_highlight)); + acc = 0; } } if let Some(cur_line) = sf.get_line(cur_lo.line - 1) { @@ -316,18 +322,22 @@ impl CodeSuggestion { } } // Add a whole line highlight per line in the snippet. + let len = part.snippet.split('\n').next().unwrap_or(&part.snippet).len(); 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 isize) as usize, }); + buf.push_str(&part.snippet); + prev_hi = sm.lookup_char_pos(part.span.hi()); + if prev_hi.line == cur_lo.line { + acc += len as isize - (prev_hi.col.0 - cur_lo.col.0) as isize; + } + 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() }); } - 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); From a29a624f86c07dc4bbfed2e278a49b6a7f7dff3e Mon Sep 17 00:00:00 2001 From: Esteban Kuber Date: Sat, 14 Aug 2021 13:31:48 +0000 Subject: [PATCH 2/5] wip --- compiler/rustc_errors/src/emitter.rs | 30 ++++++++++------------------ compiler/rustc_errors/src/lib.rs | 3 ++- 2 files changed, 13 insertions(+), 20 deletions(-) diff --git a/compiler/rustc_errors/src/emitter.rs b/compiler/rustc_errors/src/emitter.rs index 645b81b9540..ba6fe477202 100644 --- a/compiler/rustc_errors/src/emitter.rs +++ b/compiler/rustc_errors/src/emitter.rs @@ -1679,27 +1679,19 @@ impl EmitterWriter { // Colorize addition/replacements with green. for &SubstitutionHighlight { start, end } in highlight_parts { // Account for tabs when highlighting (#87972). - let start: usize = line - .chars() - .take(start) - .map(|ch| match ch { - '\t' => 4, - _ => 1, - }) - .sum(); - - let end: usize = line - .chars() - .take(end) - .map(|ch| match ch { - '\t' => 4, - _ => 1, - }) - .sum(); + // let tabs: usize = line + // .chars() + // .take(start) + // .map(|ch| match ch { + // '\t' => 3, + // _ => 0, + // }) + // .sum(); + let tabs = 0; buffer.set_style_range( row_num, - max_line_num_len + 3 + start, - max_line_num_len + 3 + end, + max_line_num_len + 3 + start + tabs, + max_line_num_len + 3 + end + tabs, Style::Addition, true, ); diff --git a/compiler/rustc_errors/src/lib.rs b/compiler/rustc_errors/src/lib.rs index cae4a6b4723..ab3f0948632 100644 --- a/compiler/rustc_errors/src/lib.rs +++ b/compiler/rustc_errors/src/lib.rs @@ -14,7 +14,7 @@ extern crate rustc_macros; pub use emitter::ColorConfig; -use tracing::debug; +use tracing::{debug, info}; use Level::*; use emitter::{is_case_difference, Emitter, EmitterWriter}; @@ -349,6 +349,7 @@ impl CodeSuggestion { while buf.ends_with('\n') { buf.pop(); } + info!(?buf, ?highlights); Some((buf, substitution.parts, highlights, only_capitalization)) }) .collect() From 5626346ac9c473f015b4dcb0801576ae76656a2f Mon Sep 17 00:00:00 2001 From: Esteban Kuber Date: Tue, 17 Aug 2021 15:46:42 +0000 Subject: [PATCH 3/5] Fixes to span locations --- compiler/rustc_errors/src/emitter.rs | 17 ++++++++--------- compiler/rustc_errors/src/lib.rs | 3 +-- 2 files changed, 9 insertions(+), 11 deletions(-) diff --git a/compiler/rustc_errors/src/emitter.rs b/compiler/rustc_errors/src/emitter.rs index ba6fe477202..25777f4133b 100644 --- a/compiler/rustc_errors/src/emitter.rs +++ b/compiler/rustc_errors/src/emitter.rs @@ -1679,15 +1679,14 @@ impl EmitterWriter { // 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(); - let tabs = 0; + 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, diff --git a/compiler/rustc_errors/src/lib.rs b/compiler/rustc_errors/src/lib.rs index ab3f0948632..a37d969f595 100644 --- a/compiler/rustc_errors/src/lib.rs +++ b/compiler/rustc_errors/src/lib.rs @@ -297,11 +297,11 @@ impl CodeSuggestion { 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 { highlights.push(std::mem::take(&mut line_highlight)); - acc = 0; count -= 1; } // push lines between the previous and current span (if any) @@ -310,7 +310,6 @@ impl CodeSuggestion { buf.push_str(line.as_ref()); buf.push('\n'); highlights.push(std::mem::take(&mut line_highlight)); - acc = 0; } } if let Some(cur_line) = sf.get_line(cur_lo.line - 1) { From 31d07edc94814069a02bea7341edfa35c7068786 Mon Sep 17 00:00:00 2001 From: Esteban Kuber Date: Tue, 17 Aug 2021 16:06:18 +0000 Subject: [PATCH 4/5] remove unnecessary `info!()` logging --- compiler/rustc_errors/src/lib.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/compiler/rustc_errors/src/lib.rs b/compiler/rustc_errors/src/lib.rs index a37d969f595..8c0a9d5aae3 100644 --- a/compiler/rustc_errors/src/lib.rs +++ b/compiler/rustc_errors/src/lib.rs @@ -14,7 +14,7 @@ extern crate rustc_macros; pub use emitter::ColorConfig; -use tracing::{debug, info}; +use tracing::debug; use Level::*; use emitter::{is_case_difference, Emitter, EmitterWriter}; @@ -348,7 +348,6 @@ impl CodeSuggestion { while buf.ends_with('\n') { buf.pop(); } - info!(?buf, ?highlights); Some((buf, substitution.parts, highlights, only_capitalization)) }) .collect() From 955e913612648056853b4021a5d9046775538fd7 Mon Sep 17 00:00:00 2001 From: Esteban Kuber Date: Mon, 23 Aug 2021 12:42:08 +0000 Subject: [PATCH 5/5] review comments --- compiler/rustc_errors/src/lib.rs | 31 ++++++++++++++++++++++++++----- 1 file changed, 26 insertions(+), 5 deletions(-) diff --git a/compiler/rustc_errors/src/lib.rs b/compiler/rustc_errors/src/lib.rs index 8c0a9d5aae3..a48d4fe8bb5 100644 --- a/compiler/rustc_errors/src/lib.rs +++ b/compiler/rustc_errors/src/lib.rs @@ -321,21 +321,42 @@ impl CodeSuggestion { } } // Add a whole line highlight per line in the snippet. - let len = part.snippet.split('\n').next().unwrap_or(&part.snippet).len(); + 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 as isize + acc) as usize, - end: (cur_lo.col.0 as isize + acc + len as isize) as usize, + end: (cur_lo.col.0 as isize + acc + len) as usize, }); buf.push_str(&part.snippet); - prev_hi = sm.lookup_char_pos(part.span.hi()); + let cur_hi = sm.lookup_char_pos(part.span.hi()); if prev_hi.line == cur_lo.line { - acc += len as isize - (prev_hi.col.0 - cur_lo.col.0) as isize; + // 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 }); } } highlights.push(std::mem::take(&mut line_highlight));