diff --git a/docs/books/src/guide/check.md b/docs/books/src/guide/check.md index e46070a..a288d0f 100644 --- a/docs/books/src/guide/check.md +++ b/docs/books/src/guide/check.md @@ -30,6 +30,11 @@ The command exits with `1` when there are errors, and with `0` otherwise. `--str | A task defined twice, an alias that clashes with another name, a parameter declared twice | error | | A task or alias named like a command (`run`, `list`, ...) | warning | | An `imports` file that is missing or imported twice under the same name | error | +| A script that reads `MX_PARAM_` for a parameter the task does not declare | warning | +| A script that reads `MX_OUTPUT_` when no dependency (direct or indirect) of the task captures it, with the reason: the task does not set `capture = true`, is not a dependency, or does not exist | warning | +| A `sources` pattern that matches no files | warning | + +The last three are mistakes that do not fail a run: the variable is just empty, and a task whose `sources` match nothing is skipped as up to date forever. Patterns are looked up from the current directory (or the task's `dir`), as a run does. A `!` pattern is an exclusion and is not looked up. Headings that contain nothing to run (such as a document title) are not tasks and are not checked. diff --git a/src/check.rs b/src/check.rs index 10a1923..586d8d6 100644 --- a/src/check.rs +++ b/src/check.rs @@ -1,7 +1,8 @@ //! `mq-task check`: validate a task file without running anything. //! //! Everything wrong is reported at once: broken `meta` blocks, unknown -//! dependencies, cycles, missing runtimes, bad globs and conditions. +//! dependencies, cycles, missing runtimes, bad globs and conditions, and +//! scripts that read a parameter or a captured output that is not there. use std::collections::{BTreeMap, HashMap, HashSet}; use std::path::Path; @@ -119,6 +120,8 @@ pub fn check_sections( check_runtimes(sections, config, &mut out); check_patterns(sections, &mut out); check_conditions(sections, &mut out); + check_script_references(sections, &mut out); + check_sources_exist(sections, &mut out); out } @@ -396,6 +399,138 @@ fn check_patterns(sections: &[Section], out: &mut Vec) { } } +/// The names that follow `prefix` (`MX_PARAM_`) in `code`, such as `REGION` +/// for `$MX_PARAM_REGION`. A prefix with nothing after it (`MX_PARAM_${x}`) +/// names nothing and is skipped. +fn names_after<'a>(code: &'a str, prefix: &str) -> Vec<&'a str> { + let mut found = Vec::new(); + for (at, _) in code.match_indices(prefix) { + let word = |c: char| c.is_ascii_alphanumeric() || c == '_'; + if code[..at].chars().next_back().is_some_and(word) { + continue; + } + let rest = &code[at + prefix.len()..]; + let end = rest.find(|c: char| !word(c)).unwrap_or(rest.len()); + if end > 0 && !found.contains(&&rest[..end]) { + found.push(&rest[..end]); + } + } + found +} + +/// Scripts that read `MX_PARAM_*` for a parameter the task does not declare, +/// or `MX_OUTPUT_*` for a task that is not a dependency capturing its output. +/// Both are silently empty at run time, so a typo goes unnoticed. +fn check_script_references(sections: &[Section], out: &mut Vec) { + for section in sections.iter().filter(|s| runnable(s)) { + let code: String = section + .codes + .iter() + .map(|b| b.code.as_str()) + .collect::>() + .join("\n"); + let task = Some(section.title.as_str()); + + let declared: Vec = section + .params + .iter() + .map(|p| p.name.to_uppercase()) + .collect(); + for name in names_after(&code, "MX_PARAM_") { + if declared.iter().any(|d| d == name) { + continue; + } + let hint = match closest(name, declared.iter().map(String::as_str)) { + Some(near) => format!(" (did you mean `MX_PARAM_{}`?)", near), + None if declared.is_empty() => " (the task declares no params)".to_string(), + None => format!( + " (declared: {})", + section + .params + .iter() + .map(|p| p.name.as_str()) + .collect::>() + .join(", ") + ), + }; + out.push(Diagnostic::warning( + task, + format!( + "the script reads MX_PARAM_{} but no such parameter is declared{}", + name, hint + ), + )); + } + + let outputs = names_after(&code, "MX_OUTPUT_"); + if outputs.is_empty() { + continue; + } + let mut available: Vec<(String, &Section)> = Vec::new(); + let mut seen: HashSet<&str> = HashSet::new(); + let mut stack: Vec<&Section> = vec![section]; + while let Some(current) = stack.pop() { + for spec in ¤t.depends { + if let Some((target, _)) = resolve_call(sections, spec) + && seen.insert(target.title.as_str()) + { + if target.capture { + available.push((crate::exec::output_var(&target.title), target)); + } + stack.push(target); + } + } + } + for name in outputs { + let var = format!("MX_OUTPUT_{}", name); + if available.iter().any(|(v, _)| *v == var) { + continue; + } + let reason = match sections + .iter() + .find(|s| crate::exec::output_var(&s.title) == var) + { + Some(owner) if !owner.capture => { + format!("task '{}' does not set `capture = true`", owner.title) + } + Some(owner) => format!("task '{}' is not one of its dependencies", owner.title), + None => match closest(&var, available.iter().map(|(v, _)| v.as_str())) { + Some(near) => format!("no task has that name (did you mean `{}`?)", near), + None => "no task has that name".to_string(), + }, + }; + out.push(Diagnostic::warning( + task, + format!( + "the script reads {} but no dependency provides it: {}", + var, reason + ), + )); + } + } +} + +/// A `sources` pattern that matches no file never changes, so the task would +/// be skipped as up to date forever. Patterns are looked up the way a run +/// does, from the current directory (or the task's `dir`). +fn check_sources_exist(sections: &[Section], out: &mut Vec) { + for section in sections.iter().filter(|s| runnable(s)) { + for pattern in section.sources.iter().filter(|p| !p.starts_with('!')) { + // Invalid patterns are reported by `check_patterns`. + if glob::Pattern::new(pattern).is_err() { + continue; + } + let found = crate::cache::expand(std::slice::from_ref(pattern), section.dir.as_deref()); + if found.is_empty() { + out.push(Diagnostic::warning( + Some(section.title.as_str()), + format!("`sources` pattern '{}' matches no files", pattern), + )); + } + } + } +} + /// `if` expressions that do not parse or evaluate. fn check_conditions(sections: &[Section], out: &mut Vec) { let env = HashMap::new(); @@ -440,6 +575,106 @@ mod tests { diagnostics.iter().map(|d| d.message.clone()).collect() } + fn with_code(mut section: Section, code: &str) -> Section { + section.codes[0].code = code.into(); + section + } + + fn reference_warnings(sections: &[Section]) -> Vec { + messages(&check(sections)) + .into_iter() + .filter(|m| m.contains("script reads")) + .collect() + } + + #[test] + fn a_script_reading_an_undeclared_param_is_warned_about() { + let mut deploy = with_code(task("deploy", &[]), "echo $MX_PARAM_REGOIN ${MX_PARAM_ENV}"); + deploy.params = vec![ + crate::runner::ParamDef { + name: "region".into(), + ..Default::default() + }, + crate::runner::ParamDef { + name: "env".into(), + ..Default::default() + }, + ]; + let found = reference_warnings(&[deploy]); + assert_eq!(found.len(), 1, "{found:?}"); + assert!( + found[0].contains("MX_PARAM_REGOIN") + && found[0].contains("did you mean `MX_PARAM_REGION`?"), + "{found:?}" + ); + + let bare = with_code(task("x", &[]), "echo $MX_PARAM_A MX_PARAM_${n} XMX_PARAM_B"); + let found = reference_warnings(&[bare]); + assert_eq!(found.len(), 1, "{found:?}"); + assert!( + found[0].contains("the task declares no params"), + "{found:?}" + ); + } + + #[test] + fn a_script_reading_an_output_needs_a_capturing_dependency() { + let mut version = task("version", &[]); + version.capture = true; + let uses = |depends: &[&str]| { + with_code( + task("release", depends), + "echo $MX_OUTPUT_VERSION $MX_OUTPUT_VERSON", + ) + }; + + // Direct and transitive dependencies count; the typo does not. + let mut mid = task("mid", &["version"]); + mid.capture = false; + let found = reference_warnings(&[version.clone(), mid, uses(&["mid"])]); + assert_eq!(found.len(), 1, "{found:?}"); + assert!( + found[0].contains("MX_OUTPUT_VERSON") + && found[0].contains("did you mean `MX_OUTPUT_VERSION`?"), + "{found:?}" + ); + + // Not a dependency at all. + let found = reference_warnings(&[version.clone(), uses(&[])]); + assert!( + found + .iter() + .any(|m| m.contains("'version' is not one of its dependencies")), + "{found:?}" + ); + + // A dependency that does not capture. + let mut quiet = version; + quiet.capture = false; + let found = reference_warnings(&[quiet, uses(&["version"])]); + assert!( + found + .iter() + .any(|m| m.contains("'version' does not set `capture = true`")), + "{found:?}" + ); + } + + #[test] + fn a_sources_pattern_that_matches_nothing_is_warned_about() { + let dir = tempfile::tempdir().unwrap(); + std::fs::write(dir.path().join("a.rs"), "").unwrap(); + let mut build = task("build", &[]); + build.dir = Some(dir.path().to_path_buf()); + build.sources = vec!["*.rs".into(), "*.zig".into(), "!a.rs".into(), "[".into()]; + let found: Vec<_> = messages(&check(&[build])) + .into_iter() + .filter(|m| m.contains("matches no files")) + .collect(); + // `*.rs` exists, `!` patterns are exclusions, `[` is reported as invalid instead. + assert_eq!(found, ["`sources` pattern '*.zig' matches no files"]); + } + #[test] fn a_clean_file_has_no_diagnostics() { let sections = vec![task("build", &[]), task("test", &["build"])];