compiletest: add option for automatically adding annotations

mejrs · mejrs · commit f61326e9b89a · 2025-05-15T14:41:53.000+02:00
diff --git a/src/tools/compiletest/src/common.rs b/src/tools/compiletest/src/common.rs
@@ -177,6 +177,10 @@ pub struct Config {
     /// `true` to overwrite stderr/stdout files instead of complaining about changes in output.
     pub bless: bool,
 
+    /// `true` to attempt to add annotations to the test file.
+    // This only adds annotations, it does not delete ones that were not found.
+    pub try_annotate: bool,
+
     /// Stop as soon as possible after any test fails.
     /// May run a few more tests before stopping, due to threading.
     pub fail_fast: bool,
@@ -913,3 +917,11 @@ pub fn incremental_dir(
 ) -> Utf8PathBuf {
     output_base_name(config, testpaths, revision).with_extension("inc")
 }
+
+#[derive(Debug, Copy, Clone)]
+pub struct Location {
+    pub line_start: usize,
+    pub line_end: usize,
+    pub column_start: usize,
+    pub column_end: usize,
+}
diff --git a/src/tools/compiletest/src/errors.rs b/src/tools/compiletest/src/errors.rs
@@ -8,6 +8,9 @@ use camino::Utf8Path;
 use regex::Regex;
 use tracing::*;
 
+use crate::common::Location;
+use crate::json::DiagnosticCode;
+
 #[derive(Copy, Clone, Debug, PartialEq, Eq, Hash)]
 pub enum ErrorKind {
     Help,
@@ -62,12 +65,44 @@ impl fmt::Display for ErrorKind {
     }
 }
 
+#[allow(dead_code)]
+#[derive(Debug, Clone)]
+pub struct ErrorMessage {
+    pub span: Option<Location>,
+    pub code: Option<DiagnosticCode>,
+    pub text: String,
+}
+
+impl ErrorMessage {
+    pub fn from_only_text(text: String) -> Self {
+        ErrorMessage { span: None, code: None, text }
+    }
+}
+
+impl fmt::Display for ErrorMessage {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        let Self { span, code, text } = self;
+        match span {
+            Some(Location { line_start, column_start, line_end, column_end }) => {
+                write!(f, "{line_start}:{column_start}: {line_end}:{column_end}")?
+            }
+            None => write!(f, "?:?: ?:?")?,
+        }
+        match &code {
+            Some(code) => write!(f, ": {text} [{}]", code.code)?,
+            None => write!(f, ": {text}")?,
+        }
+
+        Ok(())
+    }
+}
+
 #[derive(Debug)]
 pub struct Error {
     pub line_num: Option<usize>,
     /// What kind of message we expect (e.g., warning, error, suggestion).
     pub kind: ErrorKind,
-    pub msg: String,
+    pub msg: ErrorMessage,
     /// 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.
@@ -77,7 +112,12 @@ pub struct Error {
 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())
+        format!(
+            "{: <10}line {: >3}: {}",
+            self.kind,
+            self.line_num_str(),
+            self.msg.to_string().cyan()
+        )
     }
 
     pub fn line_num_str(&self) -> String {
@@ -191,7 +231,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: ErrorMessage::from_only_text(msg), require_annotation: true },
+    ))
 }
 
 #[cfg(test)]
diff --git a/src/tools/compiletest/src/json.rs b/src/tools/compiletest/src/json.rs
@@ -6,7 +6,8 @@ use std::sync::OnceLock;
 use regex::Regex;
 use serde::Deserialize;
 
-use crate::errors::{Error, ErrorKind};
+use crate::common::Location;
+use crate::errors::{Error, ErrorKind, ErrorMessage};
 
 #[derive(Deserialize)]
 struct Diagnostic {
@@ -79,10 +80,10 @@ struct DiagnosticSpanMacroExpansion {
     macro_decl_name: String,
 }
 
-#[derive(Deserialize, Clone)]
-struct DiagnosticCode {
+#[derive(Deserialize, Debug, Clone)]
+pub struct DiagnosticCode {
     /// The code itself.
-    code: String,
+    pub code: String,
 }
 
 pub fn rustfix_diagnostics_only(output: &str) -> String {
@@ -149,7 +150,8 @@ pub fn parse_output(file_name: &str, output: &str) -> Vec<Error> {
             Err(_) => errors.push(Error {
                 line_num: None,
                 kind: ErrorKind::Raw,
-                msg: line.to_string(),
+                msg: ErrorMessage::from_only_text(line.into()),
+
                 require_annotation: false,
             }),
         }
@@ -193,26 +195,18 @@ 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!("?:?: ?:?"),
+    let with_code =
+        |span: Option<&DiagnosticSpan>, text: &str| {
+            let span =
+                span.map(
+                    |&DiagnosticSpan { line_start, column_start, line_end, column_end, .. }| {
+                        Location { line_start, column_start, line_end, column_end }
+                    },
+                );
+            let code = diagnostic.code.clone();
+            let text = text.to_string();
+            ErrorMessage { span, code, text }
         };
-        match &diagnostic.code {
-            Some(code) => format!("{span_str}: {text} [{}]", code.code),
-            None => format!("{span_str}: {text}"),
-        }
-    };
 
     // Convert multi-line messages into multiple errors.
     // We expect to replace these with something more structured anyhow.
@@ -267,7 +261,7 @@ fn push_actual_errors(
                 errors.push(Error {
                     line_num: Some(span.line_start + index),
                     kind: ErrorKind::Suggestion,
-                    msg: line.to_string(),
+                    msg: ErrorMessage::from_only_text(line.to_string()),
                     // Empty suggestions (suggestions to remove something) are common
                     // and annotating them in source is not useful.
                     require_annotation: !line.is_empty(),
@@ -289,7 +283,7 @@ fn push_actual_errors(
             errors.push(Error {
                 line_num: Some(span.line_start),
                 kind: ErrorKind::Note,
-                msg: label.clone(),
+                msg: ErrorMessage::from_only_text(label.clone()),
                 // Empty labels (only underlining spans) are common and do not need annotations.
                 require_annotation: !label.is_empty(),
             });
@@ -311,7 +305,10 @@ fn push_backtrace(
         errors.push(Error {
             line_num: Some(expansion.span.line_start),
             kind: ErrorKind::Note,
-            msg: format!("in this expansion of {}", expansion.macro_decl_name),
+            msg: ErrorMessage::from_only_text(format!(
+                "in this expansion of {}",
+                expansion.macro_decl_name
+            )),
             require_annotation: true,
         });
     }
diff --git a/src/tools/compiletest/src/lib.rs b/src/tools/compiletest/src/lib.rs
@@ -135,6 +135,7 @@ pub fn parse_config(args: Vec<String>) -> Config {
             "bless",
             "overwrite stderr/stdout files instead of complaining about a mismatch",
         )
+        .optflag("", "try-annotate", "attempt to fix up error annotations in ui tests")
         .optflag("", "fail-fast", "stop as soon as possible after any test fails")
         .optflag("", "quiet", "print one character per test instead of one line")
         .optopt("", "color", "coloring: auto, always, never", "WHEN")
@@ -330,6 +331,7 @@ pub fn parse_config(args: Vec<String>) -> Config {
 
     Config {
         bless: matches.opt_present("bless"),
+        try_annotate: matches.opt_present("try-annotate"),
         fail_fast: matches.opt_present("fail-fast")
             || env::var_os("RUSTC_TEST_FAIL_FAST").is_some(),
 
diff --git a/src/tools/compiletest/src/runtest.rs b/src/tools/compiletest/src/runtest.rs
@@ -23,12 +23,12 @@ use crate::common::{
     output_base_dir, output_base_name, output_testname_unique,
 };
 use crate::compute_diff::{DiffLine, make_diff, write_diff, write_filtered_diff};
-use crate::errors::{Error, ErrorKind, load_errors};
+use crate::errors::{Error, ErrorKind, ErrorMessage, load_errors};
 use crate::header::TestProps;
+use crate::json::DiagnosticCode;
 use crate::read2::{Truncated, read2_abbreviated};
 use crate::util::{Utf8PathBufExt, add_dylib_path, logv, static_regex};
 use crate::{ColorConfig, json, stamp_file_path};
-
 mod debugger;
 
 // Helper modules that implement test running logic for each test suite.
@@ -701,14 +701,17 @@ impl<'test> TestCx<'test> {
         // Parse the JSON output from the compiler and extract out the messages.
         let actual_errors = json::parse_output(&diagnostic_file_name, &self.get_output(proc_res))
             .into_iter()
-            .map(|e| Error { msg: self.normalize_output(&e.msg, &[]), ..e });
+            .map(|mut e| {
+                e.msg.text = self.normalize_output(&e.msg.text, &[]);
+                e
+            });
 
         let mut unexpected = Vec::new();
         let mut found = vec![false; expected_errors.len()];
         for actual_error in actual_errors {
             for pattern in &self.props.error_patterns {
                 let pattern = pattern.trim();
-                if actual_error.msg.contains(pattern) {
+                if actual_error.msg.text.to_string().contains(pattern) {
                     let q = if actual_error.line_num.is_none() { "?" } else { "" };
                     self.fatal(&format!(
                         "error pattern '{pattern}' is found in structured \
@@ -723,7 +726,7 @@ impl<'test> TestCx<'test> {
                     !found[index]
                         && actual_error.line_num == expected_error.line_num
                         && actual_error.kind == expected_error.kind
-                        && actual_error.msg.contains(&expected_error.msg)
+                        && actual_error.msg.to_string().contains(&expected_error.msg.text)
                 });
 
             match opt_index {
@@ -779,6 +782,10 @@ impl<'test> TestCx<'test> {
                     println!("{}", error.render_for_expected());
                 }
                 println!("{}", "---".green());
+
+                if self.config.try_annotate {
+                    self.try_annotate(unexpected, file_name);
+                }
             }
             if !not_found.is_empty() {
                 println!("{}", "--- not found errors (from test file) ---".red());
@@ -2801,6 +2808,97 @@ impl<'test> TestCx<'test> {
             println!("init_incremental_test: incremental_dir={incremental_dir}");
         }
     }
+
+    fn try_annotate(&self, unexpected: Vec<Error>, file_name: String) {
+        use std::io::{BufRead, Seek, Write};
+
+        let mut file = std::fs::OpenOptions::new().write(true).read(true).open(&file_name).unwrap();
+        let br = BufReader::new(&file);
+        let mut lines: Vec<_> = br.lines().map(|line| (line.unwrap(), Vec::new())).collect();
+        for error in &unexpected {
+            let Some(line_no) = error.line_num else { continue };
+            let target_line = &mut lines[line_no - 1];
+            let text = error.msg.clone();
+            let annotation = (error.kind, text);
+            target_line.1.push(annotation);
+        }
+
+        file.set_len(0).unwrap();
+        file.rewind().unwrap();
+
+        let mut idx = 0;
+        while let Some((line, annots)) = lines.get(idx) {
+            write!(file, "{line}").unwrap();
+
+            if let [first, rest @ ..] = &**annots {
+                // where match ergonomics?
+                let mut rest = rest;
+
+                // Reduce instability by trying to put the first annotation on the same line.
+                // We care about this because error messages can mention the line number by,
+                // for example, naming opaque types like `{closure@file.rs:11:22}`. If they
+                // exist in a test then creating new lines before them invalidates those line numbers.
+                if line.contains("//~") {
+                    // The line already has an annotation.
+                    rest = &**annots
+                } else {
+                    let (kind, ErrorMessage { text, code, .. }) = first;
+
+                    if !(line.contains(&kind.to_string()) && line.contains(&*text)) {
+                        // In the case of revisions, where a test is ran multiple times,
+                        // we do not want to add the same annotation multiple times!
+                        write!(file, " //~{kind} {text}").unwrap();
+                        if let Some(DiagnosticCode { code }) = code {
+                            write!(file, " [{code}]").unwrap();
+                        }
+                    }
+                    writeln!(file).unwrap();
+                }
+
+                // Is the next line an error annotation? If so then
+                // we cannot push the annotation there because that will
+                // displace the one that is already there.
+                //
+                // Forward until we're clear of existing annotations.
+                let mut carets = 1;
+                while let Some((maybe_annot, expect_empty)) = lines.get(idx + 1) {
+                    if maybe_annot.trim().starts_with("//~") {
+                        assert!(expect_empty.is_empty(), "did not expect an annotation");
+                        writeln!(file, "{maybe_annot}").unwrap();
+                        carets += 1;
+                        idx += 1;
+                    } else {
+                        break;
+                    }
+                }
+
+                // What is the previous line's offset?
+                // Let's give the next annotation that offset.
+                let offset = line.split_terminator(|c: char| !c.is_whitespace()).next();
+
+                for (kind, ErrorMessage { text, code, .. }) in rest {
+                    // In the case of revisions, where a test is ran multiple times,
+                    // we do not want to add the same annotation multiple times!
+                    if !(line.contains(&kind.to_string()) && line.contains(&*text)) {
+                        if let Some(w) = offset {
+                            write!(file, "{w}").unwrap();
+                        }
+                        write!(file, "//~{:^>carets$}{kind} {text}", "").unwrap();
+                        if let Some(DiagnosticCode { code }) = code {
+                            write!(file, " [{code}]").unwrap();
+                        }
+                    }
+                    writeln!(file).unwrap();
+                    carets += 1;
+                }
+            } else {
+                writeln!(file).unwrap();
+            }
+
+            idx += 1
+        }
+        file.flush().unwrap();
+    }
 }
 
 struct ProcArgs {
diff --git a/src/tools/compiletest/src/runtest/codegen_units.rs b/src/tools/compiletest/src/runtest/codegen_units.rs
@@ -32,7 +32,7 @@ impl TestCx<'_> {
 
         let expected: Vec<MonoItem> = errors::load_errors(&self.testpaths.file, None)
             .iter()
-            .map(|e| str_to_mono_item(&e.msg[..], false))
+            .map(|e| str_to_mono_item(&e.msg.text[..], false))
             .collect();
 
         let mut missing = Vec::new();
