Repository navigation
Convert math.ldexp()'s x like any float argument and read float subclasses directly - #9520
Conversation
ldexp() accepted only a float or an int as x and raised TypeError for anything else, so an object with __float__ or __index__ was refused and an exception raised by __float__ was replaced. Bind x as ArgIntoFloat, which goes through the float protocol like the other math functions. Assisted-by: Claude Code:claude-opus-5-5
ArgIntoFloat read the value of an exact float but called __float__ on a float subclass, so a subclass overriding __float__ changed the result of math functions, struct packing, %-formatting and every other float argument. PyFloat_AsDouble, which ArgIntoFloat stands for, reads the value of any float instance and calls __float__ only for other types. Assisted-by: Claude Code:claude-opus-5-5
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Direct conversion for float instances crates/vm/src/function/number.rs, extra_tests/snippets/stdlib_math.py |
ArgIntoFloat now reads PyFloat values directly. Tests check float subclass behavior in math.sqrt and math.ldexp. |
math.ldexp argument conversion crates/stdlib/src/math.rs, extra_tests/snippets/stdlib_math.py |
math.ldexp uses ArgIntoFloat for its first argument. Tests cover __float__, __index__, conversion exceptions, and oversized integers. |
Estimated code review effort: 2 (Simple) | ~10 minutes
Change: Bug fix · Severity of issue fixed: Low
Possibly related PRs
- RustPython/RustPython#9340: Adds direct reads of
PyFloatsubclass values and conversion of other objects at a separate call site.
Suggested reviewers: youknowone
Merge Risk: 
83c31
-
No actionable merge-blocking risk is identified. The conversion changes and accompanying regression tests appear aligned with the requested behavior; merge after normal checks pass.
Pre-merge checks: 
-
Check name Status Explanation Resolution Docstring Coverage Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat… Write docstrings for the functions missing them to satisfy the coverage threshold. -
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly describes the main changes: math.ldexp()now convertsxthrough float argument protocols, and float subclasses are read directly. It is specific and related to the changeset.Linked Issues check Issue #9508has one directly linked, open issue. Its coding requirements are:
1. math.ldexp(x, i) must convert x through float or index (the issue cites CPython PyFloat_AsDouble behavior).
2. …Out of Scope Changes check The whole-PR diff touches three files:
- crates/stdlib/src/math.rs: the ldexp() binding and body change required by#9508. The removal of the now-unused PyRef and Either imports follows directly from …Full details: Linked Issues check
-
Explanation
Issue
#9508has one directly linked, open issue. Its coding requirements are:- math.ldexp(x, i) must convert x through float or index (the issue cites CPython PyFloat_AsDouble behavior).
- Exceptions raised by float must propagate.
- An int too large for a float must raise OverflowError.
- A float instance, including a subclass that overrides float, must be read directly.
Implementation at head 83c315e (diff against merge base 713e14c):
- crates/stdlib/src/math.rs: LdexpArgs.x changes from Either<PyRef<PyFloat>, PyIntRef> to ArgIntoFloat. ldexp() calls x.into_float() and passes the result to ldexp_bigint. The try_bigint_to_f64 branch is removed. This covers requirements 1 to 3 through the conversion path.
- crates/vm/src/function/number.rs: ArgIntoFloat::try_from_object first downcasts to PyFloat and returns to_f64() of the stored value. Only non-float objects reach try_float. This covers requirement 4.
- Requirements 1 to 3 depend on PyObject::try_float_opt in crates/vm/src/protocol/number.rs (read at head). For a non-exact float it calls number().float (float) first, then falls back to try_index_opt (index), then int::try_to_float for the int case. This matches the issue's requested order and error behavior.
Tests: the PR extends extra_tests/snippets/stdlib_math.py with cases for float and index mantissas, a raising float, an oversized int, and a float subclass passed to sqrt() and ldexp(). I did not read the snippet file directly, and I did not run tests. The test coverage claim rests on the PR summary and author report. The issue does not require tests as a separate deliverable, so this does not change the verdict.
All four coding requirements have an implementation in the reviewed head. No requirement is unmet.
Full details: Out of Scope Changes check
-
Explanation
The whole-PR diff touches three files:
- crates/stdlib/src/math.rs: the ldexp() binding and body change required by
#9508. The removal of the now-unused PyRef and Either imports follows directly from that change. - crates/vm/src/function/number.rs: the float-subclass fast path in ArgIntoFloat::try_from_object, which
#9508explicitly requests. This change also affects other ArgIntoFloat consumers (for example sqrt, frexp, %-formatting, struct.pack). Those effects follow the CPython PyFloat_AsDouble semantics that the issue cites, so they are part of the requested behavior and not unrelated changes. - extra_tests/snippets/stdlib_math.py: tests for the new ldexp() behavior and the float-subclass behavior. These support the objectives.
No unrelated functional change is visible in the diff.
- crates/stdlib/src/math.rs: the ldexp() binding and body change required by
-
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 1 functions across 3 files.
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
-
Finishing Touches
-
Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 83c315ed2f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| raise RuntimeError | ||
|
|
||
|
|
||
| assert math.ldexp(LdexpFloat(), 1) == 3.0 |
There was a problem hiding this comment.
This regression block adds new test logic and assertions to an existing test file, whereas the root AGENTS.md explicitly limits test-file modifications to adding/removing @unittest.expectedFailure decorators and removing associated TODO comments. Revert these additions or obtain an explicit exception before including them, since they fall outside the permitted test-change scope.
AGENTS.md reference: AGENTS.md:L280-L282
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
That section covers the CPython tests in Lib/test. For extra_tests/snippets, AGENTS.md asks for new tests in the most closely related existing file (L119-L120), which is what this does. None of these cases is in Lib/test/test_math.py: testLdexp only passes int and float mantissas.
math.ldexp()rejects anxthat defines__float__or__index__#9508Summary
LdexpArgsbindsxasEither<PyRef<PyFloat>, PyIntRef>, so anything other than afloator anintraisesTypeError: unexpected type, including objects with__float__or__index__, and an exception raised by__float__is replaced. CPython'smath_ldexpconvertsxwithPyFloat_AsDouble, as the othermathfunctions do, and RustPython's equivalent isArgIntoFloat.Binding
xasArgIntoFloatexposes a defectArgIntoFloatalready has:try_from_objectreads an exactfloatdirectly but calls__float__on afloatsubclass, whilePyFloat_AsDoublereads the stored value of anyfloatinstance. So a subclass that overrides__float__already changes the result ofmath.sqrt(),math.frexp(),%-formatting,struct.pack()and every otherArgIntoFloatargument, andldexp()would join them. This PR fixes that too.AS-IS
TO-BE
(matching CPython)
Changes
ldexp()'sxasArgIntoFloatincrates/stdlib/src/math.rs, asfrexp()does. ItsOverflowErrorfor aninttoo large for a float replaces thetry_bigint_to_f64branch.crates/vm/src/function/number.rs,ArgIntoFloat::try_from_objectreads anyfloatinstance, subclasses included, before falling back totry_float.extra_tests/snippets/stdlib_math.pywith__float__and__index__mantissas forldexp(), a raising__float__, aninttoo large for a float, and afloatsubclass overriding__float__passed tosqrt()andldexp().Test plan
Built and run on macOS aarch64 against upstream/main 713e14c, using the CI feature set
--no-default-features --features stdlib,importlib,stdio,encodings,sqlite,ssl-rustls-aws-lc,host_env,doc,ucd_3_2_0, plusthreading, compared with CPython 3.15.2.extra_tests/snippets/stdlib_math.pyfails on an unpatched build of that commit (at the__float__mantissa) and passes with the patch and under CPython 3.15.2.rustpython -m testsuite gives the same result with and without the patch, apart from onetest_sqlite3failure that did not reproduce when the module was run again on its own: 47,019 tests run, andtest_cmd_linefails identically on the unpatched build.rustpython -m test test_math test_float test_cmath test_struct test_array test_format test_statistics test_str: 1,878 run, 86 skipped, all pass.pytest -k rustpythoninextra_tests: 248 pass.cargo fmt --checkclean,cargo clippy -p rustpython-vm -- -Dwarningsandcargo clippy -p rustpython-stdlib -- -Dwarningsreport nothing, and the pre-commit hooks pass on the three changed files.I used Claude Code (claude-opus-5-5) to draft the analysis, the patch and this description; I reviewed the change and ran every command above myself.
Summary by CodeRabbit
math.ldexpnow accepts values convertible to floats, including objects that provide__float__or__index__.math.ldexpandmath.sqrtuse the stored value of float subclasses, even when they override__float__.math.ldexpraiseOverflowError.