Honor sys.int_max_str_digits in native json.loads#8087
Conversation
📝 WalkthroughWalkthroughReplaces JSON integer parsing to use bytes_to_int with the VM's int_max_str_digits limit and adds a stdlib test ensuring json.loads rejects oversized integer literals while accepting threshold-length and small integers. ChangesJSON integer size limit regression test and implementation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Hi, thanks for the PR!
Can you please read our AI policy, and check the box you've read it in your PR comment (I've edited your comment)
Rust fix required but cannot be provided without original file content.
Not sure what you mean by that, you can create your own MRE and test it against that. ATM we cannot merge your test as it would only make the CI fail, it doesn't fix anything unless I'm missing something
| import sys | ||
| _orig_limit = sys.get_int_max_str_digits() | ||
| try: | ||
| sys.set_int_max_str_digits(100) |
There was a problem hiding this comment.
Also, this wouldn't work as the minimum value for it is 600
Route native _json integer parsing through bytes_to_int with the live VM digit limit, matching int() and the pure-Python JSON path. Add snippet coverage for the boundary. Assisted-by: OpenAI GPT-5:gpt-5
Federicorao
force-pushed
the
codex/issue-8051-1781265710
branch
from
0902b83 to
16679d3
Compare
Federicorao
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@extra_tests/snippets/stdlib_json.py`:
- Line 266: The import statement "import sys" is located after top-level code
causing Ruff E402; remove the "import sys" from its current spot and add it to
the module's top-level import block alongside other stdlib imports (i.e., the
file's initial import section) so that "sys" is imported before any non-import
top-level statements.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 5e75a5c2-6d28-44aa-adec-79c58ec2a330
📒 Files selected for processing (2)
crates/stdlib/src/json.rsextra_tests/snippets/stdlib_json.py
| raise AssertionError("expected JSONDecodeError") | ||
|
|
||
| # Test that json.loads honors sys.int_max_str_digits | ||
| import sys |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
python - <<'PY'
import ast
from pathlib import Path
path = Path("extra_tests/snippets/stdlib_json.py")
tree = ast.parse(path.read_text(), filename=str(path))
seen_non_import = False
late_imports = []
for node in tree.body:
if (
isinstance(node, ast.Expr)
and isinstance(getattr(node, "value", None), ast.Constant)
and isinstance(node.value.value, str)
and not seen_non_import
):
continue
if isinstance(node, (ast.Import, ast.ImportFrom)):
if seen_non_import:
late_imports.append(node.lineno)
else:
seen_non_import = True
print({"late_import_lines": late_imports})
PYRepository: RustPython/RustPython
Length of output: 94
Move import sys into the top-level import section (Ruff E402)
extra_tests/snippets/stdlib_json.py has import sys on line 266, after non-import top-level statements; this layout can trigger Ruff E402. Move import sys up into the module’s import block.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@extra_tests/snippets/stdlib_json.py` at line 266, The import statement
"import sys" is located after top-level code causing Ruff E402; remove the
"import sys" from its current spot and add it to the module's top-level import
block alongside other stdlib imports (i.e., the file's initial import section)
so that "sys" is imported before any non-import top-level statements.
Source: Coding guidelines
|
I believe you can use the following command to test your changes locally: # this runs all of the tests (not necessary if you know which test is affected)
cargo run --release --features encodings,sqlite -- -m test -j 1 -u all --slowest --fail-env-changed -v
# this runs only the test suite named "test_whatever" (usually located at `Lib/test/test_whatever.py`)
cargo run --release --features encodings,sqlite -- -m test -j 1 -u all --slowest --fail-env-changed -v test_whateverFor a list of all resources and more command-line options, you can execute |
3 participants
Refs #8051.
Routes the native
_jsonscanner integer path throughrustpython_common::int::bytes_to_intwith the livevm.state.int_max_str_digitsvalue, matching theint()conversion path and the pure-Python JSON decoder. Adds snippet coverage for the boundary where an integer with exactly the configured digit limit parses, while one digit over the limit raisesValueError.Local verification:
python3.14 -m py_compile extra_tests/snippets/stdlib_json.pypython3.14 extra_tests/snippets/stdlib_json.pyCould not run local Rust checks because
cargois not installed in this environment; CI will validate the Rust crate build and formatting.AI assistance disclosure: OpenAI GPT-5 was used to inspect the issue and review feedback, draft the patch, and run local verification; the changes are disclosed in the commit with an
Assisted-bytrailer.Summary by CodeRabbit
Bug Fixes
Tests