Skip to content

fix(json2): keep numeric conversion independent of host locale - #7215

Open
simonbronk wants to merge 1 commit into
boykopovar:mainfrom
simonbronk:codex/json-numeric-locale
Open

simonbronk wants to merge 1 commit into
boykopovar:mainfrom
simonbronk:codex/json-numeric-locale

Conversation

@simonbronk

Copy link
Copy Markdown
Contributor

What

With a decimal-comma host LC_NUMERIC, Json2 emits invalid JSON (1.5 becomes 1,5) and silently reads JSON 1.5 as 1. In the regression, {"value":1.5,"list":[2.5,-0.125]} becomes {"value":1,"list":[2,-0]} after parsing and serializing.

Use to_chars with the existing 17 significant digits for output and from_chars for input. On an out-of-range input only, use a stream explicitly imbued with the classic locale to retain the prior underflow-to-signed-zero behavior and overflow exception. Do not change the process locale. The numeric grammar still rejects a decimal comma.

RFC 8259 section 6 requires . as the decimal point. The C++ numeric conversion specification specifies C-locale formatting. Console rounding and formatting remain unmeasured and are recorded in Json2 debt. No title unblock, percentage increase or performance gain is claimed.

Tested

Windows, WinLibs MinGW-w64 GCC 15.2.0r7 UCRT, Release, actual shared libSceJson2.prx and its CMake test executable; base 66702f1cffd3646f2d674ed093fde0a85a70761f:

  • Original production with the final regression: existing guest_json passes, new guest_json_numeric_locale fails. The output records invalid decimal commas, truncated input values, and the corrupted nested document above.
  • Fixed tree aa92ad8072ff2efc82851c64fc6c95c6af31bded: both CTests pass, no skips (2/2, 0.43 s). The comma locale selected is de_DE.UTF-8.
  • Covers fractions, exponents, maximum and minimum normal doubles, the smallest subnormal, rounding on each side of half that subnormal, integer input above uint64, positive/negative underflow to zero, nested numeric values, invalid decimal commas, overflow rejection and preservation of the host locale.
  • The locale test returns 77 (CTest skip) if no tested decimal-comma locale is installed; it did run on this Windows host.
  • check_conventions.py against the pinned base and git diff --check pass for the final committed change.
  • Built the relevant shared runtime library and test target. The full all-target build and full CTest suite were not completed. No Linux/macOS, game, console or GPU run.
  • pr_overlap.py cannot execute because gh is unavailable; live connector searches found no open Json2 numeric-locale fix. feat(libSceJson2): implement Value::setTypeMismatchHandler and store the element access failure and special float handlers #6126 adds callbacks (including special-float handling), fix(json2): release partial copies on allocation failure #6593 handles allocation failures, and fix: complete compatibility gaps #6447 is a broader compatibility batch; those address different behavior but may require textual reconciliation.

AI assistance: OpenAI Codex. Evidence is attached below.

Checklist

  • Based on current main; no other open PR implements the same functions
  • One topic per PR; follow-ups go in a new PR
  • No comments in code except technical debt
  • Unimplemented paths throw (NotImplemented_nid_no_patch); silent stubs are listed under Silent stubs in their library's technical debt file
  • No notes, investigation .md files or images added to the repository (attach them to this PR)
  • New third-party code is a submodule built from source
  • Generic behaviour, not specific to one title
  • Depends on: none
  • Fixes: none
  • Unblocks: none
  • GPUs tested: n/a
  • AI-assisted: yes
    json-numeric-locale-evidence-20261017.zip

Prevent decimal-comma LC_NUMERIC from emitting invalid JSON and silently truncating decimal-point input. Keep 17-digit output, signed underflow and overflow rejection without changing the process locale.

AI-assisted: yes (OpenAI Codex). Windows WinLibs GCC15.2 Release: original shared Json2 fails the final locale regression; fixed guest_json and guest_json_numeric_locale pass (2/2, no skips). Final conventions pass. Only relevant runtime/test targets built; full build/full suite and game/console/Linux/macOS behavior unverified. No performance or coverage increase claimed.
@oneandonlydean

Copy link
Copy Markdown
Collaborator

Full CI on 8706b82 (run 39052899521): every test passes on both lanes, and guest_json_numeric_locale runs and passes on Windows. The Linux job still fails at tools/check_skips.py:

error: guest_json_numeric_locale skipped
1 test(s) skipped for a reason that is not expected: the job lost that coverage and ctest still reports every test as passed

The Linux runner has no decimal-comma locale, so the test exits 77 with "No decimal-comma numeric locale is installed". On Linux, the guard only accepts skips that report a device limitation. Could you make the test cover Linux without an installed locale? For example, build the numeric facet in the test, or set LC_NUMERIC from a locale the test generates. If that is not practical, only register the case where it can run. We will re-run full CI on the new head.

@github-actions

github-actions Bot commented Oct 17, 2026 •

Copy link
Copy Markdown

Some comparisons could not be completed because a pull request conflicts with the base branch. Review these shared files manually before concluding that there is no overlap.

Pull request Shared files not compared
PR 6039 docs/dev/debt/libSceJson2.md
PR 6044 core/libs/prx/libSceJson2/CMakeLists.txt
core/libs/tests/GuestJson.cpp
PR 6593 core/libs/prx/libSceJson2/CMakeLists.txt
core/libs/prx/libSceJson2/Export.cpp
PR 6964 docs/dev/debt/libSceJson2.md
PR 7477 core/libs/prx/libSceJson2/Export.cpp
core/libs/tests/GuestJson.cpp
docs/dev/debt/libSceJson2.md
PR 7658 core/libs/prx/libSceJson2/Export.cpp
core/libs/tests/GuestJson.cpp
docs/dev/debt/libSceJson2.md

@MarcZX10

Copy link
Copy Markdown
Contributor

I checked the Linux skip path in NumericLocale(): it needs an installed comma-decimal locale only to change the process LC_NUMERIC, which makes the serializer/parser regression locale-sensitive. A portable fix is to expose the conversion logic through a small internal helper that takes an explicit locale (or use a test-only locale facet for the legacy host conversion), then assert the JSON path always uses . while the host locale remains unchanged. If the goal is specifically exercising setlocale, the Linux test can generate/install a locale in CI, but that adds runner setup; conditionally unregistering the test would lose Linux coverage. I have not modified this PR. The check_skips.py report on the current head is a real required-CI failure, so please update the test and request a fresh full CI run.

@jotitadev jotitadev left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Linux run of guest_json_numeric_locale with a decimal-comma locale, since the Linux CI job skips it (no such locale installed) and the description has no Linux run.

Machine: Linux x86_64, i7-8700, Ubuntu 26.04, GCC 15.2, Release, BUILD_TESTING=ON, full build.
Head 8706b8258 merged onto main 5de7a46e0 (clean merge).

The host has only C, C.utf8 and en_US.utf8. I generated de_DE.UTF-8 into a user directory without root (localedef -i de_DE -f UTF-8 <dir>/de_DE.UTF-8) and pointed glibc at it with LOCPATH=<dir>.

  • With the PR and LOCPATH set: guest_json and guest_json_numeric_locale pass, 5 of 5 runs. The test reports JSON numeric locale: de_DE.UTF-8.
  • With the PR and no LOCPATH: guest_json_numeric_locale exits 77 and CTest shows it as skipped, as in CI.
  • Test file and its registration only, on main, with LOCPATH set: guest_json_numeric_locale fails with the bug described above, e.g. Decimal-point serialization failed: 1,5, Decimal-point parsing failed: 1.5 and Nested numeric serialization failed: {"value":1,"list":[2,-0]}. So the test covers the change on Linux too.
  • Non-GPU suite (ctest -E "^agc|gpu|vulkan|video_out", no LOCPATH): 698/698, the 697 tests of main plus guest_json_numeric_locale counted as passed with its skip.

On the CI skip: on glibc a test-local locale works without installing one system-wide, as long as localedef and the locale sources (/usr/share/i18n/locales) are present. I did not check whether the GitHub runner image has them.

Written with AI assistance (Claude Code). I ran the builds and tests above on my machine.

@MarcZX10

Copy link
Copy Markdown
Contributor

Thanks for running this with a generated de_DE.UTF-8 locale and checking the negative control on main. That confirms the regression on Linux when the locale is available, while the current CI skip is the expected no-locale path. I’ve updated my earlier review concern; the current required checks are green.

@oneandonlydean oneandonlydean self-assigned this Oct 18, 2026
@TheTerr4

Copy link
Copy Markdown

Windows runtime check. Windows 11 Pro 10.0.26200, Ryzen 7 7800X3D, WinLibs GCC 15.2.0 r7 (UCRT), Release.

This PR's head 8706b8258 merged cleanly into main 385ed71f2; the full build and libs both exit 0.

  • guest_json and guest_json_numeric_locale (the locale test ran under de_DE.UTF-8, so it was not skipped) pass, three repeated -j1 runs.
  • Negative control: with libSceJson2/Export.cpp restored to main's version, guest_json_numeric_locale fails; guest_json still passes.
  • Not run: the full suite, Linux, macOS.

AI-assisted: yes (Claude Code).

@MarcZX10

Copy link
Copy Markdown
Contributor

Thanks for the Linux locale run and negative control. Generating de_DE.UTF-8 in a user directory confirms the test can exercise decimal-comma behavior without a system-wide locale install, and the restored-main failure shows it catches the bug. The Linux full-build subset also passed; platform limits are clear in your report.

@github-actions github-actions Bot added the needs rebase Conflicts with main, rebase needed label Oct 19, 2026

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

build docs needs rebase Conflicts with main, rebase needed prx

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants