-
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
fix(typescript) Getters correctly define the inner results. #2909
Conversation
This patch fixes rustwasm#2721. Let's consider the following example: ```rust pub struct Foo { /// Hello `first`. pub first: u32, /// Hello `second`. #[wasm_bindgen(readonly)] pub second: u32, } ``` It outputs the following `.d.ts` file: ```typescript export class Foo { free(): void; /** * Hello `first`. */ first: number; /** * Hello `second`. */ readonly second: void; } ``` What's wrong here is that `second` has the type `void`. It should be `number`. What's happening? For `Foo.first`, a getter and a setter are generated. The getter never has a return type (spoiler: that's the bug), but the setter has a correct return type somehow (it's infererd from its arguments, combined with a unit descriptor, see `wasm_bindgen_cli_support::wit::Context::struct_`). When the getter and the setter are processed, they both end up calling `wasm_bindgen_cli_support::js::ExportedClass::push_accessor_ts`. This function overwrites the return type for the same field everytime it is called. So when called for the getter, the return type is missing, and when called for the setter with a type, the bug is fixed. For `Foo.second`, it's different. There is no setter generated because the field is marked as `readonly`. So the bug appears. To fix that, I've updated the getter `Function` descriptor, so that it has an `inner_ret` value. This is passed to an `Adapter.inner_results` at some point in the code, which is finally passed by `JsFunction::process` to `JsFunction::typescript_signature` to generate the typescript signature. And suddently, it's fixed! However, we got: ```typescript export class Foo { free(): void; /** * Hello `first`. * @returns {number} */ first: number; /** * Hello `second`. * @returns {number} */ readonly second: number; } ``` We're making progress! Nonetheless, the documentation is wrong now: The fields don't return anything. This problem comes from the way `js_docs` is computed globally for all export kinds in `wasm_bindgen_cli_support::js::Context::generate_adapter`. This patch also updates that to compute `js_docs` lately, for each branch. It is specialized in the `AuxExportKind::Getter` and `AuxExportKind::Setter` branches. I hope this is the correct approach. I'm not sure about the difference between `Adapter.results` and `Adapter.inner_results` as there is no documentation unfortunately (kudos for every other super-well documented place though).
Thanks! Could a test be added for this as well? |
I guess it's a good idea ;-). Sorry for forgetting! |
I've added a test in |
Alas it appears I forgot to migrate that when switching to github actions, would you be up for updating those tests to run here? |
I will as soon as I find free time! |
…#2909) * fix(typescript) Getters correctly define the inner results. This patch fixes rustwasm#2721. Let's consider the following example: ```rust pub struct Foo { /// Hello `first`. pub first: u32, /// Hello `second`. #[wasm_bindgen(readonly)] pub second: u32, } ``` It outputs the following `.d.ts` file: ```typescript export class Foo { free(): void; /** * Hello `first`. */ first: number; /** * Hello `second`. */ readonly second: void; } ``` What's wrong here is that `second` has the type `void`. It should be `number`. What's happening? For `Foo.first`, a getter and a setter are generated. The getter never has a return type (spoiler: that's the bug), but the setter has a correct return type somehow (it's infererd from its arguments, combined with a unit descriptor, see `wasm_bindgen_cli_support::wit::Context::struct_`). When the getter and the setter are processed, they both end up calling `wasm_bindgen_cli_support::js::ExportedClass::push_accessor_ts`. This function overwrites the return type for the same field everytime it is called. So when called for the getter, the return type is missing, and when called for the setter with a type, the bug is fixed. For `Foo.second`, it's different. There is no setter generated because the field is marked as `readonly`. So the bug appears. To fix that, I've updated the getter `Function` descriptor, so that it has an `inner_ret` value. This is passed to an `Adapter.inner_results` at some point in the code, which is finally passed by `JsFunction::process` to `JsFunction::typescript_signature` to generate the typescript signature. And suddently, it's fixed! However, we got: ```typescript export class Foo { free(): void; /** * Hello `first`. * @returns {number} */ first: number; /** * Hello `second`. * @returns {number} */ readonly second: number; } ``` We're making progress! Nonetheless, the documentation is wrong now: The fields don't return anything. This problem comes from the way `js_docs` is computed globally for all export kinds in `wasm_bindgen_cli_support::js::Context::generate_adapter`. This patch also updates that to compute `js_docs` lately, for each branch. It is specialized in the `AuxExportKind::Getter` and `AuxExportKind::Setter` branches. I hope this is the correct approach. I'm not sure about the difference between `Adapter.results` and `Adapter.inner_results` as there is no documentation unfortunately (kudos for every other super-well documented place though). * test(typescript) Test readonly struct fields have correct types.
…apters. A bug has been raised in rustwasm#2947, which is the consequence of a bug introduced in rustwasm#2909. This patch fixes it by differentiating JS docs and TS docs clearly. The `Context::push_setter` and `Context::push_getter` methods now receive a `js_docs` and a `ts_docs`, which prevent any confusions.
…apters. (#2956) A bug has been raised in #2947, which is the consequence of a bug introduced in #2909. This patch fixes it by differentiating JS docs and TS docs clearly. The `Context::push_setter` and `Context::push_getter` methods now receive a `js_docs` and a `ts_docs`, which prevent any confusions.
This patch fixes
#2721. Let's consider
the following example:
It outputs the following
.d.ts
file:What's wrong here is that
second
has the typevoid
. It should benumber
.What's happening? For
Foo.first
, a getter and a setter aregenerated. The getter never has a return type (spoiler: that's the
bug), but the setter has a correct return type somehow (it's infererd
from its arguments, combined with a unit descriptor, see
wasm_bindgen_cli_support::wit::Context::struct_
). When the getterand the setter are processed, they both end up calling
wasm_bindgen_cli_support::js::ExportedClass::push_accessor_ts
. Thisfunction overwrites the return type for the same field everytime it is
called. So when called for the getter, the return type is missing, and
when called for the setter with a type, the bug is fixed.
For
Foo.second
, it's different. There is no setter generated becausethe field is marked as
readonly
. So the bug appears.To fix that, I've updated the getter
Function
descriptor, so that ithas an
inner_ret
value. This is passed to anAdapter.inner_results
at some point in the code, which is finally passed by
JsFunction::process
toJsFunction::typescript_signature
togenerate the typescript signature. And suddently, it's fixed!
However, we got:
We're making progress!
Nonetheless, the documentation is wrong now: The fields don't return
anything. This problem comes from the way
js_docs
is computedglobally for all export kinds in
wasm_bindgen_cli_support::js::Context::generate_adapter
. This patchalso updates that to compute
js_docs
lately, for each branch. It isspecialized in the
AuxExportKind::Getter
andAuxExportKind::Setter
branches.
I hope this is the correct approach. I'm not sure about the difference
between
Adapter.results
andAdapter.inner_results
as there is nodocumentation unfortunately (kudos for every other super-well
documented place though).