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

Wrong result from Number.prototype.toFixed #2609

Closed
magic-akari opened this issue Feb 19, 2023 · 4 comments · Fixed by #2898
Closed

Wrong result from Number.prototype.toFixed #2609

magic-akari opened this issue Feb 19, 2023 · 4 comments · Fixed by #2898
Assignees
Labels
bug Something isn't working

Comments

@magic-akari
Copy link
Contributor

Describe the bug
Wrong result from Number.prototype.toFixed

To Reproduce

const a = 1.25.toFixed(1);
console.log(a);

Expected behavior

Running this code, a should be set to "1.3" and printed, but a is instead set to "1.2". The expected behaviour can be found in the ECMAScript specification.

Build environment (please complete the following information):

  • OS: macOS arm64
  • Version: latest
  • Target triple: aarch64-apple-darwin
  • Rustc version: rustc 1.67.0 (fc594f156 2023-01-24)

Additional context
Add any other context about the problem here.

@magic-akari magic-akari added the bug Something isn't working label Feb 19, 2023
@vncsalencar
Copy link

As per my understanding this is because of

let this_fixed_num = format!("{this_num:.precision$}");

Which, as rust-lang/rust#102935 shows, Rust's format! rounds x.5 to whichever even number is closest.
So 1.25.toFixed(1) rounds down to 1.2 but 1.35.toFixed(1) rounds up to 1.4.

Screenshot 2023-02-22 at 11 01 19

@jedel1043 jedel1043 moved this to To do in Boa pre-v1 Mar 4, 2023
@magic-akari
Copy link
Contributor Author

Is there any progress here?

Regarding the one failing test; I think we should merge this without that one.

But we may have an easier solution that to fix the issues in std. We already use ryu-js crate that converts numbers to strings. Conveniently the repository for the crate is in the boa-dev org: https://github.com/boa-dev/ryu-js ;)

Without looking further into it, my idea would be to implement the functionality of num_to_exponential and num_to_exponential_with_precision there. @HalidOdat I think you probably know the most about this, do you think we should try to do that, or is ryu-js not the right place?

Originally posted by @raskad in #1620 (comment)

I have found a recommended fix mentioned in this issue, and I believe it should be implemented in ryu-js.
Is there any plans to implement to_precision and to_fixed in ryu-js?

@HalidOdat
Copy link
Member

I have found a recommended fix mentioned in this issue, and I believe it should be implemented in ryu-js.

I think putting it in the ryu-js crate is a good place. (maybe feature gated) :)

Is there any plans to implement to_precision and to_fixed in ryu-js?

I don't think there has been any work done on this, I created Issues to track the progress (boa-dev/ryu-js#33, boa-dev/ryu-js#32, boa-dev/ryu-js#31)

@HalidOdat HalidOdat self-assigned this Apr 6, 2023
@HalidOdat
Copy link
Member

HalidOdat commented Apr 6, 2023

I have started work on Number.prototype.toFixed() implementation in the ryu algorithm locally, Hopefully I will be able create a PR on ryu-js repo, when It passes all the tests, then create a PR for boa_engine with the changes here :)

@HalidOdat HalidOdat moved this from To do to In Progress in Boa pre-v1 Apr 6, 2023
HalidOdat added a commit that referenced this issue May 4, 2023
HalidOdat added a commit that referenced this issue May 8, 2023
HalidOdat added a commit that referenced this issue Jun 9, 2023
HalidOdat added a commit that referenced this issue Jun 9, 2023
@github-project-automation github-project-automation bot moved this from In Progress to Done in Boa pre-v1 Oct 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

3 participants