Repository: boa-dev/boa (rust)

Issue:
to_json() on a nested data structure containing undefined should not fail
**Describe the bug**

When calling `to_json` on an object like `{foo: undefined}`, it should result in `{}` and not panic.  Here are some various behaviors (demo's using Node) which it would be good to follow:

```
> JSON.stringify({foo: undefined, bar: 1})
'{"bar":1}'
> JSON.stringify(undefined)
undefined
> JSON.stringify([1,2,undefined,4])
'[1,2,null,4]'
```

For context, I'm building a 3rd-party plugin system for [Yaak](https://yaak.app) using Boa and users may return a nested data structure that contain `undefined` values.

**To Reproduce**

Return an object from the JS context and call `to_json` on it.

```javascript
export function callMe() {
  return { foo: undefined };
}
```

**Expected behavior**

The value should serialize to `{}` and not throw `not yet implemented: undefined to JSON`.

**Build environment (please complete the following information):**

- OS: macOS
- Version: 14
- Target triple: `aarch64-apple-darwin`
- Rustc version: `rustc 1.77.2 (25ef9e3d8 2024-04-09)`

**Additional context**

The current workaround is to return `JSON.parse(JSON.stringify(objectContainingUndefineds))`. However, as I'm using Boa to build a plugin system, I wouldn't expect users to have to do this themselves.

Another potential workaround is to recursively crawl the `JSValue` and remove undefined values from all the objects and arrays.

Thanks for taking the time to look into this!


Pull request description:
Update JsValue::to_json to support undefined
This Pull Request fixes/closes #3842.

Personally I've found `JsValue::to_json` to be fairly useless in its current implementation, as it will panic if the data structure being converted contains undefined, and I often don't have control over this.

The PR changes the following:

The `JsValue::to_json` method no longer panics if the structure contains `undefined`.

If `undefined` is encountered it will:
- Omit the property if it is part of an object.
- Replace with `null` if `undefined` is an element of an array. I don't love this, but couldn't think of a better solution.
- Return None if `to_json` is being called on `JsValue::undefined()`.

Because of the last point there is a breaking change: `JsValue::to_json` now returns `JsResult<Option<Value>>` instead of `JsResult<Value>`.

To avoid a breaking change we could perhaps introduce a new method `try_to_json` instead, and have the original `to_json` call `try_to_json` and panic if `None` is returned.

On the other hand, the new behaviour forces people to think about handling `undefined`, which will probably prevent unexpected panics in the future.

Patch:
diff --git a/Cargo.lock b/Cargo.lock
index 84f0da13..cc0da003 100644
--- a/Cargo.lock
+++ b/Cargo.lock
@@ -753,9 +753,9 @@ dependencies = [
 
 [[package]]
 name = "clap"
-version = "4.5.31"
+version = "4.5.32"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "027bb0d98429ae334a8698531da7077bdf906419543a35a55c2cb1b66437d767"
+checksum = "6088f3ae8c3608d19260cd7445411865a485688711b78b5be70d78cd96136f83"
 dependencies = [
  "clap_builder",
  "clap_derive",
@@ -763,9 +763,9 @@ dependencies = [
 
 [[package]]
 name = "clap_builder"
-version = "4.5.31"
+version = "4.5.32"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "5589e0cba072e0f3d23791efac0fd8627b49c829c196a492e88168e6a669d863"
+checksum = "22a7ef7f676155edfb82daa97f99441f3ebf4a58d5e32f295a56259f1b6facc8"
 dependencies = [
  "anstream",
  "anstyle",
@@ -775,9 +775,9 @@ dependencies = [
 
 [[package]]
 name = "clap_derive"
-version = "4.5.28"
+version = "4.5.32"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "bf4ced95c6f4a675af3da73304b9ac4ed991640c36374e4b46795c49e17cf1ed"
+checksum = "09176aae279615badda0765c0c0b3f6ed53f4709118af73cf4655d85d1530cd7"
 dependencies = [
  "heck",
  "proc-macro2",
@@ -2172,9 +2172,9 @@ checksum = "ce23b50ad8242c51a442f3ff322d56b02f08852c77e4c0b4d3fd684abc89c683"
 
 [[package]]
 name = "indexmap"
-version = "2.7.1"
+version = "2.8.0"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "8c9c992b02b5b4c94ea26e32fe5bccb7aa7d9f390ab5c1221ff895bc7ea8b652"
+checksum = "3954d50fe15b02142bf25d3b8bdadb634ec3948f103d04ffe3031bc8fe9d7058"
 dependencies = [
  "equivalent",
  "hashbrown 0.15.2",
@@ -2647,9 +2647,9 @@ dependencies = [
 
 [[package]]
 name = "once_cell"
-version = "1.20.3"
+version = "1.21.1"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "945462a4b81e43c4e3ba96bd7b49d834c6f61198356aa858733bc4acf3cbe62e"
+checksum = "d75b0bedcc4fe52caa0e03d9f1151a323e4aa5e2d78ba3580400cd3c9e2bc4bc"
 dependencies = [
  "critical-section",
  "portable-atomic",
@@ -2963,9 +2963,9 @@ dependencies = [
 
 [[package]]
 name = "quote"
-version = "1.0.39"
+version = "1.0.40"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "c1f1914ce909e1658d9907913b4b91947430c7d9be598b15a1912935b8c04801"
+checksum = "1885c039570dc00dcb4ff087a89e185fd56bae234ddc7f056a945bf36467248d"
 dependencies = [
  "proc-macro2",
 ]
@@ -3751,9 +3751,9 @@ dependencies = [
 
 [[package]]
 name = "tokio"
-version = "1.44.0"
+version = "1.44.1"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "9975ea0f48b5aa3972bf2d888c238182458437cc2a19374b81b25cdf1023fb3a"
+checksum = "f382da615b842244d4b8738c82ed1275e6c5dd90c459a30941cd07080b06c91a"
 dependencies = [
  "backtrace",
  "pin-project-lite",
diff --git a/Cargo.toml b/Cargo.toml
index d095d3b2..c586fe76 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -51,18 +51,18 @@ tag_ptr = { path = "utils/tag_ptr" }
 # Shared deps
 arbitrary = "1"
 bitflags = "2.9.0"
-clap = "4.5.31"
+clap = "4.5.32"
 colored = "3.0.0"
 cow-utils = "0.1.3"
 fast-float2 = "0.2.3"
 hashbrown = "0.15.2"
-indexmap = { version = "2.7.1", default-features = false }
+indexmap = { version = "2.8.0", default-features = false }
 indoc = "2.0.6"
 itoa = "1.0.15"
 jemallocator = "0.5.4"
 num-bigint = "0.4.6"
 num-traits = "0.2.19"
-once_cell = { version = "1.20.3", default-features = false }
+once_cell = { version = "1.21.1", default-features = false }
 phf = { version = "0.11.2", default-features = false }
 pollster = "0.4.0"
 regex = "1.11.1"
@@ -99,7 +99,7 @@ smol = "2.0.2"
 isahc = "1.7.2"
 rustyline = { version = "15.0.0", default-features = false }
 dhat = "0.3.3"
-quote = "1.0.39"
+quote = "1.0.40"
 syn = { version = "2.0.100", default-features = false }
 proc-macro2 = "1.0"
 synstructure = "0.13"
@@ -129,7 +129,7 @@ float-cmp = "0.10.0"
 futures-lite = "2.6.0"
 test-case = "3.3.1"
 url = "2.5.4"
-tokio = { version = "1.44.0", default-features = false }
+tokio = { version = "1.44.1", default-features = false }
 futures-concurrency = "7.6.3"
 
 
diff --git a/core/engine/src/value/conversions/serde_json.rs b/core/engine/src/value/conversions/serde_json.rs
index cbc1a1bc..6d699a62 100644
--- a/core/engine/src/value/conversions/serde_json.rs
+++ b/core/engine/src/value/conversions/serde_json.rs
@@ -35,7 +35,7 @@ impl JsValue {
     /// let mut context = Context::default();
     /// let value = JsValue::from_json(&json, &mut context).unwrap();
     /// #
-    /// # assert_eq!(json, value.to_json(&mut context).unwrap());
+    /// # assert_eq!(Some(json), value.to_json(&mut context).unwrap());
     /// ```
     pub fn from_json(json: &Value, context: &mut Context) -> JsResult<Self> {
         /// Biggest possible integer, as i64.
@@ -85,6 +85,9 @@ impl JsValue {
 
     /// Converts the `JsValue` to a [`serde_json::Value`].
     ///
+    /// If the `JsValue` is `Undefined`, this method will return `None`.
+    /// Otherwise it will return the corresponding `serde_json::Value`.
+    ///
     /// # Example
     ///
     /// ```
@@ -107,13 +110,9 @@ impl JsValue {
     ///
     /// let back_to_json = value.to_json(&mut context).unwrap();
     /// #
-    /// # assert_eq!(json, back_to_json);
+    /// # assert_eq!(Some(json), back_to_json);
     /// ```
-    ///
-    /// # Panics
-    ///
-    /// Panics if the `JsValue` is `Undefined`.
-    pub fn to_json(&self, context: &mut Context) -> JsResult<Value> {
+    pub fn to_json(&self, context: &mut Context) -> JsResult<Option<Value>> {
         let mut seen_objects = HashSet::new();
         self.to_json_inner(context, &mut seen_objects)
     }
@@ -122,14 +121,14 @@ impl JsValue {
         &self,
         context: &mut Context,
         seen_objects: &mut HashSet<JsObject>,
-    ) -> JsResult<Value> {
+    ) -> JsResult<Option<Value>> {
         match self.variant() {
-            JsVariant::Null => Ok(Value::Null),
-            JsVariant::Undefined => todo!("undefined to JSON"),
-            JsVariant::Boolean(b) => Ok(Value::from(b)),
-            JsVariant::String(string) => Ok(string.to_std_string_escaped().into()),
-            JsVariant::Float64(rat) => Ok(Value::from(rat)),
-            JsVariant::Integer32(int) => Ok(Value::from(int)),
+            JsVariant::Null => Ok(Some(Value::Null)),
+            JsVariant::Undefined => Ok(None),
+            JsVariant::Boolean(b) => Ok(Some(Value::from(b))),
+            JsVariant::String(string) => Ok(Some(string.to_std_string_escaped().into())),
+            JsVariant::Float64(rat) => Ok(Some(Value::from(rat))),
+            JsVariant::Integer32(int) => Ok(Some(Value::from(int))),
             JsVariant::BigInt(_bigint) => Err(JsNativeError::typ()
                 .with_message("cannot convert bigint to JSON")
                 .into()),
@@ -148,7 +147,7 @@ impl JsValue {
                             x.value()
                                 .map(|val| val.to_json_inner(context, seen_objects))
                         })
-                        .unwrap_or(Ok(Value::Null))
+                        .unwrap_or(Ok(Some(Value::Null)))
                 };
 
                 if obj.is_array() {
@@ -157,19 +156,26 @@ impl JsValue {
 
                     for k in 0..len as u32 {
                         let val = value_by_prop_key(k.into(), context)?;
-                        arr.push(val);
+                        match val {
+                            Some(val) => arr.push(val),
+
+                            // Undefined in array. Substitute with null as Value doesn't support Undefined.
+                            None => arr.push(Value::Null),
+                        }
                     }
                     // Passing the object rather than its clone that was inserted to the set should be fine
                     // as they hash to the same value and therefore HashSet can still remove the clone
                     seen_objects.remove(obj);
-                    Ok(Value::Array(arr))
+                    Ok(Some(Value::Array(arr)))
                 } else {
                     let mut map = Map::new();
 
                     for index in obj.borrow().properties().index_property_keys() {
                         let key = index.to_string();
                         let value = value_by_prop_key(index.into(), context)?;
-                        map.insert(key, value);
+                        if let Some(value) = value {
+                            map.insert(key, value);
+                        }
                     }
 
                     for property_key in obj.borrow().properties().shape.keys() {
@@ -183,10 +189,12 @@ impl JsValue {
                             }
                         };
                         let value = value_by_prop_key(property_key, context)?;
-                        map.insert(key, value);
+                        if let Some(value) = value {
+                            map.insert(key, value);
+                        }
                     }
                     seen_objects.remove(obj);
-                    Ok(Value::Object(map))
+                    Ok(Some(Value::Object(map)))
                 }
             }
             JsVariant::Symbol(_sym) => Err(JsNativeError::typ()
@@ -266,7 +274,7 @@ mod tests {
                 assert_eq!(arr.at(3, ctx).unwrap(), true.into());
             }
 
-            assert_eq!(json, value.to_json(ctx).unwrap());
+            assert_eq!(Some(json), value.to_json(ctx).unwrap());
         })]);
     }
 
@@ -274,22 +282,22 @@ mod tests {
     fn integer_ops_to_json() {
         run_test_actions([
             TestAction::assert_with_op("1000000 + 500", |v, ctx| {
-                v.to_json(ctx).unwrap() == json!(1_000_500)
+                v.to_json(ctx).unwrap() == Some(json!(1_000_500))
             }),
             TestAction::assert_with_op("1000000 - 500", |v, ctx| {
-                v.to_json(ctx).unwrap() == json!(999_500)
+                v.to_json(ctx).unwrap() == Some(json!(999_500))
             }),
             TestAction::assert_with_op("1000000 * 500", |v, ctx| {
-                v.to_json(ctx).unwrap() == json!(500_000_000)
+                v.to_json(ctx).unwrap() == Some(json!(500_000_000))
             }),
             TestAction::assert_with_op("1000000 / 500", |v, ctx| {
-                v.to_json(ctx).unwrap() == json!(2_000)
+                v.to_json(ctx).unwrap() == Some(json!(2_000))
             }),
             TestAction::assert_with_op("233894 % 500", |v, ctx| {
-                v.to_json(ctx).unwrap() == json!(394)
+                v.to_json(ctx).unwrap() == Some(json!(394))
             }),
             TestAction::assert_with_op("36 ** 5", |v, ctx| {
-                v.to_json(ctx).unwrap() == json!(60_466_176)
+                v.to_json(ctx).unwrap() == Some(json!(60_466_176))
             }),
         ]);
     }
@@ -308,4 +316,58 @@ mod tests {
             "TypeError: cyclic object value"
         );
     }
+
+    #[test]
+    fn to_json_undefined() {
+        let mut context = Context::default();
+        let undefined_value = JsValue::undefined();
+        assert!(undefined_value.to_json(&mut context).unwrap().is_none());
+    }
+
+    #[test]
+    fn to_json_undefined_in_structure() {
+        let mut context = Context::default();
+        let object_with_undefined = {
+            // Defining the following structure:
+            // {
+            //     "outer_a": 1,
+            //     "outer_b": undefined,
+            //     "outer_c": [2, undefined, 3, { "inner_a": undefined }]
+            // }
+
+            let inner = JsObject::with_null_proto();
+            inner
+                .create_data_property(js_string!("inner_a"), JsValue::undefined(), &mut context)
+                .expect("should add property");
+
+            let array = JsArray::new(&mut context);
+            array.push(2, &mut context).expect("should push");
+            array
+                .push(JsValue::undefined(), &mut context)
+                .expect("should push");
+            array.push(3, &mut context).expect("should push");
+            array.push(inner, &mut context).expect("should push");
+
+            let outer = JsObject::with_null_proto();
+            outer
+                .create_data_property(js_string!("outer_a"), JsValue::new(1), &mut context)
+                .expect("should add property");
+            outer
+                .create_data_property(js_string!("outer_b"), JsValue::undefined(), &mut context)
+                .expect("should add property");
+            outer
+                .create_data_property(js_string!("outer_c"), array, &mut context)
+                .expect("should add property");
+
+            JsValue::from(outer)
+        };
+
+        assert_eq!(
+            Some(json!({
+                "outer_a": 1,
+                "outer_c": [2, null, 3, { }]
+            })),
+            object_with_undefined.to_json(&mut context).unwrap()
+        );
+    }
 }


Test patch:
diff --git a/core/engine/tests/module.rs b/core/engine/tests/module.rs
index 93db4609..bd54b4e7 100644
--- a/core/engine/tests/module.rs
+++ b/core/engine/tests/module.rs
@@ -59,7 +59,7 @@ fn test_json_module_from_str() {
         .unwrap();
 
     assert_eq!(
-        JsString::from(json.to_json(&mut context).unwrap().to_string()),
+        JsString::from(json.to_json(&mut context).unwrap().unwrap().to_string()),
         json_string
     );
 }