Skip to content

compiletest: Make diagnostic kind mandatory on line annotations #139427

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

Closed
wants to merge 1 commit into from
Closed
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
45 changes: 21 additions & 24 deletions src/tools/compiletest/src/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,24 +30,18 @@ impl ErrorKind {

/// Either the canonical uppercase string, or some additional versions for compatibility.
/// FIXME: consider keeping only the canonical versions here.
fn from_user_str(s: &str) -> Option<ErrorKind> {
Some(match s {
"HELP" | "help" => ErrorKind::Help,
pub fn from_user_str(s: &str) -> ErrorKind {
match s {
"HELP" | "help" | "HELP_NONVIRAL" => ErrorKind::Help,
"ERROR" | "error" => ErrorKind::Error,
"NOTE" | "note" => ErrorKind::Note,
"NOTE" | "note" | "NOTE_NONVIRAL" => ErrorKind::Note,
"SUGGESTION" => ErrorKind::Suggestion,
"WARN" | "WARNING" | "warn" | "warning" => ErrorKind::Warning,
_ => return None,
})
}

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

Expand All @@ -67,24 +61,22 @@ impl fmt::Display for ErrorKind {
pub struct Error {
pub line_num: Option<usize>,
/// What kind of message we expect (e.g., warning, error, suggestion).
/// `None` if not specified or unknown message kind.
pub kind: Option<ErrorKind>,
pub kind: ErrorKind,
pub msg: String,
/// For some `Error`s, like secondary lines of multi-line diagnostics, line annotations
/// are not mandatory, even if they would otherwise be mandatory for primary errors.
/// Only makes sense for "actual" errors, not for "expected" errors.
pub require_annotation: bool,
/// A temporary measure to avoid adding too many `NOTE` and `HELP` annotations in #NNNNNN.
/// Only makes sense for "expected" errors, not for "found" errors.
pub viral: bool,
pub parent: Option<usize>,
}

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

pub fn line_num_str(&self) -> String {
Expand Down Expand Up @@ -173,9 +165,11 @@ fn parse_expected(
// Get the part of the comment after the sigil (e.g. `~^^` or ~|).
let tag = captures.get(0).unwrap();
let rest = line[tag.end()..].trim_start();
let (kind_str, _) = rest.split_once(|c: char| !c.is_ascii_alphabetic()).unwrap_or((rest, ""));
let (kind_str, _) =
rest.split_once(|c: char| c != '_' && !c.is_ascii_alphabetic()).unwrap_or((rest, ""));
let viral = kind_str != "NOTE_NONVIRAL" && kind_str != "HELP_NONVIRAL";
let kind = ErrorKind::from_user_str(kind_str);
let untrimmed_msg = if kind.is_some() { &rest[kind_str.len()..] } else { rest };
let untrimmed_msg = &rest[kind_str.len()..];
let msg = untrimmed_msg.strip_prefix(':').unwrap_or(untrimmed_msg).trim().to_owned();

let line_num_adjust = &captures["adjust"];
Expand All @@ -197,7 +191,10 @@ fn parse_expected(
kind,
msg
);
Some((follow_prev, Error { line_num, kind, msg, require_annotation: true }))
Some((
follow_prev,
Error { line_num, kind, msg, require_annotation: true, viral, parent: None },
))
}

#[cfg(test)]
Expand Down
3 changes: 1 addition & 2 deletions src/tools/compiletest/src/header.rs
Original file line number Diff line number Diff line change
Expand Up @@ -585,8 +585,7 @@ impl TestProps {
if let Some(err_kind) =
config.parse_name_value_directive(ln, DONT_REQUIRE_ANNOTATIONS)
{
self.require_annotations
.insert(ErrorKind::expect_from_user_str(&err_kind), false);
self.require_annotations.insert(ErrorKind::from_user_str(&err_kind), false);
}
},
);
Expand Down
41 changes: 33 additions & 8 deletions src/tools/compiletest/src/json.rs
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,9 @@ pub fn parse_output(file_name: &str, output: &str, proc_res: &ProcRes) -> Vec<Er
// output. This hack just skips over such lines. Yuck.
if line.starts_with('{') {
match serde_json::from_str::<Diagnostic>(line) {
Ok(diagnostic) => push_actual_errors(&mut errors, &diagnostic, &[], file_name),
Ok(diagnostic) => {
push_actual_errors(&mut errors, &diagnostic, &[], file_name, None)
}
Err(error) => {
// Ignore the future compat report message - this is handled
// by `extract_rendered`
Expand All @@ -172,6 +174,7 @@ fn push_actual_errors(
diagnostic: &Diagnostic,
default_spans: &[&DiagnosticSpan],
file_name: &str,
prev_parent: Option<usize>,
) {
// In case of macro expansions, we need to get the span of the callsite
let spans_info_in_this_file: Vec<_> = diagnostic
Expand Down Expand Up @@ -229,26 +232,37 @@ fn push_actual_errors(
// Convert multi-line messages into multiple errors.
// We expect to replace these with something more structured anyhow.
let mut message_lines = diagnostic.message.lines();
let kind = Some(ErrorKind::from_compiler_str(&diagnostic.level));
let kind = ErrorKind::from_compiler_str(&diagnostic.level);
let first_line = message_lines.next().unwrap_or(&diagnostic.message);
let mut parent = None;
if primary_spans.is_empty() {
static RE: OnceLock<Regex> = OnceLock::new();
let re_init =
|| Regex::new(r"aborting due to \d+ previous errors?|\d+ warnings? emitted").unwrap();
if parent.is_none() {
parent = Some(errors.len());
}
errors.push(Error {
line_num: None,
kind,
msg: with_code(None, first_line),
require_annotation: diagnostic.level != "failure-note"
&& !RE.get_or_init(re_init).is_match(first_line),
viral: true,
parent: prev_parent,
});
} else {
for span in primary_spans {
if parent.is_none() {
parent = Some(errors.len());
}
errors.push(Error {
line_num: Some(span.line_start),
kind,
msg: with_code(Some(span), first_line),
require_annotation: true,
viral: true,
parent: prev_parent,
});
}
}
Expand All @@ -259,6 +273,8 @@ fn push_actual_errors(
kind,
msg: with_code(None, next_line),
require_annotation: false,
viral: true,
parent: prev_parent,
});
} else {
for span in primary_spans {
Expand All @@ -267,6 +283,8 @@ fn push_actual_errors(
kind,
msg: with_code(Some(span), next_line),
require_annotation: false,
viral: true,
parent: prev_parent,
});
}
}
Expand All @@ -278,9 +296,11 @@ fn push_actual_errors(
for (index, line) in suggested_replacement.lines().enumerate() {
errors.push(Error {
line_num: Some(span.line_start + index),
kind: Some(ErrorKind::Suggestion),
kind: ErrorKind::Suggestion,
msg: line.to_string(),
require_annotation: true,
viral: true,
parent,
});
}
}
Expand All @@ -289,41 +309,46 @@ fn push_actual_errors(
// Add notes for the backtrace
for span in primary_spans {
if let Some(frame) = &span.expansion {
push_backtrace(errors, frame, file_name);
push_backtrace(errors, frame, file_name, parent);
}
}

// Add notes for any labels that appear in the message.
for span in spans_in_this_file.iter().filter(|span| span.label.is_some()) {
errors.push(Error {
line_num: Some(span.line_start),
kind: Some(ErrorKind::Note),
kind: ErrorKind::Note,
msg: span.label.clone().unwrap(),
require_annotation: true,
viral: true,
parent,
});
}

// Flatten out the children.
for child in &diagnostic.children {
push_actual_errors(errors, child, primary_spans, file_name);
push_actual_errors(errors, child, primary_spans, file_name, parent);
}
}

fn push_backtrace(
errors: &mut Vec<Error>,
expansion: &DiagnosticSpanMacroExpansion,
file_name: &str,
parent: Option<usize>,
) {
if Path::new(&expansion.span.file_name) == Path::new(&file_name) {
errors.push(Error {
line_num: Some(expansion.span.line_start),
kind: Some(ErrorKind::Note),
kind: ErrorKind::Note,
msg: format!("in this expansion of {}", expansion.macro_decl_name),
require_annotation: true,
viral: true,
parent,
});
}

if let Some(previous_expansion) = &expansion.span.expansion {
push_backtrace(errors, previous_expansion, file_name);
push_backtrace(errors, previous_expansion, file_name, parent);
}
}
90 changes: 54 additions & 36 deletions src/tools/compiletest/src/runtest.rs
Original file line number Diff line number Diff line change
Expand Up @@ -679,9 +679,7 @@ impl<'test> TestCx<'test> {
"check_expected_errors: expected_errors={:?} proc_res.status={:?}",
expected_errors, proc_res.status
);
if proc_res.status.success()
&& expected_errors.iter().any(|x| x.kind == Some(ErrorKind::Error))
{
if proc_res.status.success() && expected_errors.iter().any(|x| x.kind == ErrorKind::Error) {
self.fatal_proc_rec("process did not return an error status", proc_res);
}

Expand Down Expand Up @@ -709,22 +707,28 @@ impl<'test> TestCx<'test> {
self.testpaths.file.display().to_string()
};

let expect_help = expected_errors.iter().any(|ee| ee.kind == Some(ErrorKind::Help));
let expect_note = expected_errors.iter().any(|ee| ee.kind == Some(ErrorKind::Note));
// If the testcase being checked contains at least one expected "help"
// message, then we'll ensure that all "help" messages are expected.
// Otherwise, all "help" messages reported by the compiler will be ignored.
// This logic also applies to "note" messages.
let expect_help = expected_errors.iter().any(|ee| ee.kind == ErrorKind::Help && ee.viral);
// let expect_note = expected_errors.iter().any(|ee| ee.kind == ErrorKind::Note && ee.viral);

// Parse the JSON output from the compiler and extract out the messages.
let actual_errors = json::parse_output(&diagnostic_file_name, &proc_res.stderr, proc_res);
let mut actual_errors =
json::parse_output(&diagnostic_file_name, &proc_res.stderr, proc_res);
let mut unexpected = Vec::new();
let mut found = vec![false; expected_errors.len()];
for mut actual_error in actual_errors {
let mut unexpected_actual_indices = Vec::new();
let mut poisoned_parents = HashSet::new();
for (actual_index, actual_error) in actual_errors.iter_mut().enumerate() {
actual_error.msg = self.normalize_output(&actual_error.msg, &[]);

let opt_index =
expected_errors.iter().enumerate().position(|(index, expected_error)| {
!found[index]
&& actual_error.line_num == expected_error.line_num
&& (expected_error.kind.is_none()
|| actual_error.kind == expected_error.kind)
&& actual_error.kind == expected_error.kind
&& actual_error.msg.contains(&expected_error.msg)
});

Expand All @@ -733,25 +737,41 @@ impl<'test> TestCx<'test> {
// found a match, everybody is happy
assert!(!found[index]);
found[index] = true;
}

None => {
// If the test is a known bug, don't require that the error is annotated
if self.is_unexpected_compiler_message(&actual_error, expect_help, expect_note)
{
self.error(&format!(
"{}:{}: unexpected {}: '{}'",
file_name,
actual_error.line_num_str(),
actual_error
.kind
.as_ref()
.map_or(String::from("message"), |k| k.to_string()),
actual_error.msg
));
unexpected.push(actual_error);
if actual_error.kind == ErrorKind::Note && expected_errors[index].viral {
poisoned_parents.insert(actual_error.parent);
}
}

None => unexpected_actual_indices.push(actual_index),
}
}

for unexpected_actual_index in unexpected_actual_indices {
// If the test is a known bug, don't require that the error is annotated
let actual_error = &actual_errors[unexpected_actual_index];
let mut expect_note = false;
let mut next_parent = actual_error.parent;
loop {
if poisoned_parents.contains(&next_parent) {
expect_note = true;
break;
}
match next_parent {
Some(next) => next_parent = actual_errors[next].parent,
None => break,
}
}

if self.is_unexpected_compiler_message(&actual_error, expect_help, expect_note) {
self.error(&format!(
"{}:{}: unexpected {}: '{}'",
file_name,
actual_error.line_num_str(),
actual_error.kind,
actual_error.msg
));
unexpected.push(actual_error);
}
}

Expand All @@ -763,7 +783,7 @@ impl<'test> TestCx<'test> {
"{}:{}: expected {} not found: {}",
file_name,
expected_error.line_num_str(),
expected_error.kind.as_ref().map_or("message".into(), |k| k.to_string()),
expected_error.kind,
expected_error.msg
));
not_found.push(expected_error);
Expand Down Expand Up @@ -803,17 +823,15 @@ impl<'test> TestCx<'test> {
expect_help: bool,
expect_note: bool,
) -> bool {
// If the test being checked doesn't contain any "help" or "note" annotations, then
// we don't require annotating "help" or "note" (respecively) diagnostics at all.
actual_error.require_annotation
&& actual_error.kind.map_or(false, |err_kind| {
// If the test being checked doesn't contain any "help" or "note" annotations, then
// we don't require annotating "help" or "note" (respecively) diagnostics at all.
let default_require_annotations = self.props.require_annotations[&err_kind];
match err_kind {
ErrorKind::Help => expect_help && default_require_annotations,
ErrorKind::Note => expect_note && default_require_annotations,
_ => default_require_annotations,
}
})
&& self.props.require_annotations[&actual_error.kind]
&& match actual_error.kind {
ErrorKind::Help => expect_help,
ErrorKind::Note => expect_note,
_ => true,
}
}

fn should_emit_metadata(&self, pm: Option<PassMode>) -> Emit {
Expand Down
6 changes: 3 additions & 3 deletions src/tools/compiletest/src/runtest/ui.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,8 @@ use rustfix::{Filter, apply_suggestions, get_suggestions_from_json};
use tracing::debug;

use super::{
AllowUnused, Emit, ErrorKind, FailMode, LinkToAux, PassMode, TargetLocation, TestCx,
TestOutput, Truncated, UI_FIXED, WillExecute,
AllowUnused, Emit, FailMode, LinkToAux, PassMode, TargetLocation, TestCx, TestOutput,
Truncated, UI_FIXED, WillExecute,
};
use crate::{errors, json};

Expand Down Expand Up @@ -176,7 +176,7 @@ impl TestCx<'_> {
let msg = format!(
"line {}: cannot combine `--error-format` with {} annotations; use `error-pattern` instead",
expected_errors[0].line_num_str(),
expected_errors[0].kind.unwrap_or(ErrorKind::Error),
expected_errors[0].kind,
);
self.fatal(&msg);
}
Expand Down
Loading
Loading