Repository: denoland/deno_core
Pull request #681 (2024-03-25 14:29:27)

refactor: Make serde_v8::Value private
This commit makes serde_v8::Value variant private - meaning that users
can no longer accept `serde_v8::Value` in the ops as arguments. Instead
`v8::Value` should be used directly. It is also disallowed to use `serde_v8::Value`
in the structure arguments - instead the `v8::Value` should be passed as an
explicit argument. It lowers flexibility, but makes it so that there's only a 
single way of passing V8 values.

Unfortunately the implementation itself can't be removed because it's used internally
to provide serde for `#[serde]` arguments.

codecov-commenter: ## [Codecov](https://app.codecov.io/gh/denoland/deno_core/pull/681?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=denoland) Report
All modified and coverable lines are covered by tests :white_check_mark:
> Project coverage is 81.29%. Comparing base [(`79e73e5`)](https://app.codecov.io/gh/denoland/deno_core/commit/79e73e500e409063cfbc165b58314d2c66cbf371?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=denoland) to head [(`3b7ae37`)](https://app.codecov.io/gh/denoland/deno_core/pull/681?dropdown=coverage&src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=denoland).


<details><summary>Additional details and impacted files</summary>


```diff
@@            Coverage Diff             @@
##             main     #681      +/-   ##
==========================================
+ Coverage   81.14%   81.29%   +0.14%     
==========================================
  Files          96       95       -1     
  Lines       24347    24277      -70     
==========================================
- Hits        19756    19735      -21     
+ Misses       4591     4542      -49     
```



</details>

[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/denoland/deno_core/pull/681?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=denoland).   
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=denoland).


Patch:
diff --git a/ops/op2/signature.rs b/ops/op2/signature.rs
index ad45e2d..9a403f4 100644
--- a/ops/op2/signature.rs
+++ b/ops/op2/signature.rs
@@ -1516,7 +1516,6 @@ pub(crate) fn parse_type(
           let token = stringify_token(of.path.clone());
           if let Ok(Some(err)) = std::panic::catch_unwind(|| {
             rules!(ty => {
-              ( serde_v8::Value $( < $_lifetime:lifetime $(,)? >)? ) => Some("use v8::Value"),
               ( Value $( < $_lifetime:lifetime $(,)? >)? ) => Some("use a fully-qualified type: v8::Value or serde_json::Value"),
               ( $_ty:ty ) => None,
             })
@@ -2081,9 +2080,4 @@ mod tests {
     ArgError("s", InvalidSerdeAttributeType("&str")),
     fn f(#[serde] s: &str) {}
   );
-  expect_fail!(
-    op_with_bad_serde_value,
-    ArgError("v", InvalidSerdeType("serde_v8::Value", "use v8::Value")),
-    fn f(#[serde] v: serde_v8::Value) {}
-  );
 }
diff --git a/serde_v8/README.md b/serde_v8/README.md
index d36f693..a6fdc81 100644
--- a/serde_v8/README.md
+++ b/serde_v8/README.md
@@ -37,12 +37,6 @@ Instead of returning "nothing" via `Ok(json!({}))`, change your return type to
 rust's unit type `()` and returning `Ok(())`, `serde_v8` will efficiently encode
 that as a JS `null`.
 
-## Advanced features
-
-If you need to mix rust & v8 values in structs/tuples, you can use the special
-`serde_v8::Value` type, which will passthrough the original v8 value untouched
-when encoding/decoding.
-
 ## TODO
 
 - [ ] Experiment with KeyCache to optimize struct keys
diff --git a/serde_v8/lib.rs b/serde_v8/lib.rs
index c34eb0b..ea2b715 100644
--- a/serde_v8/lib.rs
+++ b/serde_v8/lib.rs
@@ -24,7 +24,5 @@ pub use magic::u16string::U16String;
 pub use magic::v8slice::V8Slice;
 pub use magic::v8slice::V8Sliceable;
 pub use magic::ExternalPointer;
-pub use magic::Global;
-pub use magic::Value;
 pub use ser::to_v8;
 pub use ser::Serializer;
diff --git a/serde_v8/magic/global.rs b/serde_v8/magic/global.rs
deleted file mode 100644
index d1b81c3..0000000
--- a/serde_v8/magic/global.rs
+++ /dev/null
@@ -1,41 +0,0 @@
-// Copyright 2018-2024 the Deno authors. All rights reserved. MIT license.
-
-use crate::magic::transl8::impl_magic;
-use crate::magic::transl8::FromV8;
-use crate::magic::transl8::ToV8;
-
-pub struct Global {
-  pub v8_value: v8::Global<v8::Value>,
-}
-impl_magic!(Global);
-
-impl From<v8::Global<v8::Value>> for Global {
-  fn from(v8_value: v8::Global<v8::Value>) -> Self {
-    Self { v8_value }
-  }
-}
-
-impl From<Global> for v8::Global<v8::Value> {
-  fn from(v: Global) -> Self {
-    v.v8_value
-  }
-}
-
-impl ToV8 for Global {
-  fn to_v8<'a>(
-    &mut self,
-    scope: &mut v8::HandleScope<'a>,
-  ) -> Result<v8::Local<'a, v8::Value>, crate::Error> {
-    Ok(v8::Local::new(scope, &self.v8_value))
-  }
-}
-
-impl FromV8 for Global {
-  fn from_v8(
-    scope: &mut v8::HandleScope,
-    value: v8::Local<v8::Value>,
-  ) -> Result<Self, crate::Error> {
-    let global = v8::Global::new(scope, value);
-    Ok(global.into())
-  }
-}
diff --git a/serde_v8/magic/mod.rs b/serde_v8/magic/mod.rs
index 6b9ef08..94f1553 100644
--- a/serde_v8/magic/mod.rs
+++ b/serde_v8/magic/mod.rs
@@ -5,7 +5,6 @@ pub mod buffer;
 pub mod bytestring;
 pub mod detached_buffer;
 mod external_pointer;
-mod global;
 pub(super) mod rawbytes;
 pub mod string_or_buffer;
 pub mod transl8;
@@ -13,5 +12,4 @@ pub mod u16string;
 pub mod v8slice;
 mod value;
 pub use external_pointer::ExternalPointer;
-pub use global::Global;
 pub use value::Value;
diff --git a/serde_v8/magic/value.rs b/serde_v8/magic/value.rs
index 21df094..7627f1b 100644
--- a/serde_v8/magic/value.rs
+++ b/serde_v8/magic/value.rs
@@ -4,43 +4,19 @@ use crate::magic::transl8::FromV8;
 use crate::magic::transl8::ToV8;
 use std::mem::transmute;
 
-/// serde_v8::Value allows passing through `v8::Value`s untouched
-/// when de/serializing & allows mixing rust & v8 values in structs, tuples...
+/// serde_v8::Value is used internally to serialize/deserialize values in
+/// objects and arrays. This struct was exposed to user code in the past, but
+/// we don't want to do that anymore as it leads to inefficient usages - eg. wrapping
+/// a V8 object in `serde_v8::Value` and then immediately unwrapping it.
 //
 // SAFETY: caveat emptor, the rust-compiler can no longer link lifetimes to their
-// original scope, you must take special care in ensuring your handles don't outlive their scope
+// original scope, you must take special care in ensuring your handles don't
+// outlive their scope.
 pub struct Value<'s> {
   pub v8_value: v8::Local<'s, v8::Value>,
 }
 impl_magic!(Value<'_>);
 
-impl<'s> Value<'s> {
-  /// Converts this to a [`v8::Global`] infallibly, if this `v8` type can be converted infallibly
-  /// to [`v8::Global<T>`] (eg: [`v8::Data`] or [`v8::Value`]).
-  pub fn as_global<T>(&self, scope: &mut v8::HandleScope<'s>) -> v8::Global<T>
-  where
-    v8::Local<'s, T>: From<v8::Local<'s, v8::Value>>,
-  {
-    let local = v8::Local::<T>::from(self.v8_value);
-    v8::Global::new(scope, local)
-  }
-
-  /// Converts this to a [`v8::Global`] fallibly.
-  pub fn try_as_global<T>(
-    &self,
-    scope: &mut v8::HandleScope<'s>,
-  ) -> Result<
-    v8::Global<T>,
-    <v8::Local<'s, T> as TryFrom<v8::Local<'s, v8::Value>>>::Error,
-  >
-  where
-    v8::Local<'s, T>: TryFrom<v8::Local<'s, v8::Value>>,
-  {
-    let local = v8::Local::<T>::try_from(self.v8_value)?;
-    Ok(v8::Global::new(scope, local))
-  }
-}
-
 impl<'s, T> From<v8::Local<'s, T>> for Value<'s>
 where
   v8::Local<'s, T>: Into<v8::Local<'s, v8::Value>>,
@@ -75,92 +51,3 @@ impl FromV8 for Value<'_> {
     Ok(unsafe { transmute::<Value, Value>(value.into()) })
   }
 }
-
-macro_rules! impl_try_from {
-  ($($type:ident),+) => {$(
-    /// Implements `TryFrom` for the given `v8` type. If the type is not compatible,
-    /// returns `Err(v8::DataError)`.
-    impl<'s> TryFrom<Value<'s>> for v8::Local<'s, v8::$type> {
-      type Error = v8::DataError;
-      fn try_from(value: Value<'s>) -> Result<Self, Self::Error> {
-        value.try_into()
-      }
-    }
-  )+};
-}
-
-impl_try_from!(
-  External,
-  Object,
-  Array,
-  ArrayBuffer,
-  ArrayBufferView,
-  DataView,
-  TypedArray,
-  BigInt64Array,
-  BigUint64Array,
-  Float32Array,
-  Float64Array,
-  Int16Array,
-  Int32Array,
-  Int8Array,
-  Uint16Array,
-  Uint32Array,
-  Uint8Array,
-  Uint8ClampedArray,
-  BigIntObject,
-  BooleanObject,
-  Date,
-  Function,
-  Map,
-  NumberObject,
-  Promise,
-  PromiseResolver,
-  Proxy,
-  RegExp,
-  Set,
-  SharedArrayBuffer,
-  StringObject,
-  SymbolObject,
-  WasmMemoryObject,
-  WasmModuleObject,
-  Primitive,
-  BigInt,
-  Boolean,
-  Name,
-  String,
-  Symbol,
-  Number,
-  Integer,
-  Int32,
-  Uint32
-);
-
-#[cfg(test)]
-mod tests {
-  use std::convert::Infallible;
-
-  #[test]
-  #[ignore]
-  fn test_conversions_compile() {
-    // SAFETY: We don't run this test -- it's just to make sure this code compiles
-    #[allow(invalid_value)]
-    #[allow(clippy::unnecessary_fallible_conversions)]
-    unsafe {
-      let value: v8::Local<v8::Function> = std::mem::zeroed();
-
-      // TryInto compiles for non-value
-      let serde_value: super::Value = value.into();
-      let _: Result<v8::Local<v8::Function>, v8::DataError> =
-        serde_value.try_into();
-
-      // TryInto compiles for value
-      let serde_value: super::Value = value.into();
-      let _: Result<v8::Local<v8::Value>, Infallible> = serde_value.try_into();
-
-      // Into compiles for value
-      let serde_value: super::Value = value.into();
-      let _: v8::Local<v8::Value> = serde_value.into();
-    }
-  }
-}


Test patch:
diff --git a/serde_v8/tests/magic.rs b/serde_v8/tests/magic.rs
index e2d647e..5e27d92 100644
--- a/serde_v8/tests/magic.rs
+++ b/serde_v8/tests/magic.rs
@@ -6,23 +6,17 @@ use serde_v8::Result;
 use serde_v8_utilities::js_exec;
 use serde_v8_utilities::v8_do;
 
-#[derive(Deserialize)]
-struct MagicOp<'s> {
+#[derive(Serialize, Deserialize)]
+struct MagicOp {
   #[allow(unused)]
   pub a: u64,
   #[allow(unused)]
   pub b: u64,
-  pub c: serde_v8::Value<'s>,
+  pub c: String,
   #[allow(unused)]
   pub operator: Option<String>,
 }
 
-#[derive(Serialize)]
-struct MagicContainer<'s> {
-  pub magic: bool,
-  pub contains: serde_v8::Value<'s>,
-}
-
 #[test]
 fn magic_basic() {
   v8_do(|| {
@@ -34,25 +28,17 @@ fn magic_basic() {
     // Decode
     let v = js_exec(scope, "({a: 1, b: 3, c: 'abracadabra'})");
     let mop: MagicOp = serde_v8::from_v8(scope, v).unwrap();
-    // Check string
-    let v8_value: v8::Local<v8::Value> = mop.c.into();
-    let vs = v8::Local::<v8::String>::try_from(v8_value).unwrap();
-    let s = vs.to_rust_string_lossy(scope);
-    assert_eq!(s, "abracadabra");
+    assert_eq!(mop.a, 1);
+    assert_eq!(mop.b, 3);
+    assert_eq!(mop.c, "abracadabra");
+    assert!(mop.operator.is_none());
 
     // Encode
-    let container = MagicContainer {
-      magic: true,
-      contains: v.into(),
-    };
-    let vc = serde_v8::to_v8(scope, container).unwrap();
+    let vc = serde_v8::to_v8(scope, mop).unwrap();
     // JSON stringify & check
     let json = v8::json::stringify(scope, vc).unwrap();
     let s2 = json.to_rust_string_lossy(scope);
-    assert_eq!(
-      s2,
-      r#"{"magic":true,"contains":{"a":1,"b":3,"c":"abracadabra"}}"#
-    );
+    assert_eq!(s2, r#"{"a":1,"b":3,"c":"abracadabra","operator":null}"#);
   })
 }