From 6f6908d474a60b810eebfd3e896469f46cb95c15 Mon Sep 17 00:00:00 2001 From: Leynos Date: Mon, 18 Aug 2025 08:39:05 +0100 Subject: [PATCH 01/10] Adopt miette for YAML diagnostics --- Cargo.lock | 144 ++++++++++++++++++++++++++++++++++++-- Cargo.toml | 2 + docs/netsuke-design.md | 32 +++++---- src/manifest.rs | 72 ++++++++++++++++++- tests/yaml_error_tests.rs | 26 +++++++ 5 files changed, 256 insertions(+), 20 deletions(-) create mode 100644 tests/yaml_error_tests.rs diff --git a/Cargo.lock b/Cargo.lock index bd7f99a8..86b72aa2 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -130,6 +130,15 @@ dependencies = [ "windows-targets 0.52.6", ] +[[package]] +name = "backtrace-ext" +version = "0.2.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "537beee3be4a18fb023b570f80e3ae28003db9167a751266b259926e25539d50" +dependencies = [ + "backtrace", +] + [[package]] name = "bitflags" version = "1.3.2" @@ -194,7 +203,7 @@ dependencies = [ "anstyle", "clap_lex", "strsim", - "terminal_size", + "terminal_size 0.4.2", ] [[package]] @@ -230,7 +239,7 @@ dependencies = [ "encode_unicode", "libc", "once_cell", - "unicode-width", + "unicode-width 0.2.1", "windows-sys 0.59.0", ] @@ -561,7 +570,7 @@ dependencies = [ "serde", "serde_json", "syn 2.0.104", - "textwrap", + "textwrap 0.16.2", "thiserror", "typed-builder", ] @@ -620,6 +629,12 @@ version = "0.5.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "2304e00983f87ffb38b55b444b5e3b60a884b5d30c0fca7d82fe33449bbe55ea" +[[package]] +name = "hermit-abi" +version = "0.5.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "fc0fef456e4baa96da950455cd02c081ca953b141298e41db3fc7e36b1da849c" + [[package]] name = "humantime" version = "2.2.0" @@ -690,6 +705,23 @@ dependencies = [ "libc", ] +[[package]] +name = "is-terminal" +version = "0.4.16" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "e04d7f318608d35d4b61ddd75cbdaee86b023ebe2bd5a66ee0915f0bf93095a9" +dependencies = [ + "hermit-abi", + "libc", + "windows-sys 0.59.0", +] + +[[package]] +name = "is_ci" +version = "1.2.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "7655c9839580ee829dfacba1d1278c2b7883e50a277ff7541299489d6bdfdc45" + [[package]] name = "is_terminal_polyfill" version = "1.70.1" @@ -799,6 +831,38 @@ version = "2.7.5" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "32a282da65faaf38286cf3be983213fcf1d2e2a58700e808f83f4ea9a4804bc0" +[[package]] +name = "miette" +version = "5.10.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "59bb584eaeeab6bd0226ccf3509a69d7936d148cf3d036ad350abe35e8c6856e" +dependencies = [ + "backtrace", + "backtrace-ext", + "is-terminal", + "miette-derive", + "once_cell", + "owo-colors", + "supports-color", + "supports-hyperlinks", + "supports-unicode", + "terminal_size 0.1.17", + "textwrap 0.15.2", + "thiserror", + "unicode-width 0.1.14", +] + +[[package]] +name = "miette-derive" +version = "5.10.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "49e7bc1560b95a3c4a25d03de42fe76ca718ab92d1a22a55b9b4cf67b3ae635c" +dependencies = [ + "proc-macro2", + "quote", + "syn 2.0.104", +] + [[package]] name = "minijinja" version = "2.11.0" @@ -882,6 +946,7 @@ dependencies = [ "insta", "itertools 0.12.1", "itoa", + "miette", "minijinja", "mockable", "mockall", @@ -891,6 +956,7 @@ dependencies = [ "serde", "serde_json", "serde_json_canonicalizer", + "serde_spanned", "serde_yml", "serial_test", "sha2", @@ -979,6 +1045,12 @@ version = "0.1.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "b15813163c1d831bf4a13c3610c05c0d03b39feb07f7e09fa234dac9b15aaf39" +[[package]] +name = "owo-colors" +version = "3.5.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "c1b04fb49957986fdce4d6ee7a65027d55d4b6d2265e5848bbb507b58ccfdb6f" + [[package]] name = "parking_lot" version = "0.12.4" @@ -1345,6 +1417,15 @@ dependencies = [ "serde_json", ] +[[package]] +name = "serde_spanned" +version = "1.0.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "40734c41988f7306bb04f0ecf60ec0f3f1caa34290e4e8ea471dcd3346483b83" +dependencies = [ + "serde", +] + [[package]] name = "serde_yml" version = "0.0.12" @@ -1446,6 +1527,34 @@ version = "0.11.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "7da8b5736845d9f2fcb837ea5d9e2628564b3b043a70948a3f0b778838c5fb4f" +[[package]] +name = "supports-color" +version = "2.1.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "d6398cde53adc3c4557306a96ce67b302968513830a77a95b2b17305d9719a89" +dependencies = [ + "is-terminal", + "is_ci", +] + +[[package]] +name = "supports-hyperlinks" +version = "2.1.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "f84231692eb0d4d41e4cdd0cabfdd2e6cd9e255e65f80c9aa7c98dd502b4233d" +dependencies = [ + "is-terminal", +] + +[[package]] +name = "supports-unicode" +version = "2.1.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "f850c19edd184a205e883199a261ed44471c81e39bd95b1357f5febbef00e77a" +dependencies = [ + "is-terminal", +] + [[package]] name = "syn" version = "1.0.109" @@ -1514,6 +1623,16 @@ dependencies = [ "windows-sys 0.59.0", ] +[[package]] +name = "terminal_size" +version = "0.1.17" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "633c1a546cee861a1a6d0dc69ebeca693bf4296661ba7852b9d21d159e0506df" +dependencies = [ + "libc", + "winapi", +] + [[package]] name = "terminal_size" version = "0.4.2" @@ -1539,6 +1658,17 @@ dependencies = [ "tempfile", ] +[[package]] +name = "textwrap" +version = "0.15.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "b7b3e525a49ec206798b40326a44121291b530c963cfb01018f63e135bac543d" +dependencies = [ + "smawk", + "unicode-linebreak", + "unicode-width 0.1.14", +] + [[package]] name = "textwrap" version = "0.16.2" @@ -1547,7 +1677,7 @@ checksum = "c13547615a44dc9c452a8a534638acdf07120d4b6847c8178705da06306a3057" dependencies = [ "smawk", "unicode-linebreak", - "unicode-width", + "unicode-width 0.2.1", ] [[package]] @@ -1700,6 +1830,12 @@ version = "0.1.5" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "3b09c83c3c29d37506a3e260c08c03743a6bb66a9cd432c6934ab501a190571f" +[[package]] +name = "unicode-width" +version = "0.1.14" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "7dd6e30e90baa6f72411720665d41d89b9a3d039dc45b8faea1ddd07f617f6af" + [[package]] name = "unicode-width" version = "0.2.1" diff --git a/Cargo.toml b/Cargo.toml index 0a4bd03b..a7cda91b 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -11,6 +11,8 @@ minijinja = "2.11.0" semver = { version = "1", features = ["serde"] } anyhow = "1" thiserror = "1" +miette = { version = "5", features = ["fancy"] } +serde_spanned = "1" sha2 = "0.10" itoa = "1" itertools = "0.12" diff --git a/docs/netsuke-design.md b/docs/netsuke-design.md index 40421add..15e58c9d 100644 --- a/docs/netsuke-design.md +++ b/docs/netsuke-design.md @@ -1269,12 +1269,13 @@ three fundamental questions: Found a tab character, which is not allowed. Hint: Use spaces for indentation instead."). -### 7.2 Crate Selection and Strategy: `anyhow` and `thiserror` +### 7.2 Crate Selection and Strategy: `anyhow`, `thiserror`, and `miette` -To implement this philosophy, Netsuke will adopt a hybrid error handling -strategy using the `anyhow` and `thiserror` crates. This is a common and highly -effective pattern in the Rust ecosystem for creating robust applications and -libraries.[^27] +To implement this philosophy, Netsuke adopts a hybrid error handling strategy +using the `anyhow`, `thiserror`, and `miette` crates. This is a common and +highly effective pattern in the Rust ecosystem for creating robust applications +and libraries.[^27] `miette` renders user-facing diagnostics, underlining spans +from `serde_spanned`. - `thiserror`: This crate will be used *within* Netsuke's internal library modules (e.g., `parser`, `ir`, `ninja_gen`) to define specific, structured @@ -1316,6 +1317,9 @@ pub enum IrGenError { `.with_context()` methods for adding high-level, human-readable context to errors as they bubble up the call stack.[^31] +- `miette`: Presents human-friendly diagnostics, leveraging spans from + `serde_spanned` to highlight exact error locations. + ### 7.3 Error Handling Flow The flow of an error from its origin to the user follows a clear path of @@ -1563,15 +1567,15 @@ goal. This table serves as a quick-reference guide to the core third-party crates selected for this project and the rationale for their inclusion. -| Component | Recommended Crate | Rationale | -| -------------- | ------------------ | ----------------------------------------------------------------------------------------------------------------------- | -| CLI Parsing | clap | The Rust standard for powerful, derive-based CLI development. | -| YAML Parsing | serde_yml | Mature, stable, and provides seamless integration with the serde framework. | -| Templating | minijinja | High compatibility with Jinja2, minimal dependencies, and supports runtime template loading. | -| Shell Quoting | shell-quote | A critical security component; provides robust, shell-specific escaping for command arguments. | -| Error Handling | anyhow + thiserror | An idiomatic and powerful combination for creating rich, contextual, and user-friendly error reports. | -| Logging | tracing | Structured, levelled diagnostic output for debugging and insight. | -| Versioning | semver | The standard library for parsing and evaluating Semantic Versioning strings, essential for the `netsuke_version` field. | +| Component | Recommended Crate | Rationale | +| -------------- | ------------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------- | +| CLI Parsing | clap | The Rust standard for powerful, derive-based CLI development. | +| YAML Parsing | serde_yml | Mature, stable, and provides seamless integration with the serde framework. | +| Templating | minijinja | High compatibility with Jinja2, minimal dependencies, and supports runtime template loading. | +| Shell Quoting | shell-quote | A critical security component; provides robust, shell-specific escaping for command arguments. | +| Error Handling | anyhow + thiserror + miette + serde_spanned | An idiomatic and powerful combination for creating rich, contextual, and user-friendly error reports with precise source spans. | +| Logging | tracing | Structured, levelled diagnostic output for debugging and insight. | +| Versioning | semver | The standard library for parsing and evaluating Semantic Versioning strings, essential for the `netsuke_version` field. | ### 9.3 Future Enhancements diff --git a/src/manifest.rs b/src/manifest.rs index 7f2bfd00..462c0c43 100644 --- a/src/manifest.rs +++ b/src/manifest.rs @@ -6,13 +6,81 @@ use crate::ast::{NetsukeManifest, Recipe, StringOrList, Target, Vars}; use anyhow::{Context, Result, anyhow}; +use miette::{Diagnostic, NamedSource, SourceSpan}; use minijinja::{Environment, UndefinedBehavior, context, value::Value}; +use serde_spanned::Spanned; +use serde_yml::{Error as YamlError, Location}; use serde_yml::{Mapping as YamlMapping, Value as YamlValue}; +use std::fmt::Write; use std::{fs, path::Path}; +use thiserror::Error; -const ERR_INITIAL_YAML_PARSE: &str = "initial YAML parse error"; const ERR_MANIFEST_PARSE: &str = "manifest parse error"; +// Compute a narrow highlight span from a location. +fn to_span(src: &str, loc: Location) -> SourceSpan { + let at = loc.index(); + let end = if src.as_bytes().get(at).is_some_and(|b| *b != b'\n') { + at + 1 + } else { + at + }; + let span = Spanned::new(at..end, ()); + SourceSpan::new(span.span().start.into(), span.span().len().into()) +} + +#[derive(Debug, Error, Diagnostic)] +#[error("{message}")] +struct YamlDiagnostic { + #[source_code] + src: NamedSource, + #[label("{label}")] + span: Option, + #[help] + help: Option, + #[source] + source: Option, + #[diagnostic(code(netsuke.yaml.parse))] + message: String, + label: String, +} + +fn hint_for(err_str: &str, src: &str) -> Option { + let lower = err_str.to_lowercase(); + if src.contains('\t') { + Some("Use spaces for indentation; tabs are invalid in YAML.".into()) + } else if lower.contains("did not find expected '-'") { + Some("Start list items with '-' and ensure proper indentation.".into()) + } else if lower.contains("expected ':'") { + Some("Ensure each key is followed by ':' separating key and value.".into()) + } else { + None + } +} + +fn map_yaml_error(err: &YamlError, src: &str) -> anyhow::Error { + let (line, col, span) = err.location().map_or((1, 1, None), |l| { + (l.line(), l.column(), Some(to_span(src, l))) + }); + let err_str = err.to_string(); + let hint = hint_for(&err_str, src); + let mut message = format!("YAML parse error at line {line}, column {col}: {err_str}"); + if let Some(h) = &hint { + write!(message, " Hint: {h}").expect("string write"); + } + + let diag = YamlDiagnostic { + src: NamedSource::new("manifest.yml", src.to_string()), + span, + help: hint, + source: Some(anyhow::Error::msg(err_str.clone())), + message, + label: "parse error here".into(), + }; + + anyhow::Error::new(diag) +} + /// Parse a manifest string using Jinja for value templating. /// /// The input YAML must be valid on its own. Jinja expressions are evaluated @@ -22,7 +90,7 @@ const ERR_MANIFEST_PARSE: &str = "manifest parse error"; /// /// Returns an error if YAML parsing or Jinja evaluation fails. pub fn from_str(yaml: &str) -> Result { - let mut doc: YamlValue = serde_yml::from_str(yaml).context(ERR_INITIAL_YAML_PARSE)?; + let mut doc: YamlValue = serde_yml::from_str(yaml).map_err(|e| map_yaml_error(&e, yaml))?; let mut env = Environment::new(); env.set_undefined_behavior(UndefinedBehavior::Strict); diff --git a/tests/yaml_error_tests.rs b/tests/yaml_error_tests.rs new file mode 100644 index 00000000..5200d5c2 --- /dev/null +++ b/tests/yaml_error_tests.rs @@ -0,0 +1,26 @@ +use netsuke::manifest; + +#[test] +fn reports_line_and_column_with_tab_hint() { + let yaml = "targets:\n\t- name: test\n"; + let err = manifest::from_str(yaml).expect_err("parse should fail"); + let msg = err.to_string(); + assert!(msg.contains("line 2, column 1"), "missing location: {msg}"); + assert!( + msg.contains("Use spaces for indentation"), + "missing hint: {msg}" + ); +} + +#[test] +fn suggests_colon_when_missing() { + let yaml = "targets:\n - name: hi\n command echo\n"; + let err = manifest::from_str(yaml).expect_err("parse should fail"); + let msg = err.to_string(); + assert!(msg.contains("line 3"), "missing line info: {msg}"); + assert!(msg.contains("expected ':'"), "missing error detail: {msg}"); + assert!( + msg.contains("Ensure each key is followed by ':'"), + "missing suggestion: {msg}" + ); +} From 8af78d888983a861493ce8815b9a12a218841f65 Mon Sep 17 00:00:00 2001 From: Leynos Date: Mon, 18 Aug 2025 18:45:10 +0100 Subject: [PATCH 02/10] Test YAML diagnostics via miette --- src/manifest.rs | 9 +++------ tests/yaml_error_tests.rs | 19 ++++++++++++++++--- 2 files changed, 19 insertions(+), 9 deletions(-) diff --git a/src/manifest.rs b/src/manifest.rs index 462c0c43..e0200431 100644 --- a/src/manifest.rs +++ b/src/manifest.rs @@ -11,7 +11,6 @@ use minijinja::{Environment, UndefinedBehavior, context, value::Value}; use serde_spanned::Spanned; use serde_yml::{Error as YamlError, Location}; use serde_yml::{Mapping as YamlMapping, Value as YamlValue}; -use std::fmt::Write; use std::{fs, path::Path}; use thiserror::Error; @@ -31,7 +30,8 @@ fn to_span(src: &str, loc: Location) -> SourceSpan { #[derive(Debug, Error, Diagnostic)] #[error("{message}")] -struct YamlDiagnostic { +#[doc(hidden)] +pub struct YamlDiagnostic { #[source_code] src: NamedSource, #[label("{label}")] @@ -64,10 +64,7 @@ fn map_yaml_error(err: &YamlError, src: &str) -> anyhow::Error { }); let err_str = err.to_string(); let hint = hint_for(&err_str, src); - let mut message = format!("YAML parse error at line {line}, column {col}: {err_str}"); - if let Some(h) = &hint { - write!(message, " Hint: {h}").expect("string write"); - } + let message = format!("YAML parse error at line {line}, column {col}: {err_str}"); let diag = YamlDiagnostic { src: NamedSource::new("manifest.yml", src.to_string()), diff --git a/tests/yaml_error_tests.rs b/tests/yaml_error_tests.rs index 5200d5c2..2f231562 100644 --- a/tests/yaml_error_tests.rs +++ b/tests/yaml_error_tests.rs @@ -1,10 +1,17 @@ -use netsuke::manifest; +use miette::GraphicalReportHandler; +use netsuke::manifest::{self, YamlDiagnostic}; #[test] fn reports_line_and_column_with_tab_hint() { let yaml = "targets:\n\t- name: test\n"; let err = manifest::from_str(yaml).expect_err("parse should fail"); - let msg = err.to_string(); + let diag = err + .downcast_ref::() + .expect("diagnostic type"); + let mut msg = String::new(); + GraphicalReportHandler::new() + .render_report(&mut msg, diag) + .expect("render yaml error"); assert!(msg.contains("line 2, column 1"), "missing location: {msg}"); assert!( msg.contains("Use spaces for indentation"), @@ -16,7 +23,13 @@ fn reports_line_and_column_with_tab_hint() { fn suggests_colon_when_missing() { let yaml = "targets:\n - name: hi\n command echo\n"; let err = manifest::from_str(yaml).expect_err("parse should fail"); - let msg = err.to_string(); + let diag = err + .downcast_ref::() + .expect("diagnostic type"); + let mut msg = String::new(); + GraphicalReportHandler::new() + .render_report(&mut msg, diag) + .expect("render yaml error"); assert!(msg.contains("line 3"), "missing line info: {msg}"); assert!(msg.contains("expected ':'"), "missing error detail: {msg}"); assert!( From cd371b03041b80d99279fc396b9f656de95f0b14 Mon Sep 17 00:00:00 2001 From: Leynos Date: Mon, 18 Aug 2025 19:41:24 +0100 Subject: [PATCH 03/10] Parametrise YAML error diagnostic tests --- tests/yaml_error_tests.rs | 45 ++++++++++++++------------------------- 1 file changed, 16 insertions(+), 29 deletions(-) diff --git a/tests/yaml_error_tests.rs b/tests/yaml_error_tests.rs index 2f231562..f1ed71e0 100644 --- a/tests/yaml_error_tests.rs +++ b/tests/yaml_error_tests.rs @@ -1,39 +1,26 @@ use miette::GraphicalReportHandler; -use netsuke::manifest::{self, YamlDiagnostic}; +use netsuke::manifest; +use rstest::rstest; -#[test] -fn reports_line_and_column_with_tab_hint() { - let yaml = "targets:\n\t- name: test\n"; +#[rstest] +#[case( + "targets:\n\t- name: test\n", + &["line 2, column 1", "Use spaces for indentation"], +)] +#[case( + "targets:\n - name: hi\n command echo\n", + &["line 3", "expected ':'", "Ensure each key is followed by ':'"], +)] +fn yaml_diagnostics_are_actionable(#[case] yaml: &str, #[case] needles: &[&str]) { let err = manifest::from_str(yaml).expect_err("parse should fail"); let diag = err - .downcast_ref::() + .downcast_ref::() .expect("diagnostic type"); let mut msg = String::new(); GraphicalReportHandler::new() .render_report(&mut msg, diag) .expect("render yaml error"); - assert!(msg.contains("line 2, column 1"), "missing location: {msg}"); - assert!( - msg.contains("Use spaces for indentation"), - "missing hint: {msg}" - ); -} - -#[test] -fn suggests_colon_when_missing() { - let yaml = "targets:\n - name: hi\n command echo\n"; - let err = manifest::from_str(yaml).expect_err("parse should fail"); - let diag = err - .downcast_ref::() - .expect("diagnostic type"); - let mut msg = String::new(); - GraphicalReportHandler::new() - .render_report(&mut msg, diag) - .expect("render yaml error"); - assert!(msg.contains("line 3"), "missing line info: {msg}"); - assert!(msg.contains("expected ':'"), "missing error detail: {msg}"); - assert!( - msg.contains("Ensure each key is followed by ':'"), - "missing suggestion: {msg}" - ); + for needle in needles { + assert!(msg.contains(needle), "missing: {needle}\nmessage: {msg}"); + } } From e74e0047513800ee7680e9f947233051fad3a420 Mon Sep 17 00:00:00 2001 From: Leynos Date: Mon, 18 Aug 2025 23:28:59 +0100 Subject: [PATCH 04/10] Scope YAML hints and name diagnostics --- Cargo.lock | 91 ++++++++------------------------------- Cargo.toml | 3 +- docs/netsuke-design.md | 26 +++++------ src/manifest.rs | 70 +++++++++++++++++++++--------- tests/yaml_error_tests.rs | 14 ++++++ 5 files changed, 95 insertions(+), 109 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 86b72aa2..f3927c87 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -203,7 +203,7 @@ dependencies = [ "anstyle", "clap_lex", "strsim", - "terminal_size 0.4.2", + "terminal_size", ] [[package]] @@ -570,7 +570,7 @@ dependencies = [ "serde", "serde_json", "syn 2.0.104", - "textwrap 0.16.2", + "textwrap", "thiserror", "typed-builder", ] @@ -629,12 +629,6 @@ version = "0.5.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "2304e00983f87ffb38b55b444b5e3b60a884b5d30c0fca7d82fe33449bbe55ea" -[[package]] -name = "hermit-abi" -version = "0.5.2" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "fc0fef456e4baa96da950455cd02c081ca953b141298e41db3fc7e36b1da849c" - [[package]] name = "humantime" version = "2.2.0" @@ -705,17 +699,6 @@ dependencies = [ "libc", ] -[[package]] -name = "is-terminal" -version = "0.4.16" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "e04d7f318608d35d4b61ddd75cbdaee86b023ebe2bd5a66ee0915f0bf93095a9" -dependencies = [ - "hermit-abi", - "libc", - "windows-sys 0.59.0", -] - [[package]] name = "is_ci" version = "1.2.0" @@ -833,30 +816,28 @@ checksum = "32a282da65faaf38286cf3be983213fcf1d2e2a58700e808f83f4ea9a4804bc0" [[package]] name = "miette" -version = "5.10.0" +version = "7.6.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "59bb584eaeeab6bd0226ccf3509a69d7936d148cf3d036ad350abe35e8c6856e" +checksum = "5f98efec8807c63c752b5bd61f862c165c115b0a35685bdcfd9238c7aeb592b7" dependencies = [ "backtrace", "backtrace-ext", - "is-terminal", + "cfg-if", "miette-derive", - "once_cell", "owo-colors", "supports-color", "supports-hyperlinks", "supports-unicode", - "terminal_size 0.1.17", - "textwrap 0.15.2", - "thiserror", + "terminal_size", + "textwrap", "unicode-width 0.1.14", ] [[package]] name = "miette-derive" -version = "5.10.0" +version = "7.6.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "49e7bc1560b95a3c4a25d03de42fe76ca718ab92d1a22a55b9b4cf67b3ae635c" +checksum = "db5b29714e950dbb20d5e6f74f9dcec4edbcc1067bb7f8ed198c097b8c1a818b" dependencies = [ "proc-macro2", "quote", @@ -956,7 +937,6 @@ dependencies = [ "serde", "serde_json", "serde_json_canonicalizer", - "serde_spanned", "serde_yml", "serial_test", "sha2", @@ -1047,9 +1027,9 @@ checksum = "b15813163c1d831bf4a13c3610c05c0d03b39feb07f7e09fa234dac9b15aaf39" [[package]] name = "owo-colors" -version = "3.5.0" +version = "4.2.2" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "c1b04fb49957986fdce4d6ee7a65027d55d4b6d2265e5848bbb507b58ccfdb6f" +checksum = "48dd4f4a2c8405440fd0462561f0e5806bd0f77e86f51c761481bdd4018b545e" [[package]] name = "parking_lot" @@ -1417,15 +1397,6 @@ dependencies = [ "serde_json", ] -[[package]] -name = "serde_spanned" -version = "1.0.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "40734c41988f7306bb04f0ecf60ec0f3f1caa34290e4e8ea471dcd3346483b83" -dependencies = [ - "serde", -] - [[package]] name = "serde_yml" version = "0.0.12" @@ -1529,31 +1500,24 @@ checksum = "7da8b5736845d9f2fcb837ea5d9e2628564b3b043a70948a3f0b778838c5fb4f" [[package]] name = "supports-color" -version = "2.1.0" +version = "3.0.2" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "d6398cde53adc3c4557306a96ce67b302968513830a77a95b2b17305d9719a89" +checksum = "c64fc7232dd8d2e4ac5ce4ef302b1d81e0b80d055b9d77c7c4f51f6aa4c867d6" dependencies = [ - "is-terminal", "is_ci", ] [[package]] name = "supports-hyperlinks" -version = "2.1.0" +version = "3.1.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "f84231692eb0d4d41e4cdd0cabfdd2e6cd9e255e65f80c9aa7c98dd502b4233d" -dependencies = [ - "is-terminal", -] +checksum = "804f44ed3c63152de6a9f90acbea1a110441de43006ea51bcce8f436196a288b" [[package]] name = "supports-unicode" -version = "2.1.0" +version = "3.0.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "f850c19edd184a205e883199a261ed44471c81e39bd95b1357f5febbef00e77a" -dependencies = [ - "is-terminal", -] +checksum = "b7401a30af6cb5818bb64852270bb722533397edcfc7344954a38f420819ece2" [[package]] name = "syn" @@ -1623,16 +1587,6 @@ dependencies = [ "windows-sys 0.59.0", ] -[[package]] -name = "terminal_size" -version = "0.1.17" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "633c1a546cee861a1a6d0dc69ebeca693bf4296661ba7852b9d21d159e0506df" -dependencies = [ - "libc", - "winapi", -] - [[package]] name = "terminal_size" version = "0.4.2" @@ -1658,17 +1612,6 @@ dependencies = [ "tempfile", ] -[[package]] -name = "textwrap" -version = "0.15.2" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "b7b3e525a49ec206798b40326a44121291b530c963cfb01018f63e135bac543d" -dependencies = [ - "smawk", - "unicode-linebreak", - "unicode-width 0.1.14", -] - [[package]] name = "textwrap" version = "0.16.2" diff --git a/Cargo.toml b/Cargo.toml index a7cda91b..ff49245b 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -11,8 +11,7 @@ minijinja = "2.11.0" semver = { version = "1", features = ["serde"] } anyhow = "1" thiserror = "1" -miette = { version = "5", features = ["fancy"] } -serde_spanned = "1" +miette = { version = "7.6.0", features = ["fancy"] } sha2 = "0.10" itoa = "1" itertools = "0.12" diff --git a/docs/netsuke-design.md b/docs/netsuke-design.md index 15e58c9d..bb17d416 100644 --- a/docs/netsuke-design.md +++ b/docs/netsuke-design.md @@ -1274,8 +1274,8 @@ three fundamental questions: To implement this philosophy, Netsuke adopts a hybrid error handling strategy using the `anyhow`, `thiserror`, and `miette` crates. This is a common and highly effective pattern in the Rust ecosystem for creating robust applications -and libraries.[^27] `miette` renders user-facing diagnostics, underlining spans -from `serde_spanned`. +and libraries.[^27] `miette` renders user-facing diagnostics, computing spans +directly from parser locations. - `thiserror`: This crate will be used *within* Netsuke's internal library modules (e.g., `parser`, `ir`, `ninja_gen`) to define specific, structured @@ -1317,8 +1317,8 @@ pub enum IrGenError { `.with_context()` methods for adding high-level, human-readable context to errors as they bubble up the call stack.[^31] -- `miette`: Presents human-friendly diagnostics, leveraging spans from - `serde_spanned` to highlight exact error locations. +- `miette`: Presents human-friendly diagnostics, highlighting exact error + locations with computed spans. ### 7.3 Error Handling Flow @@ -1567,15 +1567,15 @@ goal. This table serves as a quick-reference guide to the core third-party crates selected for this project and the rationale for their inclusion. -| Component | Recommended Crate | Rationale | -| -------------- | ------------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------- | -| CLI Parsing | clap | The Rust standard for powerful, derive-based CLI development. | -| YAML Parsing | serde_yml | Mature, stable, and provides seamless integration with the serde framework. | -| Templating | minijinja | High compatibility with Jinja2, minimal dependencies, and supports runtime template loading. | -| Shell Quoting | shell-quote | A critical security component; provides robust, shell-specific escaping for command arguments. | -| Error Handling | anyhow + thiserror + miette + serde_spanned | An idiomatic and powerful combination for creating rich, contextual, and user-friendly error reports with precise source spans. | -| Logging | tracing | Structured, levelled diagnostic output for debugging and insight. | -| Versioning | semver | The standard library for parsing and evaluating Semantic Versioning strings, essential for the `netsuke_version` field. | +| Component | Recommended Crate | Rationale | +| -------------- | --------------------------- | ------------------------------------------------------------------------------------------------------------------------------- | +| CLI Parsing | clap | The Rust standard for powerful, derive-based CLI development. | +| YAML Parsing | serde_yml | Mature, stable, and provides seamless integration with the serde framework. | +| Templating | minijinja | High compatibility with Jinja2, minimal dependencies, and supports runtime template loading. | +| Shell Quoting | shell-quote | A critical security component; provides robust, shell-specific escaping for command arguments. | +| Error Handling | anyhow + thiserror + miette | An idiomatic and powerful combination for creating rich, contextual, and user-friendly error reports with precise source spans. | +| Logging | tracing | Structured, levelled diagnostic output for debugging and insight. | +| Versioning | semver | The standard library for parsing and evaluating Semantic Versioning strings, essential for the `netsuke_version` field. | ### 9.3 Future Enhancements diff --git a/src/manifest.rs b/src/manifest.rs index e0200431..df648584 100644 --- a/src/manifest.rs +++ b/src/manifest.rs @@ -8,7 +8,6 @@ use crate::ast::{NetsukeManifest, Recipe, StringOrList, Target, Vars}; use anyhow::{Context, Result, anyhow}; use miette::{Diagnostic, NamedSource, SourceSpan}; use minijinja::{Environment, UndefinedBehavior, context, value::Value}; -use serde_spanned::Spanned; use serde_yml::{Error as YamlError, Location}; use serde_yml::{Mapping as YamlMapping, Value as YamlValue}; use std::{fs, path::Path}; @@ -24,32 +23,50 @@ fn to_span(src: &str, loc: Location) -> SourceSpan { } else { at }; - let span = Spanned::new(at..end, ()); - SourceSpan::new(span.span().start.into(), span.span().len().into()) + let len = end.saturating_sub(at); + SourceSpan::new(at.into(), len) } #[derive(Debug, Error, Diagnostic)] #[error("{message}")] +#[diagnostic(code(netsuke::yaml::parse))] #[doc(hidden)] pub struct YamlDiagnostic { #[source_code] - src: NamedSource, - #[label("{label}")] + src: NamedSource, + #[label("parse error here")] span: Option, #[help] help: Option, #[source] source: Option, - #[diagnostic(code(netsuke.yaml.parse))] message: String, - label: String, } -fn hint_for(err_str: &str, src: &str) -> Option { +fn hint_for(err_str: &str, src: &str, loc: Option) -> Option { let lower = err_str.to_lowercase(); - if src.contains('\t') { - Some("Use spaces for indentation; tabs are invalid in YAML.".into()) - } else if lower.contains("did not find expected '-'") { + if let Some(loc) = loc { + let idx = loc.index(); + let bytes = src.as_bytes(); + let line_start = bytes + .get(..idx) + .and_then(|b| b.iter().rposition(|b| *b == b'\n').map(|p| p + 1)) + .unwrap_or(0); + let line_end = bytes + .get(idx..) + .and_then(|b| b.iter().position(|b| *b == b'\n').map(|p| idx + p)) + .unwrap_or(bytes.len()); + if bytes + .get(line_start..line_end) + .unwrap_or(&[]) + .iter() + .take_while(|b| **b == b' ' || **b == b'\t') + .any(|b| *b == b'\t') + { + return Some("Use spaces for indentation; tabs are invalid in YAML.".into()); + } + } + if lower.contains("did not find expected '-'") { Some("Start list items with '-' and ensure proper indentation.".into()) } else if lower.contains("expected ':'") { Some("Ensure each key is followed by ':' separating key and value.".into()) @@ -58,21 +75,21 @@ fn hint_for(err_str: &str, src: &str) -> Option { } } -fn map_yaml_error(err: &YamlError, src: &str) -> anyhow::Error { - let (line, col, span) = err.location().map_or((1, 1, None), |l| { +fn map_yaml_error(err: &YamlError, src: &str, name: &str) -> anyhow::Error { + let loc = err.location(); + let (line, col, span) = loc.map_or((1, 1, None), |l| { (l.line(), l.column(), Some(to_span(src, l))) }); let err_str = err.to_string(); - let hint = hint_for(&err_str, src); + let hint = hint_for(&err_str, src, loc); let message = format!("YAML parse error at line {line}, column {col}: {err_str}"); let diag = YamlDiagnostic { - src: NamedSource::new("manifest.yml", src.to_string()), + src: NamedSource::new(name, src.to_string()), span, help: hint, - source: Some(anyhow::Error::msg(err_str.clone())), + source: Some(anyhow::Error::msg(err_str)), message, - label: "parse error here".into(), }; anyhow::Error::new(diag) @@ -86,8 +103,9 @@ fn map_yaml_error(err: &YamlError, src: &str) -> anyhow::Error { /// # Errors /// /// Returns an error if YAML parsing or Jinja evaluation fails. -pub fn from_str(yaml: &str) -> Result { - let mut doc: YamlValue = serde_yml::from_str(yaml).map_err(|e| map_yaml_error(&e, yaml))?; +fn from_str_named(yaml: &str, name: &str) -> Result { + let mut doc: YamlValue = + serde_yml::from_str(yaml).map_err(|e| map_yaml_error(&e, yaml, name))?; let mut env = Environment::new(); env.set_undefined_behavior(UndefinedBehavior::Strict); @@ -109,6 +127,18 @@ pub fn from_str(yaml: &str) -> Result { render_manifest(manifest, &env) } +/// Parse a manifest string using Jinja for value templating. +/// +/// The input YAML must be valid on its own. Jinja expressions are evaluated +/// only inside recognised string fields and the `foreach` and `when` keys. +/// +/// # Errors +/// +/// Returns an error if YAML parsing or Jinja evaluation fails. +pub fn from_str(yaml: &str) -> Result { + from_str_named(yaml, "Netsukefile") +} + /// Expand `foreach` entries within the raw YAML document. fn expand_foreach(doc: &mut YamlValue, env: &Environment) -> Result<()> { let Some(targets) = doc.get_mut("targets").and_then(|v| v.as_sequence_mut()) else { @@ -304,5 +334,5 @@ pub fn from_path(path: impl AsRef) -> Result { let path_ref = path.as_ref(); let data = fs::read_to_string(path_ref) .with_context(|| format!("Failed to read {}", path_ref.display()))?; - from_str(&data) + from_str_named(&data, &path_ref.display().to_string()) } diff --git a/tests/yaml_error_tests.rs b/tests/yaml_error_tests.rs index f1ed71e0..89b036a6 100644 --- a/tests/yaml_error_tests.rs +++ b/tests/yaml_error_tests.rs @@ -11,6 +11,20 @@ use rstest::rstest; "targets:\n - name: hi\n command echo\n", &["line 3", "expected ':'", "Ensure each key is followed by ':'"], )] +#[case( + concat!( + "targets:\n", + " - name: ok\n", + " command: echo\n", + " name: missing\n", + " command: echo\n", + ), + &["line 4", "did not find expected '-'", "Start list items with '-'"], +)] +#[case( + "targets:\n - command: [echo\n", + &["line 2", "did not find expected ',' or ']'"], +)] fn yaml_diagnostics_are_actionable(#[case] yaml: &str, #[case] needles: &[&str]) { let err = manifest::from_str(yaml).expect_err("parse should fail"); let diag = err From eab64f12cc523cff9408dae81562efe84ec925ec Mon Sep 17 00:00:00 2001 From: Leynos Date: Tue, 19 Aug 2025 00:47:58 +0100 Subject: [PATCH 05/10] Cover unhinted YAML parse errors --- Cargo.lock | 19 +++++++++++ Cargo.toml | 1 + src/manifest.rs | 69 +++++++++++++++++++++++++-------------- src/runner.rs | 10 ++++-- tests/ast_tests.rs | 3 +- tests/runner_tests.rs | 2 +- tests/steps/ir_steps.rs | 2 +- tests/yaml_error_tests.rs | 20 ++++++++---- 8 files changed, 90 insertions(+), 36 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index f3927c87..0ad2229a 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -940,6 +940,7 @@ dependencies = [ "serde_yml", "serial_test", "sha2", + "strip-ansi-escapes", "tempfile", "test_support", "thiserror", @@ -1492,6 +1493,15 @@ version = "0.3.2" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "b7c388c1b5e93756d0c740965c41e8822f866621d41acbdf6336a6a168f8840c" +[[package]] +name = "strip-ansi-escapes" +version = "0.2.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "2a8f8038e7e7969abb3f1b7c2a811225e9296da208539e0f79c5251d6cac0025" +dependencies = [ + "vte", +] + [[package]] name = "strsim" version = "0.11.1" @@ -1803,6 +1813,15 @@ version = "0.9.5" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "0b928f33d975fc6ad9f86c8f283853ad26bdd5b10b7f1542aa2fa15e2289105a" +[[package]] +name = "vte" +version = "0.14.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "231fdcd7ef3037e8330d8e17e61011a2c244126acc0a982f4040ac3f9f0bc077" +dependencies = [ + "memchr", +] + [[package]] name = "wait-timeout" version = "0.2.1" diff --git a/Cargo.toml b/Cargo.toml index ff49245b..7a929fbe 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -72,6 +72,7 @@ mockable = { version = "0.3", features = ["mock"] } serial_test = "3" mockall = "0.11" test_support = { path = "test_support" } +strip-ansi-escapes = "0.2" [[test]] name = "cucumber" diff --git a/src/manifest.rs b/src/manifest.rs index df648584..ae6862fe 100644 --- a/src/manifest.rs +++ b/src/manifest.rs @@ -5,8 +5,7 @@ //! evaluated only within string values or the `foreach` and `when` keys. use crate::ast::{NetsukeManifest, Recipe, StringOrList, Target, Vars}; -use anyhow::{Context, Result, anyhow}; -use miette::{Diagnostic, NamedSource, SourceSpan}; +use miette::{Context, Diagnostic, IntoDiagnostic, NamedSource, Report, Result, SourceSpan}; use minijinja::{Environment, UndefinedBehavior, context, value::Value}; use serde_yml::{Error as YamlError, Location}; use serde_yml::{Mapping as YamlMapping, Value as YamlValue}; @@ -39,7 +38,7 @@ pub struct YamlDiagnostic { #[help] help: Option, #[source] - source: Option, + source: YamlError, message: String, } @@ -75,7 +74,7 @@ fn hint_for(err_str: &str, src: &str, loc: Option) -> Option { } } -fn map_yaml_error(err: &YamlError, src: &str, name: &str) -> anyhow::Error { +fn map_yaml_error(err: YamlError, src: &str, name: &str) -> Report { let loc = err.location(); let (line, col, span) = loc.map_or((1, 1, None), |l| { (l.line(), l.column(), Some(to_span(src, l))) @@ -88,11 +87,11 @@ fn map_yaml_error(err: &YamlError, src: &str, name: &str) -> anyhow::Error { src: NamedSource::new(name, src.to_string()), span, help: hint, - source: Some(anyhow::Error::msg(err_str)), + source: err, message, }; - anyhow::Error::new(diag) + Report::new(diag) } /// Parse a manifest string using Jinja for value templating. @@ -105,7 +104,7 @@ fn map_yaml_error(err: &YamlError, src: &str, name: &str) -> anyhow::Error { /// Returns an error if YAML parsing or Jinja evaluation fails. fn from_str_named(yaml: &str, name: &str) -> Result { let mut doc: YamlValue = - serde_yml::from_str(yaml).map_err(|e| map_yaml_error(&e, yaml, name))?; + serde_yml::from_str(yaml).map_err(|e| map_yaml_error(e, yaml, name))?; let mut env = Environment::new(); env.set_undefined_behavior(UndefinedBehavior::Strict); @@ -114,7 +113,7 @@ fn from_str_named(yaml: &str, name: &str) -> Result { for (k, v) in vars { let key = k .as_str() - .ok_or_else(|| anyhow!("non-string key in 'vars' mapping: {k:?}"))? + .ok_or_else(|| Report::msg(format!("non-string key in 'vars' mapping: {k:?}")))? .to_string(); env.add_global(key, Value::from_serialize(v)); } @@ -122,7 +121,9 @@ fn from_str_named(yaml: &str, name: &str) -> Result { expand_foreach(&mut doc, &env)?; - let manifest: NetsukeManifest = serde_yml::from_value(doc).context(ERR_MANIFEST_PARSE)?; + let manifest: NetsukeManifest = serde_yml::from_value(doc) + .into_diagnostic() + .wrap_err(ERR_MANIFEST_PARSE)?; render_manifest(manifest, &env) } @@ -185,7 +186,8 @@ fn parse_foreach_values(expr_val: &YamlValue, env: &Environment) -> Result R None => YamlMapping::new(), Some(YamlValue::Mapping(m)) => m, Some(other) => { - return Err(anyhow!("target.vars must be a mapping, got: {other:?}")); + return Err(Report::msg(format!( + "target.vars must be a mapping, got: {other:?}" + ))); } }; vars.insert( YamlValue::String("item".into()), - serde_yml::to_value(item).context("serialise item")?, + serde_yml::to_value(item) + .into_diagnostic() + .wrap_err("serialise item")?, ); vars.insert( YamlValue::String("index".into()), @@ -229,14 +235,16 @@ fn inject_iteration_vars(map: &mut YamlMapping, item: &Value, index: usize) -> R fn as_str<'a>(value: &'a YamlValue, field: &str) -> Result<&'a str> { value .as_str() - .with_context(|| format!("{field} must be a string expression")) + .ok_or_else(|| Report::msg(format!("{field} must be a string expression"))) } fn eval_expression(env: &Environment, name: &str, expr: &str, ctx: Value) -> Result { env.compile_expression(expr) - .with_context(|| format!("{name} expression parse error"))? + .into_diagnostic() + .wrap_err_with(|| format!("{name} expression parse error"))? .eval(ctx) - .with_context(|| format!("{name} evaluation error")) + .into_diagnostic() + .wrap_err_with(|| format!("{name} evaluation error")) } /// Render all templated strings in the manifest. @@ -257,19 +265,22 @@ fn render_rule(rule: &mut crate::ast::Rule, env: &Environment) -> Result<()> { if let Some(desc) = &mut rule.description { *desc = env .render_str(desc, context! {}) - .context("render rule description")?; + .into_diagnostic() + .wrap_err("render rule description")?; } render_string_or_list(&mut rule.deps, env, &Vars::new())?; match &mut rule.recipe { Recipe::Command { command } => { *command = env .render_str(command, context! {}) - .context("render rule command")?; + .into_diagnostic() + .wrap_err("render rule command")?; } Recipe::Script { script } => { *script = env .render_str(script, context! {}) - .context("render rule script")?; + .into_diagnostic() + .wrap_err("render rule script")?; } Recipe::Rule { rule: r } => render_string_or_list(r, env, &Vars::new())?, } @@ -286,12 +297,14 @@ fn render_target(target: &mut Target, env: &Environment) -> Result<()> { Recipe::Command { command } => { *command = env .render_str(command, &target.vars) - .context("render target command")?; + .into_diagnostic() + .wrap_err("render target command")?; } Recipe::Script { script } => { *script = env .render_str(script, &target.vars) - .context("render target script")?; + .into_diagnostic() + .wrap_err("render target script")?; } Recipe::Rule { rule } => render_string_or_list(rule, env, &target.vars)?, } @@ -304,7 +317,8 @@ fn render_vars(vars: &mut Vars, env: &Environment) -> Result<()> { if let YamlValue::String(s) = value { *s = env .render_str(s, &snapshot) - .with_context(|| format!("render var '{key}'"))?; + .into_diagnostic() + .wrap_err_with(|| format!("render var '{key}'"))?; } } Ok(()) @@ -313,11 +327,17 @@ fn render_vars(vars: &mut Vars, env: &Environment) -> Result<()> { fn render_string_or_list(value: &mut StringOrList, env: &Environment, ctx: &Vars) -> Result<()> { match value { StringOrList::String(s) => { - *s = env.render_str(s, ctx).context("render string value")?; + *s = env + .render_str(s, ctx) + .into_diagnostic() + .wrap_err("render string value")?; } StringOrList::List(list) => { for item in list { - *item = env.render_str(item, ctx).context("render list value")?; + *item = env + .render_str(item, ctx) + .into_diagnostic() + .wrap_err("render list value")?; } } StringOrList::Empty => {} @@ -333,6 +353,7 @@ fn render_string_or_list(value: &mut StringOrList, env: &Environment, ctx: &Vars pub fn from_path(path: impl AsRef) -> Result { let path_ref = path.as_ref(); let data = fs::read_to_string(path_ref) - .with_context(|| format!("Failed to read {}", path_ref.display()))?; + .into_diagnostic() + .wrap_err_with(|| format!("Failed to read {}", path_ref.display()))?; from_str_named(&data, &path_ref.display().to_string()) } diff --git a/src/runner.rs b/src/runner.rs index 3fb30948..521404e4 100644 --- a/src/runner.rs +++ b/src/runner.rs @@ -6,7 +6,7 @@ use crate::cli::{BuildArgs, Cli, Commands}; use crate::{ir::BuildGraph, manifest, ninja_gen}; -use anyhow::{Context, Result}; +use anyhow::{Context, Error, Result}; use serde_json; use std::borrow::Cow; use std::fs; @@ -247,8 +247,12 @@ fn write_ninja_file(path: &Path, content: &NinjaContent) -> Result<()> { /// ``` fn generate_ninja(cli: &Cli) -> Result { let manifest_path = resolve_manifest_path(cli); - let manifest = manifest::from_path(&manifest_path) - .with_context(|| format!("loading manifest at {}", manifest_path.display()))?; + let manifest = manifest::from_path(&manifest_path).map_err(|e| { + Error::msg(format!( + "loading manifest at {}: {e}", + manifest_path.display() + )) + })?; let ast_json = serde_json::to_string_pretty(&manifest).context("serialising manifest")?; debug!("AST:\n{ast_json}"); let graph = BuildGraph::from_manifest(&manifest).context("building graph")?; diff --git a/tests/ast_tests.rs b/tests/ast_tests.rs index a5818d0a..132c20ac 100644 --- a/tests/ast_tests.rs +++ b/tests/ast_tests.rs @@ -1,11 +1,12 @@ //! Unit tests for Netsuke manifest AST deserialisation. +use miette::Result; use netsuke::{ast::*, manifest}; use rstest::rstest; use semver::Version; /// Convenience wrapper around the library manifest parser for tests. -fn parse_manifest(yaml: &str) -> anyhow::Result { +fn parse_manifest(yaml: &str) -> Result { manifest::from_str(yaml) } diff --git a/tests/runner_tests.rs b/tests/runner_tests.rs index 0a6e6d7f..2d547f65 100644 --- a/tests/runner_tests.rs +++ b/tests/runner_tests.rs @@ -70,7 +70,7 @@ fn run_exits_with_manifest_error_on_invalid_version() { let result = run(&cli); assert!(result.is_err()); let err = result.expect_err("should have error"); - assert!(err.chain().any(|e| e.to_string().contains("version"))); + assert!(err.to_string().contains("manifest parse error")); } #[rstest] diff --git a/tests/steps/ir_steps.rs b/tests/steps/ir_steps.rs index a74d3ce9..94b22eeb 100644 --- a/tests/steps/ir_steps.rs +++ b/tests/steps/ir_steps.rs @@ -50,7 +50,7 @@ fn graph_defaults(world: &mut CliWorld, count: usize) { #[when(expr = "the manifest file {string} is compiled to IR")] fn compile_manifest(world: &mut CliWorld, path: String) { match netsuke::manifest::from_path(&path) - .and_then(|m| BuildGraph::from_manifest(&m).map_err(anyhow::Error::from)) + .and_then(|m| BuildGraph::from_manifest(&m).map_err(|e| miette::Report::msg(e.to_string()))) { Ok(graph) => { world.build_graph = Some(graph); diff --git a/tests/yaml_error_tests.rs b/tests/yaml_error_tests.rs index 89b036a6..b1ca755a 100644 --- a/tests/yaml_error_tests.rs +++ b/tests/yaml_error_tests.rs @@ -1,6 +1,16 @@ +//! Regression tests for YAML parse errors. +//! +//! These tests ensure diagnostics include line numbers and optional hints, and +//! that rendering is stable across terminals. + use miette::GraphicalReportHandler; use netsuke::manifest; use rstest::rstest; +use strip_ansi_escapes::strip; + +fn normalise_report(report: &str) -> String { + String::from_utf8(strip(report.as_bytes())).expect("utf8") +} #[rstest] #[case( @@ -22,18 +32,16 @@ use rstest::rstest; &["line 4", "did not find expected '-'", "Start list items with '-'"], )] #[case( - "targets:\n - command: [echo\n", - &["line 2", "did not find expected ',' or ']'"], + "targets:\n - name: 'unterminated\n", + &["YAML parse error", "line 2"], )] fn yaml_diagnostics_are_actionable(#[case] yaml: &str, #[case] needles: &[&str]) { let err = manifest::from_str(yaml).expect_err("parse should fail"); - let diag = err - .downcast_ref::() - .expect("diagnostic type"); let mut msg = String::new(); GraphicalReportHandler::new() - .render_report(&mut msg, diag) + .render_report(&mut msg, err.as_ref()) .expect("render yaml error"); + let msg = normalise_report(&msg); for needle in needles { assert!(msg.contains(needle), "missing: {needle}\nmessage: {msg}"); } From 5ab0255e64693b7c4df8603d73a5cbf3fdfec871 Mon Sep 17 00:00:00 2001 From: Leynos Date: Tue, 19 Aug 2025 15:05:33 +0100 Subject: [PATCH 06/10] Scope YAML diagnostics and wrap runner errors with context --- src/manifest.rs | 23 +++++++++++++++-------- src/runner.rs | 31 ++++++++++++++++++------------- tests/runner_tests.rs | 7 ++++++- tests/steps/ir_steps.rs | 3 ++- 4 files changed, 41 insertions(+), 23 deletions(-) diff --git a/src/manifest.rs b/src/manifest.rs index ae6862fe..8548279c 100644 --- a/src/manifest.rs +++ b/src/manifest.rs @@ -17,20 +17,27 @@ const ERR_MANIFEST_PARSE: &str = "manifest parse error"; // Compute a narrow highlight span from a location. fn to_span(src: &str, loc: Location) -> SourceSpan { let at = loc.index(); - let end = if src.as_bytes().get(at).is_some_and(|b| *b != b'\n') { - at + 1 - } else { - at + let bytes = src.as_bytes(); + let (start, end) = match bytes.get(at) { + Some(&b) if b != b'\n' => (at, at + 1), + _ => { + // Fallback: highlight the previous byte on the same line when possible. + let start = if at > 0 && bytes.get(at - 1).is_some_and(|p| *p != b'\n') { + at - 1 + } else { + at + }; + (start, at) + } }; - let len = end.saturating_sub(at); - SourceSpan::new(at.into(), len) + let len = end.saturating_sub(start); + SourceSpan::new(start.into(), len) } #[derive(Debug, Error, Diagnostic)] #[error("{message}")] #[diagnostic(code(netsuke::yaml::parse))] -#[doc(hidden)] -pub struct YamlDiagnostic { +pub(crate) struct YamlDiagnostic { #[source_code] src: NamedSource, #[label("parse error here")] diff --git a/src/runner.rs b/src/runner.rs index 521404e4..8172fcdc 100644 --- a/src/runner.rs +++ b/src/runner.rs @@ -6,7 +6,7 @@ use crate::cli::{BuildArgs, Cli, Commands}; use crate::{ir::BuildGraph, manifest, ninja_gen}; -use anyhow::{Context, Error, Result}; +use miette::{Context, IntoDiagnostic, Result}; use serde_json; use std::borrow::Cow; use std::fs; @@ -174,7 +174,9 @@ fn handle_build(cli: &Cli, args: &BuildArgs) -> Result<()> { } let program = resolve_ninja_program(); - run_ninja(program.as_path(), cli, build_path.as_ref(), &targets)?; + run_ninja(program.as_path(), cli, build_path.as_ref(), &targets) + .into_diagnostic() + .wrap_err("run ninja")?; drop(tmp_file); Ok(()) } @@ -196,7 +198,8 @@ fn create_temp_ninja_file(content: &NinjaContent) -> Result { .prefix("netsuke.") .suffix(".ninja") .tempfile() - .context("create temp file")?; + .into_diagnostic() + .wrap_err("create temp file")?; write_ninja_file(tmp.path(), content)?; Ok(tmp) } @@ -217,10 +220,12 @@ fn write_ninja_file(path: &Path, content: &NinjaContent) -> Result<()> { // do not attempt to create the current directory on some platforms. if let Some(parent) = path.parent().filter(|p| !p.as_os_str().is_empty()) { fs::create_dir_all(parent) - .with_context(|| format!("failed to create parent directory {}", parent.display()))?; + .into_diagnostic() + .wrap_err_with(|| format!("failed to create parent directory {}", parent.display()))?; } fs::write(path, content.as_str()) - .with_context(|| format!("failed to write Ninja file to {}", path.display()))?; + .into_diagnostic() + .wrap_err_with(|| format!("failed to write Ninja file to {}", path.display()))?; info!("Generated Ninja file at {}", path.display()); Ok(()) } @@ -247,15 +252,15 @@ fn write_ninja_file(path: &Path, content: &NinjaContent) -> Result<()> { /// ``` fn generate_ninja(cli: &Cli) -> Result { let manifest_path = resolve_manifest_path(cli); - let manifest = manifest::from_path(&manifest_path).map_err(|e| { - Error::msg(format!( - "loading manifest at {}: {e}", - manifest_path.display() - )) - })?; - let ast_json = serde_json::to_string_pretty(&manifest).context("serialising manifest")?; + let manifest = manifest::from_path(&manifest_path) + .wrap_err_with(|| format!("loading manifest at {}", manifest_path.display()))?; + let ast_json = serde_json::to_string_pretty(&manifest) + .into_diagnostic() + .wrap_err("serialising manifest")?; debug!("AST:\n{ast_json}"); - let graph = BuildGraph::from_manifest(&manifest).context("building graph")?; + let graph = BuildGraph::from_manifest(&manifest) + .into_diagnostic() + .wrap_err("building graph")?; Ok(NinjaContent::new(ninja_gen::generate(&graph))) } diff --git a/tests/runner_tests.rs b/tests/runner_tests.rs index 2d547f65..95c9fd85 100644 --- a/tests/runner_tests.rs +++ b/tests/runner_tests.rs @@ -70,7 +70,12 @@ fn run_exits_with_manifest_error_on_invalid_version() { let result = run(&cli); assert!(result.is_err()); let err = result.expect_err("should have error"); - assert!(err.to_string().contains("manifest parse error")); + let chain: Vec<_> = err.chain().map(std::string::ToString::to_string).collect(); + assert!( + chain.iter().any(|s| s.contains("manifest parse error")), + "expected error chain to include 'manifest parse error', got: {chain:?}" + ); + assert!(err.to_string().contains("loading manifest at")); } #[rstest] diff --git a/tests/steps/ir_steps.rs b/tests/steps/ir_steps.rs index 94b22eeb..9459a10e 100644 --- a/tests/steps/ir_steps.rs +++ b/tests/steps/ir_steps.rs @@ -2,6 +2,7 @@ use crate::CliWorld; use cucumber::{given, then, when}; +use miette::IntoDiagnostic; use netsuke::ir::BuildGraph; fn assert_graph(world: &CliWorld) { @@ -50,7 +51,7 @@ fn graph_defaults(world: &mut CliWorld, count: usize) { #[when(expr = "the manifest file {string} is compiled to IR")] fn compile_manifest(world: &mut CliWorld, path: String) { match netsuke::manifest::from_path(&path) - .and_then(|m| BuildGraph::from_manifest(&m).map_err(|e| miette::Report::msg(e.to_string()))) + .and_then(|m| BuildGraph::from_manifest(&m).into_diagnostic()) { Ok(graph) => { world.build_graph = Some(graph); From dbc4b1eb2cda53994575f572c0ef9d89c7b5ba4b Mon Sep 17 00:00:00 2001 From: Leynos Date: Tue, 19 Aug 2025 15:37:28 +0100 Subject: [PATCH 07/10] Preserve manifest parse context --- src/runner.rs | 2 +- tests/runner_tests.rs | 12 +++++------- 2 files changed, 6 insertions(+), 8 deletions(-) diff --git a/src/runner.rs b/src/runner.rs index 8172fcdc..ffc43a5b 100644 --- a/src/runner.rs +++ b/src/runner.rs @@ -253,7 +253,7 @@ fn write_ninja_file(path: &Path, content: &NinjaContent) -> Result<()> { fn generate_ninja(cli: &Cli) -> Result { let manifest_path = resolve_manifest_path(cli); let manifest = manifest::from_path(&manifest_path) - .wrap_err_with(|| format!("loading manifest at {}", manifest_path.display()))?; + .with_context(|| format!("loading manifest at {}", manifest_path.display()))?; let ast_json = serde_json::to_string_pretty(&manifest) .into_diagnostic() .wrap_err("serialising manifest")?; diff --git a/tests/runner_tests.rs b/tests/runner_tests.rs index 95c9fd85..28b38ba5 100644 --- a/tests/runner_tests.rs +++ b/tests/runner_tests.rs @@ -67,15 +67,13 @@ fn run_exits_with_manifest_error_on_invalid_version() { })), }; - let result = run(&cli); - assert!(result.is_err()); - let err = result.expect_err("should have error"); - let chain: Vec<_> = err.chain().map(std::string::ToString::to_string).collect(); + let err = run(&cli).expect_err("should have error"); + assert!(err.to_string().contains("loading manifest at")); + let source = err.source().expect("source error").to_string(); assert!( - chain.iter().any(|s| s.contains("manifest parse error")), - "expected error chain to include 'manifest parse error', got: {chain:?}" + source.contains("manifest parse error"), + "expected parse error in source, got: {source}" ); - assert!(err.to_string().contains("loading manifest at")); } #[rstest] From d123e4f60194c39b34143386f9ec95a487ce5646 Mon Sep 17 00:00:00 2001 From: Leynos Date: Tue, 19 Aug 2025 16:06:33 +0100 Subject: [PATCH 08/10] Refactor render helpers and test error chain --- src/manifest.rs | 58 +++++++++++++++++++------------------------ tests/runner_tests.rs | 6 ++--- 2 files changed, 28 insertions(+), 36 deletions(-) diff --git a/src/manifest.rs b/src/manifest.rs index 8548279c..30438522 100644 --- a/src/manifest.rs +++ b/src/manifest.rs @@ -31,7 +31,8 @@ fn to_span(src: &str, loc: Location) -> SourceSpan { } }; let len = end.saturating_sub(start); - SourceSpan::new(start.into(), len) + #[allow(clippy::useless_conversion, reason = "future-proof span length type")] + SourceSpan::new(start.into(), len.into()) } #[derive(Debug, Error, Diagnostic)] @@ -254,6 +255,18 @@ fn eval_expression(env: &Environment, name: &str, expr: &str, ctx: Value) -> Res .wrap_err_with(|| format!("{name} evaluation error")) } +/// Render a Jinja template and label any error with the given context. +fn render_str_with( + env: &Environment, + tpl: &str, + ctx: &impl serde::Serialize, + what: impl FnOnce() -> String, +) -> Result { + env.render_str(tpl, ctx) + .into_diagnostic() + .wrap_err_with(what) +} + /// Render all templated strings in the manifest. fn render_manifest(mut manifest: NetsukeManifest, env: &Environment) -> Result { for action in &mut manifest.actions { @@ -270,24 +283,16 @@ fn render_manifest(mut manifest: NetsukeManifest, env: &Environment) -> Result Result<()> { if let Some(desc) = &mut rule.description { - *desc = env - .render_str(desc, context! {}) - .into_diagnostic() - .wrap_err("render rule description")?; + *desc = render_str_with(env, desc, &context! {}, || "render rule description".into())?; } render_string_or_list(&mut rule.deps, env, &Vars::new())?; match &mut rule.recipe { Recipe::Command { command } => { - *command = env - .render_str(command, context! {}) - .into_diagnostic() - .wrap_err("render rule command")?; + *command = + render_str_with(env, command, &context! {}, || "render rule command".into())?; } Recipe::Script { script } => { - *script = env - .render_str(script, context! {}) - .into_diagnostic() - .wrap_err("render rule script")?; + *script = render_str_with(env, script, &context! {}, || "render rule script".into())?; } Recipe::Rule { rule: r } => render_string_or_list(r, env, &Vars::new())?, } @@ -302,16 +307,12 @@ fn render_target(target: &mut Target, env: &Environment) -> Result<()> { render_string_or_list(&mut target.order_only_deps, env, &target.vars)?; match &mut target.recipe { Recipe::Command { command } => { - *command = env - .render_str(command, &target.vars) - .into_diagnostic() - .wrap_err("render target command")?; + *command = render_str_with(env, command, &target.vars, || { + "render target command".into() + })?; } Recipe::Script { script } => { - *script = env - .render_str(script, &target.vars) - .into_diagnostic() - .wrap_err("render target script")?; + *script = render_str_with(env, script, &target.vars, || "render target script".into())?; } Recipe::Rule { rule } => render_string_or_list(rule, env, &target.vars)?, } @@ -322,10 +323,7 @@ fn render_vars(vars: &mut Vars, env: &Environment) -> Result<()> { let snapshot = vars.clone(); for (key, value) in vars.iter_mut() { if let YamlValue::String(s) = value { - *s = env - .render_str(s, &snapshot) - .into_diagnostic() - .wrap_err_with(|| format!("render var '{key}'"))?; + *s = render_str_with(env, s, &snapshot, || format!("render var '{key}'"))?; } } Ok(()) @@ -334,17 +332,11 @@ fn render_vars(vars: &mut Vars, env: &Environment) -> Result<()> { fn render_string_or_list(value: &mut StringOrList, env: &Environment, ctx: &Vars) -> Result<()> { match value { StringOrList::String(s) => { - *s = env - .render_str(s, ctx) - .into_diagnostic() - .wrap_err("render string value")?; + *s = render_str_with(env, s, ctx, || "render string value".into())?; } StringOrList::List(list) => { for item in list { - *item = env - .render_str(item, ctx) - .into_diagnostic() - .wrap_err("render list value")?; + *item = render_str_with(env, item, ctx, || "render list value".into())?; } } StringOrList::Empty => {} diff --git a/tests/runner_tests.rs b/tests/runner_tests.rs index 28b38ba5..347e8ab7 100644 --- a/tests/runner_tests.rs +++ b/tests/runner_tests.rs @@ -69,10 +69,10 @@ fn run_exits_with_manifest_error_on_invalid_version() { let err = run(&cli).expect_err("should have error"); assert!(err.to_string().contains("loading manifest at")); - let source = err.source().expect("source error").to_string(); + let chain: Vec = err.chain().map(ToString::to_string).collect(); assert!( - source.contains("manifest parse error"), - "expected parse error in source, got: {source}" + chain.iter().any(|s| s.contains("manifest parse error")), + "expected error chain to include 'manifest parse error', got: {chain:?}" ); } From ac6992b8b70713e43a5a6355e2020ccdf991026a Mon Sep 17 00:00:00 2001 From: Leynos Date: Tue, 19 Aug 2025 17:30:30 +0100 Subject: [PATCH 09/10] Add context to IR generation errors --- tests/steps/ir_steps.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/steps/ir_steps.rs b/tests/steps/ir_steps.rs index 9459a10e..18a9b396 100644 --- a/tests/steps/ir_steps.rs +++ b/tests/steps/ir_steps.rs @@ -2,7 +2,7 @@ use crate::CliWorld; use cucumber::{given, then, when}; -use miette::IntoDiagnostic; +use miette::{Context, IntoDiagnostic}; use netsuke::ir::BuildGraph; fn assert_graph(world: &CliWorld) { @@ -52,6 +52,7 @@ fn graph_defaults(world: &mut CliWorld, count: usize) { fn compile_manifest(world: &mut CliWorld, path: String) { match netsuke::manifest::from_path(&path) .and_then(|m| BuildGraph::from_manifest(&m).into_diagnostic()) + .with_context(|| format!("IR generation failed for {path}")) { Ok(graph) => { world.build_graph = Some(graph); From 988dbc17adddb592a8d28a4cf4b8389b388327cf Mon Sep 17 00:00:00 2001 From: Leynos Date: Tue, 19 Aug 2025 17:30:35 +0100 Subject: [PATCH 10/10] Wrap IR build errors with context --- tests/steps/ir_steps.rs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/tests/steps/ir_steps.rs b/tests/steps/ir_steps.rs index 18a9b396..ceac21b9 100644 --- a/tests/steps/ir_steps.rs +++ b/tests/steps/ir_steps.rs @@ -51,7 +51,11 @@ fn graph_defaults(world: &mut CliWorld, count: usize) { #[when(expr = "the manifest file {string} is compiled to IR")] fn compile_manifest(world: &mut CliWorld, path: String) { match netsuke::manifest::from_path(&path) - .and_then(|m| BuildGraph::from_manifest(&m).into_diagnostic()) + .and_then(|m| { + BuildGraph::from_manifest(&m) + .into_diagnostic() + .wrap_err("building IR from manifest") + }) .with_context(|| format!("IR generation failed for {path}")) { Ok(graph) => {