From f61326e9b89af88bccea3f597d64f41f0406035d Mon Sep 17 00:00:00 2001 From: mejrs <59372212+mejrs@users.noreply.github.com> Date: Thu, 15 May 2025 14:41:53 +0200 Subject: [PATCH] compiletest: add option for automatically adding annotations --- src/tools/compiletest/src/common.rs | 12 ++ src/tools/compiletest/src/errors.rs | 49 +++++++- src/tools/compiletest/src/json.rs | 51 ++++----- src/tools/compiletest/src/lib.rs | 2 + src/tools/compiletest/src/runtest.rs | 108 +++++++++++++++++- .../compiletest/src/runtest/codegen_units.rs | 2 +- 6 files changed, 188 insertions(+), 36 deletions(-) diff --git a/src/tools/compiletest/src/common.rs b/src/tools/compiletest/src/common.rs index 4f93b49874134..141f134e1d037 100644 --- 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 index b5a2b7feac9d6..5e490a88f0f8a 100644 --- 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, + pub code: Option, + 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, /// 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 index 6ed2b52c66d21..f87df04779e01 100644 --- 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 { 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 index 0db4d3f6a4100..88c9d8f12025a 100644 --- a/src/tools/compiletest/src/lib.rs +++ b/src/tools/compiletest/src/lib.rs @@ -135,6 +135,7 @@ pub fn parse_config(args: Vec) -> 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) -> 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 index 40c9f29375b22..da7e1ec91d3ba 100644 --- 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, 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 index 8dfa8d18d1a0b..1085be034144b 100644 --- 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 = 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();