Skip to content

Commit

Permalink
fix(typescript) Getters correctly define the inner results. (rustwasm…
Browse files Browse the repository at this point in the history
…#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.
  • Loading branch information
Hywan authored and expenses committed Jun 14, 2022
1 parent 06b07c9 commit 24ed053
Show file tree
Hide file tree
Showing 5 changed files with 32 additions and 2 deletions.
1 change: 1 addition & 0 deletions crates/cli-support/src/js/binding.rs
Expand Up @@ -229,6 +229,7 @@ impl<'a, 'b> Builder<'a, 'b> {
asyncness,
);
let js_doc = self.js_doc_comments(&function_args, &arg_tys, &ts_ret_ty);

Ok(JsFunction {
code,
ts_sig,
Expand Down
8 changes: 7 additions & 1 deletion crates/cli-support/src/js/mod.rs
Expand Up @@ -2521,9 +2521,10 @@ impl<'a> Context<'a> {
false => None,
};

let docs = format_doc_comments(&export.comments, Some(js_doc));
match &export.kind {
AuxExportKind::Function(name) => {
let docs = format_doc_comments(&export.comments, Some(js_doc));

if let Some(ts_sig) = ts_sig {
self.typescript.push_str(&docs);
self.typescript.push_str("export function ");
Expand All @@ -2535,6 +2536,7 @@ impl<'a> Context<'a> {
self.globals.push_str("\n");
}
AuxExportKind::Constructor(class) => {
let docs = format_doc_comments(&export.comments, Some(js_doc));
let exported = require_class(&mut self.exported_classes, class);
if exported.has_constructor {
bail!("found duplicate constructor for class `{}`", class);
Expand All @@ -2543,6 +2545,7 @@ impl<'a> Context<'a> {
exported.push(&docs, "constructor", "", &code, ts_sig);
}
AuxExportKind::Getter { class, field, .. } => {
let docs = format_doc_comments(&export.comments, None);
let ret_ty = match export.generate_typescript {
true => match &ts_ret_ty {
Some(s) => Some(s.as_str()),
Expand All @@ -2554,6 +2557,7 @@ impl<'a> Context<'a> {
exported.push_getter(&docs, field, &code, ret_ty);
}
AuxExportKind::Setter { class, field, .. } => {
let docs = format_doc_comments(&export.comments, None);
let arg_ty = match export.generate_typescript {
true => Some(ts_arg_tys[0].as_str()),
false => None,
Expand All @@ -2562,10 +2566,12 @@ impl<'a> Context<'a> {
exported.push_setter(&docs, field, &code, arg_ty, might_be_optional_field);
}
AuxExportKind::StaticFunction { class, name } => {
let docs = format_doc_comments(&export.comments, Some(js_doc));
let exported = require_class(&mut self.exported_classes, class);
exported.push(&docs, name, "static ", &code, ts_sig);
}
AuxExportKind::Method { class, name, .. } => {
let docs = format_doc_comments(&export.comments, Some(js_doc));
let exported = require_class(&mut self.exported_classes, class);
exported.push(&docs, name, "", &code, ts_sig);
}
Expand Down
2 changes: 1 addition & 1 deletion crates/cli-support/src/wit/mod.rs
Expand Up @@ -806,7 +806,7 @@ impl<'a> Context<'a> {
arguments: vec![Descriptor::I32],
shim_idx: 0,
ret: descriptor.clone(),
inner_ret: None,
inner_ret: Some(descriptor.clone()),
};
let getter_id = self.export_adapter(getter_id, getter_descriptor)?;
self.aux.export_map.insert(
Expand Down
19 changes: 19 additions & 0 deletions crates/typescript-tests/src/getters_setters.rs
Expand Up @@ -66,3 +66,22 @@ impl ColorWithGetterAndSetter {
};
}
}

#[wasm_bindgen]
pub struct ColorWithReadonly {
#[wasm_bindgen(readonly)]
pub r: f64,
#[wasm_bindgen(readonly)]
pub g: f64,
#[wasm_bindgen(readonly)]
pub b: f64,
pub a: u8,
}

#[wasm_bindgen]
impl ColorWithReadonly {
#[wasm_bindgen(constructor)]
pub fn new(r: f64, g: f64, b: f64) -> ColorWithReadonly {
Self { r, b, g, a: 0 }
}
}
4 changes: 4 additions & 0 deletions crates/typescript-tests/src/getters_setters.ts
Expand Up @@ -9,3 +9,7 @@ colorWithSetter.r = 1;
const colorWithGetterAndSetter: wbg.ColorWithGetterAndSetter = new wbg.ColorWithGetterAndSetter;
colorWithGetterAndSetter.r = 1;
const _b = colorWithGetterAndSetter.r;

const colorWithReadonly: wbg.ColorWithReadonly = new wbg.ColorWithReadonly(1, 2, 3);
const _r: number = colorWithReadonly.r;
colorWithReadonly.a = 4;

0 comments on commit 24ed053

Please sign in to comment.