Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Feb 15, 2023

Conversation

mmastrac
Copy link
Contributor

@mmastrac mmastrac commented Feb 14, 2023

Fixes #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, 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] serde-rs/serde#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] serde-rs/serde#741
[5] serde-rs/serde#2348

@CLAassistant
Copy link

CLAassistant commented Feb 14, 2023

CLA assistant check
All committers have signed the CLA.

Copy link
Member

@littledivy littledivy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you!

@littledivy littledivy merged commit 4104a67 into denoland:main Feb 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

FFi error messages should be more detailed
3 participants