Support variadic javascript function parameters #726
Conversation
richard-uk1
changed the title
|
This is ready for review. I think it needs more tests, but I'm not sure what type. Please advise :) |
|
I also need to implement this the other way so that javascript can call a variadic rust function, and the variadic part of the arguments gets turned into a |
|
I think the ci error is spurious. |
crates/cli-support/src/js/rust2js.rs
Outdated
| // contents to strings (javascript is very strange). | ||
| write!(ret, "\ | ||
| let args = [{}];\n\ | ||
| {}.forEach(extra_arg => args.push(extra_arg));\n\ |
There was a problem hiding this comment.
We're already using pretty modern syntax elsewhere, so instead of this could it instead generate foo(a, b, ...c) perhaps?
There was a problem hiding this comment.
This seems to work - I was expecting the problem mentioned in the comment.
I'll add plenty of tests to try with lots of different types.
crates/macro-support/src/parser.rs
Outdated
| /// This method always fails if the BindgenAttrs contain variadic | ||
| fn assert_not_variadic(attrs: &BindgenAttrs) -> Result<(), Diagnostic> { | ||
| match attrs.variadic() { | ||
| true => Err(Diagnostic::error("the `variadic` attribute can only be applied to imported \ |
There was a problem hiding this comment.
Could bail_span! be used to attach this error to a particular span as well?
crates/macro-support/src/parser.rs
Outdated
| syn::Type::Slice(_) => Ok(()), | ||
| _ => Err(not_slice_error(ty)) | ||
| }, | ||
| _ => Err(not_slice_error(ty)) |
There was a problem hiding this comment.
This might be a bit easier to define if the match here is lifted to the Captured clause above (and probably using if let in a few places)
Then when the end of the function is reached (by falling through) it'd end in bail_span!
There was a problem hiding this comment.
done.
I've refactored this, but will have to change it again if I support Vecs as well.
EDIT I've added support for ::std::vec::Vec, which has made the checking code a bit more complicated.
tests/wasm/variadic.js
Outdated
| const wasm = require('wasm-bindgen-test.js'); | ||
| const assert = require('assert'); | ||
|
|
||
| function variadic_sum() { |
There was a problem hiding this comment.
Could this use function variadic_sum(args...) syntax?
|
Looking good! Could some more test for some more types be added too? I think we'll eventually want to support things like |
|
I'm adding loads of tests for all the types I can think of. I'm planning to comment out the ones we decide not to support for now. Because the last slice needs to go through the language boundary before being expanded, all the slice types need to implement I also should support Question: should I just remove the tests for <i/u>size, I'm guessing they aren't supported because of the issues I saw with <i/u>64? |
|
Stretch goal: the last parameter can be anything that implements It would have to actually be |
I think we can't support this yet since IntoWasmAbi isn't implemented for &[JsValue]. Maybe best to do in a separate PR. I can add the test for it and comment it out for now. |
|
Ready for review again. |
|
gentle ping :) |
|
Ah oops, sorry for the delay! After reading this again, what do you think of basically entirely removing the slice validation of the last argument? That way the attribute would be sort of "duck typed" but as long as the last item was iterable it should be able to be passed to the splat we then pass to JS, right? That way we can naturally pick up support for more list-like types as they're implemented. Additionally could you modify the |
|
I've implemented the duck-typing suggestion, and it works fine. The issue is an incorrect usage of the To be honest, I don't think we can check this at compiletime for user-defined types (or aliased standard types), since we operate before path resolution. Support would somehow have to be built into the compiler. |
|
Ready for review again. In particular are you happy with the documentation? |
| ); | ||
| let mut invoc = if self.variadic { | ||
| if self.js_arguments.is_empty() { | ||
| return Err(failure::err_msg("a function with no arguments cannot be variadic")); |
There was a problem hiding this comment.
Stylistically I think this could just be bail!("...")
|
Looks great to me, thanks! |
The main bit to do is the actual logic to convert the slice into a javascript array, and then do
fn.apply(something, our_slice).I've put some diagnostic logic in to stop incorrect usage, I think there are more opportunities for improvement here.
A question: I want to test failures when the attribute is in the wrong place. Is there a way to do this?
Relevant issue: #503.