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

Function Attributes #4394

Open
wants to merge 31 commits into
base: main
Choose a base branch
from
Open

Function Attributes #4394

wants to merge 31 commits into from

Conversation

rouzwelt
Copy link

@rouzwelt rouzwelt commented Jan 6, 2025

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/or typescript-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:

  • Cannot specify documentation for each function argument or return individually unless handcoded the whole documentation block using skip-jsdoc and/or typescript-custom-section.
  • Cannot return wrapped types (such as Vec<(Foo, Bar)>) as return type of async functions (and in some cases non async functions) without wrapping them in new struct and actually implementing wasm_bindgen for it.
  • Cannot end up having a typed function signature in typescript (not having any in typescript) where arguments and return types are only typescript interfaces and not a js class, as JsValue in rust side where they are just serialized/deserialized using serde.
  • Generated function arguments' names follow rust convention snake_case, but js/ts naming convention is camelCase, atm there is no way to convert function argument names to camelCase 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:

/// Description for foo
#[wasm_bindgen(unchecked_return_type = "Foo", return_description = "some description for return type")]
pub async fn foo(
    #[wasm_bindgen(js_name = "firstArg", param_description = "some description for firstArg")]
    arg1: String,
    #[wasm_bindgen(js_name = "secondArg", unchecked_param_type = "Bar")]
    arg2: JsValue,
) -> Result<JsValue, JsValue> {
    // function body
}

This will generate the following js bindings:

/**
* Description for foo
* @param {string} firstArg - some description for firstArg
* @param {Bar} secondArg
* @returns {Promise<Foo>} some description for return type
*/
export function foo(firstArg, secondArg) {
    // generated body
};

And will generate the following ts bindings:

/**
* Description for foo
* @param firstArg - some description for firstArg
* @param secondArg
* @returns some description for return type
*/
export function foo(firstArg: string, secondArg: Bar): Promise<Foo>;

Same thing applies to rust struct's (and enums) impl methods and their equivalent js/ts class methods:

/// Description for Foo
#[wasm_bindgen]
pub struct Foo {
    Foo: String,
}

#[wasm_bindgen]
impl Foo {
    /// Description for foo
    #[wasm_bindgen(unchecked_return_type = "Baz", return_description = "some description for return type")]
    pub fn foo(
        &self, // cannot use function attributes on "self" argument
        #[wasm_bindgen(param_description = "some description for arg1")]
        arg1: String,
        #[wasm_bindgen(unchecked_param_type = "Bar")]
        arg2: JsValue,
    ) -> JsValue {
        // function body
    }
}

This will generate the following js bindings:

/**
* Description for Foo
*/
export class Foo {
    /**
    * Description for foo
    * @param {string} arg1 - some description for arg1
    * @param {Bar} arg2
    * @returns {Baz} some description for return type
    */
    foo(arg1, arg2) {
        // generated body
    };
}

And will generate the following ts bindings:

/**
* Description for Foo
*/
export class Foo {
    /**
    * Description for foo
    * @param arg1 - some description for arg1
    * @param arg2
    * @returns some description for return type
    */
    foo(arg1: string, arg2: Bar): Baz;
}

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 in crates/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 to shared_api! generated Function struct which now has a new property that holds those attrs with same format:

struct FunctionAttributes {
    ret: FunctionComponentAttributes,
    args: Vec<FunctionComponentAttributes>,
}
struct FunctionComponentAttributes {
    ty: Option<String>,
    desc: Option<String>,
}

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 of AuxExport converted from decode::Export, which makes those attrs available for consumption at crates/cli-support/js/bindings.rs where the bindings get generated.

@rouzwelt
Copy link
Author

rouzwelt commented Jan 6, 2025

@daxpedda

@rouzwelt rouzwelt marked this pull request as ready for review January 6, 2025 15:41
@daxpedda
Copy link
Collaborator

daxpedda commented Jan 7, 2025

Please refrain from pinging maintainers like this unless there is a more specific reason then just reviewing a PR.

Copy link
Collaborator

@daxpedda daxpedda left a 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/backend/src/ast.rs Outdated Show resolved Hide resolved
Comment on lines 97 to 113
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
))
}
}
Copy link
Collaborator

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)?

Copy link
Author

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.

Copy link
Collaborator

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).

crates/cli-support/src/js/binding.rs Outdated Show resolved Hide resolved
crates/cli-support/src/wit/nonstandard.rs Outdated Show resolved Hide resolved
crates/macro-support/src/parser.rs Outdated Show resolved Hide resolved
Comment on lines 1194 to 1196
if is_js_keyword(&value) {
return Err(meta.error("collides with js/ts keywords"));
}
Copy link
Collaborator

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.

Copy link
Author

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

Copy link
Author

@rouzwelt rouzwelt Jan 7, 2025

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

Copy link
Collaborator

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.

Copy link
Author

@rouzwelt rouzwelt Jan 8, 2025

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

crates/macro-support/src/parser.rs Outdated Show resolved Hide resolved
@rouzwelt
Copy link
Author

rouzwelt commented Jan 7, 2025

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.

Makes sense, I'll remove that.

WDYT of wrapping param_type and return_type with unsafe(...), to make sure users are aware that we can't check the type here?

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:
unchecked_param_type and unchecked_return_type


@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.

@daxpedda
Copy link
Collaborator

daxpedda commented Jan 7, 2025

I can think of this format as well:
unchecked_param_type and unchecked_return_type

This might be more appropriate actually. Let go with that, we can do more bikeshedding after we are done.

- rm "optional" attr
- separate js/ts doc by adding ts_doc_comment()
- fix docs
- append `unchecked` for type overrides attrs
- use attrgen for parsing function attrs
- update docs
@rouzwelt rouzwelt requested a review from daxpedda January 8, 2025 15:35
crates/backend/src/ast.rs Outdated Show resolved Hide resolved
crates/backend/src/ast.rs Outdated Show resolved Hide resolved
crates/shared/src/lib.rs Outdated Show resolved Hide resolved
crates/macro/ui-tests/unused-attributes.rs Outdated Show resolved Hide resolved
Comment on lines 11 to 47
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.
Copy link
Collaborator

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.

@daxpedda daxpedda added the waiting for author Waiting for author to respond label Jan 8, 2025
@rouzwelt rouzwelt requested a review from daxpedda January 9, 2025 00:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
waiting for author Waiting for author to respond
Projects
None yet
2 participants