From 7a4f09c2240ef2eb4d1e383f0d471b85ce8d0720 Mon Sep 17 00:00:00 2001 From: Vadim Petrochenkov Date: Sat, 3 May 2025 15:53:52 +0300 Subject: compiletest: Improve diagnostics for line annotation mismatches --- src/tools/compiletest/src/errors.rs | 46 +++++------ src/tools/compiletest/src/header.rs | 2 +- src/tools/compiletest/src/json.rs | 40 ++++------ src/tools/compiletest/src/runtest.rs | 147 ++++++++++++++++++++++++++++------- 4 files changed, 159 insertions(+), 76 deletions(-) (limited to 'src') diff --git a/src/tools/compiletest/src/errors.rs b/src/tools/compiletest/src/errors.rs index b5a2b7feac9..9fa26305f6b 100644 --- a/src/tools/compiletest/src/errors.rs +++ b/src/tools/compiletest/src/errors.rs @@ -16,6 +16,8 @@ pub enum ErrorKind { Suggestion, Warning, Raw, + /// Used for better recovery and diagnostics in compiletest. + Unknown, } impl ErrorKind { @@ -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 { + 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`" + ) + }) } } @@ -58,6 +64,7 @@ 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"), } } } @@ -65,6 +72,7 @@ impl fmt::Display for ErrorKind { #[derive(Debug)] pub struct Error { pub line_num: Option, + pub column_num: Option, /// What kind of message we expect (e.g., warning, error, suggestion). pub kind: ErrorKind, pub msg: String, @@ -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. @@ -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"]; @@ -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={:?}", @@ -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)] diff --git a/src/tools/compiletest/src/header.rs b/src/tools/compiletest/src/header.rs index 8bee9caacc9..2b203bb309c 100644 --- a/src/tools/compiletest/src/header.rs +++ b/src/tools/compiletest/src/header.rs @@ -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())); } }, ); diff --git a/src/tools/compiletest/src/json.rs b/src/tools/compiletest/src/json.rs index 6ed2b52c66d..a8e6416e56c 100644 --- a/src/tools/compiletest/src/json.rs +++ b/src/tools/compiletest/src/json.rs @@ -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, suggested_replacement: Option, @@ -148,6 +146,7 @@ pub fn parse_output(file_name: &str, output: &str) -> Vec { 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, @@ -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. @@ -225,8 +208,9 @@ 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), }); @@ -234,8 +218,9 @@ fn push_actual_errors( 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, }); } @@ -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, }); } @@ -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 @@ -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. @@ -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, diff --git a/src/tools/compiletest/src/runtest.rs b/src/tools/compiletest/src/runtest.rs index 42c851ea999..980e89889ab 100644 --- a/src/tools/compiletest/src/runtest.rs +++ b/src/tools/compiletest/src/runtest.rs @@ -11,7 +11,7 @@ use std::{env, iter, str}; use build_helper::fs::remove_and_create_dir_all; use camino::{Utf8Path, Utf8PathBuf}; -use colored::Colorize; +use colored::{Color, Colorize}; use regex::{Captures, Regex}; use tracing::*; @@ -677,9 +677,6 @@ impl<'test> TestCx<'test> { return; } - // On Windows, translate all '\' path separators to '/' - let file_name = self.testpaths.file.to_string().replace(r"\", "/"); - // On Windows, keep all '\' path separators to match the paths reported in the JSON output // from the compiler let diagnostic_file_name = if self.props.remap_src_base { @@ -704,6 +701,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 { @@ -738,14 +736,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); } } } @@ -755,39 +748,140 @@ 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); + + // Emit locations in a format that is short (relative paths) but "clickable" in editors. + // Also normalize path separators to `/`. + let file_name = self + .testpaths + .file + .strip_prefix(self.config.src_root.as_str()) + .unwrap_or(&self.testpaths.file) + .to_string() + .replace(r"\", "/"); + let line_str = |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(), + }; + format!("{file_name}:{line_num}{opt_col_num}") + }; + let print_error = |e| println!("{}: {}: {}", line_str(e), e.kind, e.msg.cyan()); + let push_suggestion = + |suggestions: &mut Vec<_>, e: &Error, kind, line, msg, color, rank| { + let mut ret = String::new(); + if kind { + ret += &format!("{} {}", "with kind".color(color), e.kind); + } + if line { + if !ret.is_empty() { + ret.push(' '); + } + ret += &format!("{} {}", "on line".color(color), line_str(e)); + } + if msg { + if !ret.is_empty() { + ret.push(' '); + } + ret += &format!("{} {}", "with message".color(color), e.msg.cyan()); + } + suggestions.push((ret, rank)); + }; + let show_suggestions = |mut suggestions: Vec<_>, prefix: &str, color| { + // Only show suggestions with the highest rank. + suggestions.sort_by_key(|(_, rank)| *rank); + if let Some(&(_, top_rank)) = suggestions.first() { + for (suggestion, rank) in suggestions { + if rank == top_rank { + println!(" {} {suggestion}", prefix.color(color)); + } + } + } + }; + + // 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()); + let header = "--- reported in JSON output but not expected in test file ---"; + println!("{}", header.green()); for error in &unexpected { - println!("{}", error.render_for_expected()); + print_error(error); + let mut suggestions = Vec::new(); + for candidate in ¬_found { + let mut push_red_suggestion = |line, msg, rank| { + push_suggestion( + &mut suggestions, + candidate, + candidate.kind != error.kind, + line, + msg, + Color::Red, + rank, + ) + }; + if error.msg.contains(&candidate.msg) { + push_red_suggestion(candidate.line_num != error.line_num, false, 0); + } else if candidate.line_num.is_some() + && candidate.line_num == error.line_num + { + push_red_suggestion(false, true, 1); + } + } + + show_suggestions(suggestions, "expected", Color::Red); } println!("{}", "---".green()); } if !not_found.is_empty() { - println!("{}", "--- not found errors (from test file) ---".red()); + let header = "--- expected in test file but not reported in JSON output ---"; + println!("{}", header.red()); for error in ¬_found { - println!("{}", error.render_for_expected()); + print_error(error); + let mut suggestions = Vec::new(); + for candidate in unexpected.iter().chain(&unimportant) { + let mut push_green_suggestion = |line, msg, rank| { + push_suggestion( + &mut suggestions, + candidate, + candidate.kind != error.kind, + line, + msg, + Color::Green, + rank, + ) + }; + if candidate.msg.contains(&error.msg) { + push_green_suggestion(candidate.line_num != error.line_num, false, 0); + } else if candidate.line_num.is_some() + && candidate.line_num == error.line_num + { + push_green_suggestion(false, true, 1); + } + } + + show_suggestions(suggestions, "reported", Color::Green); } - println!("{}", "---\n".red()); + println!("{}", "---".red()); } - panic!("errors differ from expected"); + panic!( + "errors differ from expected\nstatus: {}\ncommand: {}\n", + proc_res.status, proc_res.cmdline + ); } } @@ -2073,7 +2167,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`" -- cgit 1.4.1-3-g733a5