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

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

Merged
merged 2 commits into from May 31, 2022

Conversation

Hywan
Copy link
Contributor

@Hywan Hywan commented May 23, 2022

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

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
Copy link
Contributor

@alexcrichton alexcrichton commented May 24, 2022

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

@Hywan
Copy link
Contributor Author

@Hywan Hywan commented May 25, 2022

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

@Hywan
Copy link
Contributor Author

@Hywan Hywan commented May 30, 2022

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
Copy link
Contributor

@alexcrichton alexcrichton commented May 31, 2022

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

@alexcrichton alexcrichton merged commit 96eca58 into rustwasm:main May 31, 2022
21 checks passed
@Hywan
Copy link
Contributor Author

@Hywan Hywan commented May 31, 2022

I will as soon as I find free time!

expenses pushed a commit to expenses/wasm-bindgen that referenced this issue Jun 14, 2022
…#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.
Hywan added a commit to Hywan/wasm-bindgen that referenced this issue Jun 20, 2022
…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.
alexcrichton pushed a commit that referenced this issue Jun 21, 2022
…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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants