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

invalid type: floating point after math operations #1962

Closed
lastmjs opened this issue Mar 21, 2022 · 4 comments
Closed

invalid type: floating point after math operations #1962

lastmjs opened this issue Mar 21, 2022 · 4 comments
Assignees
Labels
bug Something isn't working execution Issues or PRs related to code execution
Milestone

Comments

@lastmjs
Copy link
Contributor

lastmjs commented Mar 21, 2022

Describe the bug

I have been using the new JsValue to_json functionality and I'm running into some issues. I am dealing with numbers in JS that should be valid integers, whether they be u32, i32, etc. boa seems to have issues with to_json because it is converting them into Rationals for an unknown reason. This might be related to #1961.

To Reproduce

fn main() {
    println!("get_u32: {}", get_u32());
}

fn get_u32() -> u32 {
    let mut context = boa_engine::Context::default();

    let return_value = context.eval(r#"
        1000000 - 500
    "#).unwrap();

    serde_json::from_value(return_value.to_json(& mut context).unwrap()).unwrap()
}

Expected behavior

This should print get_u32: 999500 and should not error out. Instead I get this error:

thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: Error("invalid type: floating point `999500`, expected u32", line: 0, column: 0)', src/main.rs:12:74

Build environment (please complete the following information):

  • OS: Ubuntu 20.04
  • Version: latest main
  • Target triple: wasm32-unknown-unknown or x86_64-unknown-linux-gnu
  • Rustc version: rustc 1.58.1 (db9d1b20b 2022-01-20)

Additional context

It seems like certain math operations are causing a Rust representation of Rational unnecessarily.

@lastmjs lastmjs added the bug Something isn't working label Mar 21, 2022
@jedel1043 jedel1043 added the execution Issues or PRs related to code execution label Mar 21, 2022
@jedel1043 jedel1043 added this to the v0.15.0 milestone Mar 21, 2022
@jedel1043
Copy link
Member

This is pretty unrelated to #1961 :)
We currently convert to f64 when any operation between i32 is executed, but we should be checking whether the result is representable by a i32, or else, convert both operands to f64 and execute the operation. I'll work on this while we discuss #1961

@RageKnify
Copy link
Contributor

I sort of mentioned this before in #1147

@bors bors bot closed this as completed in e1c2e14 Mar 22, 2022
@Razican
Copy link
Member

Razican commented Mar 22, 2022

I am dealing with numbers in JS that should be valid integers, whether they be u32, i32, etc. boa seems to have issues with to_json because it is converting them into Rationals for an unknown reason.

Just to explain this. The reason is that there is no such thing as an "integer" type in JavaScript. All numeric types are 64-bit floating point numbers. So this should never work like this: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Data_structures#number_type

@lastmjs
Copy link
Contributor Author

lastmjs commented Mar 22, 2022

Yes thank you. This goes back to my latest comment here: #1961 (comment)

I would just like some kind of JsValue::into to easily convert my JsValues to another type.

let my_u32: u32 = my_js_value.into();

If we could have something like that for arbitrary structs, enums, etc that would be excellent. Does this functionality exist?

Razican pushed a commit that referenced this issue Jun 8, 2022
This Pull Request fixes/closes #1962.

It changes the following:

- When executing arithmetic operations on `JsValue`s, try to use integer operations and fallback to `f64` operations on error.
- Adds tests for serde_json conversions from integer operations.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working execution Issues or PRs related to code execution
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants