Skip to content

FIX InexactError when using allownan with typemax(Int) - #479

Open
Kiwy3 wants to merge 8 commits into
JuliaIO:masterfrom
Kiwy3:master
Open

FIX InexactError when using allownan with typemax(Int)#479
Kiwy3 wants to merge 8 commits into
JuliaIO:masterfrom
Kiwy3:master

Conversation

@Kiwy3

@Kiwy3 Kiwy3 commented Aug 20, 2026

Copy link
Copy Markdown

Summary

  • correct parsenumber function and check_special macro to fix the bug
  • add 2 tests (for Int64 and Int128) to ensure future versions will work with this

Root Cause

When using allownan, any number was parse as Float, and then convert as Int if needed. But the parsing as Float64 was rounded from 2e63 -1 to 2e63.

Notice: it's my first PR, so I may have done some mistakes. I ran all tests locally, and it was working.

Fixes #478

@codecov

codecov Bot commented Aug 20, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.31%. Comparing base (233d644) to head (527057f).

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #479      +/-   ##
==========================================
+ Coverage   89.99%   90.31%   +0.32%     
==========================================
  Files           7        7              
  Lines        1519     1528       +9     
==========================================
+ Hits         1367     1380      +13     
+ Misses        152      148       -4     

☔ View full report in Codecov by Harness.
📢 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.

@quinnj

quinnj commented Aug 20, 2026

Copy link
Copy Markdown
Member

Thanks for the clear writeup and for jumping on a first PR the same day.

Diagnosis

With allownan=true, parsenumber currently skips the Int64/BigInt lexer and always goes through Parsers.xparse2(Float64, ...). Float64(typemax(Int64)) rounds to 2^63, and converting that to Int64 throws InexactError. Same class of bug for any integer with |n| > 2^53 (silent wrong value) and for typemax(Int128) / typemax(UInt64).

Always running the integer lexer, and only taking the float path for actual floats / specials, is the right fix and the right layer.

@check_special rewrite

This part I don't want to take as-is.

Master already double-increments i, so multi-char tokens almost never match — NaN / Inf / Infinity actually work via the Parsers fallback. The new loop matches correctly, but a prefix of the configured token at EOF still @goto invalid. Default inf is "Infinity", so:

JSON.parse("Inf"; allownan=true)    # UnexpectedEOF after this PR
JSON.parse("[Inf]"; allownan=true)  # still works (`]` → failed match → Parsers)

That's an inconsistency, and CI doesn't see it because the existing test overrides inf="Inf". Docs advertise allownan as allowing NaN, Inf, -Inf.

Also a behavior change worth calling out: untyped JSON.parse("123"; allownan=true) (and +1) will now return Int64 instead of Float64. That's the correct consistency with allownan=false.

Suggested fix

Either of these is fine:

  1. Smaller patch: don't touch @check_special. Remove the if !opts.allownan wrapper, add elseif opts.allownan; isfloat = true, and change if isfloat || opts.allownan to if isfloat. Specials stay on the Parsers path like today.

  2. Keep the new structure, but treat mid-token EOF as a failed match (then reset, which you already do) so "Inf" can fall through to Parsers as your comment intends.

Tests to add

  • the original struct-field MWE from Inexact Error when parsing using typemax(Int) and allownan=true #478
  • default "NaN" / "Infinity" / "-Infinity" and "Inf" / "-Inf" without overriding inf/ninf
  • nested: JSON.parse("[Inf,NaN,-Infinity]"; allownan=true)
  • typemin(Int64), 2^53+1, maybe typemax(UInt64)
  • untyped JSON.parse(string(typemax(Int64)); allownan=true) === typemax(Int64)

Minor: trailing spaces on the new test lines, and "see note above" has no note.

Happy to merge once the Inf prefix case is fixed and those tests are in — this is a solid first PR.

[analysis & commentary by AI; reviewed by quinnj]

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 this pull request may close these issues.

Inexact Error when parsing using typemax(Int) and allownan=true

2 participants