Unrolled build for rust-lang#133478

Rollup merge of rust-lang#133478 - aDotInTheVoid:finally, r=fmease

jsondocck: Parse, don't validate commands.

Centralizes knowledge of jsondocck syntax into the parser, so the checker doesn't need to know what the indexes are.

[Vaguely related zulip discussion](https://rust-lang.zulipchat.com/#narrow/channel/266220-t-rustdoc/topic/jsondocck.20rewrite)

I'm very happy this is negative LoC, despite adding a big, documented enum!

r? ``@fmease``

Author: rust-timer

src/tools/jsondocck/src/cache.rs

@@ -28,7 +28,8 @@ impl Cache {
         }
     }
 
-    pub fn value(&self) -> &Value {
-        &self.value
+    // FIXME: Make this failible, so jsonpath syntax error has line number.
+    pub fn select(&self, path: &str) -> Vec<&Value> {
+        jsonpath_lib::select(&self.value, path).unwrap()
     }
 }

src/tools/jsondocck/src/error.rs

@@ -1,29 +1,7 @@
-use std::error::Error;
-use std::fmt;
-
 use crate::Command;
 
 #[derive(Debug)]
-pub enum CkError {
-    /// A check failed. File didn't exist or failed to match the command
-    FailedCheck(String, Command),
-    /// An error triggered by some other error
-    Induced(Box<dyn Error>),
-}
-
-impl fmt::Display for CkError {
-    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
-        match self {
-            CkError::FailedCheck(msg, cmd) => {
-                write!(f, "Failed check: {} on line {}", msg, cmd.lineno)
-            }
-            CkError::Induced(err) => write!(f, "Check failed: {}", err),
-        }
-    }
-}
-
-impl<T: Error + 'static> From<T> for CkError {
-    fn from(err: T) -> CkError {
-        CkError::Induced(Box::new(err))
-    }
+pub struct CkError {
+    pub message: String,
+    pub command: Command,
 }

src/tools/jsondocck/src/main.rs

@@ -1,8 +1,8 @@
 use std::borrow::Cow;
+use std::process::ExitCode;
 use std::sync::OnceLock;
-use std::{env, fmt, fs};
+use std::{env, fs};
 
-use jsonpath_lib::select;
 use regex::{Regex, RegexBuilder};
 use serde_json::Value;
 
@@ -14,90 +14,134 @@ use cache::Cache;
 use config::parse_config;
 use error::CkError;
 
-fn main() -> Result<(), String> {
+fn main() -> ExitCode {
     let config = parse_config(env::args().collect());
 
     let mut failed = Vec::new();
     let mut cache = Cache::new(&config);
-    let commands = get_commands(&config.template)
-        .map_err(|_| format!("Jsondocck failed for {}", &config.template))?;
+    let Ok(commands) = get_commands(&config.template) else {
+        eprintln!("Jsondocck failed for {}", &config.template);
+        return ExitCode::FAILURE;
+    };
 
     for command in commands {
-        if let Err(e) = check_command(command, &mut cache) {
-            failed.push(e);
+        if let Err(message) = check_command(&command, &mut cache) {
+            failed.push(CkError { command, message });
         }
     }
 
     if failed.is_empty() {
-        Ok(())
+        ExitCode::SUCCESS
     } else {
         for i in failed {
-            eprintln!("{}", i);
+            eprintln!("{}:{}, command failed", config.template, i.command.lineno);
+            eprintln!("{}", i.message)
         }
-        Err(format!("Jsondocck failed for {}", &config.template))
+        ExitCode::FAILURE
     }
 }
 
 #[derive(Debug)]
 pub struct Command {
-    negated: bool,
     kind: CommandKind,
-    args: Vec<String>,
+    path: String,
     lineno: usize,
 }
 
 #[derive(Debug)]
-pub enum CommandKind {
-    Has,
-    Count,
-    Is,
-    IsMany,
-    Set,
+enum CommandKind {
+    /// `//@ has <path>`
+    ///
+    /// Checks the path exists.
+    HasPath,
+
+    /// `//@ has <path> <value>`
+    ///
+    /// Check one thing at the path  is equal to the value.
+    HasValue { value: String },
+
+    /// `//@ !has <path>`
+    ///
+    /// Checks the path doesn't exist.
+    HasNotPath,
+
+    /// `//@ is <path> <value>`
+    ///
+    /// Check the path is the given value.
+    Is { value: String },
+
+    /// `//@ is <path> <value> <value>...`
+    ///
+    /// Check that the path matches to exactly every given value.
+    IsMany { values: Vec<String> },
+
+    /// `//@ !is <path> <value>`
+    ///
+    /// Check the path isn't the given value.
+    IsNot { value: String },
+
+    /// `//@ count <path> <value>`
+    ///
+    /// Check the path has the expected number of matches.
+    CountIs { expected: usize },
+
+    /// `//@ set <name> = <path>`
+    Set { variable: String },
 }
 
 impl CommandKind {
-    fn validate(&self, args: &[String], lineno: usize) -> bool {
-        // FIXME(adotinthevoid): We should "parse, don't validate" here, so we avoid ad-hoc
-        // indexing in check_command.
-        let count = match self {
-            CommandKind::Has => (1..=2).contains(&args.len()),
-            CommandKind::IsMany => args.len() >= 2,
-            CommandKind::Count | CommandKind::Is => 2 == args.len(),
-            CommandKind::Set => 3 == args.len(),
-        };
+    /// Returns both the kind and the path.
+    ///
+    /// Returns `None` if the command isn't from jsondocck (e.g. from compiletest).
+    fn parse<'a>(command_name: &str, negated: bool, args: &'a [String]) -> Option<(Self, &'a str)> {
+        let kind = match (command_name, negated) {
+            ("count", false) => {
+                assert_eq!(args.len(), 2);
+                let expected = args[1].parse().expect("invalid number for `count`");
+                Self::CountIs { expected }
+            }
 
-        if !count {
-            print_err(&format!("Incorrect number of arguments to `{}`", self), lineno);
-            return false;
-        }
+            ("ismany", false) => {
+                // FIXME: Make this >= 3, and migrate len(values)==1 cases to @is
+                assert!(args.len() >= 2, "Not enough args to `ismany`");
+                let values = args[1..].to_owned();
+                Self::IsMany { values }
+            }
 
-        if let CommandKind::Count = self {
-            if args[1].parse::<usize>().is_err() {
-                print_err(
-                    &format!(
-                        "Second argument to `count` must be a valid usize (got `{}`)",
-                        args[1]
-                    ),
-                    lineno,
-                );
-                return false;
+            ("is", false) => {
+                assert_eq!(args.len(), 2);
+                Self::Is { value: args[1].clone() }
+            }
+            ("is", true) => {
+                assert_eq!(args.len(), 2);
+                Self::IsNot { value: args[1].clone() }
             }
-        }
 
-        true
-    }
-}
+            ("set", false) => {
+                assert_eq!(args.len(), 3);
+                assert_eq!(args[1], "=");
+                return Some((Self::Set { variable: args[0].clone() }, &args[2]));
+            }
 
-impl fmt::Display for CommandKind {
-    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
-        let text = match self {
-            CommandKind::Has => "has",
-            CommandKind::IsMany => "ismany",
-            CommandKind::Count => "count",
-            CommandKind::Is => "is",
-            CommandKind::Set => "set",
+            ("has", false) => match args {
+                [_path] => Self::HasPath,
+                [_path, value] => Self::HasValue { value: value.clone() },
+                _ => panic!("`//@ has` must have 2 or 3 arguments, but got {args:?}"),
+            },
+            ("has", true) => {
+                assert_eq!(args.len(), 1, "args={args:?}");
+                Self::HasNotPath
+            }
+
+            (_, false) if KNOWN_DIRECTIVE_NAMES.contains(&command_name) => {
+                return None;
+            }
+            _ => {
+                panic!("Invalid command `//@ {}{command_name}`", if negated { "!" } else { "" })
+            }
         };
-        write!(f, "{}", text)
+
+        Some((kind, &args[0]))
     }
 }
 
@@ -125,8 +169,7 @@ fn print_err(msg: &str, lineno: usize) {
 //        See <https://github.com/rust-lang/rust/issues/125813#issuecomment-2141953780>.
 include!(concat!(env!("CARGO_MANIFEST_DIR"), "/../compiletest/src/directive-list.rs"));
 
-/// Get a list of commands from a file. Does the work of ensuring the commands
-/// are syntactically valid.
+/// Get a list of commands from a file.
 fn get_commands(template: &str) -> Result<Vec<Command>, ()> {
     let mut commands = Vec::new();
     let mut errors = false;
@@ -142,217 +185,102 @@ fn get_commands(template: &str) -> Result<Vec<Command>, ()> {
 
         let negated = cap.name("negated").unwrap().as_str() == "!";
 
-        let cmd = match cap.name("cmd").unwrap().as_str() {
-            "has" => CommandKind::Has,
-            "count" => CommandKind::Count,
-            "is" => CommandKind::Is,
-            "ismany" => CommandKind::IsMany,
-            "set" => CommandKind::Set,
-            // FIXME: See the comment above the `include!(...)`.
-            cmd if KNOWN_DIRECTIVE_NAMES.contains(&cmd) => continue,
-            cmd => {
-                print_err(&format!("Unrecognized command name `{cmd}`"), lineno);
-                errors = true;
-                continue;
-            }
-        };
-
-        let args = cap.name("args").map_or(Some(vec![]), |m| shlex::split(m.as_str()));
-
-        let args = match args {
+        let args_str = &cap["args"];
+        let args = match shlex::split(args_str) {
             Some(args) => args,
             None => {
-                print_err(
-                    &format!(
-                        "Invalid arguments to shlex::split: `{}`",
-                        cap.name("args").unwrap().as_str()
-                    ),
-                    lineno,
-                );
+                print_err(&format!("Invalid arguments to shlex::split: `{args_str}`",), lineno);
                 errors = true;
                 continue;
             }
         };
 
-        if !cmd.validate(&args, lineno) {
-            errors = true;
-            continue;
+        if let Some((kind, path)) = CommandKind::parse(&cap["cmd"], negated, &args) {
+            commands.push(Command { kind, lineno, path: path.to_owned() })
         }
-
-        commands.push(Command { negated, kind: cmd, args, lineno })
     }
 
     if !errors { Ok(commands) } else { Err(()) }
 }
 
-/// Performs the actual work of ensuring a command passes. Generally assumes the command
-/// is syntactically valid.
-fn check_command(command: Command, cache: &mut Cache) -> Result<(), CkError> {
-    // FIXME: Be more granular about why, (e.g. syntax error, count not equal)
-    let result = match command.kind {
-        CommandKind::Has => {
-            match command.args.len() {
-                // `has <jsonpath>`: Check that `jsonpath` exists.
-                1 => {
-                    let val = cache.value();
-                    let results = select(val, &command.args[0]).unwrap();
-                    !results.is_empty()
-                }
-                // `has <jsonpath> <value>`: Check *any* item matched by `jsonpath` equals `value`.
-                2 => {
-                    let val = cache.value().clone();
-                    let results = select(&val, &command.args[0]).unwrap();
-                    let pat = string_to_value(&command.args[1], cache);
-                    let has = results.contains(&pat.as_ref());
-                    // Give better error for when `has` check fails.
-                    if !command.negated && !has {
-                        return Err(CkError::FailedCheck(
-                            format!(
-                                "{} matched to {:?} but didn't have {:?}",
-                                &command.args[0],
-                                results,
-                                pat.as_ref()
-                            ),
-                            command,
-                        ));
-                    } else {
-                        has
-                    }
-                }
-                _ => unreachable!(),
+/// Performs the actual work of ensuring a command passes.
+fn check_command(command: &Command, cache: &mut Cache) -> Result<(), String> {
+    let matches = cache.select(&command.path);
+    match &command.kind {
+        CommandKind::HasPath => {
+            if matches.is_empty() {
+                return Err("matched to no values".to_owned());
+            }
+        }
+        CommandKind::HasNotPath => {
+            if !matches.is_empty() {
+                return Err(format!("matched to {matches:?}, but wanted no matches"));
+            }
+        }
+        CommandKind::HasValue { value } => {
+            let want_value = string_to_value(value, cache);
+            if !matches.contains(&want_value.as_ref()) {
+                return Err(format!("matched to {matches:?}, which didn't contain {want_value:?}"));
+            }
+        }
+        CommandKind::Is { value } => {
+            let want_value = string_to_value(value, cache);
+            let matched = get_one(&matches)?;
+            if matched != want_value.as_ref() {
+                return Err(format!("matched to {matched:?} but want {want_value:?}"));
+            }
+        }
+        CommandKind::IsNot { value } => {
+            let wantnt_value = string_to_value(value, cache);
+            let matched = get_one(&matches)?;
+            if matched == wantnt_value.as_ref() {
+                return Err(format!("got value {wantnt_value:?}, but want anything else"));
             }
         }
-        // `ismany <path> <jsonpath> <value...>`
-        CommandKind::IsMany => {
-            assert!(!command.negated, "`ismany` may not be negated");
-            let (query, values) = if let [query, values @ ..] = &command.args[..] {
-                (query, values)
-            } else {
-                unreachable!("Checked in CommandKind::validate")
-            };
-            let val = cache.value();
-            let got_values = select(val, &query).unwrap();
 
+        CommandKind::IsMany { values } => {
             // Serde json doesn't implement Ord or Hash for Value, so we must
             // use a Vec here. While in theory that makes setwize equality
             // O(n^2), in practice n will never be large enough to matter.
             let expected_values =
                 values.iter().map(|v| string_to_value(v, cache)).collect::<Vec<_>>();
-            if expected_values.len() != got_values.len() {
-                return Err(CkError::FailedCheck(
-                    format!(
-                        "Expected {} values, but `{}` matched to {} values ({:?})",
-                        expected_values.len(),
-                        query,
-                        got_values.len(),
-                        got_values
-                    ),
-                    command,
+            if expected_values.len() != matches.len() {
+                return Err(format!(
+                    "Expected {} values, but matched to {} values ({:?})",
+                    expected_values.len(),
+                    matches.len(),
+                    matches
                 ));
             };
-            for got_value in got_values {
+            for got_value in matches {
                 if !expected_values.iter().any(|exp| &**exp == got_value) {
-                    return Err(CkError::FailedCheck(
-                        format!("`{}` has match {:?}, which was not expected", query, got_value),
-                        command,
-                    ));
+                    return Err(format!("has match {got_value:?}, which was not expected",));
                 }
             }
-            true
-        }
-        // `count <jsonpath> <count>`: Check that `jsonpath` matches exactly `count` times.
-        CommandKind::Count => {
-            assert_eq!(command.args.len(), 2);
-            let expected: usize = command.args[1].parse().unwrap();
-            let val = cache.value();
-            let results = select(val, &command.args[0]).unwrap();
-            let eq = results.len() == expected;
-            if !command.negated && !eq {
-                return Err(CkError::FailedCheck(
-                    format!(
-                        "`{}` matched to `{:?}` with length {}, but expected length {}",
-                        &command.args[0],
-                        results,
-                        results.len(),
-                        expected
-                    ),
-                    command,
-                ));
-            } else {
-                eq
-            }
         }
-        // `has <jsonpath> <value>`: Check` *exactly one* item matched by `jsonpath`, and it equals `value`.
-        CommandKind::Is => {
-            assert_eq!(command.args.len(), 2);
-            let val = cache.value().clone();
-            let results = select(&val, &command.args[0]).unwrap();
-            let pat = string_to_value(&command.args[1], cache);
-            let is = results.len() == 1 && results[0] == pat.as_ref();
-            if !command.negated && !is {
-                return Err(CkError::FailedCheck(
-                    format!(
-                        "{} matched to {:?}, but expected {:?}",
-                        &command.args[0],
-                        results,
-                        pat.as_ref()
-                    ),
-                    command,
+        CommandKind::CountIs { expected } => {
+            if *expected != matches.len() {
+                return Err(format!(
+                    "matched to `{matches:?}` with length {}, but expected length {expected}",
+                    matches.len(),
                 ));
-            } else {
-                is
             }
         }
-        // `set <name> = <jsonpath>`
-        CommandKind::Set => {
-            assert!(!command.negated, "`set` may not be negated");
-            assert_eq!(command.args.len(), 3);
-            assert_eq!(command.args[1], "=", "Expected an `=`");
-            let val = cache.value().clone();
-            let results = select(&val, &command.args[2]).unwrap();
-            assert_eq!(
-                results.len(),
-                1,
-                "Expected 1 match for `{}` (because of `set`): matched to {:?}",
-                command.args[2],
-                results
-            );
-            match results.len() {
-                0 => false,
-                1 => {
-                    let r = cache.variables.insert(command.args[0].clone(), results[0].clone());
-                    assert!(r.is_none(), "Name collision: {} is duplicated", command.args[0]);
-                    true
-                }
-                _ => {
-                    panic!(
-                        "Got multiple results in `set` for `{}`: {:?}",
-                        &command.args[2], results,
-                    );
-                }
-            }
+        CommandKind::Set { variable } => {
+            let value = get_one(&matches)?;
+            let r = cache.variables.insert(variable.to_owned(), value.clone());
+            assert!(r.is_none(), "name collision: {variable:?} is duplicated");
         }
-    };
+    }
 
-    if result == command.negated {
-        if command.negated {
-            Err(CkError::FailedCheck(
-                format!("`!{} {}` matched when it shouldn't", command.kind, command.args.join(" ")),
-                command,
-            ))
-        } else {
-            // FIXME: In the future, try 'peeling back' each step, and see at what level the match failed
-            Err(CkError::FailedCheck(
-                format!(
-                    "`{} {}` didn't match when it should",
-                    command.kind,
-                    command.args.join(" ")
-                ),
-                command,
-            ))
-        }
-    } else {
-        Ok(())
+    Ok(())
+}
+
+fn get_one<'a>(matches: &[&'a Value]) -> Result<&'a Value, String> {
+    match matches {
+        [] => Err("matched to no values".to_owned()),
+        [matched] => Ok(matched),
+        _ => Err(format!("matched to multiple values {matches:?}, but want exactly 1")),
     }
 }