fix(error): fs diagnostics become own properties of the error, not of its message string

changelog.d/8889-error-own-properties.md

@@ -0,0 +1,46 @@
+Fixed an fs error's `code`/`errno`/`syscall`/`path`: they are now own properties
+of the **error object** rather than entries in side tables keyed by the error's
+**message string address**.
+
+Two defects followed from that keying, both verified against node.
+
+**The wrong error got the metadata.** Any `new Error(m)` built from the same
+message text inherited an unrelated fs error's fields — `.code` returned
+`ENOENT` where node returns `undefined`, along with `.syscall`, `.errno` and
+`.path`. The metadata belonged to the string, so anything holding that string
+answered to it.
+
+**They were invisible to reflection.** In node these are ordinary own
+properties; served from a side table behind property *getters* they appeared in
+none of the enumeration paths:
+
+| | before | after (= node) |
+|---|---|---|
+| `Object.keys(e)` | `[]` | `code,errno,path,syscall` |
+| `hasOwnProperty('code')` | `false` | `true` |
+| `getOwnPropertyDescriptor` | `undefined` | `{value,writable,enumerable,configurable}` |
+| `JSON.stringify(e)` | `{}` | `{"errno":-2,"code":"ENOENT",…}` |
+| `{...e}` | `{}` | same |
+
+Any code that logged or serialised a caught fs error lost its whole payload.
+
+Three sites each held a different wrong assumption about errors. The fs builders
+keyed on the message string. `JSON.stringify` hardcoded `"{}"` for
+`GC_TYPE_ERROR` — correct for a *plain* error, whose `message`/`name`/`stack`
+are non-enumerable, but wrong once an error carries enumerable own properties,
+so it also dropped **user-assigned** ones (`e.foo=1; JSON.stringify(e)` gave
+`{}` where node gives `{"foo":1}`; that half is independent of fs).
+`Object.assign`/spread had no Error arm and copied nothing. All three now
+enumerate through `exotic_own_keys(.., enumerable_only = true)` — the same
+enumeration `Object.keys` uses — so they cannot drift apart again.
+
+Property **order** is fixed too. `ERROR_USER_PROPS` was a `HashMap` with an
+alphabetical `sort_by` bolted on for determinism: stable, but not node's. Own
+string keys enumerate in insertion order per ECMA-262, and that order is
+observable through all four paths above. The store is insertion-ordered now,
+reassignment keeps a key's original position (`o.a=1; o.b=2; o.a=3` enumerates
+`a,b`), and the fs fields install in node's `uvException` order. The GC root
+scanner over these properties moved to the ordered store.
+
+Verified by running three repro programs against node on the same host:
+byte-identical output, key order included.

crates/perry-runtime/src/fs/errors.rs

@@ -102,6 +102,54 @@ pub(crate) fn io_error_errno(err: &std::io::Error) -> i32 {
     }
 }
 
+/// Attach Node's fs diagnostic fields to `err_ptr` as **own properties of the
+/// error object**.
+///
+/// These used to be registered in six side tables keyed by the MESSAGE
+/// STRING's address (`register_error_code_pub` and friends), which produced two
+/// defects:
+///
+/// * **Wrong error.** Any `new Error(m)` built from the same message text
+///   inherited the unrelated fs error's fields — `new Error(e.message).code`
+///   returned `ENOENT` where node returns `undefined`, along with `.syscall`,
+///   `.errno` and `.path`. Metadata belonged to the string, not the throw.
+/// * **Invisible to reflection.** In node these are ordinary own properties:
+///   `Object.keys(e)` is `code,errno,path,syscall`, and `JSON.stringify(e)` and
+///   `{...e}` carry them. Served from a side table behind property *getters*
+///   they were absent from all of it — perry returned `{}` for both, so any
+///   code that logs or serialises an fs error silently lost every field.
+///
+/// Keying on the error object fixes both at once, and each field then reaches
+/// reflection through the same path a user assignment does.
+unsafe fn attach_fs_error_props(
+    err_ptr: *mut crate::error::ErrorHeader,
+    code: &str,
+    errno: i32,
+    syscall: &str,
+    path: Option<&str>,
+    dest: Option<&str>,
+) {
+    use crate::node_submodules::set_error_user_prop;
+    let owner = err_ptr as usize;
+    let put_str = |key: &str, s: &str| {
+        let boxed = js_string_from_bytes(s.as_ptr(), s.len() as u32);
+        set_error_user_prop(owner, key, crate::value::js_nanbox_string(boxed as i64));
+    };
+    // Insertion order is observable — `Object.keys`, `for…in`, `{...err}` and
+    // `JSON.stringify` all report it — so install these in the same order
+    // node's `uvException` does: errno, code, syscall, path, dest.
+    // `errno` is numeric in node (-2 for ENOENT), not a string.
+    set_error_user_prop(owner, "errno", errno as f64);
+    put_str("code", code);
+    put_str("syscall", syscall);
+    if let Some(p) = path {
+        put_str("path", p);
+    }
+    if let Some(d) = dest {
+        put_str("dest", d);
+    }
+}
+
 pub(crate) unsafe fn build_fs_error_value(
     err: &std::io::Error,
     syscall: &'static str,
@@ -112,13 +160,7 @@ pub(crate) unsafe fn build_fs_error_value(
     let msg = format!("{}: {}, {} '{}'", code, err, syscall, path);
     let msg_ptr = js_string_from_bytes(msg.as_ptr(), msg.len() as u32);
     let err_ptr = crate::error::js_error_new_with_message(msg_ptr);
-    // Register code/syscall/path in the per-message side tables so the
-    // `.code`, `.syscall`, `.path` property getters in `field_get_set`
-    // surface Node-compatible values on caught errors.
-    crate::node_submodules::register_error_code_pub(msg_ptr, code);
-    crate::node_submodules::register_error_errno(msg_ptr, errno);
-    crate::node_submodules::register_error_syscall(msg_ptr, syscall);
-    crate::node_submodules::register_error_path(msg_ptr, path.to_string());
+    attach_fs_error_props(err_ptr, code, errno, syscall, Some(path), None);
     crate::value::js_nanbox_pointer(err_ptr as i64)
 }
 
@@ -136,11 +178,7 @@ pub(crate) unsafe fn build_fs_error_value_with_dest(
     let msg = format!("{}: {}, {} '{}' -> '{}'", code, err, syscall, path, dest);
     let msg_ptr = js_string_from_bytes(msg.as_ptr(), msg.len() as u32);
     let err_ptr = crate::error::js_error_new_with_message(msg_ptr);
-    crate::node_submodules::register_error_code_pub(msg_ptr, code);
-    crate::node_submodules::register_error_errno(msg_ptr, errno);
-    crate::node_submodules::register_error_syscall(msg_ptr, syscall);
-    crate::node_submodules::register_error_path(msg_ptr, path.to_string());
-    crate::node_submodules::register_error_dest(msg_ptr, dest.to_string());
+    attach_fs_error_props(err_ptr, code, errno, syscall, Some(path), Some(dest));
     crate::value::js_nanbox_pointer(err_ptr as i64)
 }
 
@@ -153,9 +191,7 @@ pub(crate) unsafe fn build_fs_error_value_no_path(
     let msg = format!("{}: {}, {}", code, err, syscall);
     let msg_ptr = js_string_from_bytes(msg.as_ptr(), msg.len() as u32);
     let err_ptr = crate::error::js_error_new_with_message(msg_ptr);
-    crate::node_submodules::register_error_code_pub(msg_ptr, code);
-    crate::node_submodules::register_error_errno(msg_ptr, errno);
-    crate::node_submodules::register_error_syscall(msg_ptr, syscall);
+    attach_fs_error_props(err_ptr, code, errno, syscall, None, None);
     crate::value::js_nanbox_pointer(err_ptr as i64)
 }
 

crates/perry-runtime/src/json/stringify.rs

@@ -357,6 +357,56 @@ pub(crate) unsafe fn arm_to_json_result_guard(result: f64) {
     }
 }
 
+/// Serialize an `Error`'s own ENUMERABLE properties, the way node does.
+///
+/// Errors do not have the JSObject keys/values layout, so they cannot go
+/// through `stringify_object` — but they are still ordinary property bearers to
+/// an observer. `exotic_own_keys(.., enumerable_only = true)` is the same
+/// enumeration `Object.keys` uses on an error, so JSON output and `Object.keys`
+/// cannot disagree.
+///
+/// `depth` is `Some` when called from the depth-tracking variant, so nested
+/// values keep the cycle/recursion budget of the caller.
+unsafe fn stringify_error_own_props(ptr: *const u8, buf: &mut String, depth: Option<u32>) {
+    let ptr = ptr as usize;
+    use crate::object::exotic_expando::{exotic_get_own_property, exotic_own_keys, ExoticKind};
+    let keys = exotic_own_keys(ExoticKind::Error, ptr, true);
+    buf.push('{');
+    let mut first = true;
+    for key in keys {
+        let Some(v) = exotic_get_own_property(
+            ptr,
+            ExoticKind::Error,
+            &key,
+            f64::from_bits(bits_of_ptr(ptr)),
+        ) else {
+            continue;
+        };
+        // `undefined` own properties are omitted from objects, per JSON.stringify.
+        if v.to_bits() == crate::value::TAG_UNDEFINED {
+            continue;
+        }
+        if !first {
+            buf.push(',');
+        }
+        first = false;
+        write_escaped_string(buf, &key);
+        buf.push(':');
+        match depth {
+            Some(d) => stringify_value_depth(v, 0, buf, d + 1),
+            None => stringify_value(v, 0, buf),
+        }
+    }
+    buf.push('}');
+}
+
+/// NaN-box `ptr` back into the pointer value an exotic `[[Get]]` wants as its
+/// `receiver` (used only to rebind `this` for an accessor property).
+#[inline]
+fn bits_of_ptr(ptr: usize) -> u64 {
+    crate::value::js_nanbox_pointer(ptr as i64).to_bits()
+}
+
 #[inline]
 pub(crate) unsafe fn stringify_value(value: f64, type_hint: u32, buf: &mut String) {
     let bits: u64 = value.to_bits();
@@ -554,15 +604,24 @@ pub(crate) unsafe fn stringify_value(value: f64, type_hint: u32, buf: &mut Strin
                 }
             }
             crate::gc::GC_TYPE_ERROR => {
-                // Issue #928: Built-in Error objects (and subclasses
-                // like TypeError) have a dedicated `ErrorHeader` layout —
-                // not the JSObject keys/values layout. Routing them
-                // through `stringify_object` derefs garbage as a
-                // `keys_array` pointer and segfaults the process.
-                // Node's `JSON.stringify(new Error("x"))` returns "{}"
-                // because Error's intrinsic props (`message`, `name`,
-                // `stack`) are non-enumerable; mirror that.
-                buf.push_str("{}");
+                // Issue #928: Built-in Error objects (and subclasses like
+                // TypeError) have a dedicated `ErrorHeader` layout — not the
+                // JSObject keys/values layout — so they must never reach
+                // `stringify_object`, which would deref garbage as a
+                // `keys_array` pointer and segfault.
+                //
+                // They are NOT always "{}", though. Node emits an error's own
+                // ENUMERABLE properties like any other object; `{}` is merely
+                // what a *plain* error produces, because `message`/`name`/
+                // `stack` are non-enumerable:
+                //
+                //   JSON.stringify(new Error("x"))            -> {}
+                //   e.foo = 1; JSON.stringify(e)              -> {"foo":1}
+                //   JSON.stringify(fsError)                   -> {"errno":-2,"code":"ENOENT",…}
+                //
+                // Hardcoding "{}" silently dropped every one of those, so any
+                // code that logs a caught error as JSON lost its whole payload.
+                stringify_error_own_props(ptr, buf, None);
             }
             crate::gc::GC_TYPE_MAP | crate::gc::GC_TYPE_SET => {
                 // Map/Set have a `{size, capacity, entries/elements}` header,
@@ -770,7 +829,7 @@ pub(crate) unsafe fn stringify_value_depth(
             }
             crate::gc::GC_TYPE_ERROR => {
                 // Issue #928: see the matching branch in `stringify_value`.
-                buf.push_str("{}");
+                stringify_error_own_props(ptr, buf, Some(depth));
             }
             crate::gc::GC_TYPE_MAP | crate::gc::GC_TYPE_SET => {
                 // See the matching branch in `stringify_value` — Map/Set

crates/perry-runtime/src/node_submodules/diagnostics.rs

@@ -602,7 +602,19 @@ thread_local! {
     /// string/primitive own properties. Stale entries after a GC move of the
     /// error are harmless (same model as the message-keyed tables above): a
     /// lookup at the new address simply misses.
-    pub(crate) static ERROR_USER_PROPS: RefCell<HashMap<usize, HashMap<String, ErrUserProp>>> =
+    /// Insertion-ORDERED per error: a `Vec`, not a `HashMap`.
+    ///
+    /// ECMA-262 enumerates an object's own string keys in insertion order, and
+    /// that order is observable through `Object.keys`, `for…in`, `{...err}` and
+    /// `JSON.stringify`. Backed by a `HashMap` this list came out in hash order,
+    /// so `error_user_props` sorted it alphabetically to at least be
+    /// deterministic — which is stable but still not node's order. A caught fs
+    /// error serialized as `{"code":…,"errno":…,"path":…,"syscall":…}` where
+    /// node writes `{"errno":…,"code":…,"syscall":…,"path":…}`.
+    ///
+    /// An error carries a handful of properties, so a linear scan is cheaper
+    /// than hashing and the order falls out for free.
+    pub(crate) static ERROR_USER_PROPS: RefCell<HashMap<usize, Vec<(String, ErrUserProp)>>> =
         RefCell::new(HashMap::new());
 }
 
@@ -629,10 +641,14 @@ pub fn set_error_user_prop(error_ptr: usize, key: &str, value: f64) {
         ErrUserProp::Bits(value.to_bits())
     };
     ERROR_USER_PROPS.with(|m| {
-        m.borrow_mut()
-            .entry(error_ptr)
-            .or_default()
-            .insert(key.to_string(), stored);
+        let mut map = m.borrow_mut();
+        let props = map.entry(error_ptr).or_default();
+        // Reassigning an existing key keeps its original position — `o.a=1;
+        // o.b=2; o.a=3` still enumerates `a,b` in node.
+        match props.iter_mut().find(|(k, _)| k == key) {
+            Some(slot) => slot.1 = stored,
+            None => props.push((key.to_string(), stored)),
+        }
     });
 }
 
@@ -645,7 +661,7 @@ pub fn error_user_prop(error_ptr: usize, key: &str) -> Option<f64> {
     }
     ERROR_USER_PROPS.with(|m| {
         m.borrow().get(&error_ptr).and_then(|props| {
-            props.get(key).map(|v| match v {
+            props.iter().find(|(k, _)| k == key).map(|(_, v)| match v {
                 ErrUserProp::Str(s) => {
                     let ptr = js_string_from_bytes(s.as_ptr(), s.len() as u32);
                     f64::from_bits(crate::js_nanbox_string(ptr as i64).to_bits())
@@ -666,7 +682,13 @@ pub fn remove_error_user_prop(error_ptr: usize, key: &str) -> bool {
     ERROR_USER_PROPS.with(|m| {
         m.borrow_mut()
             .get_mut(&error_ptr)
-            .map(|props| props.remove(key).is_some())
+            .map(|props| match props.iter().position(|(k, _)| k == key) {
+                Some(i) => {
+                    props.remove(i);
+                    true
+                }
+                None => false,
+            })
             .unwrap_or(false)
     })
 }
@@ -701,7 +723,8 @@ pub fn error_user_props(error_ptr: usize) -> Vec<(String, f64)> {
             (key, materialized)
         })
         .collect();
-    props.sort_by(|a, b| a.0.cmp(&b.0));
+    // No sort: the Vec is already in insertion order, which is the order
+    // ECMA-262 specifies and node emits.
     props
 }
 
@@ -1998,3 +2021,69 @@ mod tests {
         DIAG_CHANNELS.with(|m| m.borrow_mut().clear());
     }
 }
+
+#[cfg(test)]
+mod error_prop_order_tests {
+    use super::*;
+
+    /// Own string keys enumerate in INSERTION order, not hash or alphabetical
+    /// order. This is observable through `Object.keys`, `for…in`, `{...err}`
+    /// and `JSON.stringify`, so a caught fs error must serialize as node's
+    /// `{"errno":…,"code":…,"syscall":…,"path":…}`.
+    ///
+    /// The store was a `HashMap` with an alphabetical `sort_by` bolted on for
+    /// determinism, which is stable but wrong: it emitted `code` before
+    /// `errno`. Reverting to any unordered container fails this test.
+    #[test]
+    fn user_props_enumerate_in_insertion_order() {
+        let err = 0x4000_1000usize;
+        for k in ["errno", "code", "syscall", "path"] {
+            set_error_user_prop(err, k, 1.0);
+        }
+        let keys: Vec<String> = error_user_props(err).into_iter().map(|(k, _)| k).collect();
+        assert_eq!(
+            keys,
+            vec![
+                "errno".to_string(),
+                "code".to_string(),
+                "syscall".to_string(),
+                "path".to_string()
+            ],
+            "fs error fields must enumerate in node's insertion order, not sorted"
+        );
+    }
+
+    /// Reassigning an existing key keeps its ORIGINAL position — in node,
+    /// `o.a=1; o.b=2; o.a=3` still enumerates `a,b`. An implementation that
+    /// removed-then-appended would report `b,a`.
+    #[test]
+    fn reassignment_keeps_original_position() {
+        let err = 0x4000_2000usize;
+        set_error_user_prop(err, "a", 1.0);
+        set_error_user_prop(err, "b", 2.0);
+        set_error_user_prop(err, "a", 3.0);
+        let keys: Vec<String> = error_user_props(err).into_iter().map(|(k, _)| k).collect();
+        assert_eq!(keys, vec!["a".to_string(), "b".to_string()]);
+        assert_eq!(
+            error_user_prop(err, "a"),
+            Some(3.0),
+            "reassignment must still update the value"
+        );
+    }
+
+    /// Removing a key must not disturb the order of the survivors.
+    #[test]
+    fn removal_preserves_order_of_the_rest() {
+        let err = 0x4000_3000usize;
+        for k in ["one", "two", "three"] {
+            set_error_user_prop(err, k, 0.0);
+        }
+        assert!(remove_error_user_prop(err, "two"));
+        let keys: Vec<String> = error_user_props(err).into_iter().map(|(k, _)| k).collect();
+        assert_eq!(keys, vec!["one".to_string(), "three".to_string()]);
+        assert!(
+            !remove_error_user_prop(err, "two"),
+            "second remove is a no-op"
+        );
+    }
+}

crates/perry-runtime/src/node_submodules/diagnostics_gc.rs

@@ -133,7 +133,7 @@ pub(crate) fn finalize_dead_copied_minor_from_space_errors() {
 pub(crate) fn scan_error_user_props_roots_mut(visitor: &mut crate::gc::RuntimeRootVisitor<'_>) {
     ERROR_USER_PROPS.with(|m| {
         for props in m.borrow_mut().values_mut() {
-            for v in props.values_mut() {
+            for (_, v) in props.iter_mut() {
                 if let ErrUserProp::Bits(bits) = v {
                     visitor.visit_nanbox_u64_slot(bits);
                 }