[WIP] compiletest: Make diagnostic kind mandatory on line annotations

Author: petrochenkov

src/tools/compiletest/src/errors.rs

@@ -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`"
-            )
-        })
+            ),
+        }
     }
 }
 
@@ -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 {
@@ -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"];
@@ -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)]

src/tools/compiletest/src/header.rs

@@ -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);
                     }
                 },
             );

src/tools/compiletest/src/json.rs

@@ -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`
@@ -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
@@ -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,
             });
         }
     }
@@ -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 {
@@ -267,6 +283,8 @@ fn push_actual_errors(
                     kind,
                     msg: with_code(Some(span), next_line),
                     require_annotation: false,
+                    viral: true,
+                    parent: prev_parent,
                 });
             }
         }
@@ -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,
                 });
             }
         }
@@ -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);
     }
 }

src/tools/compiletest/src/runtest.rs

@@ -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);
         }
 
@@ -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)
                 });
 
@@ -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);
             }
         }
 
@@ -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);
@@ -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 {

src/tools/compiletest/src/runtest/ui.rs

@@ -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};
 
@@ -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);
         }