compiletest: Improve diagnostics for line annotation mismatches

4 files changed, 159 insertions, 76 deletions

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<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`"
+            )
+        })
     }
 }
 
@@ -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<usize>,
+    pub column_num: Option<usize>,
     /// 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<String>,
     suggested_replacement: Option<String>,
@@ -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,
@@ -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 &not_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 &not_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`"