M6: a refused move doesn't copy inherited values onto the field first (#2696)

Review on #2957: own_inherited wrote what the field inherits onto it before
check_place could refuse the move, so a refused move still changed the field.
What it inherits is now read first, counted as its own by the check, and
written only once the move is allowed.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

crates/forms/src/author.rs

@@ -548,9 +548,13 @@ const PASSED_VALUES: [&[u8]; 3] = [b"V", b"DV", b"MaxLen"];
 /// Whether `field` may go in the field tree under the full name `parts`, changing nothing. The
 /// field list holds only terminal fields, so a parent left empty by a move isn't in it: a node
 /// of the same name there is found here.
-fn check_place(doc: &Document, af: ObjRef, field: ObjRef, parts: &[&str]) -> Result<(), FormError> {
+/// `inherited` is what the field will hold as its own before it moves (see [`inherited`]).
+fn check_place(doc: &Document, af: ObjRef, field: ObjRef, parts: &[&str], inherited: &Dict) -> Result<(), FormError> {
     let Some((own, parents)) = parts.split_last() else { return invalid("field names can't be empty") };
-    let owned = doc.get(field).as_dict().cloned().unwrap_or_default();
+    let mut owned = doc.get(field).as_dict().cloned().unwrap_or_default();
+    for (k, v) in inherited.iter() {
+        owned.set(k.to_vec(), v.clone());
+    }
     let mut at: Option<ObjRef> = None;
     let mut whole_path = true;
     for part in parents {
@@ -578,7 +582,7 @@ fn check_place(doc: &Document, af: ObjRef, field: ObjRef, parts: &[&str]) -> Res
 /// not (Acrobat makes `a.b` a field `b` under a field `a`).
 fn attach(doc: &mut Document, af: ObjRef, field: ObjRef, parts: &[&str]) -> Result<(), FormError> {
     let Some((own, parents)) = parts.split_last() else { return invalid("field names can't be empty") };
-    check_place(doc, af, field, parts)?;
+    check_place(doc, af, field, parts, &Dict::new())?;
     let mut parent: Option<ObjRef> = None;
     let mut inherits = false;
     for part in parents {
@@ -621,8 +625,8 @@ fn attach(doc: &mut Document, af: ObjRef, field: ObjRef, parts: &[&str]) -> Resu
     link(doc, af, parent, field)
 }
 
-/// Copy onto `field` what it inherits from its parents, so it keeps them when it moves.
-fn own_inherited(doc: &mut Document, field: ObjRef) -> Result<(), FormError> {
+/// What `field` inherits from its parents and doesn't hold itself, changing nothing.
+fn inherited(doc: &Document, field: ObjRef) -> Dict {
     let own = doc.get(field).as_dict().cloned().unwrap_or_default();
     let mut add = Dict::new();
     let mut seen = std::collections::HashSet::new();
@@ -639,6 +643,11 @@ fn own_inherited(doc: &mut Document, field: ObjRef) -> Result<(), FormError> {
         }
         parent = d.reference(b"Parent");
     }
+    add
+}
+
+/// Copy onto `field` what it inherits (`add`, from [`inherited`]), so it keeps it when it moves.
+fn own_inherited(doc: &mut Document, field: ObjRef, add: &Dict) -> Result<(), FormError> {
     if add.iter().next().is_some() {
         doc.update_dict(field, |d| {
             for (k, v) in add.iter() {
@@ -1114,9 +1123,10 @@ pub fn set_props(doc: &mut Document, name: &str, props: &FieldProps) -> Result<S
                 return invalid(why);
             }
             let af = ensure_form(doc)?;
-            own_inherited(doc, f.obj)?;
-            // Before it leaves its place, so a refused move leaves it where it was.
-            check_place(doc, af, f.obj, &parts)?;
+            // Checked before anything changes, so a refused move leaves the field as it was.
+            let add = inherited(doc, f.obj);
+            check_place(doc, af, f.obj, &parts, &add)?;
+            own_inherited(doc, f.obj, &add)?;
             detach(doc, f.obj)?;
             attach(doc, af, f.obj, &parts)?;
             new_name = n.to_string();

crates/forms/src/tests.rs

@@ -3169,6 +3169,26 @@ fn dotted_field_names_are_made_with_parent_fields() {
     let at_top = top(&doc);
     assert!(set_props(&mut doc, "name", &FieldProps { name: Some("address.name".into()), ..FieldProps::default() }).is_err());
     assert_eq!(top(&doc), at_top, "name is still in the form's /Fields");
+    // Nor is what it inherits copied onto it first (review on #2957): src.f inherits /V from src.
+    add_field(&mut doc, 0, [300.0, 540.0, 450.0, 560.0], &text, Some("src.f")).unwrap();
+    let f = field(&fields(&doc), "src.f").obj;
+    let src = doc.get(f).as_dict().and_then(|d| d.reference(b"Parent")).unwrap();
+    doc.update_dict(src, |d| d.set(b"V".to_vec(), Object::String(PdfString::literal(b"inh".to_vec())))).unwrap();
+    assert!(set_props(&mut doc, "src.f", &FieldProps { name: Some("address.f".into()), ..FieldProps::default() }).is_err());
+    assert!(!doc.get(f).as_dict().unwrap().contains(b"V"), "src.f still only inherits its value");
+    // What it inherits counts as its own where it goes: with /V on address and no /MaxLen,
+    // src.f moves, keeping its value.
+    doc.update_dict(address, |d| {
+        d.remove(b"MaxLen");
+        d.set(b"V".to_vec(), Object::String(PdfString::literal(b"x".to_vec())));
+    })
+    .unwrap();
+    set_props(&mut doc, "src.f", &FieldProps { name: Some("address.f".into()), ..FieldProps::default() }).unwrap();
+    assert_eq!(field(&fields(&doc), "address.f").value, ["inh"]);
+    doc.update_dict(address, |d| {
+        d.remove(b"V");
+    })
+    .unwrap();
     doc.update_dict(address, |d| {
         d.remove(b"MaxLen");
     })