compiletest: Make diagnostic kind mandatory on line annotations

Author: petrochenkov

Cargo.lock

@@ -694,6 +694,7 @@ dependencies = [
  "serde_json",
  "tracing",
  "tracing-subscriber",
+ "unicode-xid",
  "unified-diff",
  "walkdir",
  "windows 0.59.0",

src/tools/compiletest/Cargo.toml

@@ -26,6 +26,7 @@ walkdir = "2"
 glob = "0.3.0"
 anyhow = "1"
 home = "0.5.5"
+unicode-xid = "0.2"
 
 [target.'cfg(unix)'.dependencies]
 libc = "0.2"

src/tools/compiletest/src/errors.rs

@@ -3,7 +3,6 @@ use std::fs::File;
 use std::io::BufReader;
 use std::io::prelude::*;
 use std::path::Path;
-use std::str::FromStr;
 use std::sync::OnceLock;
 
 use regex::Regex;
@@ -18,30 +17,42 @@ pub enum ErrorKind {
     Warning,
 }
 
-impl FromStr for ErrorKind {
-    type Err = ();
-    fn from_str(s: &str) -> Result<Self, Self::Err> {
-        let s = s.to_uppercase();
-        let part0: &str = s.split(':').next().unwrap();
-        match part0 {
-            "HELP" => Ok(ErrorKind::Help),
-            "ERROR" => Ok(ErrorKind::Error),
-            "NOTE" => Ok(ErrorKind::Note),
-            "SUGGESTION" => Ok(ErrorKind::Suggestion),
-            "WARN" | "WARNING" => Ok(ErrorKind::Warning),
-            _ => Err(()),
+impl ErrorKind {
+    pub fn from_compiler_str(s: &str) -> ErrorKind {
+        match s {
+            "help" => ErrorKind::Help,
+            "error" | "error: internal compiler error" => ErrorKind::Error,
+            "note" | "failure-note" => ErrorKind::Note,
+            "warning" => ErrorKind::Warning,
+            _ => panic!("unexpected compiler diagnostic kind `{s}`"),
+        }
+    }
+
+    fn from_user_str(s: &str) -> ErrorKind {
+        match s {
+            "HELP" => ErrorKind::Help,
+            "HELP_NONVIRAL" => ErrorKind::Help,
+            "ERROR" => ErrorKind::Error,
+            "NOTE" => ErrorKind::Note,
+            "NOTE_NONVIRAL" => ErrorKind::Note,
+            "SUGGESTION" => ErrorKind::Suggestion,
+            "WARN" | "WARNING" => ErrorKind::Warning,
+            _ => panic!(
+                "unexpected diagnostic kind `{s}`, expected \
+                 `ERROR`, `WARN`, `NOTE`, `HELP` or `SUGGESTION`"
+            ),
         }
     }
 }
 
 impl fmt::Display for ErrorKind {
     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
         match *self {
-            ErrorKind::Help => write!(f, "help message"),
-            ErrorKind::Error => write!(f, "error"),
-            ErrorKind::Note => write!(f, "note"),
-            ErrorKind::Suggestion => write!(f, "suggestion"),
-            ErrorKind::Warning => write!(f, "warning"),
+            ErrorKind::Help => write!(f, "HELP"),
+            ErrorKind::Error => write!(f, "ERROR"),
+            ErrorKind::Note => write!(f, "NOTE"),
+            ErrorKind::Suggestion => write!(f, "SUGGESTION"),
+            ErrorKind::Warning => write!(f, "WARN"),
         }
     }
 }
@@ -50,20 +61,21 @@ 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 "found" errors, not for "expected" errors.
+    pub should_annotate: 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,
 }
 
 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().to_uppercase(),
-            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 {
@@ -150,18 +162,13 @@ fn parse_expected(
     }
 
     // Get the part of the comment after the sigil (e.g. `~^^` or ~|).
-    let whole_match = captures.get(0).unwrap();
-    let (_, mut msg) = line.split_at(whole_match.end());
-
-    let first_word = msg.split_whitespace().next().expect("Encountered unexpected empty comment");
-
-    // If we find `//~ ERROR foo` or something like that, skip the first word.
-    let kind = first_word.parse::<ErrorKind>().ok();
-    if kind.is_some() {
-        msg = &msg.trim_start().split_at(first_word.len()).1;
-    }
-
-    let msg = msg.trim().to_owned();
+    let tag = captures.get(0).unwrap();
+    let rest = line[tag.end()..].trim_start();
+    let (kind, _) =
+        rest.split_once(|c| !unicode_xid::UnicodeXID::is_xid_continue(c)).unwrap_or((rest, ""));
+    let msg = rest[kind.len()..].trim().to_owned();
+    let viral = kind != "NOTE_NONVIRAL" && kind != "HELP_NONVIRAL";
+    let kind = ErrorKind::from_user_str(kind);
 
     let line_num_adjust = &captures["adjust"];
     let (follow_prev, line_num) = if line_num_adjust == "|" {
@@ -177,12 +184,12 @@ fn parse_expected(
     debug!(
         "line={:?} tag={:?} follow_prev={:?} kind={:?} msg={:?}",
         line_num,
-        whole_match.as_str(),
+        tag.as_str(),
         follow_prev,
         kind,
         msg
     );
-    Some((follow_prev, Error { line_num, kind, msg }))
+    Some((follow_prev, Error { line_num, kind, msg, should_annotate: true, viral }))
 }
 
 #[cfg(test)]

src/tools/compiletest/src/json.rs

@@ -1,7 +1,6 @@
 //! These structs are a subset of the ones found in `rustc_errors::json`.
 
 use std::path::{Path, PathBuf};
-use std::str::FromStr;
 use std::sync::OnceLock;
 
 use regex::Regex;
@@ -239,6 +238,7 @@ fn push_expected_errors(
     // Convert multi-line messages into multiple expected
     // errors. We expect to replace these with something
     // more structured shortly anyhow.
+    let kind = ErrorKind::from_compiler_str(&diagnostic.level);
     let mut message_lines = diagnostic.message.lines();
     if let Some(first_line) = message_lines.next() {
         let ignore = |s| {
@@ -250,30 +250,42 @@ fn push_expected_errors(
         };
 
         if primary_spans.is_empty() && !ignore(first_line) {
-            let msg = with_code(None, first_line);
-            let kind = ErrorKind::from_str(&diagnostic.level).ok();
-            expected_errors.push(Error { line_num: None, kind, msg });
+            expected_errors.push(Error {
+                line_num: None,
+                kind,
+                msg: with_code(None, first_line),
+                should_annotate: diagnostic.level != "failure-note",
+                viral: true,
+            });
         } else {
             for span in primary_spans {
-                let msg = with_code(Some(span), first_line);
-                let kind = ErrorKind::from_str(&diagnostic.level).ok();
-                expected_errors.push(Error { line_num: Some(span.line_start), kind, msg });
+                expected_errors.push(Error {
+                    line_num: Some(span.line_start),
+                    kind,
+                    msg: with_code(Some(span), first_line),
+                    should_annotate: diagnostic.level != "failure-note",
+                    viral: true,
+                });
             }
         }
     }
     for next_line in message_lines {
         if primary_spans.is_empty() {
             expected_errors.push(Error {
                 line_num: None,
-                kind: None,
+                kind,
                 msg: with_code(None, next_line),
+                should_annotate: false,
+                viral: true,
             });
         } else {
             for span in primary_spans {
                 expected_errors.push(Error {
                     line_num: Some(span.line_start),
-                    kind: None,
+                    kind,
                     msg: with_code(Some(span), next_line),
+                    should_annotate: false,
+                    viral: true,
                 });
             }
         }
@@ -285,8 +297,10 @@ fn push_expected_errors(
             for (index, line) in suggested_replacement.lines().enumerate() {
                 expected_errors.push(Error {
                     line_num: Some(span.line_start + index),
-                    kind: Some(ErrorKind::Suggestion),
+                    kind: ErrorKind::Suggestion,
                     msg: line.to_string(),
+                    should_annotate: true,
+                    viral: true,
                 });
             }
         }
@@ -303,8 +317,10 @@ fn push_expected_errors(
     for span in spans_in_this_file.iter().filter(|span| span.label.is_some()) {
         expected_errors.push(Error {
             line_num: Some(span.line_start),
-            kind: Some(ErrorKind::Note),
+            kind: ErrorKind::Note,
             msg: span.label.clone().unwrap(),
+            should_annotate: true,
+            viral: true,
         });
     }
 
@@ -322,8 +338,10 @@ fn push_backtrace(
     if Path::new(&expansion.span.file_name) == Path::new(&file_name) {
         expected_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),
+            should_annotate: true,
+            viral: true,
         });
     }
 

src/tools/compiletest/src/runtest.rs

@@ -680,9 +680,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);
         }
 
@@ -714,8 +712,8 @@ impl<'test> TestCx<'test> {
         // 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 == Some(ErrorKind::Help));
-        let expect_note = expected_errors.iter().any(|ee| ee.kind == Some(ErrorKind::Note));
+        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);
@@ -728,8 +726,7 @@ impl<'test> TestCx<'test> {
                 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)
                 });
 
@@ -748,10 +745,7 @@ impl<'test> TestCx<'test> {
                             "{}:{}: unexpected {}: '{}'",
                             file_name,
                             actual_error.line_num_str(),
-                            actual_error
-                                .kind
-                                .as_ref()
-                                .map_or(String::from("message"), |k| k.to_string()),
+                            actual_error.kind,
                             actual_error.msg
                         ));
                         unexpected.push(actual_error);
@@ -768,7 +762,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);
@@ -811,11 +805,12 @@ impl<'test> TestCx<'test> {
         expect_note: bool,
     ) -> bool {
         !actual_error.msg.is_empty()
+            && actual_error.should_annotate
             && match actual_error.kind {
-                Some(ErrorKind::Help) => expect_help,
-                Some(ErrorKind::Note) => expect_note,
-                Some(ErrorKind::Error) | Some(ErrorKind::Warning) => true,
-                Some(ErrorKind::Suggestion) | None => false,
+                ErrorKind::Help => expect_help,
+                ErrorKind::Note => expect_note,
+                ErrorKind::Error | ErrorKind::Warning => true,
+                ErrorKind::Suggestion => false,
             }
     }
 

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

tests/incremental/const-generics/hash-tyvid-regression-1.rs

@@ -20,9 +20,9 @@ where
 {
     use std::convert::TryFrom;
     <[T; N.get()]>::try_from(())
-    //~^ error: the trait bound
-    //~| error: the trait bound
-    //~| error: mismatched types
+    //~^ ERROR the trait bound
+    //~| ERROR the trait bound
+    //~| ERROR mismatched types
 }
 
 fn main() {}

tests/incremental/const-generics/hash-tyvid-regression-2.rs

@@ -23,7 +23,7 @@ where
 {
     fn eq(&self, other: &&'a [A]) -> bool {
         self.0 == other
-        //~^ error: can't compare
+        //~^ ERROR can't compare
     }
 }
 

tests/incremental/const-generics/hash-tyvid-regression-3.rs

@@ -16,7 +16,7 @@ where
     fn new() -> Self {
         let mut node = Node::new();
         node.keys.some_function();
-        //~^ error: no method named
+        //~^ ERROR no method named
         node
     }
 }

tests/incremental/const-generics/hash-tyvid-regression-4.rs

@@ -22,7 +22,7 @@ where
     fn split(&mut self, i: usize, k: K, right: bool) -> Node<K, D> {
         let mut node = Node::new();
         node.keys.push(k);
-        //~^ error: no method named
+        //~^ ERROR no method named
         node
     }
 }