Skip to content

Add exporting Rust functions as variadic JS functions #2954

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

Merged
merged 10 commits into from
Jun 22, 2022
Merged

Add exporting Rust functions as variadic JS functions #2954

merged 10 commits into from
Jun 22, 2022

Conversation

joaofreires
Copy link
Contributor

@joaofreires joaofreires commented Jun 19, 2022

This is an attempt to solve #694.

I kept the variadic bindgen attribute and assumed the last argument will be received as a JS Array object with the variadic values. Added a variadic flag inside the Functions structures, and changed the export signature of functions flagged as variadic, then only the JS signature changed.

I found a limitation in this implementation around the argument typing export. If we export a variadic function with a type different from JsValue, we can generate the TS exports with wrong types.

For example:

#[wasm_bindgen(variadic)]
pub fn example(arr: &Array) {}

Will generate:

export function example(...arr: Array<any>): void;

The workaround I found is create a new type and export the typescript_type (I tested extending the Array structure and it worked as "expected").

Suggestions are welcome.

TODO:

  • create more unit tests covering more variadic function use posibilities
  • add the variadic "dots" in the JSDoc generation code.

Copy link
Contributor

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

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

Seems reasonable to me! Could a typescript test be added to ensure that it shows up in the *.d.ts file correctly too?

// return Err(Diagnostic::span_error(*span, msg));
// }
// Ok(())
// }
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this can be removed now?

Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

We shouldn't have dead code in there without a reason

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, removed!

@@ -411,7 +411,7 @@ impl<'a> ConvertToAst<BindgenAttrs> for &'a mut syn::ItemStruct {
};

let attrs = BindgenAttrs::find(&mut field.attrs)?;
assert_not_variadic(&attrs)?;
// assert_not_variadic(&attrs)?;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why have dead code in there? Can you please add a comment explaining why this is commented out?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed, no longer needed.

// return Err(Diagnostic::span_error(*span, msg));
// }
// Ok(())
// }
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

We shouldn't have dead code in there without a reason

@joaofreires
Copy link
Contributor Author

joaofreires commented Jun 21, 2022

I created tests for simple variadic functions inside crates/typescript-tests/src/simple_fn.[rs | ts]. I'm not sure if we have more places with TS tests.
I also made a small update to the docs, but I'm sure it can be improved.

Note: I needed to rebase this branch to pass on CI.

@alexcrichton alexcrichton merged commit df1b591 into rustwasm:main Jun 22, 2022
@alexcrichton
Copy link
Contributor

Thanks!

@ishitatsuyuki
Copy link
Contributor

I think this regressed some part of processing somehow. I still haven't reduced a reproduction from my private codebase, but meanwhile here's the panic stacktrace I get:

thread 'main' panicked at 'assertion failed: mid <= self.len()', crates/cli-support/src/decode.rs:43:27
stack backtrace:
   0: rust_begin_unwind
             at /rustc/28b891916d4c85cd10fb2e9cfa8bc836a2c459f3/library/std/src/panicking.rs:584:5
   1: core::panicking::panic_fmt
             at /rustc/28b891916d4c85cd10fb2e9cfa8bc836a2c459f3/library/core/src/panicking.rs:142:14
   2: core::panicking::panic
             at /rustc/28b891916d4c85cd10fb2e9cfa8bc836a2c459f3/library/core/src/panicking.rs:48:5
   3: <&str as wasm_bindgen_cli_support::decode::Decode>::decode
   4: <alloc::vec::Vec<T> as wasm_bindgen_cli_support::decode::Decode>::decode
   5: <wasm_bindgen_cli_support::decode::Program as wasm_bindgen_cli_support::decode::Decode>::decode
   6: wasm_bindgen_cli_support::wit::process
   7: wasm_bindgen_cli_support::Bindgen::generate_output
   8: wasm_bindgen_cli_support::Bindgen::generate
   9: wasm_bindgen::main
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.

Reverting the merge commit makes things working again.

@ishitatsuyuki
Copy link
Contributor

Nevermind, looks like it's just a schema breaking change and I need to align my project's dependency to the git version too.

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.

4 participants