fix(typescript) Getters correctly define the inner results. #2909

Hywan · 2022-05-23T10:22:48Z

This patch fixes
#2721. Let's consider
the following example:

pub struct Foo {
    /// Hello `first`.
    pub first: u32,

    /// Hello `second`.
    #[wasm_bindgen(readonly)]
    pub second: u32,
}

It outputs the following .d.ts file:

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:

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

@returns

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

alexcrichton · 2022-05-24T19:53:34Z

Thanks! Could a test be added for this as well?

Hywan · 2022-05-25T11:16:44Z

I guess it's a good idea ;-). Sorry for forgetting!

Hywan · 2022-05-30T07:24:36Z

I've added a test in crates/typescript-tests/. I first wanted to add a test in crates/cli/tests/reference/ but the existing tests are broken on my machine, and as far as I can see, they seem to not be run in the CI at all.

alexcrichton · 2022-05-31T14:25:22Z

Alas it appears I forgot to migrate that when switching to github actions, would you be up for updating those tests to run here?

Hywan · 2022-05-31T14:50:54Z

I will as soon as I find free time!

@returns

…#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.

test(typescript) Test readonly struct fields have correct types.

f43c0b9

alexcrichton merged commit 96eca58 into rustwasm:main May 31, 2022

dsherret mentioned this pull request Jun 16, 2022

Getters and setters are missing jsdoc types in 0.2.81 (not 0.2.80) #2947

Closed

Hywan mentioned this pull request Jun 20, 2022

fix(cli-support) Differentiate JS docs and TS docs when generating adapters #2956

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(typescript) Getters correctly define the inner results. #2909

fix(typescript) Getters correctly define the inner results. #2909

Hywan commented May 23, 2022

alexcrichton commented May 24, 2022

Hywan commented May 25, 2022

Hywan commented May 30, 2022

alexcrichton commented May 31, 2022

Hywan commented May 31, 2022

fix(typescript) Getters correctly define the inner results. #2909

fix(typescript) Getters correctly define the inner results. #2909

Conversation

Hywan commented May 23, 2022

alexcrichton commented May 24, 2022

Hywan commented May 25, 2022

Hywan commented May 30, 2022

alexcrichton commented May 31, 2022

Hywan commented May 31, 2022