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

Improved TS type generation from WASM #4229

Merged
merged 12 commits into from
Nov 14, 2024

Conversation

RunDevelopment
Copy link
Contributor

This PR improves the TS generation for the WASM file and the InitOutput interface.

Changes:

  • The type generation code is now shared between the 2 use cases with a single module_export_types function. module_export_types uses interior iteration to pass all name-type pairs to the respective function for formatting.
  • WASM value types are now properly translated to TS types instead of assuming number.
  • Multi-value returns are now typed with tuple types. E.g. (a: number) => [number, bigint, number]. This accurately represents the actual result of WASM functions.

Another mostly aesthetic change is that the generated .d.ts files for the .wasm file don't use export function name(arg: S): T anymore. They now uses export const name: (arg: S) => T. Typing-wise, these two styles are equivalent, but the arrow function style makes code generation easier, because it allows the code to be shared between the .d.ts and interface code.

Before:

export const memory: WebAssembly.Memory;
export function __wbg_problem_free(a: number, b: number): void;

After:

export const memory: WebAssembly.Memory;
export const __wbg_problem_free: (a: number, b: number) => void;

Just like #4210, this PR also fixes #4207. However, the goal of this PR was not to fix this issue. So please merge #4210 before this PR if possible.

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.

Could you add some integration tests for those changes?

@RunDevelopment
Copy link
Contributor Author

There currently isn't any infrastructure for testing this, so how would you like me to test this?

This might be a hammer and nail situation for me, since I've done a lot with references.rs lately, but how about we generalize it to allow references to specify command line args? This would also make it possible to snapshot test the code gen for specific --targets, which would be nice for #4065.

What do you think?

@daxpedda
Copy link
Collaborator

daxpedda commented Nov 9, 2024

This might be a hammer and nail situation for me, since I've done a lot with references.rs lately, but how about we generalize it to allow references to specify command line args? This would also make it possible to snapshot test the code gen for specific --targets, which would be nice for #4065.

What do you think?

Yes, that would be great!

@daxpedda daxpedda added the waiting for author Waiting for author to respond label Nov 9, 2024
@RunDevelopment
Copy link
Contributor Author

Alright, I added a test with --target=web, so we can see the improved type generation.

I initially thought about adding a feature for type-only tests, but then removed it again, because I wanted to see the .js and .wat for the test I added.

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.

  • Multi-value returns are now typed with tuple types. E.g. (a: number) => [number, bigint, number]. This accurately represents the actual result of WASM functions.

We are currently only testing [number, number] output. How about testing u128 and Option<u128>?


I should probably mention that, right now, this almost entirely affects code that we shouldn't generate in the first place. Just guessing the historical context here, but I assume this was actually implemented in the first place to generate a TS interface for functions not exported via wasm-bindgen, but via extern "C" fn() { ... }. Unfortunately this inadvertently also exported all the internal wasm-bindgen functions, which should never be used. Ideally, we identify all these functions and don't export them as part of InitOutput. But maybe I'm missing something and these exports have a use-case after all.

However, the general code improvements are obviously very appreciated!

@RunDevelopment
Copy link
Contributor Author

How about testing u128 and Option<u128>?

Done.

I should probably mention that, right now, this almost entirely affects code that we shouldn't generate in the first place. Just guessing the historical context here, but I assume this was actually implemented in the first place to generate a TS interface for functions not exported via wasm-bindgen, but via extern "C" fn() { ... }.

I wonder whether this is how people actually use it. Given that proposals like reference-types have made a lot the JS glue code just thin wrappers around the underlying wasm export, I suspect that there is a growing number of people that might be tempted to use the wasm module directly.

So if users really are not supposed to even touch the functions exported with #[wasm_bindgen], I would suggest adding a sort of name mangling step. Similar to import functions, we could add a small hash at the end of the name and/or prefix it with __wbg_internal_export or similar. Right now, these exports look like they are supported.

@daxpedda
Copy link
Collaborator

I should probably mention that, right now, this almost entirely affects code that we shouldn't generate in the first place. Just guessing the historical context here, but I assume this was actually implemented in the first place to generate a TS interface for functions not exported via wasm-bindgen, but via extern "C" fn() { ... }.

I wonder whether this is how people actually use it. Given that proposals like reference-types have made a lot the JS glue code just thin wrappers around the underlying wasm export, I suspect that there is a growing number of people that might be tempted to use the wasm module directly.

So if users really are not supposed to even touch the functions exported with #[wasm_bindgen], I would suggest adding a sort of name mangling step. Similar to import functions, we could add a small hash at the end of the name and/or prefix it with __wbg_internal_export or similar. Right now, these exports look like they are supported.

We should indeed, but we should definitely not expose them in the generated TS as well.

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.

Excellent!

@daxpedda daxpedda merged commit 50c7504 into rustwasm:main Nov 14, 2024
41 checks passed
@RunDevelopment RunDevelopment deleted the wasm-to-ts-impr branch November 14, 2024 14:43
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
Development

Successfully merging this pull request may close these issues.

Functions returning Result<T,E> result in incorrect typescript function output type Array
2 participants