Skip to content

fix(math): correct Math.acosh for large finite inputs#5230

Merged
jedel1043 merged 4 commits intoboa-dev:mainfrom
HiteshShonak:fix/math-acosh-large-finite
Mar 23, 2026
Merged

fix(math): correct Math.acosh for large finite inputs#5230
jedel1043 merged 4 commits intoboa-dev:mainfrom
HiteshShonak:fix/math-acosh-large-finite

Conversation

@HiteshShonak
Copy link
Contributor

@HiteshShonak HiteshShonak commented Mar 22, 2026

This Pull Request fixes/closes #5229.

It changes the following:

  • When the input is finite and greater than 1/√f64::EPSILON (67_108_864.0), fall back to n.ln() + LN_2 instead of calling f64::acosh() directly, avoiding an internal overflow that produces Infinity. Threshold follows the Boost math library convention.
  • Add regression tests for Math.acosh(1e308) and Math.acosh(Number.MAX_VALUE).

Testing:

cargo test -p boa_engine math -- --nocapture

Spec reference: https://tc39.es/ecma262/#sec-math.acosh

@HiteshShonak HiteshShonak requested a review from a team as a code owner March 22, 2026 19:03
Copilot AI review requested due to automatic review settings March 22, 2026 19:03
@github-actions github-actions bot added C-Tests Issues and PRs related to the tests. C-Builtins PRs and Issues related to builtins/intrinsics Waiting On Review Waiting on reviews from the maintainers labels Mar 22, 2026
@github-actions github-actions bot added this to the v1.0.0 milestone Mar 22, 2026
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR fixes Math.acosh returning Infinity for very large finite inputs by avoiding an overflow inside Rust’s f64::acosh() implementation, and adds regression tests to cover the affected cases.

Changes:

  • Add a large-input fast path in Math.acosh for finite n > 1e154 using ln(n) + LN_2.
  • Add regression tests for Math.acosh(1e308) and Math.acosh(Number.MAX_VALUE).

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
core/engine/src/builtins/math/mod.rs Adds a large-finite-input fallback to avoid f64::acosh() overflow.
core/engine/src/builtins/math/tests.rs Adds regression coverage for large finite inputs that previously produced Infinity.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@github-actions
Copy link

github-actions bot commented Mar 22, 2026

Test262 conformance changes

Test result main count PR count difference
Total 52,963 52,963 0
Passed 50,545 50,545 0
Ignored 1,426 1,426 0
Failed 992 992 0
Panics 2 2 0
Conformance 95.43% 95.43% 0.00%

Tested main commit: 66a1cd268ce6006918032b5d0f98d3a63c81b188
Tested PR commit: 6efedce72fce1a7976c19fba9cdb4f8f38950b9e
Compare commits: 66a1cd2...6efedce

@codecov
Copy link

codecov bot commented Mar 22, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 59.84%. Comparing base (6ddc2b4) to head (6efedce).
⚠️ Report is 917 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff             @@
##             main    #5230       +/-   ##
===========================================
+ Coverage   47.24%   59.84%   +12.60%     
===========================================
  Files         476      582      +106     
  Lines       46892    63459    +16567     
===========================================
+ Hits        22154    37979    +15825     
- Misses      24738    25480      +742     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@jedel1043
Copy link
Member

Would it be better to follow the bounds established by the Boost library?
https://www.boost.org/doc/libs/latest/libs/math/doc/html/math_toolkit/inv_hyper/acosh.html

It documents that the ln approximation is valid for numbers bigger than 1/√epsilon

@HiteshShonak
Copy link
Contributor Author

Would it be better to follow the bounds established by the Boost library? https://www.boost.org/doc/libs/latest/libs/math/doc/html/math_toolkit/inv_hyper/acosh.html

It documents that the ln approximation is valid for numbers bigger than 1/√epsilon

updated to use 1/sqrt(epsilon) matching the Boost threshold, thanks for the reference

Copy link
Member

@jedel1043 jedel1043 left a comment

Choose a reason for hiding this comment

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

Looks great! Thanks!

@jedel1043 jedel1043 added this pull request to the merge queue Mar 23, 2026
Merged via the queue into boa-dev:main with commit 5b0f62a Mar 23, 2026
22 checks passed
@github-actions github-actions bot removed the Waiting On Review Waiting on reviews from the maintainers label Mar 23, 2026
@jedel1043 jedel1043 added the A-Bug Something isn't working label Mar 23, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-Bug Something isn't working C-Builtins PRs and Issues related to builtins/intrinsics C-Tests Issues and PRs related to the tests.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Math.acosh returns Infinity for large finite inputs like 1e308 and Number.MAX_VALUE

3 participants