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

Float parsing fails the toml-test when using Nim devel #45

Closed
kaushalmodi opened this issue Jun 5, 2021 · 3 comments · Fixed by #46
Closed

Float parsing fails the toml-test when using Nim devel #45

kaushalmodi opened this issue Jun 5, 2021 · 3 comments · Fixed by #46

Comments

@kaushalmodi
Copy link
Member

kaushalmodi commented Jun 5, 2021

This is the root cause of the recent failure of parsetoml tests on Travis weekly cron: https://travis-ci.org/github/NimParsers/parsetoml/builds/773550319

See nim-lang/Nim#18182 (comment) .

@kaushalmodi
Copy link
Member Author

kaushalmodi commented Jun 5, 2021

@PMunch

The parseDecimalPart is creating an incorrect outcome on Nim devel.

Here's a minimal reproducible example using parsetoml:

import std/[unittest]
import parsetoml # nimble install parsetoml

test "TOML float":
  check $("some_float = 0.123".parseString()["some_float"].getFloat()) == "0.123"

I dived into the code and ended up in parseDecimalPart:

parsetoml/src/parsetoml.nim

Lines 271 to 299 in 3760867

proc parseDecimalPart(state: var ParserState): float64 =
var
nextChar: char
invPowerOfTen = 10.0
firstPos = true
wasUnderscore = false
result = 0.0
while true:
wasUnderscore = nextChar == '_'
nextChar = state.getNextChar()
if nextChar == '_':
if firstPos or wasUnderscore:
raise(newTomlError(state,
"underscore must be surrounded by digit"))
continue
if nextChar notin strutils.Digits:
if wasUnderscore:
raise(newTomlError(state,
"underscore must be surrounded by digit"))
state.pushBackChar(nextChar)
if firstPos:
raise newTomlError(state, "decimal part empty")
break
result = result + (int(nextChar) - int('0')).float / invPowerOfTen
invPowerOfTen *= 10
firstPos = false

Here's a minimal example that shows where things are going wrong on devel:

import math, sugar

var
  invPowerOfTen = 10.0
  decimalPart: float64

for ch in ['1', '2', '3']: # 0.123
  decimalPart += (int(ch) - int('0')).float / invPowerOfTen
  dump ch
  dump decimalPart
  invPowerOfTen *= 10

doAssert $decimalPart == "0.123"

Output:

ch = 1
decimalPart = 0.1
ch = 2
decimalPart = 0.12000000000000001
ch = 3
decimalPart = 0.12300000000000001
/home/kmodi/sandbox/nim/bug_reports/t18182.nim(13) t18182
/home/kmodi/usr_local/apps/7/nim/devel/lib/system/assertions.nim(39) failedAssertImpl
/home/kmodi/usr_local/apps/7/nim/devel/lib/system/assertions.nim(29) raiseAssert
/home/kmodi/usr_local/apps/7/nim/devel/lib/system/fatal.nim(53) sysFatal
Error: unhandled exception: /home/kmodi/sandbox/nim/bug_reports/t18182.nim(13, 10) `$decimalPart == "0.123"` 

When I run the same code on latest Nim stable on Playground: https://play.nim-lang.org/#ix=3oMQ, the output is:

ch = 1
decimalPart = 0.1
ch = 2
decimalPart = 0.12
ch = 3
decimalPart = 0.123

These is how far I could get. I don't understand the low level details on why the outcome is different. Also copying @timotheecour in case he's interested in helping fix this in parsetoml.

@kaushalmodi
Copy link
Member Author

kaushalmodi commented Jun 5, 2021

OK, so this fails on devel:

nim --eval:'assert 0.1 + 0.02 == 0.12'

I am still not sure if this is something that should be fixed in Nim devel. But assuming it's not, can something like this be done in parsetoml:

nim --eval:'assert (10 + 2)/100.0 == 0.12'

Above passes. So we first need to figure out how many decimal point digits we have, scale up the decimal parts and then normalize using a float division.


Or somehow strutils.parseFloat needs to be used as suggested in nim-lang/Nim#18182 (comment) so that it does the Right Thing.

@timotheecour
Copy link
Contributor

OK, so this fails on devel:
nim --eval:'assert 0.1 + 0.02 == 0.12'

not just nim devel, 1.4.4 too, and indeed, 0.1 + 0.02 != 0.12 with FP semantics.

Or somehow strutils.parseFloat

parseutils.parseFloat is IMO what should be used

kaushalmodi added a commit to kaushalmodi-forks/parsetoml that referenced this issue Jun 7, 2021
With the float string representation fixed in Nim devel (See
nim-lang/Nim#18139), that uncovered a bug in
the logic for parsing float TOML values.

For e.g. to parse "foo = 0.123", internally, 0.1 + 0.02 + 0.003 was
done which would evaluate to 0.12300000000000001. Earlier
float stringification bug caused that value to print out as "0.123"
instead of "0.12300000000000001".

This is now fixed by using parseutils.parsefloat, which parses the
"0.123" float value as 0.123 and not 0.12300000000000001.

Fixes NimParsers#45.
kaushalmodi added a commit to kaushalmodi-forks/parsetoml that referenced this issue Jun 7, 2021
With the float string representation fixed in Nim devel (See
nim-lang/Nim#18139), that uncovered a bug in
the logic for parsing float TOML values.

For e.g. to parse "foo = 0.123", internally, 0.1 + 0.02 + 0.003 was
done which would evaluate to 0.12300000000000001. Earlier
float stringification bug caused that value to print out as "0.123"
instead of "0.12300000000000001".

This is now fixed by using parseutils.parsefloat, which parses the
"0.123" float value as 0.123 and not 0.12300000000000001.

Fixes NimParsers#45.
kaushalmodi added a commit to kaushalmodi-forks/parsetoml that referenced this issue Jun 7, 2021
With the float string representation fixed in Nim devel (See
nim-lang/Nim#18139), that uncovered a bug in
the logic for parsing float TOML values.

For e.g. to parse "foo = 0.123", internally, 0.1 + 0.02 + 0.003 was
done which would evaluate to 0.12300000000000001. Earlier
float stringification bug caused that value to print out as "0.123"
instead of "0.12300000000000001".

This is now fixed by using parseutils.parsefloat, which parses the
"0.123" float value as 0.123 and not 0.12300000000000001.

Fixes NimParsers#45.
kaushalmodi added a commit that referenced this issue Jun 7, 2021
With the float string representation fixed in Nim devel (See
nim-lang/Nim#18139), that uncovered a bug in
the logic for parsing float TOML values.

For e.g. to parse "foo = 0.123", internally, 0.1 + 0.02 + 0.003 was
done which would evaluate to 0.12300000000000001. Earlier
float stringification bug caused that value to print out as "0.123"
instead of "0.12300000000000001".

This is now fixed by using parseutils.parsefloat, which parses the
"0.123" float value as 0.123 and not 0.12300000000000001.

Fixes #45.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants