GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — Convert `math.ldexp()`'s `x` like any float argument and read float subclasses directly by jseop-lim · Pull Request #9520 · RustPython/RustPython

Conversation

jseop-lim commented Nov 1, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Contributor

Closes math.ldexp() rejects an x that defines __float__ or __index__ #9508
This PR follows our AI policy

Summary

LdexpArgs binds x as Either<PyRef<PyFloat>, PyIntRef>, so anything other than a float or an int raises TypeError: unexpected type, including objects with __float__ or __index__, and an exception raised by __float__ is replaced. CPython's math_ldexp converts x with PyFloat_AsDouble, as the other math functions do, and RustPython's equivalent is ArgIntoFloat.

Binding x as ArgIntoFloat exposes a defect ArgIntoFloat already has: try_from_object reads an exact float directly but calls __float__ on a float subclass, while PyFloat_AsDouble reads the stored value of any float instance. So a subclass that overrides __float__ already changes the result of math.sqrt(), math.frexp(), %-formatting, struct.pack() and every other ArgIntoFloat argument, and ldexp() would join them. This PR fixes that too.

AS-IS

>>> import math
>>> math.ldexp(F(), 1)  # __float__ returns 1.5
TypeError: unexpected type F
>>> math.ldexp(RaisingFloat(), 1)  # __float__ raises RuntimeError
TypeError: unexpected type RaisingFloat
>>> class FloatSubclass(float):
...     def __float__(self): return 9.0
...
>>> math.sqrt(FloatSubclass(4.0))
3.0
>>> '%.1f' % FloatSubclass(1.0)
'9.0'

TO-BE

>>> math.ldexp(F(), 1)
3.0
>>> math.ldexp(RaisingFloat(), 1)
RuntimeError
>>> math.sqrt(FloatSubclass(4.0))
2.0
>>> '%.1f' % FloatSubclass(1.0)
'1.0'

(matching CPython)

Changes

Bind ldexp()'s x as ArgIntoFloat in crates/stdlib/src/math.rs, as frexp() does. Its OverflowError for an int too large for a float replaces the try_bigint_to_f64 branch.
In crates/vm/src/function/number.rs, ArgIntoFloat::try_from_object reads any float instance, subclasses included, before falling back to try_float.
Extend extra_tests/snippets/stdlib_math.py with __float__ and __index__ mantissas for ldexp(), a raising __float__, an int too large for a float, and a float subclass overriding __float__ passed to sqrt() and ldexp().

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, plus threading, compared with CPython 3.15.2.

The extended extra_tests/snippets/stdlib_math.py fails on an unpatched build of that commit (at the __float__ mantissa) and passes with the patch and under CPython 3.15.2.
The full rustpython -m test suite gives the same result with and without the patch, apart from one test_sqlite3 failure that did not reproduce when the module was run again on its own: 47,019 tests run, and test_cmd_line fails 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 rustpython in extra_tests: 248 pass.
cargo fmt --check clean, cargo clippy -p rustpython-vm -- -Dwarnings and cargo clippy -p rustpython-stdlib -- -Dwarnings report 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

New Features
- math.ldexp now accepts values convertible to floats, including objects that provide __float__ or __index__.
Bug Fixes
- math.ldexp and math.sqrt use the stored value of float subclasses, even when they override __float__.
- Exceptions raised during float conversion are preserved, and oversized integer inputs to math.ldexp raise OverflowError.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

jseop-lim added 2 commits

November 1, 2026 13:37


          Convert the math.ldexp() mantissa like any float argument

315dbb7

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


          Read a float subclass directly in ArgIntoFloat

83c315e

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

chatgpt-codex-connector Bot commented Nov 1, 2026 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-11-01T05:38:40.672913Z	`83c315e`	PR opened

ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

coderabbitai Bot commented Nov 1, 2026 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Contributor

No actionable comments were generated in the recent review.

Recent review info

Run configuration

Source mode: Auto
Repository: RustPython/RustPython
Branch: main
File: .coderabbit.yml
Commit: 88f3733
Layers: Repository YAML
Review profile: CHILL
Plan: Advanced
Run ID: 45044a69-1331-48d4-a7ea-7eea22edd16d

Commits

Reviewing files that changed from the base of the PR and between 88f3733 and 83c315e.

Files selected for processing: 3

crates/stdlib/src/math.rs
crates/vm/src/function/number.rs
extra_tests/snippets/stdlib_math.py

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 7 remain after this review.

Walkthrough

The change routes math.ldexp's first argument through ArgIntoFloat. ArgIntoFloat reads PyFloat values directly and uses the existing float conversion for other objects. Tests cover __float__, __index__, conversion exceptions, oversized integers, and float subclasses.

Changes: math.ldexp float conversion

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 PyFloat subclass values and conversion of other objects at a separate call site.

Suggested reviewers: youknowone

Merge Risk: Minimal · up to 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: 4 · 1

Failed checks: 1 warning

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.

Passed checks: 4 passed

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 converts `x` through float argument protocols, and float subclasses are read directly. It is specific and related to the changeset.
Linked Issues check		Issue `#9508` has 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 #9508 has 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 #9508 explicitly 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.

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.

Share

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

chatgpt-codex-connector Bot reviewed

Nov 1, 2026

View reviewed changes

chatgpt-codex-connector Bot left a comment

Copy link

Copy Markdown

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

💡 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".

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

extra_tests/snippets/stdlib_math.py

		raise RuntimeError


		assert math.ldexp(LdexpFloat(), 1) == 3.0

chatgpt-codex-connector Bot Nov 1, 2026

Copy link

Copy Markdown

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Honor the restriction on modifying existing test files

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 👍 / 👎.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

jseop-lim Nov 1, 2026

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

youknowone approved these changes

Nov 1, 2026

View reviewed changes

youknowone left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

👍

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

youknowone merged commit a444ffb into RustPython:main

Nov 1, 2026

20 checks passed

Uh oh!

There was an error while loading. Please reload this page.

jseop-lim deleted the fix/math-ldexp-float-protocol branch

November 1, 2026 16:25

This was referenced Nov 3, 2026

Fix integer-to-float overflow rounding boundary #9584

Merged

Honor scalar conversion protocols across SSL backends #9653

Merged

Align numeric conversion and rounding boundaries #9697

Merged

Reviewers

chatgpt-codex-connector[bot]

chatgpt-codex-connector[bot] left review comments

youknowone

youknowone approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

math.ldexp() rejects an x that defines __float__ or __index__

Uh oh!

There was an error while loading. Please reload this page.

2 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

GeezerVison

Conversation

jseop-lim commented Nov 1, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

AS-IS

TO-BE

Changes

Test plan

Summary by CodeRabbit

Uh oh!

chatgpt-codex-connector Bot commented Nov 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codex Review Summary

Uh oh!

coderabbitai Bot commented Nov 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector Bot Nov 1, 2026

Choose a reason for hiding this comment

Uh oh!

jseop-lim Nov 1, 2026

Choose a reason for hiding this comment

Uh oh!

youknowone left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

jseop-lim commented Nov 1, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

There was an error while loading. Please reload this page.

chatgpt-codex-connector Bot commented Nov 1, 2026 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

coderabbitai Bot commented Nov 1, 2026 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.