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

embind complains about missing std::optional fields. #23587

Open
rubenlg opened this issue Feb 4, 2025 · 2 comments
Open

embind complains about missing std::optional fields. #23587

rubenlg opened this issue Feb 4, 2025 · 2 comments
Assignees

Comments

@rubenlg
Copy link

rubenlg commented Feb 4, 2025

Version of emscripten/emsdk:
emcc (Emscripten gcc/clang-like replacement + linker emulating GNU ld) 4.0.2 (7591f1c)
clang version 21.0.0git (https:/github.com/llvm/llvm-project 9534d27e3321a3b9e6e79fe6328445575bf26b7b)
Target: wasm32-unknown-emscripten
Thread model: posix

Here is a simple repro case:

#include <emscripten/bind.h>

struct Test {
  float x;
  std::optional<float> y;
};
void consumeTest(const Test &test) {}

EMSCRIPTEN_BINDINGS(test) {

emscripten::value_object<Test>("Test")
  .field("x", &Test::x)
  .field("y", &Test::y)
;
emscripten::register_optional<float>();
emscripten::function("consumeTest", &consumeTest);

} // EMSCRIPTEN_BINDINGS

Now from Javascript if I call Module.consumeTest({x: 2}) it fails with the error TypeError: Missing field: "y".
I would need to call Module.consumeTest({x: 2, y: undefined}) for this to work.

The whole point of making y optional is being able to omit it. Also, having to pass undefined is very brittle, because JSON serialization/deserialization drops undefined fields.

My proposal is to make .field aware of std::optional and don't complain when an optional field is missing. Does this make sense?

Aside: It would also be nice for the .d.ts typescript definitions to use optional fields as well (i.e. y?: number).

@rubenlg
Copy link
Author

rubenlg commented Feb 5, 2025

I took a look at libembind.js, and the fix doesn't require complex wiring. _embind_finalize_value_object already knows which fields are optional - it's an explicit property in each field inside fieldTypes. Passing the optional property down to the fields map that's sent to toWireType is enough for it to be able to ignore missing optional fields.

Since I'm not familiar with the code of libembind.js, I don't know if this could have other unintended consequences, but at first glance it seems to be a low-risk change.

@brendandahl
Copy link
Collaborator

That sounds reasonable. I'd be happy to review a PR. Please also add a test in embind_test.cpp and the corresponding embind.test.js file.

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

No branches or pull requests

2 participants