From 994ce630997b79aa1084f03307165e987b67d109 Mon Sep 17 00:00:00 2001 From: mtlcl410 <2368091+mtlcl410@users.noreply.github.com> Date: Fri, 23 Oct 2026 20:31:13 +0800 Subject: [PATCH] M6: a document script's error is told apart by where it came from, not its wording (#3940) Review follow-up on #3985: JsOutput deduplicated document-script errors by their text (document script N: ...), so a field script that threw the same words was dropped by append, and a document script's error was dropped by absorb when a field script had already reported that text. JsOutput now records which errors came from document scripts (doc_errors) and deduplicates only those; pdfcraft_js::is_doc_error, which guessed from the wording, is removed. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_0175FWj4xFhMhSH3UpMU8DSC --- crates/engine/src/js.rs | 54 ++++++++++++++++++++++++------------- crates/js/src/lib.rs | 14 +--------- crates/ui-egui/src/js_ui.rs | 10 +++++-- 3 files changed, 45 insertions(+), 33 deletions(-) diff --git a/crates/engine/src/js.rs b/crates/engine/src/js.rs index 301b0592f..19b6c8cde 100644 --- a/crates/engine/src/js.rs +++ b/crates/engine/src/js.rs @@ -19,6 +19,10 @@ pub struct JsOutput { pub requests: Vec, /// Script errors (as Acrobat's console shows them). pub errors: Vec, + /// Which of `errors` are document scripts' (#3940), told apart by where they came from and + /// not by their text, which a field script's error can share: each is reported once, however + /// often the document scripts run again. + pub doc_errors: Vec, } impl JsOutput { @@ -34,7 +38,8 @@ impl JsOutput { // The document scripts run before every field script, so a failing one would repeat // once per calculation; it is reported once. for e in &o.doc_errors { - if !self.errors.contains(e) { + if !self.doc_errors.contains(e) { + self.doc_errors.push(e.clone()); self.errors.push(e.clone()); } } @@ -45,11 +50,17 @@ impl JsOutput { self.console.extend(other.console); self.requests.extend(other.requests); // As in `absorb`: a document script's error, already reported, is not repeated; a field - // script's errors are each their own. + // script's errors are each their own, even one worded like a document script's. + let mut docs = other.doc_errors; for e in other.errors { - if !(pdfcraft_js::is_doc_error(&e) && self.errors.contains(&e)) { - self.errors.push(e); + if let Some(k) = docs.iter().position(|d| *d == e) { + docs.swap_remove(k); + if self.doc_errors.contains(&e) { + continue; + } + self.doc_errors.push(e.clone()); } + self.errors.push(e); } } } @@ -462,22 +473,29 @@ mod review_tests { } /// Review on #3985: merging two outputs reports a failing document script once, as one run - /// does, while each field script error stays. + /// does, while each field script error stays: told apart by where an error came from, so a + /// field script that throws a document script's very words is still reported. #[test] fn merged_output_reports_a_document_script_error_once() { - let run = |errors: &[&str]| JsOutput { errors: errors.iter().map(|e| e.to_string()).collect(), ..JsOutput::default() }; - let mut out = run(&["document script 2: TypeError: not a callable function", "ReferenceError: x is not defined"]); - out.append(run(&["document script 2: TypeError: not a callable function", "ReferenceError: x is not defined"])); - assert_eq!( - out.errors, - ["document script 2: TypeError: not a callable function", "ReferenceError: x is not defined", "ReferenceError: x is not defined"] - ); - // A field script's message that only looks like one (no script number) is still its own. - for look_alike in ["document script x: thrown by a field", "document script : thrown by a field"] { - out.append(run(&[look_alike])); - out.append(run(&[look_alike])); - assert_eq!(out.errors.iter().filter(|e| *e == look_alike).count(), 2, "{look_alike}"); - } + let doc = "document script 2: TypeError: not a callable function"; + let field = "ReferenceError: x is not defined"; + let run = |error: &str| { + let mut out = JsOutput::default(); + out.absorb(&Outcome { error: Some(error.into()), doc_errors: vec![doc.into()], ..Outcome::default() }); + out + }; + let mut out = run(field); + out.append(run(field)); + assert_eq!(out.errors, [field, doc, field]); + // CodeRabbit on #3985: a field script that throws the document script's text. + out.append(run(doc)); + assert_eq!(out.errors, [field, doc, field, doc]); + let alone = run(doc); + assert_eq!(alone.errors, [doc, doc], "the field's error and the document script's"); + let count = |o: &JsOutput| o.errors.iter().filter(|e| *e == doc).count(); + let mut merged = run(field); + merged.append(alone); + assert_eq!(count(&merged), 2, "the field's copy stays; the document script's is listed once"); } #[test] diff --git a/crates/js/src/lib.rs b/crates/js/src/lib.rs index cc835c79e..f6d5a076c 100644 --- a/crates/js/src/lib.rs +++ b/crates/js/src/lib.rs @@ -1274,21 +1274,9 @@ impl HostHooks for CheckedCodeFromStrings { } } -/// The prefix of a document script's error ([`doc_error`]). -const DOC_ERROR_PREFIX: &str = "document script "; - /// How [`Outcome::doc_errors`] names a failing document script: by its 1-based place. fn doc_error(index: usize, error: &dyn std::fmt::Display) -> String { - format!("{DOC_ERROR_PREFIX}{}: {error}", index.saturating_add(1)) -} - -/// Whether `error` is one of [`Outcome::doc_errors`]: a document script's failure, which repeats -/// each time the document scripts run again, unlike a field script's own error. -pub fn is_doc_error(error: &str) -> bool { - error - .strip_prefix(DOC_ERROR_PREFIX) - .and_then(|rest| rest.split_once(": ")) - .is_some_and(|(n, _)| !n.is_empty() && n.bytes().all(|b| b.is_ascii_digit())) + format!("document script {}: {error}", index.saturating_add(1)) } /// `doc_scripts` are the document scripts to run with their places among all of them; diff --git a/crates/ui-egui/src/js_ui.rs b/crates/ui-egui/src/js_ui.rs index 7bc3dd044..f5ec01625 100644 --- a/crates/ui-egui/src/js_ui.rs +++ b/crates/ui-egui/src/js_ui.rs @@ -88,7 +88,13 @@ impl PdfCraftApp { { self.views[i].document_changed(&info); } - let out = JsOutput { alerts: o.alerts, console: o.console, requests: o.requests, errors: o.error.into_iter().collect() }; + let out = JsOutput { + alerts: o.alerts, + console: o.console, + requests: o.requests, + errors: o.error.into_iter().collect(), + doc_errors: Vec::new(), + }; self.handle_js(id, out); } Err(e) => self.notify_fmt("{field}: {e}", &[("field", field), ("e", &e.to_string())]), @@ -128,7 +134,7 @@ impl PdfCraftApp { self.views[i].document_changed(&info); } let (error, result) = (o.error.clone(), o.result.clone()); - let out = JsOutput { alerts: o.alerts, console: o.console, requests: o.requests, errors: Vec::new() }; + let out = JsOutput { alerts: o.alerts, console: o.console, requests: o.requests, errors: Vec::new(), doc_errors: Vec::new() }; self.handle_js(id, out); match error { Some(e) => self.js_console.log.push(format!("Error: {e}")),