Add math-related intrinsics/functions for `JsValue`s #2629

Jules-Bertholet · 2021-07-19T10:32:35Z

Adds handling for BigInt, among other things.

alexcrichton · 2021-07-19T14:17:21Z

Thanks! Overall though I'd prefer to keep the set of intrinsics and such to a minimum unless there's a good reason to expand things. Could you elaborate a bit more on the motivation for this? It seems, for example, that some of the conversions (e.g. From<JsValue> for f64 aren't necessarily idiomatic in Rust since they can be fallible. If it's applying to a specific JS operation then ideally that'd either be done with the Reflect structure from js-sys but if necessary it seems fine to have an intrinsic.

Jules-Bertholet · 2021-07-19T15:24:27Z

@alexcrichton JS operators generally don't have corresponding methods, so Reflect isn't an option in those cases. I could remove the Object.is intrinsic, though. From definitely shouldn't be fallible, I'll fix that. But do you think it's a problem if traits like Add are fallible?

ghost · 2021-07-19T19:19:20Z

crates/cli-support/src/intrinsic.rs

@@ -102,9 +105,75 @@ intrinsics! {
        #[symbol = "__wbindgen_is_string"]
        #[signature = fn(ref_externref()) -> Boolean]
        IsString,
+        #[symbol = "__wbindgen_is_bigint"]
+        #[signature = fn(ref_externref()) -> Boolean]


Why special-case this? __wbindgen_typeof can be used to implement this.

@crimsoncodes0 The other types are already special-cased, so I special-cased this one too for consistency
Edit: also, maybe JS engines might have special optimizations for typeof to avoid a full string comparison? (No idea, but presumably they could do this)

ghost · 2021-07-19T19:26:05Z

crates/cli-support/src/intrinsic.rs

+        #[signature = fn(ref_externref()) -> Externref]
+        Neg,
+        #[symbol = "__wbindgen_bit_and"]
+        #[signature = fn(ref_externref(), ref_externref()) -> Externref]


These bitwise operators always return an i32, besides unsigned_shr, which yields u32.
They're currently typed as yielding Externref.

But, on another note, these are already native Rust operators, why use the significantly slower, more unsound JS operators? Is there anything that can't be done without these?

@crimsoncodes0 These operators only return 32-bit ints when passed JS numbers. When passed bigints they return bigints. The only exception is >>> (unsigned_shr), which does not accept bigints; I included it for completeness but if it's deemed unnecessary I can remove it

Jules-Bertholet · 2021-07-19T21:10:44Z

I've removed the Object.is intrinsic and made the From<JsValue> f64 impl into a TryFrom

alexcrichton · 2021-07-20T15:04:54Z

Can you elaborate more on the motivation for this? This is a lot of library support with a lot of traits and a lot of intrinsics. It's a good deal to maintain in what should ideally be a pretty lean core library.

Jules-Bertholet · 2021-07-20T16:53:03Z

The motivation for this is to allow performing mathematical operations on JS bigints. There currently isn't any other method in wasm-bindgen for handling these values. And the only way to add that functionality is to allow accessing the JS math operators, because there aren't any equivalent methods. I plan to make a follow-up PR to add a BigInt type to js-sys to expose this functionality in a more strongly-typed manner, so I'm not particularly attached to the current API in this PR. If it's deemed better to expose these as methods on JsValue rather than as trait impls, that's fine.

The only two intrinsics this PR adds that aren't useful for bigints are the unary +, >>>, and in. I added these for completeness (if every other JS operator is going to be made available, why leave out these three?), but I'm not attached to them. I suppose + and >>> could be marginally useful when dealing with objects with a custom Symbol.toPrimitive, but I don't think that's a very common use case.

Jules-Bertholet · 2021-07-21T12:47:52Z

follow-up PR to add a BigInt type to js-sys to expose this functionality in a more strongly-typed manner

I've been working on this, but I've run into a roadblock: the BigInt() constructor doesn't use new, and as far as I can tell wasm-bindgen doesn't have any facilities to account for this.

alexcrichton · 2021-07-22T14:34:01Z

Ah ok, if possible then I think it would be best to bind BigInt in js-sys without adding new features to wasm-bindgen itself. Perhaps we could try to work through the issues that you're seeing there?

For example for the BigInt constructor you could do something like:

impl BigInt {
    pub fn new(arg: &JsValue) -> BigInt {
        // manually call the `BigInt` name bound as a function
    }
}

where it's not necessarily pure extern "C" { ... } blocks but that's ok.

Jules-Bertholet · 2021-07-23T16:24:52Z

Ready for review

trevyn · 2021-08-27T14:13:56Z

I think it would be best to bind BigInt in js-sys without adding new features to wasm-bindgen itself.

@Jules-Bertholet Are you still working to get this PR merged? I have a need for a straightforward BigInt binding, and it looks like you've included that here.

xref: #1087

Useful for dealing with JS `BigInt`s

Jules-Bertholet · 2021-08-27T14:22:12Z

@trevyn This PR should be ready for review

trevyn · 2021-08-27T14:41:40Z

This PR should be ready for review

Thanks! ping @alexcrichton

trevyn · 2021-08-28T04:24:39Z

This BigInt support is working perfectly for me! 👍🏼

alexcrichton

Sorry for the delay. I'm not overly in love with how this is such a large increase in the API surface area, but I dunno I don't really have anything against it. I would prefer to avoid the num-traits dep but other than that seems fine I guess?

alexcrichton · 2021-08-31T14:28:15Z

Cargo.toml

@@ -40,6 +40,7 @@ wasm-bindgen-macro = { path = "crates/macro", version = "=0.2.76" }
 serde = { version = "1.0", optional = true }
 serde_json = { version = "1.0", optional = true }
 cfg-if = "1.0.0"
+num-traits = { version = "0.2.14", optional = true }


This is a small enough addition could this be a method instead of a trait implementation?

Jules-Bertholet · 2021-08-31T20:31:37Z

@alexcrichton The num-traits dep is just there to allow easier integration with other Rust crates that also use num-traits, you don't miss out on features by not enabling it. There's a standalone pow method you can use instead of its Pow trait, and all of the other trait impls are trivial.

alexcrichton · 2021-09-01T18:42:51Z

Makes sense, yeah, but I'd prefer to leave it out and keep wasm-bindgen at least somewhat slim in deps.

…t-num` further

alexcrichton · 2021-09-02T14:05:59Z

crates/js-sys/Cargo.toml

 [dependencies]
 wasm-bindgen = { path = "../..", version = "0.2.76" }
+num-bigint = { version = "0.4.1", optional = true }
+num-traits = { version = "0.2.14", optional = true }


Sorry but personally I do not want to add new public dependencies to these crates. There is no story for landing breaking changes to js-sys at this time and having new public dependencies increases the risk that a breaking change may be needed (in case these deps have a major-version-bump)

I realize these are optional but it's still part of the public API and something that I'm not willing to commit to.

Using BigInt as the macro parameters leads to absurd signatures like slices of BigInt objects (can't exist). Instead, just use the numeric types. bindgen already converts them to bigint when appropriate. Closes: rustwasm#3009 Fixes: d6d056c ("Add math-related intrinsics/functions for `JsValue`s (rustwasm#2629)")

Using BigInt as the macro parameters leads to absurd signatures like slices of BigInt objects (can't exist). Instead, just use the numeric types. bindgen already converts them to bigint when appropriate. Closes: #3009 Fixes: d6d056c ("Add math-related intrinsics/functions for `JsValue`s (#2629)")

Jules-Bertholet force-pushed the jsvalue-math branch from c48a68f to 7a8dbf5 Compare July 19, 2021 18:43

ghost reviewed Jul 19, 2021

View reviewed changes

Jules-Bertholet force-pushed the jsvalue-math branch from 8fee37c to 02d2b8c Compare July 20, 2021 10:24

Jules-Bertholet force-pushed the jsvalue-math branch 2 times, most recently from 98d4a2d to f72e74b Compare July 21, 2021 09:23

Jules-Bertholet force-pushed the jsvalue-math branch 6 times, most recently from 09fae07 to ac4bb37 Compare July 23, 2021 16:07

Jules-Bertholet force-pushed the jsvalue-math branch 4 times, most recently from 7b8f8f5 to 14fec9b Compare August 6, 2021 11:59

Jules-Bertholet added 5 commits August 27, 2021 10:17

Allow accessing JS math operators from Rust

40d38f3

Useful for dealing with JS `BigInt`s

Add typeof and in methods/intrinsics

6277e62

Add comparison operators

098aef7

Remove Object.is intrinsic

2587439

Make conversion into f64 faillible

9184409

Jules-Bertholet force-pushed the jsvalue-math branch from 1e9dadc to 7185873 Compare August 27, 2021 14:18

Jules-Bertholet added 3 commits August 27, 2021 10:18

Add more trait impls and feature for integration with rust-num

747459a

Add PartialOrd and Ord impls for more types

f976076

Cargo fmt

7185873

This was referenced Aug 28, 2021

Expose BigInt bindings #1087

Closed

Use a wasm-bindgen crates.io release with BigInt support trevyn/turbocharger#6

Closed

alexcrichton reviewed Aug 31, 2021

View reviewed changes

Jules-Bertholet added 4 commits September 1, 2021 19:37

Remove num-traits from wasm-bindgen, integrate js-sys with `rus…

5455ae3

…t-num` further

Update Cargo.toml

589532e

Update Rust version for CI (to allow proc-macro2 to build again)

087d291

Fix link in Markdown

76b8065

Jules-Bertholet requested a review from alexcrichton September 2, 2021 01:45

alexcrichton reviewed Sep 2, 2021

View reviewed changes

Remove all rust-num code from js-sys

2dc02ff

Jules-Bertholet requested a review from alexcrichton September 2, 2021 15:14

alexcrichton merged commit d6d056c into rustwasm:master Sep 2, 2021

Jules-Bertholet deleted the jsvalue-math branch September 2, 2021 18:51

This was referenced Sep 3, 2021

feat: cast JSValue from/to u64 #2351

Closed

Better support for u64 through JSValue::as_u64() and u64::into() -> JsValue #2350

Closed

RReverser mentioned this pull request Sep 28, 2021

implement i64/u64 using BigInt RReverser/serde-wasm-bindgen#21

Closed

ishitatsuyuki mentioned this pull request Jul 29, 2022

Convert Big[Ui,I]nt64Array bindings to use [u/i]64 #3011

Merged

DimDroll mentioned this pull request Apr 24, 2024

Playground to_unix_timestamp nanoseconds precision error vectordotdev/vector#20325

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add math-related intrinsics/functions for `JsValue`s #2629

Add math-related intrinsics/functions for `JsValue`s #2629

Jules-Bertholet commented Jul 19, 2021 •

edited

Loading

alexcrichton commented Jul 19, 2021

Jules-Bertholet commented Jul 19, 2021

ghost Jul 19, 2021

Jules-Bertholet Jul 19, 2021 •

edited

Loading

ghost Jul 19, 2021 •

edited by ghost

Loading

Jules-Bertholet Jul 19, 2021 •

edited

Loading

Jules-Bertholet commented Jul 19, 2021

alexcrichton commented Jul 20, 2021

Jules-Bertholet commented Jul 20, 2021 •

edited

Loading

Jules-Bertholet commented Jul 21, 2021

alexcrichton commented Jul 22, 2021

Jules-Bertholet commented Jul 23, 2021

trevyn commented Aug 27, 2021

Jules-Bertholet commented Aug 27, 2021

trevyn commented Aug 27, 2021

trevyn commented Aug 28, 2021

alexcrichton left a comment

alexcrichton Aug 31, 2021

Jules-Bertholet commented Aug 31, 2021

alexcrichton commented Sep 1, 2021

alexcrichton Sep 2, 2021

Add math-related intrinsics/functions for JsValues #2629

Add math-related intrinsics/functions for JsValues #2629

Conversation

Jules-Bertholet commented Jul 19, 2021 • edited Loading

alexcrichton commented Jul 19, 2021

Jules-Bertholet commented Jul 19, 2021

ghost Jul 19, 2021

Choose a reason for hiding this comment

Jules-Bertholet Jul 19, 2021 • edited Loading

Choose a reason for hiding this comment

ghost Jul 19, 2021 • edited by ghost Loading

Choose a reason for hiding this comment

Jules-Bertholet Jul 19, 2021 • edited Loading

Choose a reason for hiding this comment

Jules-Bertholet commented Jul 19, 2021

alexcrichton commented Jul 20, 2021

Jules-Bertholet commented Jul 20, 2021 • edited Loading

Jules-Bertholet commented Jul 21, 2021

alexcrichton commented Jul 22, 2021

Jules-Bertholet commented Jul 23, 2021

trevyn commented Aug 27, 2021

Jules-Bertholet commented Aug 27, 2021

trevyn commented Aug 27, 2021

trevyn commented Aug 28, 2021

alexcrichton left a comment

Choose a reason for hiding this comment

alexcrichton Aug 31, 2021

Choose a reason for hiding this comment

Jules-Bertholet commented Aug 31, 2021

alexcrichton commented Sep 1, 2021

alexcrichton Sep 2, 2021

Choose a reason for hiding this comment

Add math-related intrinsics/functions for `JsValue`s #2629

Add math-related intrinsics/functions for `JsValue`s #2629

Jules-Bertholet commented Jul 19, 2021 •

edited

Loading

Jules-Bertholet Jul 19, 2021 •

edited

Loading

ghost Jul 19, 2021 •

edited by ghost

Loading

Jules-Bertholet Jul 19, 2021 •

edited

Loading

Jules-Bertholet commented Jul 20, 2021 •

edited

Loading