Skip to content

compiletest: Improve diagnostics for line annotation mismatches #140622

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

compiletest: Improve diagnostics for line annotation mismatches #140622

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6acd815
compiletest: Improve diagnostics for line annotation mismatches
petrochenkov May 3, 2025
776730e
WIP
petrochenkov May 21, 2025
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
46 changes: 23 additions & 23 deletions src/tools/compiletest/src/errors.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ pub enum ErrorKind {
Suggestion,
Warning,
Raw,
/// Used for better recovery and diagnostics in compiletest.
Unknown,
}

impl ErrorKind {
Expand All @@ -31,21 +33,25 @@ impl ErrorKind {

/// Either the canonical uppercase string, or some additional versions for compatibility.
/// FIXME: consider keeping only the canonical versions here.
pub fn from_user_str(s: &str) -> ErrorKind {
match s {
fn from_user_str(s: &str) -> Option<ErrorKind> {
Some(match s {
"HELP" | "help" => ErrorKind::Help,
"ERROR" | "error" => ErrorKind::Error,
// `MONO_ITEM` makes annotations in `codegen-units` tests syntactically correct,
// but those tests never use the error kind later on.
"NOTE" | "note" | "MONO_ITEM" => ErrorKind::Note,
"NOTE" | "note" => ErrorKind::Note,
"SUGGESTION" => ErrorKind::Suggestion,
"WARN" | "WARNING" | "warn" | "warning" => ErrorKind::Warning,
"RAW" => ErrorKind::Raw,
_ => panic!(
_ => return None,
})
}

pub fn expect_from_user_str(s: &str) -> ErrorKind {
ErrorKind::from_user_str(s).unwrap_or_else(|| {
panic!(
"unexpected diagnostic kind `{s}`, expected \
`ERROR`, `WARN`, `NOTE`, `HELP` or `SUGGESTION`"
),
}
`ERROR`, `WARN`, `NOTE`, `HELP`, `SUGGESTION` or `RAW`"
)
})
}
}

Expand All @@ -58,13 +64,15 @@ impl fmt::Display for ErrorKind {
ErrorKind::Suggestion => write!(f, "SUGGESTION"),
ErrorKind::Warning => write!(f, "WARN"),
ErrorKind::Raw => write!(f, "RAW"),
ErrorKind::Unknown => write!(f, "UNKNOWN"),
}
}
}

#[derive(Debug)]
pub struct Error {
pub line_num: Option<usize>,
pub column_num: Option<usize>,
/// What kind of message we expect (e.g., warning, error, suggestion).
pub kind: ErrorKind,
pub msg: String,
Expand All @@ -74,17 +82,6 @@ pub struct Error {
pub require_annotation: bool,
}

impl Error {
pub fn render_for_expected(&self) -> String {
use colored::Colorize;
format!("{: <10}line {: >3}: {}", self.kind, self.line_num_str(), self.msg.cyan())
}

pub fn line_num_str(&self) -> String {
self.line_num.map_or("?".to_string(), |line_num| line_num.to_string())
}
}

/// Looks for either "//~| KIND MESSAGE" or "//~^^... KIND MESSAGE"
/// The former is a "follow" that inherits its target from the preceding line;
/// the latter is an "adjusts" that goes that many lines up.
Expand Down Expand Up @@ -168,8 +165,10 @@ fn parse_expected(
let rest = line[tag.end()..].trim_start();
let (kind_str, _) =
rest.split_once(|c: char| c != '_' && !c.is_ascii_alphabetic()).unwrap_or((rest, ""));
let kind = ErrorKind::from_user_str(kind_str);
let untrimmed_msg = &rest[kind_str.len()..];
let (kind, untrimmed_msg) = match ErrorKind::from_user_str(kind_str) {
Some(kind) => (kind, &rest[kind_str.len()..]),
None => (ErrorKind::Unknown, rest),
};
let msg = untrimmed_msg.strip_prefix(':').unwrap_or(untrimmed_msg).trim().to_owned();

let line_num_adjust = &captures["adjust"];
Expand All @@ -182,6 +181,7 @@ fn parse_expected(
} else {
(false, Some(line_num - line_num_adjust.len()))
};
let column_num = Some(tag.start() + 1);

debug!(
"line={:?} tag={:?} follow_prev={:?} kind={:?} msg={:?}",
Expand All @@ -191,7 +191,7 @@ fn parse_expected(
kind,
msg
);
Some((follow_prev, Error { line_num, kind, msg, require_annotation: true }))
Some((follow_prev, Error { line_num, column_num, kind, msg, require_annotation: true }))
}

#[cfg(test)]
Expand Down
2 changes: 1 addition & 1 deletion src/tools/compiletest/src/header.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -593,7 +593,7 @@ impl TestProps {
config.parse_name_value_directive(ln, DONT_REQUIRE_ANNOTATIONS)
{
self.dont_require_annotations
.insert(ErrorKind::from_user_str(err_kind.trim()));
.insert(ErrorKind::expect_from_user_str(err_kind.trim()));
}
},
);
Expand Down
40 changes: 15 additions & 25 deletions src/tools/compiletest/src/json.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,9 +36,7 @@ struct UnusedExternNotification {
struct DiagnosticSpan {
file_name: String,
line_start: usize,
line_end: usize,
column_start: usize,
column_end: usize,
is_primary: bool,
label: Option<String>,
suggested_replacement: Option<String>,
Expand Down Expand Up @@ -148,6 +146,7 @@ pub fn parse_output(file_name: &str, output: &str) -> Vec<Error> {
Ok(diagnostic) => push_actual_errors(&mut errors, &diagnostic, &[], file_name),
Err(_) => errors.push(Error {
line_num: None,
column_num: None,
kind: ErrorKind::Raw,
msg: line.to_string(),
require_annotation: false,
Expand Down Expand Up @@ -193,25 +192,9 @@ fn push_actual_errors(
// also ensure that `//~ ERROR E123` *always* works. The
// assumption is that these multi-line error messages are on their
// way out anyhow.
let with_code = |span: Option<&DiagnosticSpan>, text: &str| {
// FIXME(#33000) -- it'd be better to use a dedicated
// UI harness than to include the line/col number like
// this, but some current tests rely on it.
//
// Note: Do NOT include the filename. These can easily
// cause false matches where the expected message
// appears in the filename, and hence the message
// changes but the test still passes.
let span_str = match span {
Some(DiagnosticSpan { line_start, column_start, line_end, column_end, .. }) => {
format!("{line_start}:{column_start}: {line_end}:{column_end}")
}
None => format!("?:?: ?:?"),
};
match &diagnostic.code {
Some(code) => format!("{span_str}: {text} [{}]", code.code),
None => format!("{span_str}: {text}"),
}
let with_code = |text| match &diagnostic.code {
Some(code) => format!("{text} [{}]", code.code),
None => format!("{text}"),
};

// Convert multi-line messages into multiple errors.
Expand All @@ -225,17 +208,19 @@ fn push_actual_errors(
|| Regex::new(r"aborting due to \d+ previous errors?|\d+ warnings? emitted").unwrap();
errors.push(Error {
line_num: None,
column_num: None,
kind,
msg: with_code(None, first_line),
msg: with_code(first_line),
require_annotation: diagnostic.level != "failure-note"
&& !RE.get_or_init(re_init).is_match(first_line),
});
} else {
for span in primary_spans {
errors.push(Error {
line_num: Some(span.line_start),
column_num: Some(span.column_start),
kind,
msg: with_code(Some(span), first_line),
msg: with_code(first_line),
require_annotation: true,
});
}
Expand All @@ -244,16 +229,18 @@ fn push_actual_errors(
if primary_spans.is_empty() {
errors.push(Error {
line_num: None,
column_num: None,
kind,
msg: with_code(None, next_line),
msg: with_code(next_line),
require_annotation: false,
});
} else {
for span in primary_spans {
errors.push(Error {
line_num: Some(span.line_start),
column_num: Some(span.column_start),
kind,
msg: with_code(Some(span), next_line),
msg: with_code(next_line),
require_annotation: false,
});
}
Expand All @@ -266,6 +253,7 @@ fn push_actual_errors(
for (index, line) in suggested_replacement.lines().enumerate() {
errors.push(Error {
line_num: Some(span.line_start + index),
column_num: Some(span.column_start),
kind: ErrorKind::Suggestion,
msg: line.to_string(),
// Empty suggestions (suggestions to remove something) are common
Expand All @@ -288,6 +276,7 @@ fn push_actual_errors(
if let Some(label) = &span.label {
errors.push(Error {
line_num: Some(span.line_start),
column_num: Some(span.column_start),
kind: ErrorKind::Note,
msg: label.clone(),
// Empty labels (only underlining spans) are common and do not need annotations.
Expand All @@ -310,6 +299,7 @@ fn push_backtrace(
if Path::new(&expansion.span.file_name) == Path::new(&file_name) {
errors.push(Error {
line_num: Some(expansion.span.line_start),
column_num: Some(expansion.span.column_start),
kind: ErrorKind::Note,
msg: format!("in this expansion of {}", expansion.macro_decl_name),
require_annotation: true,
Expand Down
85 changes: 62 additions & 23 deletions src/tools/compiletest/src/runtest.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -704,6 +704,7 @@ impl<'test> TestCx<'test> {
.map(|e| Error { msg: self.normalize_output(&e.msg, &[]), ..e });

let mut unexpected = Vec::new();
let mut unimportant = Vec::new();
let mut found = vec![false; expected_errors.len()];
for actual_error in actual_errors {
for pattern in &self.props.error_patterns {
Expand Down Expand Up @@ -738,14 +739,9 @@ impl<'test> TestCx<'test> {
&& expected_kinds.contains(&actual_error.kind)
&& !self.props.dont_require_annotations.contains(&actual_error.kind)
{
self.error(&format!(
"{}:{}: unexpected {}: '{}'",
file_name,
actual_error.line_num_str(),
actual_error.kind,
actual_error.msg
));
unexpected.push(actual_error);
} else {
unimportant.push(actual_error);
}
}
}
Expand All @@ -755,39 +751,83 @@ impl<'test> TestCx<'test> {
// anything not yet found is a problem
for (index, expected_error) in expected_errors.iter().enumerate() {
if !found[index] {
self.error(&format!(
"{}:{}: expected {} not found: {}",
file_name,
expected_error.line_num_str(),
expected_error.kind,
expected_error.msg
));
not_found.push(expected_error);
}
}

if !unexpected.is_empty() || !not_found.is_empty() {
self.error(&format!(
"{} unexpected errors found, {} expected errors not found",
"{} unexpected diagnostics reported, {} expected diagnostics not reported",
unexpected.len(),
not_found.len()
));
println!("status: {}\ncommand: {}\n", proc_res.status, proc_res.cmdline);
let print = |e: &Error| {
let line_num = e.line_num.map_or("?".to_string(), |line_num| line_num.to_string());
// `file:?:NUM` may be confusing to editors and unclickable.
let opt_col_num = match e.column_num {
Some(col_num) if line_num != "?" => format!("{col_num}:"),
_ => "".to_string(),
};
println!("{file_name}:{line_num}:{opt_col_num} {}: {}", e.kind, e.msg.cyan())
};
// Fuzzy matching quality:
// - message and line / message and kind - great, suggested
// - only message - good, suggested
// - known line and kind - ok, suggested
// - only known line - meh, but suggested
// - others are not worth suggesting
if !unexpected.is_empty() {
println!("{}", "--- unexpected errors (from JSON output) ---".green());
println!("{}", "--- reported but not expected (from JSON output) ---".green());
for error in &unexpected {
println!("{}", error.render_for_expected());
print(error);
for candidate in &not_found {
if error.msg.contains(&candidate.msg) {
let prefix = if candidate.line_num != error.line_num {
"expected on a different line"
} else {
"expected with a different kind"
}
.red();
print!(" {prefix}: ");
print(candidate);
} else if candidate.line_num.is_some()
&& candidate.line_num == error.line_num
{
print!(" {}: ", "expected with a different message".red());
print(candidate);
}
}
}
println!("{}", "---".green());
}
if !not_found.is_empty() {
println!("{}", "--- not found errors (from test file) ---".red());
println!("{}", "--- expected but not reported (from test file) ---".red());
for error in &not_found {
println!("{}", error.render_for_expected());
print(error);
for candidate in unexpected.iter().chain(&unimportant) {
if candidate.msg.contains(&error.msg) {
let prefix = if candidate.line_num != error.line_num {
"reported on a different line"
} else {
"reported with a different kind"
}
.green();
print!(" {prefix}: ");
print(candidate);
} else if candidate.line_num.is_some()
&& candidate.line_num == error.line_num
{
print!(" {}: ", "reported with a different message".green());
print(candidate);
}
}
}
println!("{}", "---\n".red());
println!("{}", "---".red());
}
panic!("errors differ from expected");
panic!(
"errors differ from expected\nstatus: {}\ncommand: {}\n",
proc_res.status, proc_res.cmdline
);
}
}

Expand Down Expand Up @@ -2073,7 +2113,6 @@ impl<'test> TestCx<'test> {
println!("{}", String::from_utf8_lossy(&output.stdout));
eprintln!("{}", String::from_utf8_lossy(&output.stderr));
} else {
use colored::Colorize;
eprintln!("warning: no pager configured, falling back to unified diff");
eprintln!(
"help: try configuring a git pager (e.g. `delta`) with `git config --global core.pager delta`"
Expand Down
Loading
Loading