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

Style changes to compiler messages #29989

Closed
wants to merge 1 commit into from
Closed

Style changes to compiler messages #29989

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion src/libsyntax/codemap.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -799,7 +799,7 @@ impl CodeMap {

let lo = self.lookup_char_pos_adj(sp.lo);
let hi = self.lookup_char_pos_adj(sp.hi);
return (format!("{}:{}:{}: {}:{}",
return (format!("{}: {}:{} -> {}:{}",
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 space is added only to separate the span from the file name (for visibility).
The colon has been replaced with an arrow, because we already use it to separate the line number from the character number.

Copy link
Member

Choose a reason for hiding this comment

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

I think the first space shouldn't be there. Many editors let you say open foo.rs:1:2 on the command line and will open focused on that line. It's easier to copy-paste if there is no space.

Copy link
Member

Choose a reason for hiding this comment

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

I would not use -> since we use it for function types/signatures. I'd use to or ... or keep the :

lo.filename,
lo.line,
lo.col.to_usize() + 1,
Expand Down
33 changes: 9 additions & 24 deletions src/libsyntax/diagnostic.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -293,8 +293,8 @@ impl fmt::Display for Level {

match *self {
Bug => "error: internal compiler error".fmt(f),
Fatal | Error => "error".fmt(f),
Warning => "warning".fmt(f),
Fatal | Error => "\n error".fmt(f),
Warning => "\n warning".fmt(f),
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've put this here only for a preview. If we're gonna avoid the file path (in a similar fashion) for the other messages too, then we can put it directly into the function.

But, if we're going for relative paths, then this won't look nice, and this should be reverted back. So, what's the plan?

Copy link
Member

Choose a reason for hiding this comment

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

I really like the relative paths because printing the full path leaks paths that don't exist, ie, to the buildbot that the stdlib was built on. This is annoying both because it is a nonexistant path, but because it includes "slave", which is really awkward.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed! I'm happy about relative paths :)

Copy link
Member

Choose a reason for hiding this comment

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

One slight issue with relative paths is that when cargo spits out errors for multiple deps it's confusing which dep is being talked about. Though I'm not sure if this will still be an issue.

Copy link
Member

Choose a reason for hiding this comment

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

I like absolute paths when building locally - I know I can always copy and paste and it will work - with relative paths I have to be in the right place, which with a complex build system doesn't always happen.

Copy link
Member

Choose a reason for hiding this comment

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

(Also, I think the /slave/foo thing only happens in assertions, not in compiler errors. In compiler errors that only turns up when a Rust PR fails buildbot which is okay)

Note => "note".fmt(f),
Help => "help".fmt(f),
}
Expand Down Expand Up @@ -550,32 +550,17 @@ impl EmitterWriter {
// Display only the first MAX_LINES lines.
let all_lines = lines.lines.len();
let display_lines = cmp::min(all_lines, MAX_LINES);
let display_line_infos = &lines.lines[..display_lines];
let display_line_strings = &line_strings[..display_lines];

// Calculate the widest number to format evenly and fix #11715
assert!(display_line_infos.len() > 0);
let mut max_line_num = display_line_infos[display_line_infos.len() - 1].line_index + 1;
let mut digits = 0;
while max_line_num > 0 {
max_line_num /= 10;
digits += 1;
}

assert!(display_line_strings.len() > 0);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since our idea is to drop displaying file paths for the latter part of the span, we'd be reverting the patches made for #11715 and some of its sub-issues. Is that okay?

Copy link
Member

Choose a reason for hiding this comment

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

We'll be preserving the old behavior via a command line flag, so this stays in the old-behavior method?

// Print the offending lines
for (line_info, line) in display_line_infos.iter().zip(display_line_strings) {
try!(write!(&mut self.dst, "{}:{:>width$} {}\n",
fm.name,
line_info.line_index + 1,
line,
width=digits));
for line in display_line_strings {
try!(write!(&mut self.dst, " {}\n", line));
}

// If we elided something, put an ellipsis.
if display_lines < all_lines {
let last_line_index = display_line_infos.last().unwrap().line_index;
let s = format!("{}:{} ", fm.name, last_line_index + 1);
try!(write!(&mut self.dst, "{0:1$}...\n", "", s.len()));
try!(write!(&mut self.dst, " ...\n"));
}

// FIXME (#3260)
Expand All @@ -591,7 +576,7 @@ impl EmitterWriter {
let mut s = String::new();
// Skip is the number of characters we need to skip because they are
// part of the 'filename:line ' part of the previous line.
let skip = fm.name.chars().count() + digits + 3;
let skip = digits + 4;
for _ in 0..skip {
s.push(' ');
}
Expand All @@ -608,7 +593,7 @@ impl EmitterWriter {
// position.
match ch {
'\t' => {
col += 8 - col%8;
col += 8 - col % 8;
s.push('\t');
},
_ => {
Expand All @@ -622,7 +607,7 @@ impl EmitterWriter {
let mut s = String::from("^");
let count = match lastc {
// Most terminals have a tab stop every eight columns by default
'\t' => 8 - col%8,
'\t' => 8 - col % 8,
_ => 1,
};
col += count;
Expand Down