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

Rustc explain #48337

Merged
merged 5 commits into from
Feb 26, 2018
Merged

Rustc explain #48337

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
9b597a1
Add rustc --explain back
GuillaumeGomez Feb 18, 2018
1dc2015
Update tools code
GuillaumeGomez Feb 19, 2018
5747fd6
Update ui tests
GuillaumeGomez Feb 19, 2018
16fb6b0
Reduce error codes length when too much are thrown
GuillaumeGomez Feb 24, 2018
ce6429a
Fix new tests
GuillaumeGomez Feb 26, 2018
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
50 changes: 43 additions & 7 deletions src/librustc_errors/emitter.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ use std::io::prelude::*;
use std::io;
use std::rc::Rc;
use term;
use std::collections::HashMap;
use std::collections::{HashMap, HashSet};
use std::cmp::min;
use unicode_width;

Expand Down Expand Up @@ -107,6 +107,7 @@ pub struct EmitterWriter {
cm: Option<Rc<CodeMapper>>,
short_message: bool,
teach: bool,
error_codes: HashSet<String>,
}

struct FileWithAnnotatedLines {
Expand All @@ -115,6 +116,30 @@ struct FileWithAnnotatedLines {
multiline_depth: usize,
}

impl Drop for EmitterWriter {
fn drop(&mut self) {
Copy link
Member

Choose a reason for hiding this comment

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

@GuillaumeGomez (more belated commentary in light of #48550) What was the motivation for putting the this in drop()? Semantically, printing the message notifying the user about --explain does not seem like a "cleanup" action. As Niko suggested, I would kind of expect this to go with the "aborting due to previous error" message in Handler.abort_if_errors—and it looks like the Handler already knows which codes have been emitted, too.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hum, if this is true, it might allow to make my task for #48562.

if !self.short_message && !self.error_codes.is_empty() {
let mut error_codes = self.error_codes.clone().into_iter().collect::<Vec<_>>();
error_codes.sort();
if error_codes.len() > 1 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this restrict the error list to some reasonable amount? I'm ok with any amount, but I'd constrain it to at most two full 80 col lines.

Copy link
Member Author

Choose a reason for hiding this comment

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

Does it really matter? I can do it but I'm not sure if this is very useful... You're not supposed to try to have as much errors as possible when writing code haha.

Copy link
Contributor

Choose a reason for hiding this comment

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

It should be rare, but I see no reason for us not to gracefully handle that case :)

It won't ever be possible for a given project to have more than a subset of the ~600 errors the compiler has, but regardless, it is worthless to list the error codes, once the user understands how to use the flag, it makes more sense to scroll to the error that they want more information on and copy the error code from there, as the error code on it's own is meaningless.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, I'll limit then.

writeln!(self.dst,
"You've got a few errors: {}",
error_codes.join(", ")).expect("failed to give tips...");
writeln!(self.dst,
"If you want more information on an error, try using \
\"rustc --explain {}\"",
&error_codes[0]).expect("failed to give tips...");
} else {
writeln!(self.dst,
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this message show up for errors that don't show any extra information if --explain is run?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure to understand your point...

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry I confused this with the teach flag

"If you want more information on this error, try using \
\"rustc --explain {}\"",
&error_codes[0]).expect("failed to give tips...");
}
self.dst.flush().expect("failed to emit errors");
}
}
}

impl EmitterWriter {
pub fn stderr(color_config: ColorConfig,
code_map: Option<Rc<CodeMapper>>,
Expand All @@ -128,13 +153,15 @@ impl EmitterWriter {
cm: code_map,
short_message,
teach,
error_codes: HashSet::new(),
}
} else {
EmitterWriter {
dst: Raw(Box::new(io::stderr())),
cm: code_map,
short_message,
teach,
error_codes: HashSet::new(),
}
}
}
Expand All @@ -149,6 +176,7 @@ impl EmitterWriter {
cm: code_map,
short_message,
teach,
error_codes: HashSet::new(),
}
}

Expand Down Expand Up @@ -975,12 +1003,14 @@ impl EmitterWriter {
if primary_span != &&DUMMY_SP {
(cm.lookup_char_pos(primary_span.lo()), cm)
} else {
emit_to_destination(&buffer.render(), level, &mut self.dst, self.short_message)?;
emit_to_destination(&buffer.render(), level, &mut self.dst, self.short_message,
&mut self.error_codes)?;
return Ok(());
}
} else {
// If we don't have span information, emit and exit
emit_to_destination(&buffer.render(), level, &mut self.dst, self.short_message)?;
emit_to_destination(&buffer.render(), level, &mut self.dst, self.short_message,
&mut self.error_codes)?;
return Ok(());
};
if let Ok(pos) =
Expand Down Expand Up @@ -1153,7 +1183,8 @@ impl EmitterWriter {
}

// final step: take our styled buffer, render it, then output it
emit_to_destination(&buffer.render(), level, &mut self.dst, self.short_message)?;
emit_to_destination(&buffer.render(), level, &mut self.dst, self.short_message,
&mut self.error_codes)?;

Ok(())

Expand Down Expand Up @@ -1241,7 +1272,8 @@ impl EmitterWriter {
let msg = format!("and {} other candidates", suggestions.len() - MAX_SUGGESTIONS);
buffer.puts(row_num, 0, &msg, Style::NoStyle);
}
emit_to_destination(&buffer.render(), level, &mut self.dst, self.short_message)?;
emit_to_destination(&buffer.render(), level, &mut self.dst, self.short_message,
&mut self.error_codes)?;
}
Ok(())
}
Expand Down Expand Up @@ -1269,7 +1301,7 @@ impl EmitterWriter {
draw_col_separator_no_space(&mut buffer, 0, max_line_num_len + 1);
}
match emit_to_destination(&buffer.render(), level, &mut self.dst,
self.short_message) {
self.short_message, &mut self.error_codes) {
Ok(()) => (),
Err(e) => panic!("failed to emit error: {}", e)
}
Expand Down Expand Up @@ -1362,7 +1394,8 @@ fn overlaps(a1: &Annotation, a2: &Annotation, padding: usize) -> bool {
fn emit_to_destination(rendered_buffer: &Vec<Vec<StyledString>>,
lvl: &Level,
dst: &mut Destination,
short_message: bool)
short_message: bool,
error_codes: &mut HashSet<String>)
-> io::Result<()> {
use lock;

Expand All @@ -1383,6 +1416,9 @@ fn emit_to_destination(rendered_buffer: &Vec<Vec<StyledString>>,
for part in line {
dst.apply_style(lvl.clone(), part.style)?;
write!(dst, "{}", part.text)?;
if !short_message && part.text.len() == 12 && part.text.starts_with("error[E") {
error_codes.insert(part.text[6..11].to_owned());
}
dst.reset_attrs()?;
}
if !short_message {
Expand Down
19 changes: 0 additions & 19 deletions src/tools/compiletest/src/json.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,25 +57,6 @@ struct DiagnosticCode {
explanation: Option<String>,
}

pub fn extract_rendered(output: &str, proc_res: &ProcRes) -> String {
output.lines()
.filter_map(|line| if line.starts_with('{') {
match json::decode::<Diagnostic>(line) {
Ok(diagnostic) => diagnostic.rendered,
Err(error) => {
proc_res.fatal(Some(&format!("failed to decode compiler output as json: \
`{}`\noutput: {}\nline: {}",
error,
line,
output)));
}
}
} else {
None
})
.collect()
}

pub fn parse_output(file_name: &str, output: &str, proc_res: &ProcRes) -> Vec<Error> {
output.lines()
.flat_map(|line| parse_line(file_name, line, output, proc_res))
Expand Down
40 changes: 21 additions & 19 deletions src/tools/compiletest/src/runtest.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -248,7 +248,7 @@ impl<'test> TestCx<'test> {
}

fn run_cfail_test(&self) {
let proc_res = self.compile_test();
let proc_res = self.compile_test(&[]);
self.check_if_test_should_compile(&proc_res);
self.check_no_compiler_crash(&proc_res);

Expand All @@ -267,7 +267,7 @@ impl<'test> TestCx<'test> {
}

fn run_rfail_test(&self) {
let proc_res = self.compile_test();
let proc_res = self.compile_test(&[]);

if !proc_res.status.success() {
self.fatal_proc_rec("compilation failed!", &proc_res);
Expand Down Expand Up @@ -309,7 +309,7 @@ impl<'test> TestCx<'test> {
}

fn run_rpass_test(&self) {
let proc_res = self.compile_test();
let proc_res = self.compile_test(&[]);

if !proc_res.status.success() {
self.fatal_proc_rec("compilation failed!", &proc_res);
Expand All @@ -336,7 +336,7 @@ impl<'test> TestCx<'test> {
return self.run_rpass_test();
}

let mut proc_res = self.compile_test();
let mut proc_res = self.compile_test(&[]);

if !proc_res.status.success() {
self.fatal_proc_rec("compilation failed!", &proc_res);
Expand Down Expand Up @@ -578,7 +578,7 @@ impl<'test> TestCx<'test> {
let mut cmds = commands.join("\n");

// compile test file (it should have 'compile-flags:-g' in the header)
let compiler_run_result = self.compile_test();
let compiler_run_result = self.compile_test(&[]);
if !compiler_run_result.status.success() {
self.fatal_proc_rec("compilation failed!", &compiler_run_result);
}
Expand Down Expand Up @@ -835,7 +835,7 @@ impl<'test> TestCx<'test> {

fn run_debuginfo_lldb_test_no_opt(&self) {
// compile test file (it should have 'compile-flags:-g' in the header)
let compile_result = self.compile_test();
let compile_result = self.compile_test(&[]);
if !compile_result.status.success() {
self.fatal_proc_rec("compilation failed!", &compile_result);
}
Expand Down Expand Up @@ -1272,12 +1272,15 @@ impl<'test> TestCx<'test> {
}
}

fn compile_test(&self) -> ProcRes {
fn compile_test(&self, extra_args: &[&'static str]) -> ProcRes {
let mut rustc = self.make_compile_args(
&self.testpaths.file,
TargetLocation::ThisFile(self.make_exe_name()),
);

if !extra_args.is_empty() {
rustc.args(extra_args);
}
rustc.arg("-L").arg(&self.aux_output_dir_name());

match self.config.mode {
Expand Down Expand Up @@ -1629,8 +1632,11 @@ impl<'test> TestCx<'test> {
.iter()
.any(|s| s.starts_with("--error-format"))
{
rustc.args(&["--error-format", "json"]);
},
// In case no "--error-format" has been given in the test, we'll compile
// a first time to get the compiler's output then compile with
// "--error-format json" to check if all expected errors are actually there
// and that no new one appeared.
}
MirOpt => {
rustc.args(&[
"-Zdump-mir=all",
Expand Down Expand Up @@ -2109,7 +2115,7 @@ impl<'test> TestCx<'test> {
fn run_codegen_units_test(&self) {
assert!(self.revision.is_none(), "revisions not relevant here");

let proc_res = self.compile_test();
let proc_res = self.compile_test(&[]);

if !proc_res.status.success() {
self.fatal_proc_rec("compilation failed!", &proc_res);
Expand Down Expand Up @@ -2493,7 +2499,7 @@ impl<'test> TestCx<'test> {
.iter()
.any(|s| s.contains("--error-format"));

let proc_res = self.compile_test();
let proc_res = self.compile_test(&[]);
self.check_if_test_should_compile(&proc_res);

let expected_stderr_path = self.expected_output_path(UI_STDERR);
Expand All @@ -2505,13 +2511,8 @@ impl<'test> TestCx<'test> {
let normalized_stdout =
self.normalize_output(&proc_res.stdout, &self.props.normalize_stdout);

let stderr = if explicit {
proc_res.stderr.clone()
} else {
json::extract_rendered(&proc_res.stderr, &proc_res)
};

let normalized_stderr = self.normalize_output(&stderr, &self.props.normalize_stderr);
let normalized_stderr = self.normalize_output(&proc_res.stderr,
&self.props.normalize_stderr);

let mut errors = 0;
errors += self.compare_output("stdout", &normalized_stdout, &expected_stdout);
Expand Down Expand Up @@ -2544,6 +2545,7 @@ impl<'test> TestCx<'test> {
}
}
if !explicit {
let proc_res = self.compile_test(&["--error-format", "json"]);
Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, let's just compile all the tests twice.
Who cares about time, certainly not Appveyor.

if !expected_errors.is_empty() || !proc_res.status.success() {
// "// error-pattern" comments
self.check_expected_errors(expected_errors, &proc_res);
Expand All @@ -2555,7 +2557,7 @@ impl<'test> TestCx<'test> {
}

fn run_mir_opt_test(&self) {
let proc_res = self.compile_test();
let proc_res = self.compile_test(&[]);

if !proc_res.status.success() {
self.fatal_proc_rec("compilation failed!", &proc_res);
Expand Down