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.

Sign up for GitHub

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

Jump to bottom

Fix optional arguments in TypeScript #1483

Merged
merged 3 commits into from
May 14, 2019
Merged

Fix optional arguments in TypeScript #1483

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2384af2
Fix optional arguments in TypeScript
dbrgn Apr 25, 2019
d9c559f
fixup! Fix optional arguments in TypeScript
dbrgn May 9, 2019
608a819
fixup! Fix optional arguments in TypeScript
dbrgn May 9, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
86 changes: 62 additions & 24 deletions crates/cli-support/src/js/js2rust.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,22 @@ use crate::descriptor::{Descriptor, Function};
use crate::js::Context;
use failure::{bail, Error};

pub struct JsArgument {
pub optional: bool,
pub name: String,
pub type_: String,
}

impl JsArgument {
fn required(name: String, type_: String) -> Self {
Self { optional: false, name, type_ }
}

fn optional(name: String, type_: String) -> Self {
Self { optional: true, name, type_ }
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the names of these methods be changed to new_(required|optional)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there any guidelines / recommendations on this topic? I couldn't find anything in https://github.com/rust-dev-tools/fmt-rfcs/.

I personally prefer the shorter version, it's clear from the signature that it's a constructor.


/// Helper struct for manufacturing a shim in JS used to translate JS types to
/// Rust, aka pass from JS back into Rust
pub struct Js2Rust<'a, 'b: 'a> {
Expand All @@ -12,7 +28,7 @@ pub struct Js2Rust<'a, 'b: 'a> {
rust_arguments: Vec<String>,

/// Arguments and their types to the JS shim.
pub js_arguments: Vec<(String, String)>,
pub js_arguments: Vec<JsArgument>,

/// Conversions that happen before we invoke the wasm function, such as
/// converting a string to a ptr/length pair.
Expand Down Expand Up @@ -173,7 +189,7 @@ impl<'a, 'b> Js2Rust<'a, 'b> {

if let Some(kind) = arg.vector_kind() {
self.js_arguments
.push((name.clone(), kind.js_ty().to_string()));
.push(JsArgument::required(name.clone(), kind.js_ty().to_string()));

let func = self.cx.pass_to_wasm_function(kind)?;
let val = if optional {
Expand Down Expand Up @@ -221,7 +237,7 @@ impl<'a, 'b> Js2Rust<'a, 'b> {
}

if arg.is_anyref() {
self.js_arguments.push((name.clone(), "any".to_string()));
self.js_arguments.push(JsArgument::required(name.clone(), "any".to_string()));
if self.cx.config.anyref {
if optional {
self.cx.expose_add_to_anyref_table()?;
Expand Down Expand Up @@ -250,7 +266,7 @@ impl<'a, 'b> Js2Rust<'a, 'b> {

if arg.is_wasm_native() {
self.js_arguments
.push((name.clone(), "number | undefined".to_string()));
.push(JsArgument::optional(name.clone(), "number".to_string()));

if self.cx.config.debug {
self.cx.expose_assert_num();
Expand All @@ -272,7 +288,7 @@ impl<'a, 'b> Js2Rust<'a, 'b> {

if arg.is_abi_as_u32() {
self.js_arguments
.push((name.clone(), "number | undefined".to_string()));
.push(JsArgument::optional(name.clone(), "number".to_string()));

if self.cx.config.debug {
self.cx.expose_assert_num();
Expand All @@ -299,7 +315,7 @@ impl<'a, 'b> Js2Rust<'a, 'b> {
};
self.cx.expose_uint32_memory();
self.js_arguments
.push((name.clone(), "BigInt | undefined".to_string()));
.push(JsArgument::optional(name.clone(), "BigInt".to_string()));
self.prelude(&format!(
"
{f}[0] = isLikeNone({name}) ? BigInt(0) : {name};
Expand All @@ -320,7 +336,7 @@ impl<'a, 'b> Js2Rust<'a, 'b> {
match *arg {
Descriptor::Boolean => {
self.js_arguments
.push((name.clone(), "boolean | undefined".to_string()));
.push(JsArgument::optional(name.clone(), "boolean".to_string()));
if self.cx.config.debug {
self.cx.expose_assert_bool();
self.prelude(&format!(
Expand All @@ -337,21 +353,21 @@ impl<'a, 'b> Js2Rust<'a, 'b> {
}
Descriptor::Char => {
self.js_arguments
.push((name.clone(), "string | undefined".to_string()));
.push(JsArgument::optional(name.clone(), "string".to_string()));
self.rust_arguments.push(format!(
"isLikeNone({0}) ? 0xFFFFFF : {0}.codePointAt(0)",
name
));
}
Descriptor::Enum { hole } => {
self.js_arguments
.push((name.clone(), "number | undefined".to_string()));
.push(JsArgument::optional(name.clone(), "number".to_string()));
self.rust_arguments
.push(format!("isLikeNone({0}) ? {1} : {0}", name, hole));
}
Descriptor::RustStruct(ref s) => {
self.js_arguments
.push((name.clone(), format!("{} | undefined", s)));
.push(JsArgument::optional(name.clone(), s.to_string()));
self.prelude(&format!("let ptr{} = 0;", i));
self.prelude(&format!("if (!isLikeNone({0})) {{", name));
self.assert_class(&name, s);
Expand All @@ -371,7 +387,7 @@ impl<'a, 'b> Js2Rust<'a, 'b> {
}

if let Some(s) = arg.rust_struct() {
self.js_arguments.push((name.clone(), s.to_string()));
self.js_arguments.push(JsArgument::required(name.clone(), s.to_string()));
self.assert_class(&name, s);
self.assert_not_moved(&name);
if arg.is_by_ref() {
Expand All @@ -385,7 +401,7 @@ impl<'a, 'b> Js2Rust<'a, 'b> {
}

if arg.number().is_some() {
self.js_arguments.push((name.clone(), "number".to_string()));
self.js_arguments.push(JsArgument::required(name.clone(), "number".to_string()));

if self.cx.config.debug {
self.cx.expose_assert_num();
Expand All @@ -403,7 +419,7 @@ impl<'a, 'b> Js2Rust<'a, 'b> {
self.cx.expose_uint64_cvt_shim()
};
self.cx.expose_uint32_memory();
self.js_arguments.push((name.clone(), "BigInt".to_string()));
self.js_arguments.push(JsArgument::required(name.clone(), "BigInt".to_string()));
self.prelude(&format!(
"
{f}[0] = {name};
Expand All @@ -420,7 +436,7 @@ impl<'a, 'b> Js2Rust<'a, 'b> {
}

if arg.is_ref_anyref() {
self.js_arguments.push((name.clone(), "any".to_string()));
self.js_arguments.push(JsArgument::required(name.clone(), "any".to_string()));
if self.cx.config.anyref {
self.anyref_args.push((self.rust_arguments.len(), false));
self.rust_arguments.push(name);
Expand All @@ -440,7 +456,7 @@ impl<'a, 'b> Js2Rust<'a, 'b> {
match *arg {
Descriptor::Boolean => {
self.js_arguments
.push((name.clone(), "boolean".to_string()));
.push(JsArgument::required(name.clone(), "boolean".to_string()));
if self.cx.config.debug {
self.cx.expose_assert_bool();
self.prelude(&format!(
Expand All @@ -453,7 +469,7 @@ impl<'a, 'b> Js2Rust<'a, 'b> {
self.rust_arguments.push(format!("{}", name));
}
Descriptor::Char => {
self.js_arguments.push((name.clone(), "string".to_string()));
self.js_arguments.push(JsArgument::required(name.clone(), "string".to_string()));
self.rust_arguments.push(format!("{}.codePointAt(0)", name))
}
_ => bail!(
Expand Down Expand Up @@ -735,7 +751,11 @@ impl<'a, 'b> Js2Rust<'a, 'b> {
let mut ret: String = self
.js_arguments
.iter()
.map(|a| format!("@param {{{}}} {}\n", a.1, a.0))
.map(|a| if a.optional {
format!("@param {{{} | undefined}} {}\n", a.type_, a.name)
} else {
format!("@param {{{}}} {}\n", a.type_, a.name)
})
.collect();
ret.push_str(&format!("@returns {{{}}}", self.ret_ty));
ret
Expand All @@ -759,7 +779,7 @@ impl<'a, 'b> Js2Rust<'a, 'b> {
let js_args = self
.js_arguments
.iter()
.map(|s| &s.0[..])
.map(|s| &s.name[..])
.collect::<Vec<_>>()
.join(", ");
let mut js = format!("{}({}) {{\n", prefix, js_args);
Expand All @@ -785,12 +805,30 @@ impl<'a, 'b> Js2Rust<'a, 'b> {
};
js.push_str(&invoc);
js.push_str("\n}");
let ts_args = self
.js_arguments
.iter()
.map(|s| format!("{}: {}", s.0, s.1))
.collect::<Vec<_>>()
.join(", ");

// Determine TS parameter list
let mut omittable = true;
let mut ts_args = Vec::with_capacity(self.js_arguments.len());
for arg in self.js_arguments.iter().rev() {
// In TypeScript, we can mark optional parameters as omittable
// using the `?` suffix, but only if they're not followed by
// non-omittable parameters. Therefore iterate the parameter list
// in reverse and stop using the `?` suffix for optional params as
// soon as a non-optional parameter is encountered.
if arg.optional {
if omittable {
ts_args.push(format!("{}?: {}", arg.name, arg.type_));
} else {
ts_args.push(format!("{}: {} | undefined", arg.name, arg.type_));
}
} else {
omittable = false;
ts_args.push(format!("{}: {}", arg.name, arg.type_));
}
}
ts_args.reverse();
let ts_args = ts_args.join(", ");

let mut ts = if prefix.is_empty() {
format!("{}({})", self.js_name, ts_args)
} else {
Expand Down
2 changes: 1 addition & 1 deletion crates/cli-support/src/js/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2963,7 +2963,7 @@ impl<'a, 'b> SubContext<'a, 'b> {
"\n {}{}: {};",
if field.readonly { "readonly " } else { "" },
field.name,
&cx.js_arguments[0].1
&cx.js_arguments[0].type_
));
cx.finish("", &format!("wasm.{}", wasm_setter), setter).0
};
Expand Down
10 changes: 9 additions & 1 deletion crates/typescript-tests/src/opt_args_and_ret.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,14 @@
use wasm_bindgen::prelude::*;

#[wasm_bindgen]
pub fn opt_fn(_a: Option<i32>) -> Option<i32> {
/// Optional parameters followed by non-optional parameters.
/// Only the parameter _c may be marked as omittable.
pub fn opt_fn_mixed(_a: Option<i32>, _b: i32, _c: Option<i32>) -> Option<i32> {
None
}

#[wasm_bindgen]
/// Only optional parameters. All of them may be marked as omittable.
pub fn opt_fn_only(_a: Option<i32>, _b: Option<i32>, _c: Option<i32>) -> Option<i32> {
None
}
3 changes: 2 additions & 1 deletion crates/typescript-tests/src/opt_args_and_ret.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import * as wbg from '../pkg/typescript_tests';

const opt_fn: (a: number | undefined) => number | undefined = wbg.opt_fn;
const opt_fn_mixed: (a: number | undefined, b: number, c?: number) => number | undefined = wbg.opt_fn_mixed;
const opt_fn_only: (a?: number, b?: number, c?: number) => number | undefined = wbg.opt_fn_only;