From d7c8e6ad1bda51d6b374cc7b043ca12b2de9784e Mon Sep 17 00:00:00 2001 From: Grzegorz Martens Date: Sun, 25 Oct 2026 17:37:58 +0100 Subject: [PATCH] M6: calculation checks: a notation's last name keeps the escaped space that ends it parse_calculate trimmed the notation with str::trim, which also took an escaped space ending its last name: "Qty * Total\ " read back as "Qty * Total\", a field the form doesn't have, while the script's JavaScript still read "Total ". It now trims to its first and last token, as the field-reading scan does. notation_storable compares the names and operators read back with those given, so a notation ending in a lone \ (which escapes the space after it) is refused; a proptest reads back notations of any names (from the review). Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01P6yUtvWGPsJpMLyh6pxSXG --- crates/automation/src/forms.rs | 4 +-- .../forms/proptest-regressions/af/tests.txt | 7 +++++ crates/forms/src/af.rs | 2 +- crates/forms/src/af/script.rs | 28 ++++++++++++++----- crates/forms/src/af/tests.rs | 17 ++++++++++- 5 files changed, 47 insertions(+), 11 deletions(-) create mode 100644 crates/forms/proptest-regressions/af/tests.txt diff --git a/crates/automation/src/forms.rs b/crates/automation/src/forms.rs index a9add477f2..eb38cb2b4e 100644 --- a/crates/automation/src/forms.rs +++ b/crates/automation/src/forms.rs @@ -1,7 +1,7 @@ //! Form tools: list fields, fill them (several at once, one undo step), clear the form, and //! prepare a form (add fields one at a time or as a table, change and delete them). -use pdfcraft_engine::form_scripts::{CalcOp, Calculate, Format, Validate, notation_storable}; +use pdfcraft_engine::form_scripts::{CalcOp, Calculate, Format, NOTATION_NOT_STORABLE, Validate, notation_storable}; use pdfcraft_engine::{CalculationIssue, Edit, FieldProps, FieldValue, FormField, FormFieldKind, NewField, field_flags}; use serde_json::{Value, json}; @@ -136,7 +136,7 @@ fn calculate_arg(v: &Value) -> Result { let n = n.as_str().ok_or_else(|| bad("notation must be a string such as \"Price * Qty\""))?; // Refused rather than stored as a different formula. if !notation_storable(n) { - return Err(bad("the notation can't be stored as written: it is empty, or holds EVCALC or */, which would end it early")); + return Err(bad(NOTATION_NOT_STORABLE)); } return Ok(Calculate::Notation(n.to_string())); } diff --git a/crates/forms/proptest-regressions/af/tests.txt b/crates/forms/proptest-regressions/af/tests.txt new file mode 100644 index 0000000000..70c4d0b525 --- /dev/null +++ b/crates/forms/proptest-regressions/af/tests.txt @@ -0,0 +1,7 @@ +# Seeds for failure cases proptest has generated in the past. It is +# automatically read and these particular cases re-run before any +# novel cases are generated. +# +# It is recommended to check this file in to source control so that +# everyone who runs the test benefits from these saved cases. +cc 81dc3893ad0b46189d4e80199f44f8e58fcf0d193f67e266e37f3846f756a11e # shrinks to names = [" "] diff --git a/crates/forms/src/af.rs b/crates/forms/src/af.rs index c83ce29cf3..86c7971c31 100644 --- a/crates/forms/src/af.rs +++ b/crates/forms/src/af.rs @@ -22,7 +22,7 @@ use calc::{Notation, eval_notation, notation_name, notation_tokens, unique}; pub use calc::{calculate, calculation_reads, notation_js, reads_built_names, rename_in_calculation}; pub use format::{DATE_PRESETS, DateTime, TIME_PRESETS, format_date, format_value, js_number, keystroke, make_number, parse_date, validate}; use script::{Arg, call}; -pub use script::{calculate_js, format_js, notation_storable, parse_calculate, parse_format, parse_validate, validate_js}; +pub use script::{NOTATION_NOT_STORABLE, calculate_js, format_js, notation_storable, parse_calculate, parse_format, parse_validate, validate_js}; /// The Format tab. #[derive(Clone, Debug, PartialEq, Default)] diff --git a/crates/forms/src/af/script.rs b/crates/forms/src/af/script.rs index 1544258a2c..f15714c411 100644 --- a/crates/forms/src/af/script.rs +++ b/crates/forms/src/af/script.rs @@ -1,4 +1,4 @@ -use super::calc::{js_tokens, notation_js, notation_range}; +use super::calc::{js_tokens, notation_js, notation_range, notation_tokens}; use super::format::{DATE_PRESETS, TIME_PRESETS}; use super::{CalcOp, Calculate, Format, Validate}; @@ -255,7 +255,7 @@ pub fn parse_calculate(js: &str) -> Option { if js.contains("BVCALC") && let Some(range) = notation_range(js, &js_tokens(js).1) { - return Some(Calculate::Notation(js.get(range)?.trim().to_string())); + return Some(Calculate::Notation(notation_text(js.get(range)?).to_string())); } let args = call(js, "AFSimple_Calculate")?; let op = match args.first().and_then(Arg::string)?.as_str() { @@ -325,13 +325,27 @@ pub fn validate_js(v: &Validate) -> Option { } } +/// Why [`notation_storable`] refuses a notation, in the words the Calculate tab and the tools say it. +pub const NOTATION_NOT_STORABLE: &str = "the notation can't be stored as written: it is empty, or holds EVCALC or */, or ends in a lone \\"; + /// Whether simplified field notation `expr` can be stored: the script keeps it in a comment, -/// between `BVCALC` and `EVCALC`, and it has to read back from there as itself. An empty one, or -/// one holding `EVCALC` or `*/` (either ends it early), can't. +/// between `BVCALC` and `EVCALC`, and it has to read back from there as the same names and +/// operators. One holding `EVCALC` or `*/` ends early, and one ending in a lone `\` escapes the +/// space after it. An empty one is refused too: it would be stored as `event.value = ;`. pub fn notation_storable(expr: &str) -> bool { - !expr.trim().is_empty() - && calculate_js(&Calculate::Notation(expr.to_owned())).and_then(|js| parse_calculate(&js)) - == Some(Calculate::Notation(expr.trim().to_owned())) + let tokens = |s: &str| notation_tokens(s).into_iter().map(|(range, _)| s.get(range).unwrap_or_default().to_owned()).collect::>(); + let read = calculate_js(&Calculate::Notation(expr.to_owned())).and_then(|js| parse_calculate(&js)); + !tokens(expr).is_empty() && matches!(read, Some(Calculate::Notation(n)) if tokens(&n) == tokens(expr)) +} + +/// `src` from its first notation token to its last: the spaces around it left out, but not an +/// escaped space that ends the last name (`Total\ `), as `str::trim` would. +fn notation_text(src: &str) -> &str { + let tokens = notation_tokens(src); + match (tokens.first(), tokens.last()) { + (Some((first, _)), Some((last, _))) => src.get(first.start..last.end).unwrap_or_default(), + _ => "", + } } pub fn calculate_js(c: &Calculate) -> Option { diff --git a/crates/forms/src/af/tests.rs b/crates/forms/src/af/tests.rs index d0246d3016..79c5c7be02 100644 --- a/crates/forms/src/af/tests.rs +++ b/crates/forms/src/af/tests.rs @@ -222,7 +222,12 @@ fn hostile_scripts_are_read_in_one_pass() { fn a_notation_is_stored_only_when_it_reads_back() { assert!(notation_storable("Price * Qty")); assert!(notation_storable(" a + b "), "the spaces around it are left out when it is read"); - for expr in ["", " ", "a EVCALC b", "a */ x /*", "a */ b"] { + // A last name that ends in a space keeps it, escaped, when the notation is read back. + let total = "Qty * Total\\ "; + assert!(notation_storable(total)); + let read = parse_calculate(&calculate_js(&Calculate::Notation(total.into())).unwrap()); + assert_eq!(read.as_ref().map(Calculate::reads), Some(vec!["Qty".to_owned(), "Total ".to_owned()])); + for expr in ["", " ", "a EVCALC b", "a */ x /*", "a */ b", "a\\"] { assert!(!notation_storable(expr), "{expr:?}"); } } @@ -274,6 +279,16 @@ proptest::proptest! { proptest::prop_assert_eq!(back, Some(js)); } + /// A notation of any names, written into a script, reads back from it as written. + #[test] + fn a_notation_reads_back_as_written(names in proptest::collection::vec("\\PC{1,8}", 1..5)) { + proptest::prop_assume!(!names.iter().any(|n| n.contains("VCALC"))); + let expr = names.iter().map(|n| notation_name(n)).collect::>().join(" + "); + let js = calculate_js(&Calculate::Notation(expr.clone())).unwrap(); + proptest::prop_assert_eq!(parse_calculate(&js), Some(Calculate::Notation(expr.clone()))); + proptest::prop_assert!(notation_storable(&expr), "{expr:?}"); + } + /// What a formula reads is the same whether it is read from the tab's settings or from /// the script they are written as. #[test]