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

DiplomatOption support for JS #637

Merged
merged 13 commits into from
Sep 2, 2024
Merged

Conversation

Manishearth
Copy link
Contributor

@Manishearth Manishearth commented Aug 16, 2024

Next steps for #246

Fixes #659

@Manishearth
Copy link
Contributor Author

I think it is ideal to fix #656 and #657 first.

@Manishearth
Copy link
Contributor Author

Note to self: unions are always compiled to [n x alignment]. ez.

@Manishearth
Copy link
Contributor Author

Not easy, actually. We don't currently codegen JS-to-Rust code for writing to a segment of memory formatted that way. ugh.

@Manishearth
Copy link
Contributor Author

Manishearth commented Aug 27, 2024

I could cheat by just disallowing DiplomatOption<Struct> in JS inputs for now. We don't need it in ICU4X

@Manishearth
Copy link
Contributor Author

Manishearth commented Aug 27, 2024

Playing with endianness:

#[repr(C)]
#[derive(Debug)]
pub struct MyStruct {
    x: u8,
    y: u16,
    z: u32,
}
#[no_mangle]
pub extern "C" fn opt(x: DiplomatOption<MyStruct>) {
    let val = unsafe { x.value.ok };
    log(&format!("{val:#x?}"));
}
wasm.opt(0x12345678, 0x9ABCDEF0);

Produces

MyStruct {
        x: 0x78,
        y: 0x1234,
        z: 0x9abcdef0,
    }

The toFFI for MyStruct would produce [0x78, padding, 0x1234, 0x9ABCDEF0]. To produce the right argument list we need to do [toFFI[0] + toFFI[1] << 8 + toFFI[2] << 16, toFFI[3] ].

@Manishearth
Copy link
Contributor Author

The union ABI stuff changes how this needs to be done, stashing old code at the old-js-option branch, and will start this branch anew.

@Manishearth Manishearth force-pushed the js-option branch 7 times, most recently from 88b0c62 to 3ab2f4c Compare September 2, 2024 20:37
@Manishearth Manishearth marked this pull request as ready for review September 2, 2024 20:37
@Manishearth
Copy link
Contributor Author

And we're actually ready! Would love a review from @ambiguousname if you can get to it.

Copy link
Member

@ambiguousname ambiguousname left a comment

Choose a reason for hiding this comment

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

This seems to do the job just fine, and this looks good to me. The context handling in rust is very clever, and I'm glad things are working there without too much hassle.

There is some unavoidable mess though. My biggest fears are with the really long helper functions (like writeOptionToArrayBuffer and optionToArgsForCalling). I'm glad they're documented, and most of the mess seems to stem from the current WASM ABI. So once we update to the new ABI, hopefully we can untangle some of that. My dream would be a more readable JS backend, which I believe means redesigning functions like _intoFFI or _writeToArrayBuffer (per my comments on #668).

As a short term fix, maybe we could group optionToArgsForCalling, readOption, and writeOptionToArrayBuffer into one helper class? But that's a minor nitpick.

So, to summarize: this looks good, but I believe the JS backend feels so convoluted from handling edge cases (amplified with the current ABI and the new type). Tests will improve our ability to detect these, and hopefully changing to the new ABI in the future will help.

@Manishearth
Copy link
Contributor Author

Note that the new ABI basically would be entirely dependent on _writeToArrayBuffer() as opposed to _toFFI(). Which is slightly better, we remove the duplication.

@Manishearth
Copy link
Contributor Author

As a short term fix, maybe we could group optionToArgsForCalling, readOption, and writeOptionToArrayBuffer into one helper class?

Ultimately I think the class wouldn't actually wrap anything, I'm overall fine with having a bunch of free functions floating around. Classes help when they have state, or a coherent lifecycle, but these things all get called at different times.

@Manishearth Manishearth merged commit 242c7d0 into rust-diplomat:main Sep 2, 2024
16 checks passed
@Manishearth Manishearth deleted the js-option branch September 2, 2024 22:04
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.

Support Option<T> in JS
2 participants