fix: problems introduced with patch to Rust.

[EvanCarroll]

Resolves #26

Rust patch rust-lang/rust#149868 introduced some breaking changes by removing --allow-undefined from WASM linker defaults. The compiler ignores #[link(wasm_import_module)] on non-C ABI, and these were never real WASM imports. The fix was to use unsafe extern "C" instead of unsafe #declared_abi on WASM.

[WorldSEnder]

Are the any differences in allowed argument types for extern "C" and extern "Rust" functions? I suppose not, in which case the simple allow(improper_ctypes, improper_ctypes_definitions) should suffice and I would merge this asap. Thanks for the quick feedback and catching the breakage 👍

[WorldSEnder]

Never mind the clippy failure, will fix this after the merge before the patch release.

[WorldSEnder]

Found out that extern "C" can declare variadics as the last argument where other ABIs can't which might get surprisingly accepted on wasm targets but not others without a clean user error. One way to lint for it would be to additionally emit an unused, not linked dummy signature with the user declared ABI (or "Rust").

[EvanCarroll]

@WorldSEnder Clippy fixes in. I'm not sure if you want to me to do anything here or not, but just be explicit if so because this is probably a bit out of my area of expertise/pay grade.

As far as variadics, you say "but not others without a clean user error". I'm not sure I understand this concern enough to answer. The use case here is splitting something that's already wasm, into wasm + async loaded wasm. If wasm already accepts variadics then in what case would a non-wasm target be used here for this to be a concern?

Also isn't this patch sufficiently gated to conditionally compile on wasm to address this concern to whatever degree it exists #[cfg(target_family = "wasm")]? If that condition isn't met, the patch doesn't have any effect.

[WorldSEnder]

First of all, it's not something I would expect you to do. It's mostly notes for myself because I want to properly ensure there are no surprises that could be worked around down the road before I put out the patch release.

The concern would be that by rewriting the declared ABI we get mismatching compile errors on wasm/non-wasm targets. It's also above my expertise to know if there are any additional (or different) checks for a function declared as extern "C" vs one declared as extern "Rust". I'm trying to find out. I found variadics which are allowed with "C" but not "Rust". I am still trying to figure out how this behaves with different (impoper C) argument types. So far, seems fine and the compiler just paints over it with improper_ctypes.

[EvanCarroll]

cool. I'm down for helping I just don't know how useful I'll be. Here is another thing that may help,

Panic Behavior: By default, if a panic occurs in an extern "C" function, it may cause an abort or undefined behavior because C cannot handle Rust's unwinding. To allow unwinding across FFI, use extern "C-unwind".

I assume this brings extern "C" to extern "rust" parity? But still unsure if this is an issue, and I think it's a unstable feature, https://dev-doc.rust-lang.org/beta/unstable-book/language-features/c-unwind.html

[WorldSEnder]

Panics are even more hacky to get right, since they are also caught in a very hacky way in wasm-bindgen-test-runner. In particular, even catching them with catch_unwind will log them and lead to a test failure. Some code there claims that wasm is configured with panic=abort which I can't verify though.

Under these circumstances, I think the best way is to document this as a limitation and say that panics and unwinding across a split point are at the moment not possible (on wasm).

With regards to different signatures, since the handler gets declared with the same signature and without any changes to the the declared ABI, that also seems okay and any mismatch will be caught. It does lead to a bit noisier output in the error log on a mismatch, but that is okay with me for now.

[gbj]

Thanks to you both ❤️