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 10 commits into
base: main
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 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
5fa214f
ignore errno from atan2 in cmath_polar_impl()
skirpichev Sep 5, 2026
23d948e
Merge branch 'master' into complex-abs-errno/155526
skirpichev Sep 5, 2026
28ba074
restore errno on success
skirpichev Sep 6, 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
39 changes: 27 additions & 12 deletions Lib/test/test_capi/test_complex.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -281,18 +281,33 @@ def test_py_c_abs(self):
# Test _Py_c_abs()
_py_c_abs = _testcapi._py_c_abs

self.assertEqual(_py_c_abs(-1), (1.0, 0))
self.assertEqual(_py_c_abs(1j), (1.0, 0))

self.assertEqual(_py_c_abs(complex('+inf+1j')), (INF, 0))
self.assertEqual(_py_c_abs(complex('-inf+1j')), (INF, 0))
self.assertEqual(_py_c_abs(complex('1.25+infj')), (INF, 0))
self.assertEqual(_py_c_abs(complex('1.25-infj')), (INF, 0))

self.assertTrue(isnan(_py_c_abs(complex('1.25+nanj'))[0]))
self.assertTrue(isnan(_py_c_abs(complex('nan-1j'))[0]))

self.assertEqual(_py_c_abs(complex(*[DBL_MAX]*2))[1], errno.ERANGE)
try:
_testcapi.set_errno(0)
self.assertEqual(_py_c_abs(-1), (1.0, 0))

@vstinner vstinner Sep 7, 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.

If I understand correctly, the PR changes two things in _Py_c_abs():

  • It no longer checks errno to decide if an exception should be raised.
  • It no longer modifies errno.

So I would prefer to set errno to ERANGE (or another errno different than zero?), and check that errno is unchanged (is still ERANGE) once the function completes. Something like:

            def c_abs(num):
                _testcapi.set_errno(errno.ERANGE)
                # _Py_c_abs() doesn't use errno and leaves errno unchanged
                result = _py_c_abs(-1) 
                self.assertEqual(_testcapi.get_errno(), errno.ERANGE)
                return result

            self.assertEqual(_py_c_abs(-1), (1.0, 0))

It seems like you have to add _testcapi.get_errno().

@hpkfft hpkfft Sep 7, 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.

  • [This PR changes _Py_c_abs() so that] it no longer modifies errno.

That's not the case. This PR retains the documented behavior that the function _Py_c_abs() sets errno to ERANGE on overflows.
It's not clear that anyone uses this function (viz. capi-workgroup), but it seems like a good idea to retain that behavior. Somebody may be using it similarly to how Numba uses _Py_c_pow().

On success, I think it's good to leave errno unchanged. Somebody might set errno to zero, and then do more than one library call with the understanding that errno is "sticky". The Linux man page for errno says, "the value of errno is never set to zero by any system call or library function." The philosophy, if I understand correctly, is that there may be false positives but not false negatives. That is, if errno is set to zero, then multiple library calls, then errno is still zero, then all the library calls were successful.

_testcapi.set_errno(0)
self.assertEqual(_py_c_abs(1j), (1.0, 0))

_testcapi.set_errno(0)
self.assertEqual(_py_c_abs(complex('+inf+1j')), (INF, 0))
_testcapi.set_errno(0)
self.assertEqual(_py_c_abs(complex('-inf+1j')), (INF, 0))
_testcapi.set_errno(0)
self.assertEqual(_py_c_abs(complex('1.25+infj')), (INF, 0))
_testcapi.set_errno(0)
self.assertEqual(_py_c_abs(complex('1.25-infj')), (INF, 0))

_testcapi.set_errno(0)
self.assertTrue(isnan(_py_c_abs(complex('1.25+nanj'))[0]))
_testcapi.set_errno(0)
self.assertTrue(isnan(_py_c_abs(complex('nan-1j'))[0]))

_testcapi.set_errno(0)
self.assertEqual(_py_c_abs(complex(*[DBL_MAX]*2))[1], errno.ERANGE)

_testcapi.set_errno(errno.EACCES) # preserve errno
self.assertEqual(_py_c_abs(1j), (1, errno.EACCES))
finally:
_testcapi.set_errno(0)


if __name__ == "__main__":
Expand Down
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 @@ -860,8 +863,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

@vstinner vstinner Sep 7, 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.

"Correct errno handling" is vague. Please explain in details what behavior changes. In Python, we do no give access to errno, so it's unclear to me how users are impacted.

Also, _Py_c_abs() is documented. If its behavior changes, a "versionchanged" note should be added in its documentation, and a separated Changelog (NEWS) entry should be added for _Py_c_abs() (basically repeats the abs(complex) change).

Kirpichev.
1 change: 0 additions & 1 deletion Modules/_testcapi/complex.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,6 @@ _py_c_abs(PyObject *Py_UNUSED(module), PyObject* obj)
return NULL;
}

errno = 0;
res = _Py_c_abs(complex);
return Py_BuildValue("di", res, errno);
}
Expand Down
2 changes: 1 addition & 1 deletion Modules/cmathmodule.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1029,8 +1029,8 @@ cmath_polar_impl(PyObject *module, Py_complex z)
{
double r, phi;

errno = 0;
phi = atan2(z.imag, z.real); /* should not cause any exception */
errno = 0;
r = _Py_c_abs(z); /* sets errno to ERANGE on overflow */
if (errno != 0)
return math_error();
Expand Down
15 changes: 10 additions & 5 deletions Objects/complexobject.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -379,32 +379,34 @@ 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 •
edited
Loading

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

Edit: I was wrong. The function polar() does not set errno = 0 immediately before calling _Py_c_abs(z), so it's not OK. Thanks, Serhiy.

@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;
int saved_errno = errno;

if (!isfinite(z.real) || !isfinite(z.imag)) {
/* C99 rules: if either the real or the imaginary part is an
infinity, return infinity, even if the other part is a
NaN. */
if (isinf(z.real)) {
result = fabs(z.real);
errno = 0;
errno = saved_errno;
return result;
}
if (isinf(z.imag)) {
result = fabs(z.imag);
errno = 0;
errno = saved_errno;
return result;
}
/* either the real or imaginary part is a NaN,
and neither is infinite. Result should be NaN. */
errno = saved_errno;
return Py_NAN;
}
result = hypot(z.real, z.imag);
if (!isfinite(result))
errno = ERANGE;
else
errno = 0;
errno = saved_errno;
return result;
}

Expand Down Expand Up @@ -812,7 +814,10 @@ static PyObject *
complex_abs(PyObject *op)
{
PyComplexObject *v = _PyComplexObject_CAST(op);
double result = _Py_c_abs(v->cval);
double result;

errno = 0;
result = _Py_c_abs(v->cval);
if (errno == ERANGE) {
PyErr_SetString(PyExc_OverflowError,
"absolute value too large");
Expand Down
Loading