Skip to content

gh-161926: Honor overridden __float__ in complex() - #161949

Open
losewayy wants to merge 1 commit into
python:mainfrom
losewayy:fix-gh-161926-complex-float
Open

losewayy wants to merge 1 commit into
python:mainfrom
losewayy:fix-gh-161926-complex-float

Conversation

@losewayy

@losewayy losewayy commented Nov 13, 2026 •

Copy link
Copy Markdown

Summary

The single-positional-argument form of complex() has bypassed an overridden __float__ on float subclasses since 3.14, returning the stored float value and suppressing exceptions raised by the override. Convert through PyNumber_Float(), as the keyword and two-argument forms already do.

Add regression coverage for the converted value and exception propagation, including construction of a complex subclass, and a NEWS entry.

Fixes #161926.

Testing

Built and tested on Windows x64 in Debug mode:

PCbuild\build.bat -c Debug /m:8
PCbuild\amd64\python_d.exe -m test test_complex -m '*test_constructor_float_subclass*' -v
PCbuild\amd64\python_d.exe -m test test_complex test_capi.test_complex test_cmath test_float test_builtin -j 4
PCbuild\amd64\python_d.exe -m test test_complex test_capi.test_complex -R 3:3
PCbuild\amd64\python_d.exe Tools\patchcheck\patchcheck.py
  • Before: On main at 2c2d8018a20da33c749f42cc6a4ae55664a88321, the two new regression tests produce four failures: the overridden value is ignored and the expected exception is absent for both constructor classes.
  • After: A Windows x64 Debug build succeeds with no warnings or errors. test_complex, test_capi.test_complex, test_cmath, test_float, and test_builtin pass, with 306 tests run and 9 skipped. test_complex and test_capi.test_complex also pass -R 3:3 reference-leak checks. Patchcheck, applicable pre-commit hooks, and git diff --check pass.

The default full suite (PCbuild\amd64\python_d.exe -m test -j 4 --timeout 180 --slowest) ran 52,079 tests with two failures, both also reproduced on an unmodified build of the same base commit. test_device_encoding depends on the console handle and passes when the whole test_os.test_os file is rerun in a terminal. test_wrong_cert_tls13 can receive a localized Windows connection-reset message that does not match its English-only assertion; the complete test_ssl file has the same failure on the base build.

@python-cla-bot

python-cla-bot Bot commented Nov 13, 2026 •

Copy link
Copy Markdown

All commit authors signed the Contributor License Agreement.

CLA signed

@chatgpt-codex-connector

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

Copy link
Copy Markdown
Codex Review Summary: ✅ Completed

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

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-11-13T18:42:36.798702Z f4c00d4 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.

@eendebakpt

Copy link
Copy Markdown
Contributor

With this PR PyComplex_AsCComplex is unchanged, so cmath.sqrt(FloatSub()) will ignore the override. Also note that PyFloat_AsDouble (https://docs.python.org/3/c-api/float.html#c.PyFloat_AsDouble) has the same behavior: for subclasses of float, the __float__ is not called.

I am not sure this is something that needs to be fixed. @skirpichev

@skirpichev
skirpichev self-requested a review November 13, 2026 21:36
@skirpichev

skirpichev commented Nov 13, 2026 •

Copy link
Copy Markdown
Member

I am not sure this is something that needs to be fixed.

Maybe there is, see issue thread. But wrt current changeset - I would prefer rather to keep existing behavior.

@skirpichev skirpichev left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think this will introduce speed regression on handling float subclasses with no override for __float__() dunder. Can you benchmark that?

@losewayy

Copy link
Copy Markdown
Author

You were right about the original patch: calling PyNumber_Float on a normal float subclass creates a temporary float and made it ~5% slower.

I tested a narrower fast path locally (keep PyFloat_AsDouble for non-float, read PyFloat_AS_DOUBLE directly when nb_float is unchanged, and only call PyNumber_Float when a float subclass overrides it). Windows x64 MSVC Release (non-PGO, pyperf on a pinned core, 16 processes / 80 values per case, median of per-process medians):

Input Baseline Local fast path Change
float 76.67 ns 75.78 ns -1.16%
float subclass (default __float__) 77.54 ns 76.75 ns -1.01%
__index__-only object 127.96 ns 131.96 ns +3.13%
float subclass, complex(real=x) 111.07 ns 115.99 ns +4.43%

Normal float subclasses are back at baseline here (there is still an unexplained ~3–4% diff on __index__-only and the untouched complex(real=x) path in these non-PGO builds).

If you're open to restoring the pre-3.14 complex(x) behavior with this fast path, I can push the update; otherwise, happy to close this if you'd rather keep 3.14's behavior.

@skirpichev

Copy link
Copy Markdown
Member

I don't think that this pr needs any changes. Issue must be discussed first.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

complex(x) ignores an overridden __float__ of a float subclass since 3.14

3 participants