gh-109218: Deprecate weird cases in the complex() constructor by serhiy-storchaka · Pull Request #119620 · python/cpython

serhiy-storchaka · 2024-05-27T17:09:29Z

Passing a string as the "real" keyword argument is now an error; it should only be passed as a single positional argument.
Passing a complex number as the real or imag argument is now deprecated; it should only be passed as a single positional argument.

Issue: Invalid "equivalents" of the complex type constructor in docs #109218

📚 Documentation preview 📚: https://cpython-previews--119620.org.readthedocs.build/

* Passing a string as the "real" keyword argument is now an error; it should only be passed as a single positional argument. * Passing a complex number as the *real* or *imag* argument is now deprecated; it should only be passed as a single positional argument.

* Share common classes. * Use exactly representable floats and exact tests. * Check the sign of zero components. * Remove duplicated tests (mostly left after merging int and long). * Reorder tests in more consistent way. * Test more error messages. * Add tests for missed cases.

skirpichev

tmp == NULL condition on L1092 also now only partially covered.

Just checked coverage report after ./python -m test test_complex test_capi.test_complex

skirpichev · 2024-05-28T02:22:01Z

Objects/complexobject.c

@@ -930,31 +998,23 @@ complex_new_impl(PyTypeObject *type, PyObject *r, PyObject *i)
    if (r == NULL) {
        r = _PyLong_GetZero();


This inaccessible.

It is now covered by a new test added in #119635 for complex(imag=1.5).

skirpichev · 2024-05-28T02:22:57Z

Objects/complexobject.c

+        if (PyErr_WarnFormat(PyExc_DeprecationWarning, 1,
+                "complex() argument 'real' must be a real number, not %T",
+                r)) {
+            return NULL;


Nit: this is not tested. Ditto for other warning.

skirpichev · 2024-05-28T02:25:02Z

Objects/complexobject.c


    /* Special-case for a single argument when type(arg) is complex. */
    if (PyComplex_CheckExact(r) && i == NULL &&
        type == &PyComplex_Type) {


Can this be false? If so, this is not tested.

Yes, it can be false. Added tests.

skirpichev · 2024-05-28T02:27:35Z

Objects/complexobject.c

-    }

    tmp = try_complex_special_method(r);
    if (tmp) {


else if (PyErr_Occurred()) branch is not tested.

skirpichev · 2024-05-28T02:36:38Z

Objects/complexobject.c

@@ -970,9 +1030,8 @@ complex_new_impl(PyTypeObject *type, PyObject *r, PyObject *i)
        (nbr->nb_float == NULL && nbr->nb_index == NULL && !PyComplex_Check(r)))


Hmm, it seems there is a coverage regression, not all branches are tested.

This should be covered by new tests added in test added in #119635.

Now it's better, yet one branch seems to be missed:

1029 [ + - ]: 3869 : if (nbr == NULL ||

Looks like it's nbr!=NULL.

skirpichev · 2024-05-28T02:41:32Z

Objects/complexobject.c

-                     Py_TYPE(r)->tp_name);
+                     "complex() argument 'real' must be a real number, not %T",
+                     r);
        if (own_r) {


Actually, own_r condition here is inaccessible. If we are here - try_complex_special_method() call above was unsuccessful.

See #109642. Maybe it worth to include constructor-related changes from that pr.

This code will be removed after the end of the deprecation period. So it is not worth to spent effort to optimize it.

serhiy-storchaka · 2024-05-28T08:29:34Z

Thank you for your review @skirpichev. Much of coverage is added by test added in #119635.

skirpichev · 2024-05-28T09:23:16Z

Objects/complexobject.c

+        if (nbr == NULL ||
+            (nbr->nb_float == NULL && nbr->nb_index == NULL))


This also looks untested:

1072 [ + - ]: 10 : if (nbr == NULL || 1073 [ + - + - ]: 10 : (nbr->nb_float == NULL && nbr->nb_index == NULL))

skirpichev · 2024-05-28T09:25:41Z

Objects/complexobject.c

+        Py_complex c = ((PyComplexObject*)arg)->cval;
+        res = complex_subtype_from_doubles(type, c.real, c.imag);
+    }
+    else if ((nbr = Py_TYPE(arg)->tp_as_number) != NULL &&


One branch missed:

952 [ + - ]: 55 : else if ((nbr = Py_TYPE(arg)->tp_as_number) != NULL && 953 [ + + + + ]: 55 : (nbr->nb_float != NULL || nbr->nb_index != NULL))

How to interpret this? It looks to me that all possible combinations are covered.

nbr == NULL: complex({})

nbr != NULL && nbr->nb_float != NULL: complex(MockFloat(4.25))

nbr != NULL && nbr->nb_float == NULL && nbr->nb_index != NULL: complex(MockIndex(42))

nbr != NULL && nbr->nb_float == NULL && nbr->nb_index == NULL: complex(MockInt())

Hmm, branch coverage output for C code looks cryptic. Try complex([]), i.e. nbr != NULL, but it has no nb_float/index, that works for me.

I think complex(MyInt()) (where MyInt has the __int__ method already covers this).

No. Oh, I see: that should be nbr == NULL condition. So complex(object()) will work too.

mdickinson · 2024-05-28T16:37:22Z

Objects/complexobject.c

    return result;
 }

+static PyObject *


Would it be worth adding a comment above this function explaining the purpose (and explaining why this is different from complex_new)?

mdickinson

LGTM in principle, and works as expected in manual testing. I won't claim to have examined all the branching possibilities, but it looks as though @skirpichev is on top of that. :-)

skirpichev · 2024-05-28T16:28:29Z

Objects/complexobject.c

+    else if (PyErr_Occurred()) {
+        return NULL;
+    }
+    else if (PyComplex_Check(arg)) {


FYI, this is not tested. But I'm not sure if that's a right logic. Complex subclasses should be covered by try_complex_special_method(), c.f. PyNumber_Float().

try_complex_special_method is relatively expensive in comparison to PyNumber_Float(), because there is no nb_complex slot. And __complex__ is looked up even for exact complex, float and int. I planned to do something with this, but in a different PR. We can not also completely exclude complex subclasses without __complex__.

try_complex_special_method is relatively expensive in comparison to PyNumber_Float()

That seems to be an implementation detail. I wonder if we could add nb_complex slot: there is a reserved slot right now anyway.

And complex is looked up even for exact complex, float and int.

The current (i.e. in the main) code - uses here same logic as the float constructor: there is a case for exact complex (as for exact float in PyNumber_Float()).

We can not also completely exclude complex subclasses without complex.

People could break thing in very crazy ways, but should we support such cases? (Looks as a variant of #112636.)

FYI, discourse thread: https://discuss.python.org/t/use-nb-reserved-slot-of-pynumbermethods-to-support-complex-method/56620

skirpichev · 2024-05-28T16:51:17Z

Objects/complexobject.c

+        Py_complex c = ((PyComplexObject*)arg)->cval;
+        res = complex_subtype_from_doubles(type, c.real, c.imag);
+    }
+    else if ((nbr = Py_TYPE(arg)->tp_as_number) != NULL &&


Hmm, branch coverage output for C code looks cryptic. Try complex([]), i.e. nbr != NULL, but it has no nb_float/index, that works for me.

skirpichev · 2024-05-28T16:57:27Z

Objects/complexobject.c

-    }
+    PyObject *orig_r = r;

    tmp = try_complex_special_method(r);


IIUIC, this logic will be dropped after a deprecation period as well. I'm not sure if this is obvious, maybe worth a comment.

skirpichev

LGTM, except for useless PyComplex_Check() branch in the actual_complex_new() and one missing branch coverage here.

The rest, probably, not worth for improving, as it will be eventually removed. Though, I think that removing inaccessible cases (various own_r branches) will make code more readable.

skirpichev · 2024-05-29T03:26:10Z

Objects/complexobject.c

+        Py_complex c = ((PyComplexObject*)arg)->cval;
+        res = complex_subtype_from_doubles(type, c.real, c.imag);
+    }
+    else if ((nbr = Py_TYPE(arg)->tp_as_number) != NULL &&


No. Oh, I see: that should be nbr == NULL condition. So complex(object()) will work too.

skirpichev · 2024-05-29T03:29:02Z

Objects/complexobject.c

+    else if (PyComplex_Check(arg)) {
+        /* Note that if arg is of a complex subtype, we're only
+           retaining its real & imag parts here, and the return
+           value is (properly) of the builtin complex type. */
+        Py_complex c = ((PyComplexObject*)arg)->cval;
+        res = complex_subtype_from_doubles(type, c.real, c.imag);
+    }


Suggested change

else if (PyComplex_Check(arg)) {

/* Note that if arg is of a complex subtype, we're only

retaining its real & imag parts here, and the return

value is (properly) of the builtin complex type. */

Py_complex c = ((PyComplexObject*)arg)->cval;

res = complex_subtype_from_doubles(type, c.real, c.imag);

}

That seems redundant (and untested). Complex subclasses have __complex__() dunder, so they should be handled by try_complex_special_method() helper (even if the dunder is broken somehow).

…ythonGH-119620) * Passing a string as the "real" keyword argument is now an error; it should only be passed as a single positional argument. * Passing a complex number as the "real" or "imag" argument is now deprecated; it should only be passed as a single positional argument.

serhiy-storchaka requested a review from mdickinson May 27, 2024 17:09

bedevere-app bot added the awaiting core review label May 27, 2024

bedevere-app bot mentioned this pull request May 27, 2024

Invalid "equivalents" of the complex type constructor in docs #109218

Closed

serhiy-storchaka force-pushed the complex-constructor branch from 9ec7587 to 9035969 Compare May 27, 2024 18:06

skirpichev reviewed May 28, 2024

View reviewed changes

serhiy-storchaka added 6 commits May 28, 2024 10:19

Merge branch 'main' into complex-constructor-tests

607e2d5

Merge branch 'main' into complex-constructor

d7b2c2f

Merge branch 'complex-constructor-tests' into complex-constructor

d7a9120

Address review comments.

537cc4e

Add more tests.

e8b6ba9

Merge branch 'complex-constructor-tests' into complex-constructor

ddfed7b

skirpichev reviewed May 28, 2024

View reviewed changes

Simplify some code.

9e66399

skirpichev mentioned this pull request May 28, 2024

gh-109802: removed inaccessible code from complexobject.c #109642

Closed

mdickinson reviewed May 28, 2024

View reviewed changes

mdickinson approved these changes May 28, 2024

View reviewed changes

bedevere-app bot added awaiting merge and removed awaiting core review labels May 28, 2024

skirpichev reviewed May 28, 2024

View reviewed changes

Add comments.

573b2cd

skirpichev mentioned this pull request May 29, 2024

gh-109218: Improve documentation for the complex() constructor #119687

Merged

skirpichev approved these changes May 29, 2024

View reviewed changes

serhiy-storchaka added 3 commits May 29, 2024 10:22

Polishing.

14598f0

Rename Mock to With.

90029cf

Merge branch 'main' into complex-constructor

2a60cc8

serhiy-storchaka merged commit ef01e95 into python:main May 30, 2024

bedevere-app bot removed the awaiting merge label May 30, 2024

serhiy-storchaka deleted the complex-constructor branch May 30, 2024 20:31

hugovk mentioned this pull request Jul 24, 2024

gh-122085: Use include files for whatsnew/3.14.rst deprecations #122242

Merged

		@@ -930,31 +998,23 @@ complex_new_impl(PyTypeObject type, PyObject r, PyObject *i)
		if (r == NULL) {
		r = _PyLong_GetZero();

		@@ -970,9 +1030,8 @@ complex_new_impl(PyTypeObject type, PyObject r, PyObject *i)
		(nbr->nb_float == NULL && nbr->nb_index == NULL && !PyComplex_Check(r)))

		if (nbr == NULL \|\|
		(nbr->nb_float == NULL && nbr->nb_index == NULL))

Uh oh!

Conversation

serhiy-storchaka commented May 27, 2024 • edited by github-actions bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

skirpichev left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

skirpichev May 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka commented May 28, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mdickinson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

skirpichev left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

serhiy-storchaka commented May 27, 2024 •

edited by github-actions bot

Loading

skirpichev May 28, 2024 •

edited

Loading