-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Function Attributes #4394
base: main
Are you sure you want to change the base?
Function Attributes #4394
Conversation
Please refrain from pinging maintainers like this unless there is a more specific reason then just reviewing a PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe we should not add support for optional
without a good use-case. While JsValue
is a good use case, we should instead discuss support for Option<JsValue>
in a separate issue.
WDYT of wrapping param_type
and return_type
with unsafe(...)
, to make sure users are aware that we can't check the type here?
Thank you for doing this, great work!
crates/cli-support/src/decode.rs
Outdated
impl Debug for FunctionAttributes { | ||
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { | ||
f.write_fmt(format_args!( | ||
"FunctionAttributes {{ ret: {:?}, args: {:?} }}", | ||
self.ret, self.args | ||
)) | ||
} | ||
} | ||
|
||
impl Debug for FunctionComponentAttributes { | ||
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { | ||
f.write_fmt(format_args!( | ||
"FunctionComponentAttributes {{ ty: {:?}, desc: {:?}, optional: {} }}", | ||
self.ty, self.desc, self.optional | ||
)) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whats going on here? Isn't this already covered by derive(Debug)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these 2 structs have the same name as the 2 structs in backedn::ast
, but they are not the same, these 2 are declared in shared_api!
just like other structs that have kinda a dup in shared_api!
and backend::ast
and in shared_api!
I couldnt really add #[derive(Debug)] to shared_api!
macro body which is invoked in nested form, unless I modified or added new arms to the macro to handle the derive
, so I ended up impl Debug manually for them which for me sounded the easier approach between the two.
I understand if we dont wanna declare these 2 structs in shared_api!
, but to achieve that we need to add a dependency of backend
crate to shared
crate so we can use them there, which I wasn't sure if it is desired or not, so I tried to just follow and replicate whatever convention and structure that was already in place.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right. I see we current just do a lot of replication with all of these Aux
types ... that's horrible. Lets see how it goes after we resolve #4394 (comment).
guide/src/reference/attributes/on-rust-exports/function-attributes.md
Outdated
Show resolved
Hide resolved
guide/src/reference/attributes/on-rust-exports/function-attributes.md
Outdated
Show resolved
Hide resolved
crates/macro-support/src/parser.rs
Outdated
if is_js_keyword(&value) { | ||
return Err(meta.error("collides with js/ts keywords")); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should also check that this is a valid JS ident, which I believe we have a function lying around of somewhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Roger that, I'll fix it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
swicthed to useing BindgenAttrs::find
, so it should already handle that, as it is using the attrgen
js_name
logic
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets add a UI test for that as well while we are at it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wasnt able to find any function that validates a js identifier besides is_js_keyword()
, can you point that for me if it exists? if not, I can make some fn for this, probably using regex
Makes sense, I'll remove that.
Yea I understand the concerns here, I personally am in the camp of the responsibility of using these attrs are upon the user/dev, as these are not something everyone needs everytime, but are something you might need at times, so using them comes with the responsibility of making sure you know what you are doing, that said, I totally understand the concerns and am totally ok to impl this or any other syntax/format you see fit for this. I can think of this format as well: @daxpedda Lastly, thanks for identifying and correcting the typos and mistakes I had in documentation, and also I replied to other review comments/suggestion as well, some of which need your final call so I can go ahead to impl, so please let me know how should we go about them. |
This might be more appropriate actually. Let go with that, we can do more bikeshedding after we are done. |
guide/src/reference/attributes/on-rust-exports/function-attributes.md
Outdated
Show resolved
Hide resolved
guide/src/reference/attributes/on-rust-exports/function-attributes.md
Outdated
Show resolved
Hide resolved
For example a rust function can return `JsValue` by serializing a rust type using serde, yet on generated ts bindings instead of `any` as the return type, it can be overriden to the ts interface of the serialized rust type equivalent defined using `typescript-custom-section` (or using [Tsify Crate](https://crates.io/crates/tsify)): | ||
```rust | ||
// we wont use "#[wasm_bindgen]" for this struct | ||
#[derive(serde::Serialize, serde::Deserialize)] | ||
pub struct Person { | ||
pub name: String; | ||
} | ||
|
||
// define a ts interface equivalent of Person struct | ||
// alternatively Tsify crate can be used to generate ts interface from rust types | ||
#[wasm_bindgen(typescript_custom_section)] | ||
const TS_INTERFACE_EXPORT: &'static str = r" | ||
export interface Person { name: string; } | ||
"; | ||
|
||
#[wasm_bindgen(return_type = "Person", return_description = "a Person object")] | ||
pub async fn build_person( | ||
#[wasm_bindgen(js_name = "personName", param_description = "Specifies the person's name")] | ||
person_name: String, | ||
) -> Result<JsValue, JsValue> { | ||
// | ||
// some async operations | ||
// | ||
Ok(serde_wasm_bindgen::to_value(&Person { name: person_name })?) | ||
} | ||
``` | ||
this will generate the following ts bindings: | ||
```ts | ||
export interface Person { name: string; } | ||
|
||
/** | ||
* @param personName - Specifies the person's name | ||
* @returns a Person object | ||
*/ | ||
export function build_person(personName: string): Promise<Person>; | ||
``` | ||
As you can see, using function attributes, we can now return a js/ts object without ever using `wasm_bindgen` macro on `Person` struct that would have generated a js/ts class object in the bindings which might not be the desired outcome for every case, you can see as well that instead of handcoding the full documentation for `build_person()` with all js_doc/ts_doc tags and syntax, we can just write specific docs for each individual component of the function and as a result end up with a fully typed and documented function in the bindings. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant the example and not just the reference to tsify
. serde_wasm_bindgen
is still an external crate.
Motivation
resolves #4223
resolves #1847
resolves #4385
resolves #4377
At the moment, by default, exported Rust functions/methods generate function signature from equivalent rust types identifiers and without any documentations, unless completely handcoded using
skip-jsdoc
and/ortypescript-custom-section
, but this has some limitations as disscussed in details in issues mentioned above which have been requested to be addressed for long time, some examples of which are:skip-jsdoc
and/ortypescript-custom-section
.Vec<(Foo, Bar)>
) as return type of async functions (and in some cases non async functions) without wrapping them in new struct and actually implementingwasm_bindgen
for it.any
in typescript) where arguments and return types are only typescript interfaces and not a js class, asJsValue
in rust side where they are just serialized/deserialized using serde.snake_case
, but js/ts naming convention iscamelCase
, atm there is no way to convert function argument names tocamelCase
or optionally rename them.Proposed Solution
By introcusing function attributes these limitations and requested features can be addressed, these attributes will make it possible to override function arguments and return types and to write specific documentation for each of them individually as desired with relatively very minimal changes:
#[wasm_bindgen(unchecked_return_type)]
and#[wasm_bindgen(return_description)]
used to override function's return type and to specify description for generated js/ts bindings.#[wasm_bindgen(js_name)]
,#[wasm_bindgen(unchecked_param_type)]
and#[wasm_bindgen(param_description)]
applied to a rust function argument to override that argument's name and type and to specify description for generated js/ts bindings.These attributes will provide a great level of control and customization over functions/methods generated bindings that are essential for creating well defined and structured bindings.
Let's look at some examples for more details:
This will generate the following js bindings:
And will generate the following ts bindings:
Same thing applies to rust struct's (and enums) impl methods and their equivalent js/ts class methods:
This will generate the following js bindings:
And will generate the following ts bindings:
High Level Implementation Details
These attributes can be implemented with very minimal changes, for function arguments attributes, we need to parse and extract them before passing it token stream, and for return type attributes they will be added to
attrgen!
macro incrates/macro-support/src/parser.rs
, this guarantees that if they were used elsewhere other than a function or a method, it'll produce error or unused code warnings.Once they are parsed and extracted, they can be stored and then will be passed around during encode/decode process of a
ast::Function
toshared_api!
generatedFunction
struct which now has a new property that holds those attrs with same format:These new structs can be easily extended when/if more features need to be supported.
Next it'll passed as
export_fn_attrs: Option<FunctionAttributes>
property ofAuxExport
converted fromdecode::Export
, which makes those attrs available for consumption atcrates/cli-support/js/bindings.rs
where the bindings get generated.