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.

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

Replace tabs earlier in diagnostics #79757

Merged
merged 2 commits into from
Jan 12, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
25 changes: 20 additions & 5 deletions compiler/rustc_errors/src/emitter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -644,6 +644,8 @@ impl EmitterWriter {
code_offset: usize,
margin: Margin,
) {
// Tabs are assumed to have been replaced by spaces in calling code.
assert!(!source_string.contains('\t'));
Comment on lines +647 to +648
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a bit heavy handed but ok with me (I will assume we'll see reports on nightly if this causes issues before it hits stable).

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 okay, mostly I just wanted it to be ultra clear to future code readers which paths should not have tabs, but it could be relaxed to a comment of course. I'll check json output now.

let line_len = source_string.len();
// Create the source line we will highlight.
let left = margin.left(line_len);
Expand Down Expand Up @@ -707,7 +709,7 @@ impl EmitterWriter {
}

let source_string = match file.get_line(line.line_index - 1) {
Some(s) => s,
Some(s) => replace_tabs(&*s),
None => return Vec::new(),
};

Expand Down Expand Up @@ -1376,8 +1378,17 @@ impl EmitterWriter {
let file = annotated_file.file.clone();
let line = &annotated_file.lines[line_idx];
if let Some(source_string) = file.get_line(line.line_index - 1) {
let leading_whitespace =
source_string.chars().take_while(|c| c.is_whitespace()).count();
let leading_whitespace = source_string
.chars()
.take_while(|c| c.is_whitespace())
.map(|c| {
match c {
// Tabs are displayed as 4 spaces
'\t' => 4,
_ => 1,
jryans marked this conversation as resolved.
Show resolved Hide resolved
}
})
.sum();
if source_string.chars().any(|c| !c.is_whitespace()) {
whitespace_margin = min(whitespace_margin, leading_whitespace);
}
Expand Down Expand Up @@ -1502,7 +1513,7 @@ impl EmitterWriter {

self.draw_line(
&mut buffer,
&unannotated_line,
&replace_tabs(&unannotated_line),
annotated_file.lines[line_idx + 1].line_index - 1,
last_buffer_line_num,
width_offset,
Expand Down Expand Up @@ -1598,7 +1609,7 @@ impl EmitterWriter {
);
// print the suggestion
draw_col_separator(&mut buffer, row_num, max_line_num_len + 1);
buffer.append(row_num, line, Style::NoStyle);
buffer.append(row_num, &replace_tabs(line), Style::NoStyle);
row_num += 1;
}

Expand Down Expand Up @@ -1930,6 +1941,10 @@ impl FileWithAnnotatedLines {
}
}

fn replace_tabs(str: &str) -> String {
str.replace('\t', " ")
}

fn draw_col_separator(buffer: &mut StyledBuffer, line: usize, col: usize) {
buffer.puts(line, col, "| ", Style::LineNumber);
}
Expand Down
27 changes: 3 additions & 24 deletions compiler/rustc_errors/src/styled_buffer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,34 +13,13 @@ impl StyledBuffer {
StyledBuffer { text: vec![], styles: vec![] }
}

fn replace_tabs(&mut self) {
for (line_pos, line) in self.text.iter_mut().enumerate() {
let mut tab_pos = vec![];
for (pos, c) in line.iter().enumerate() {
if *c == '\t' {
tab_pos.push(pos);
}
}
// start with the tabs at the end of the line to replace them with 4 space chars
for pos in tab_pos.iter().rev() {
assert_eq!(line.remove(*pos), '\t');
// fix the position of the style to match up after replacing the tabs
let s = self.styles[line_pos].remove(*pos);
for _ in 0..4 {
line.insert(*pos, ' ');
self.styles[line_pos].insert(*pos, s);
}
Comment on lines -28 to -32
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm slightly concerned by this removal. Have you tried to see the output in your local CLI? I need to double check the code, but can you confirm that the underlines are all colored correctly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe this is safe, since the style handling here is about adjusting them for the adding tabs, but we now handle the tabs earlier on, so there's no need to adjust at this stage any more.

The output looks correct to me as well. Here are a few examples:

image

image

}
}
}
pub fn render(&self) -> Vec<Vec<StyledString>> {
// Tabs are assumed to have been replaced by spaces in calling code.
assert!(self.text.iter().all(|r| !r.contains(&'\t')));

pub fn render(&mut self) -> Vec<Vec<StyledString>> {
let mut output: Vec<Vec<StyledString>> = vec![];
let mut styled_vec: Vec<StyledString> = vec![];

// before we render, replace tabs with spaces
self.replace_tabs();

for (row, row_style) in self.text.iter().zip(&self.styles) {
let mut current_style = Style::NoStyle;
let mut current_text = String::new();
Expand Down
13 changes: 13 additions & 0 deletions src/test/ui/terminal-width/tabs-trimming.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
// Test for #78438: ensure underline alignment with many tabs on the left, long line on the right

// ignore-tidy-linelength
// ignore-tidy-tab

fn main() {
let money = 42i32;
match money {
v @ 1 | 2 | 3 => panic!("You gave me too little money {}", v), // Long text here: TTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTT
//~^ ERROR variable `v` is not bound in all patterns
v => println!("Enough money {}", v),
}
}
12 changes: 12 additions & 0 deletions src/test/ui/terminal-width/tabs-trimming.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
error[E0408]: variable `v` is not bound in all patterns
--> $DIR/tabs-trimming.rs:9:16
|
LL | ... v @ 1 | 2 | 3 => panic!("You gave me too little money {}", v), // Long text here: TTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTTT...
jryans marked this conversation as resolved.
Show resolved Hide resolved
| - ^ ^ pattern doesn't bind `v`
| | |
| | pattern doesn't bind `v`
| variable not in all patterns

error: aborting due to previous error

For more information about this error, try `rustc --explain E0408`.