fix: REDUCE-created instances re-encode with REDUCE (#32)

CHANGES.md

@@ -2,6 +2,17 @@
 
 ## unreleased
 
+- Instances that a pickler wrote as `REDUCE(cls, ()) + BUILD` (a `__reduce__`
+  returning the class with empty arguments, for example `OrderedDict`
+  subclasses with a `__dict__`) re-encode as `REDUCE` again, so `__init__`
+  runs on unpickling as it did for the original pickle; they used to come
+  back as `NEWOBJ`. The JSON carries `"@newobj": false` next to `@cls`/`@s`
+  for such instances; absent means `NEWOBJ`, so stored data is unaffected.
+  The Python-dict readers now recognise instance dicts of five keys
+  (`@cls`, `@s`, `@newobj`, `@items`, `@appends`), and every reader rejects
+  `@newobj`, `@items` or `@appends` on a bare class reference without `@s`
+  (the JSON reader used to drop them silently) [#32]
+
 - Docs: journal entry 22 explains the PGO slowdown of the PG JSON path
   (the profile-guided build put the end-of-slice check on the pointer step of
   the string loop) and the performance page is re-measured with the fixed

docs/.vale/styles/config/vocabularies/Project/accept.txt

@@ -144,7 +144,7 @@ special_types
 wide_dict
 [Bb]ackend[s]?
 [Aa]uto(detect(ed|s|ion)?|discover(ed|s|y)|creat(ed|es))
-unpickle(s|d)?
+unpickl(e|es|ed|ing)
 mimalloc
 glibc
 pymalloc

docs/sources/reference/json-format.md

@@ -215,6 +215,28 @@ ZODB record.
 }
 ```
 
+### `@newobj` -- Construction Kind
+
+Written as `false`, next to `@cls`/`@s`, when the pickle created the instance
+by calling the callable named by `@cls`, usually the class (`REDUCE` with
+empty arguments, the shape of a `__reduce__` that returns `(cls, (), state)`,
+for example `OrderedDict` subclasses with a `__dict__`). Absent means the
+instance was created with `NEWOBJ` (`cls.__new__(cls)`), the common case. The
+encoders emit the same opcode again, so `__init__` runs on unpickling exactly
+when it did for the original pickle. Note the opposite default of the
+`newobj` key inside `@reduce` below: there, absent means `REDUCE`; each
+default is the common case of its form. `@newobj`, like `@items` and
+`@appends`, is only valid next to `@s`; readers reject it on a bare class
+reference.
+
+```json
+{
+  "@cls": ["myapp.models", "Registry"],
+  "@s": {"entries": 3},
+  "@newobj": false
+}
+```
+
 ### `@ref` -- Persistent Reference
 
 ZODB persistent object reference, using hex OID format (16 hex digits,
@@ -290,7 +312,7 @@ a state that is exactly `{"@args": ..., "@state": ...}` is always read as
 constructor arguments plus state. A `@reduce` with both `newobj` and `state`
 is written only when the class itself is not a global (`NEWOBJ` on a class
 pickled by value, then `BUILD`); for a global class the `@cls`/`@s` form above
-is used. Readers accept both.
+is used, with `@newobj` marking the `REDUCE` kind. Readers accept both.
 
 ### `@ns` -- String with NUL bytes (PostgreSQL storage)
 
@@ -350,8 +372,8 @@ order:
 
 **Multi-key markers:**
 
-`@cls` + `@s` (instance with BTree detection), `@dt` + `@tz`
-(timezone-aware datetime)
+`@cls` + `@s` (instance with BTree detection; `@items`, `@appends` and
+`@newobj` ride along), `@dt` + `@tz` (timezone-aware datetime)
 
 **Fallback:** Plain JSON object becomes a Python dict.
 

src/decode.rs

@@ -679,6 +679,7 @@ impl<'a> Decoder<'a> {
                                 state: Box::new(state),
                                 dict_items: None,
                                 list_items: None,
+                                newobj: true,
                             })), build_depth);
                         }
                         PickleValue::Instance(inst) => {
@@ -689,6 +690,7 @@ impl<'a> Decoder<'a> {
                                 state: Box::new(state),
                                 dict_items: inst.dict_items,
                                 list_items: inst.list_items,
+                                newobj: inst.newobj,
                             })), build_depth);
                         }
                         PickleValue::Reduce {
@@ -711,6 +713,7 @@ impl<'a> Decoder<'a> {
                                             state: Box::new(state),
                                             dict_items,
                                             list_items,
+                                            newobj,
                                         })), build_depth);
                                     } else if newobj {
                                         // NEWOBJ with constructor args: the stored shape keeps
@@ -725,6 +728,7 @@ impl<'a> Decoder<'a> {
                                             ])),
                                             dict_items,
                                             list_items,
+                                            newobj: true,
                                         })), Self::nest(build_depth)?);
                                     } else {
                                         // REDUCE with args then BUILD: re-emitting as NEWOBJ would
@@ -766,6 +770,7 @@ impl<'a> Decoder<'a> {
                                 ])),
                                 dict_items: None,
                                 list_items: None,
+                                newobj: true,
                             })), Self::nest(build_depth)?);
                         }
                     }
@@ -1434,6 +1439,7 @@ mod tests {
                 )])),
                 dict_items: None,
                 list_items: None,
+                newobj: true,
             }));
 
             // First element: the Instance from BUILD (stack top)
@@ -1773,6 +1779,37 @@ mod tests {
         }
     }
 
+    #[test]
+    fn test_reduce_empty_args_build_keeps_reduce_kind() {
+        // GLOBAL m.C, EMPTY_TUPLE, REDUCE, {"x": 1}, BUILD: a __reduce__ returning (cls, (), state)
+        let data = b"\x80\x03cm\nC\n)R}X\x01\x00\x00\x00xK\x01sb.";
+        match decode_pickle(data).unwrap() {
+            PickleValue::Instance(inst) => {
+                assert_eq!(inst.module, "m");
+                assert!(!inst.newobj, "REDUCE-created instance must not be marked newobj");
+            }
+            other => panic!("expected Instance, got {other:?}"),
+        }
+        // NEWOBJ stays newobj
+        let data = b"\x80\x03cm\nC\n)\x81}X\x01\x00\x00\x00xK\x01sb.";
+        match decode_pickle(data).unwrap() {
+            PickleValue::Instance(inst) => assert!(inst.newobj),
+            other => panic!("expected Instance, got {other:?}"),
+        }
+        // a second BUILD keeps the kind
+        let data = b"\x80\x03cm\nC\n)R}X\x01\x00\x00\x00xK\x01sb}X\x01\x00\x00\x00yK\x02sb.";
+        match decode_pickle(data).unwrap() {
+            PickleValue::Instance(inst) => {
+                assert!(!inst.newobj);
+                assert_eq!(
+                    *inst.state,
+                    PickleValue::Dict(vec![(PickleValue::String("y".into()), PickleValue::Int(2))])
+                );
+            }
+            other => panic!("expected Instance, got {other:?}"),
+        }
+    }
+
     #[test]
     fn test_newobj_without_build_is_flagged() {
         let data: &[u8] = &[0x80, 0x03, b'c', b'm', b'\n', b'C', b'\n', 0x8c, 0x01, b'a', 0x85, 0x81, b'.'];

src/encode.rs

@@ -339,17 +339,18 @@ impl Encoder {
                 self.write_u8(b'\n');
             }
             PickleValue::Instance(inst) => {
-                let InstanceData { module, name, state, dict_items, list_items } = inst.as_ref();
+                let InstanceData { module, name, state, dict_items, list_items, newobj } = inst.as_ref();
                 if module.is_empty() && name.is_empty() {
                     return self.encode_anonymous_instance(state, depth);
                 }
-                // Emit as: GLOBAL module\nname\n args NEWOBJ [items] state BUILD, where args is
-                // EMPTY_TUPLE unless the state carries constructor args (@args/@state, #12).
+                // Emit as: GLOBAL module\nname\n args NEWOBJ|REDUCE [items] state BUILD, where args
+                // is EMPTY_TUPLE unless the state carries constructor args (@args/@state, #12)
+                // and the opcode follows the construction kind (#32).
                 let (args, state) = match newobj_args_state(state) {
                     Some((args, inner)) => (Some(args), inner),
                     None => (None, state.as_ref()),
                 };
-                self.buf.reserve(5 + module.len() + name.len()); // GLOBAL+mod+\n+name+\n+EMPTY_TUPLE+NEWOBJ
+                self.buf.reserve(5 + module.len() + name.len()); // GLOBAL+mod+\n+name+\n+EMPTY_TUPLE+opcode
                 self.write_u8(GLOBAL);
                 self.write_bytes(module.as_bytes());
                 self.write_u8(b'\n');
@@ -359,7 +360,7 @@ impl Encoder {
                     Some(args) => self.encode_value(args, depth + 1)?,
                     None => self.write_u8(EMPTY_TUPLE),
                 }
-                self.write_u8(NEWOBJ);
+                self.write_u8(if *newobj { NEWOBJ } else { REDUCE });
                 // Dict/list subclass items go before the state, in the order CPython's
                 // save_reduce writes them: APPENDS, then SETITEMS, then BUILD.
                 if let Some(items) = list_items {
@@ -586,6 +587,7 @@ mod tests {
             )])),
             dict_items: Some(Box::new(vec![(PickleValue::String("k".into()), PickleValue::Int(2))])),
             list_items: None,
+            newobj: true,
         }));
         let bytes = encode_pickle(&val).unwrap();
         // CPython's save_reduce: NEWOBJ, then the items (MARK ... SETITEMS), then state + BUILD.
@@ -609,6 +611,7 @@ mod tests {
             state: Box::new(PickleValue::Dict(vec![])),
             dict_items: Some(Box::new(vec![(PickleValue::Int(1), PickleValue::Int(2))])),
             list_items: Some(Box::new(vec![PickleValue::Int(3)])),
+            newobj: true,
         }));
         let bytes = encode_pickle(&val).unwrap();
         let newobj = bytes.iter().position(|&b| b == NEWOBJ).unwrap();
@@ -620,6 +623,36 @@ mod tests {
         assert_eq!(decode_pickle(&bytes).unwrap(), val);
     }
 
+    #[test]
+    fn test_reduce_kind_instance_emits_reduce() {
+        let val = PickleValue::Instance(Box::new(InstanceData {
+            module: "m".into(),
+            name: "C".into(),
+            state: Box::new(PickleValue::Dict(vec![])),
+            dict_items: None,
+            list_items: None,
+            newobj: false,
+        }));
+        let bytes = encode_pickle(&val).unwrap();
+        assert!(bytes.contains(&REDUCE) && !bytes.contains(&NEWOBJ));
+        // with the @args/@state shape: GLOBAL args REDUCE state BUILD
+        let val = PickleValue::Instance(Box::new(InstanceData {
+            module: "m".into(),
+            name: "C".into(),
+            state: Box::new(PickleValue::Dict(vec![
+                (PickleValue::String("@args".into()), PickleValue::Tuple(vec![PickleValue::Int(1)])),
+                (PickleValue::String("@state".into()), PickleValue::Dict(vec![])),
+            ])),
+            dict_items: None,
+            list_items: None,
+            newobj: false,
+        }));
+        let bytes = encode_pickle(&val).unwrap();
+        let r = bytes.iter().position(|&b| b == REDUCE).unwrap();
+        let b = bytes.iter().position(|&b| b == BUILD).unwrap();
+        assert!(r < b && !bytes.contains(&NEWOBJ));
+    }
+
     fn reduce(newobj: bool, state: Option<PickleValue>) -> PickleValue {
         PickleValue::Reduce {
             callable: Box::new(PickleValue::Global { module: "m".into(), name: "C".into() }),
@@ -659,6 +692,7 @@ mod tests {
             ])),
             dict_items: None,
             list_items: None,
+            newobj: true,
         }));
         let bytes = encode_pickle(&val).unwrap();
         let newobj = bytes.iter().position(|&b| b == NEWOBJ).unwrap();