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

1 files changed, 74 insertions, 2 deletions

diff --git a/ext/ffi/dlfcn.rs b/ext/ffi/dlfcn.rs
index 570af09fc..c2b123246 100644
--- a/ext/ffi/dlfcn.rs
+++ b/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)]
-#[serde(untagged)]
+#[derive(Debug)]
 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"));
+  }
 }