Skip to content

gh-155526: correct errno handling in complex_abs() #155527

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
skirpichev wants to merge 7 commits into
base: main
Choose a base branch
from
+37 −8
Open

gh-155526: correct errno handling in complex_abs() #155527

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
5aa9e73
gh-155526: correct errno handling in complex_abs()
skirpichev Aug 11, 2026
9702894
Merge branch 'master' into complex-abs-errno/155526
skirpichev Aug 11, 2026
576f6cd
address review: restore errno + readability
skirpichev Aug 11, 2026
31d9fc3
Merge branch 'master' into complex-abs-errno/155526
skirpichev Aug 27, 2026
3b851ac
+ test special components for complex_abs()
skirpichev Aug 27, 2026
c4f365e
Merge branch 'master' into complex-abs-errno/155526
skirpichev Aug 31, 2026
fde8fc7
+ don't use errno in complex_abs()
skirpichev Aug 31, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 25 additions & 0 deletions Lib/test/test_complex.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
import errno
import unittest
import sys
from test import support
from test.support import import_helper
from test.support.testcase import ComplexesAreIdenticalMixin
from test.support.numbers import (
VALID_UNDERSCORE_LITERALS,
Expand All @@ -9,6 +11,7 @@

from random import random
from math import isnan, copysign
import cmath
import operator

INF = float("inf")
Expand Down Expand Up @@ -789,8 +792,30 @@ def test_abs(self):
for num in nums:
self.assertAlmostEqual((num.real**2 + num.imag**2) ** 0.5, abs(num))

for x in 0.0, -0.0, INF, -INF, NAN:
for y in 0.0, -0.0, INF, -INF, NAN:
with self.subTest(x=x, y=y):
z = complex(x, y)
r = abs(z)
if cmath.isfinite(z):
self.assertFloatsAreIdentical(r, 0.0)
elif cmath.isinf(z):
self.assertEqual(r, INF)
else:
self.assertTrue(cmath.isnan(z))
self.assertTrue(isnan(r))

self.assertRaises(OverflowError, abs, complex(DBL_MAX, DBL_MAX))

def test_abs_errno_handling(self):
_testcapi = import_helper.import_module('_testcapi')
z = complex('nan')
_testcapi.set_errno(errno.ERANGE)
try:
self.assertTrue(isnan(abs(z)))
finally:
_testcapi.set_errno(0)

def test_repr_str(self):
def test(v, expected, test_fn=self.assertEqual):
test_fn(repr(v), expected)
Expand Down
2 changes: 2 additions & 0 deletions Misc/NEWS.d/next/Core_and_Builtins/2026-08-11-06-04-26.gh-issue-155526.W7ZHXu.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Correct ``errno`` handling in ``abs(complex)``. Patch by Sergey B
Kirpichev.
18 changes: 10 additions & 8 deletions Objects/complexobject.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -367,7 +367,7 @@ c_powi(Py_complex x, long n)
double
_Py_c_abs(Py_complex z)
{
/* sets errno = ERANGE on overflow; otherwise errno = 0 */
/* sets errno = ERANGE on overflow */

@vstinner vstinner Aug 31, 2026

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.

No longer setting errno to 0 sounds risky. With this change, multiple cmath function now depends on the current errno value: polar() and isclose().

Since _Py_c_abs() is our custom API, why not change its API to report the error, rather than relying on the global variable errno?

For example, change the API to int _Py_c_abs(Py_complex z, double *result): set *result and return 0 on success, return -1 on error.

@hpkfft hpkfft Aug 31, 2026

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.

Unfortunately, _Py_c_abs() is documented.
An external API should not set errno to zero when the function succeeds. (So, I don't think we should change the documentation to say it does. We should change the implementation.)

The function polar() sets errno = 0 before calling _Py_c_abs(z), so it's OK.
The function isclose() does not read errno, so it's OK.

I suggest: https://github.com/hpkfft/cpython/blob/erange/Objects/complexobject.c#L380-L417
This keeps the documented API, but adds a new function c_abs() for internal use.
If c_abs() is useful in cmathmodule.c, maybe it needs a better name (and, of course, cannot be static).
This can be done as part of #156145

@vstinner vstinner Aug 31, 2026

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.

Oh, I forgot that _Py_c_abs() is part of the public C API (but is private).

@skirpichev skirpichev Aug 31, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

No longer setting errno to 0 sounds risky.

This is not a part of the documentation and looks as a mistake. Note that other private complex C-API functions don't do this. This also corresponds to libc behavior. The C standard says:

The value of errno in the initial thread is zero at program startup (the initial representation of the object designated by errno in other threads is indeterminate), but is never set to zero by any library function.

and

Thus, a program that uses errno for error checking would set it to zero before a library function call, then inspect it before a subsequent library function call.

I think that rare C-API users adopt above pattern, like we do in polar().

Though, I'm fine with reversion of that part if you aren't OK with arguments above.

If c_abs() is useful in cmathmodule.c, maybe it needs a better name (and, of course, cannot be static).

Yes, it seems that errno-free helpers shows some speedup in simple tests (5-10%). But if we decide to change internal API functions in this way, lets do that more systematically, not just for one function.

double result;

if (!isfinite(z.real) || !isfinite(z.imag)) {
Expand All @@ -376,23 +376,20 @@ _Py_c_abs(Py_complex z)
NaN. */
if (isinf(z.real)) {
result = fabs(z.real);
errno = 0;
return result;
}
if (isinf(z.imag)) {
result = fabs(z.imag);
errno = 0;
return result;
}
/* either the real or imaginary part is a NaN,
and neither is infinite. Result should be NaN. */
return Py_NAN;
}
result = hypot(z.real, z.imag);
if (!isfinite(result))
if (!isfinite(result)) {
errno = ERANGE;
else
errno = 0;
}
return result;
}

Expand Down Expand Up @@ -796,8 +793,13 @@ static PyObject *
complex_abs(PyObject *op)
{
PyComplexObject *v = _PyComplexObject_CAST(op);
double result = _Py_c_abs(v->cval);
if (errno == ERANGE) {
double result;

result = hypot(v->cval.real, v->cval.imag);
/* Testing FE_OVERFLOW floating-point exception is slow. */

@vstinner vstinner Aug 31, 2026

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.

Can you mention in the comment that errno is not used on purpose, maybe with a reference to gh-155526?

@skirpichev skirpichev Aug 31, 2026 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

How about following?

We have only one other place, where errno tested for libm's functions. That one removed in #156694.

If that PR will be merged, we can add a generic commentary to this file: that we don't test errno values, coming from the library functions.

if (isfinite(v->cval.real) && isfinite(v->cval.imag)
&& !isfinite(result))
{
PyErr_SetString(PyExc_OverflowError,
"absolute value too large");
return NULL;
Expand Down
Loading