From 96eca588e6a647e33bdd3944f0502bb324369ffc Mon Sep 17 00:00:00 2001 From: Ivan Enderlin Date: Tue, 31 May 2022 16:25:27 +0200 Subject: [PATCH] fix(typescript) Getters correctly define the inner results. (#2909) * fix(typescript) Getters correctly define the inner results. This patch fixes https://github.com/rustwasm/wasm-bindgen/issues/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. --- crates/cli-support/src/js/binding.rs | 1 + crates/cli-support/src/js/mod.rs | 8 +++++++- crates/cli-support/src/wit/mod.rs | 2 +- .../typescript-tests/src/getters_setters.rs | 19 +++++++++++++++++++ .../typescript-tests/src/getters_setters.ts | 4 ++++ 5 files changed, 32 insertions(+), 2 deletions(-) diff --git a/crates/cli-support/src/js/binding.rs b/crates/cli-support/src/js/binding.rs index 50931ea9300..c5eda1c9e79 100644 --- a/crates/cli-support/src/js/binding.rs +++ b/crates/cli-support/src/js/binding.rs @@ -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, diff --git a/crates/cli-support/src/js/mod.rs b/crates/cli-support/src/js/mod.rs index d0ae901a98d..8c7e1bb01f6 100644 --- a/crates/cli-support/src/js/mod.rs +++ b/crates/cli-support/src/js/mod.rs @@ -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 "); @@ -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); @@ -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()), @@ -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, @@ -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); } diff --git a/crates/cli-support/src/wit/mod.rs b/crates/cli-support/src/wit/mod.rs index 529c5a5c362..ee36d7edbfe 100644 --- a/crates/cli-support/src/wit/mod.rs +++ b/crates/cli-support/src/wit/mod.rs @@ -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( diff --git a/crates/typescript-tests/src/getters_setters.rs b/crates/typescript-tests/src/getters_setters.rs index f2206ab3ce0..2895a639433 100644 --- a/crates/typescript-tests/src/getters_setters.rs +++ b/crates/typescript-tests/src/getters_setters.rs @@ -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 } + } +} diff --git a/crates/typescript-tests/src/getters_setters.ts b/crates/typescript-tests/src/getters_setters.ts index 1c1a59783b4..5ff7abc9b37 100644 --- a/crates/typescript-tests/src/getters_setters.ts +++ b/crates/typescript-tests/src/getters_setters.ts @@ -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;