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

Rails 6.1 regression issue with big decimals precision #42302

Closed
mildred opened this issue May 27, 2021 · 6 comments
Closed

Rails 6.1 regression issue with big decimals precision #42302

mildred opened this issue May 27, 2021 · 6 comments

Comments

@mildred
Copy link

@mildred mildred commented May 27, 2021

Steps to reproduce

In rails console:

  > include ActionView::Helpers::NumberHelper
  > number_with_precision(0.287298702e23.to_d, precision: 0)

It seems to me that this commit might be at fault: 7905bdf (not checked it though, just ran git blame on relevant classes and found it)

Expected behavior

 => "28729870200000000000000"

in context :

  > Rails.version
 => "6.0.3.7"

Actual behavior

 => "28729870199999998984192"

in context :

  > Rails.version
 => "6.1.3.1"

System configuration

Rails version: 6.1.3.1

Ruby version: ruby 3.0.0p0 (2020-12-25 revision 95aff21468) [x86_64-linux]

@mildred
Copy link
Author

@mildred mildred commented May 27, 2021

It seems the issue is caused by this diff : 7905bdf#diff-cda12a0b7fe1607d710c06d2be6dc0e09d10a8f272a7156a70e5e1003dd40e8c

diff activesupport/lib/active_support/number_helper/number_to_rounded_converter.rb
           end
 
           formatted_string =
-            if BigDecimal === rounded_number && rounded_number.finite?
+            if rounded_number.nan? || rounded_number.infinite? || rounded_number == rounded_number.to_i
+              "%00.#{precision}f" % rounded_number
+            else
               s = rounded_number.to_s("F")
               s << "0" * precision
               a, b = s.split(".", 2)
               a << "."
               a << b[0, precision]
-            else
-              "%00.#{precision}f" % rounded_number
             end
         else
           formatted_string = rounded_number

Let consider that rounded_number is 0.287298702e23.to_d, we have the old behaviour:

  • BigDecimal === rounded_number && rounded_number.finite? is true
  • rounded_number.to_s("F") is "28729870200000000000000.0"

With the new behaviour,

  • rounded_number == rounded_number.to_i is true
  • "%00.#{precision}f" % rounded_number is "28729870199999998984192"

@zzak
Copy link
Member

@zzak zzak commented May 27, 2021

@tom-lord Since you added this back in #38148 would you mind taking a look? 🙇 /cc @rafaelfranca

@tom-lord
Copy link
Contributor

@tom-lord tom-lord commented May 27, 2021

Ah yes, how odd!...

"%00.0f" % 0.287298702e23.to_d
  #=> "28729870199999998984192"

0.287298702e23.to_d.to_s("F")
  #=> "28729870200000000000000.0"

There must be some missing test coverage around this scenario. I remember changing that line to better encapsulate handing NaN, and because type checks are generally frowned upon, but clearly this is a scenario that was missed.

BTW this should probably be tagged as activesupport, not actionview.

@zzak
Copy link
Member

@zzak zzak commented May 31, 2021

@tom-lord Mind giving this a sanity check if #42316 resolves this? 🙇

@mildred
Copy link
Author

@mildred mildred commented Jun 3, 2021

Thank you for the fix, are you considering releasing a patch release for the 6.1 branch?

@pixeltrix
Copy link
Member

@pixeltrix pixeltrix commented Jun 3, 2021

I've backported it here: #42374

Just waiting for CI to pass.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

5 participants