fix(ext/ffi): improve error messages in FFI module (#17786)

Fixes denoland#16922.

The error messages in the `ffi` module are somewhat cryptic when passing
functions that have invalid `parameters` or `result` type strings. While
the generated serializer for the `ForeignFunction` struct correctly
outputs a correct and verbose message, the user sees a far less helpful
`data did not match any variant` message instead.

The underlying cause appears to be the fallback message in the
auto-derived deserializer for untagged enums [1] generated as a result
of `ForeignSymbol` being marked as `#[serde(untagged)]` [2]. Passing an
unexpected value for `NativeType` causes it to error out while
attempting to deserialize both enum variants -- once because it's not a
match for the `ForeignStatic` variant, and once because the
`ForeignFunction` deserializer rejects the invalid type for the
parameters/return type. This is currently open as [serde
#773](https://github.com/serde-rs/serde/issues/773), and not a trivial
exercise to fix generically.

[1]
https://github.com/serde-rs/serde/blob/v0.9.7/serde_derive/src/de.rs#L730
[2] https://github.com/denoland/deno/blob/main/ext/ffi/dlfcn.rs#L102
[3] https://github.com/serde-rs/serde/issues/773

Note that the auto-generated deserializer for untagged enums uses a
private API to buffer deserializer content that we don't have access to.
Instead, we can make use of the `serde_value` crate to buffer the
values. This can likely be removed once the official buffering API lands
(see [4] and [5]). In addition, this crate pulls in `serde_json` as a
cheap way to test that the deserializer works properly.

[4] https://github.com/serde-rs/serde/issues/741
[5] https://github.com/serde-rs/serde/pull/2348

Cargo.lock

@@ -1065,6 +1065,8 @@ dependencies = [
  "dynasmrt",
  "libffi",
  "serde",
+ "serde-value",
+ "serde_json",
  "tokio",
  "winapi",
 ]
@@ -3121,6 +3123,15 @@ version = "0.1.5"
 source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "ff011a302c396a5197692431fc1948019154afc178baf7d8e37367442a4601cf"
 
+[[package]]
+name = "ordered-float"
+version = "2.10.0"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "7940cf2ca942593318d07fcf2596cdca60a85c9e7fab408a5e21a4f9dcd40d87"
+dependencies = [
+ "num-traits",
+]
+
 [[package]]
 name = "os_pipe"
 version = "1.0.1"
@@ -4012,6 +4023,16 @@ dependencies = [
  "serde_derive",
 ]
 
+[[package]]
+name = "serde-value"
+version = "0.7.0"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "f3a1a3341211875ef120e117ea7fd5228530ae7e7036a779fdc9117be6b3282c"
+dependencies = [
+ "ordered-float",
+ "serde",
+]
+
 [[package]]
 name = "serde_bytes"
 version = "0.11.9"

ext/ffi/Cargo.toml

@@ -19,6 +19,8 @@ dlopen.workspace = true
 dynasmrt = "1.2.3"
 libffi = "3.1.0"
 serde.workspace = true
+serde-value = "0.7"
+serde_json = "1.0"
 tokio.workspace = true
 
 [target.'cfg(windows)'.dependencies]

ext/ffi/dlfcn.rs

@@ -15,6 +15,7 @@ use deno_core::Resource;
 use deno_core::ResourceId;
 use dlopen::raw::Library;
 use serde::Deserialize;
+use serde_value::ValueDeserializer;
 use std::borrow::Cow;
 use std::collections::HashMap;
 use std::ffi::c_void;
@@ -97,13 +98,31 @@ struct ForeignStatic {
   _type: String,
 }
 
-#[derive(Deserialize, Debug)]
+#[derive(Debug)]
-#[serde(untagged)]
 enum ForeignSymbol {
   ForeignFunction(ForeignFunction),
   ForeignStatic(ForeignStatic),
 }
 
+impl<'de> Deserialize<'de> for ForeignSymbol {
+  fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
+  where
+    D: serde::Deserializer<'de>,
+  {
+    let value = serde_value::Value::deserialize(deserializer)?;
+
+    // Probe a ForeignStatic and if that doesn't match, assume ForeignFunction to improve error messages
+    if let Ok(res) = ForeignStatic::deserialize(
+      ValueDeserializer::<D::Error>::new(value.clone()),
+    ) {
+      Ok(ForeignSymbol::ForeignStatic(res))
+    } else {
+      ForeignFunction::deserialize(ValueDeserializer::<D::Error>::new(value))
+        .map(ForeignSymbol::ForeignFunction)
+    }
+  }
+}
+
 #[derive(Deserialize, Debug)]
 pub struct FfiLoadArgs {
   path: String,
@@ -395,6 +414,11 @@ pub(crate) fn format_error(e: dlopen::Error, path: String) -> String {
 
 #[cfg(test)]
 mod tests {
+  use super::ForeignFunction;
+  use super::ForeignSymbol;
+  use crate::symbol::NativeType;
+  use serde_json::json;
+
   #[cfg(target_os = "windows")]
   #[test]
   fn test_format_error() {
@@ -409,4 +433,52 @@ mod tests {
       "foo.dll is not a valid Win32 application.\r\n".to_string(),
     );
   }
+
+  /// Ensure that our custom serialize for ForeignSymbol is working using `serde_json`.
+  #[test]
+  fn test_serialize_foreign_symbol() {
+    let symbol: ForeignSymbol = serde_json::from_value(json! {{
+      "name": "test",
+      "type": "type is unused"
+    }})
+    .expect("Failed to parse");
+    assert!(matches!(symbol, ForeignSymbol::ForeignStatic(..)));
+
+    let symbol: ForeignSymbol = serde_json::from_value(json! {{
+      "name": "test",
+      "parameters": ["i64"],
+      "result": "bool"
+    }})
+    .expect("Failed to parse");
+    if let ForeignSymbol::ForeignFunction(ForeignFunction {
+      name: Some(expected_name),
+      parameters,
+      ..
+    }) = symbol
+    {
+      assert_eq!(expected_name, "test");
+      assert_eq!(parameters, vec![NativeType::I64]);
+    } else {
+      panic!("Failed to parse ForeignFunction as expected");
+    }
+  }
+
+  #[test]
+  fn test_serialize_foreign_symbol_failures() {
+    let error = serde_json::from_value::<ForeignSymbol>(json! {{
+      "name": "test",
+      "parameters": ["int"],
+      "result": "bool"
+    }})
+    .expect_err("Expected this to fail");
+    assert!(error.to_string().contains("expected one of"));
+
+    let error = serde_json::from_value::<ForeignSymbol>(json! {{
+      "name": "test",
+      "parameters": ["i64"],
+      "result": "int"
+    }})
+    .expect_err("Expected this to fail");
+    assert!(error.to_string().contains("expected one of"));
+  }
 }