Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork32k
gh-109218: Deprecate weird cases in the complex() constructor#119620
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.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
gh-109218: Deprecate weird cases in the complex() constructor#119620
Uh oh!
There was an error while loading.Please reload this page.
Conversation
* 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.
9ec7587 to9035969Compare* 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.
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.
tmp == NULL condition on L1092 also now only partially covered.
Just checked coverage report after./python -m test test_complex test_capi.test_complex
| @@ -930,31 +998,23 @@ complex_new_impl(PyTypeObject *type, PyObject *r, PyObject *i) | |||
| if (r == NULL) { | |||
| r = _PyLong_GetZero(); | |||
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.
This inaccessible.
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.
It is now covered by a new test added in#119635 forcomplex(imag=1.5).
Objects/complexobject.c Outdated
| if (PyErr_WarnFormat(PyExc_DeprecationWarning, 1, | ||
| "complex() argument 'real' must be a real number, not %T", | ||
| r)) { | ||
| return NULL; |
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.
Nit: this is not tested. Ditto for other warning.
Objects/complexobject.c Outdated
| @@ -930,31 +998,23 @@ complex_new_impl(PyTypeObject *type, PyObject *r, PyObject *i) | |||
| if (r == NULL) { | |||
| r = _PyLong_GetZero(); | |||
| } | |||
| PyObject *orig_r = r; | |||
| /* Special-case for a single argument when type(arg) is complex. */ | |||
| if (PyComplex_CheckExact(r) && i == NULL && | |||
| type == &PyComplex_Type) { | |||
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.
Can this be false? If so, this is not tested.
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.
Yes, it can be false. Added tests.
| PyErr_SetString(PyExc_TypeError, | ||
| "complex() second arg can't be a string"); | ||
| return NULL; | ||
| } | ||
| tmp = try_complex_special_method(r); | ||
| if (tmp) { |
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.
else if (PyErr_Occurred()) branch is not tested.
| @@ -970,9 +1030,8 @@ complex_new_impl(PyTypeObject *type, PyObject *r, PyObject *i) | |||
| (nbr->nb_float == NULL && nbr->nb_index == NULL && !PyComplex_Check(r))) | |||
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.
Hmm, it seems there is a coverage regression, not all branches are tested.
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.
This should be covered by new tests added in test added in#119635.
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.
Now it's better, yet one branch seems to be missed:
1029 [ + - ]: 3869 : if (nbr == NULL ||Looks like it'snbr!=NULL.
| "not '%.200s'", | ||
| Py_TYPE(r)->tp_name); | ||
| "complex() argument 'real' must be a real number, not %T", | ||
| r); | ||
| if (own_r) { |
skirpichevMay 28, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
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.
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.
I'll see.
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.
This code will be removed after the end of the deprecation period. So it is not worth to spent effort to optimize it.
Thank you for your review@skirpichev. Much of coverage is added by test added in#119635. |
| if (nbr == NULL || | ||
| (nbr->nb_float == NULL && nbr->nb_index == NULL)) |
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.
This also looks untested:
1072 [ + - ]: 10 : if (nbr == NULL || 1073 [ + - + - ]: 10 : (nbr->nb_float == NULL && nbr->nb_index == NULL))| 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 && |
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.
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))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.
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())
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.
Hmm, branch coverage output for C code looks cryptic. Trycomplex([]), i.e. nbr != NULL, but it has no nb_float/index, that works for me.
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.
I thinkcomplex(MyInt()) (whereMyInt has the__int__ method already covers this).
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.
No. Oh, I see: that should benbr == NULL condition. Socomplex(object()) will work too.
| @@ -905,6 +905,67 @@ complex_subtype_from_string(PyTypeObject *type, PyObject *v) | |||
| return result; | |||
| } | |||
| static PyObject * | |||
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.
Would it be worth adding a comment above this function explaining the purpose (and explaining why this is different fromcomplex_new)?
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.
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. :-)
| else if (PyErr_Occurred()) { | ||
| return NULL; | ||
| } | ||
| else if (PyComplex_Check(arg)) { |
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.
FYI, this is not tested. But I'm not sure if that's a right logic. Complex subclasses should be covered bytry_complex_special_method(), c.f. PyNumber_Float().
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.
try_complex_special_method is relatively expensive in comparison toPyNumber_Float(), because there is nonb_complex slot. And__complex__ is looked up even for exactcomplex,float andint. I planned to do something with this, but in a different PR. We can not also completely excludecomplex subclasses without__complex__.
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.
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.
Andcomplex 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 withoutcomplex.
People could break thing in very crazy ways, but should we support such cases? (Looks as a variant of#112636.)
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.
| 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 && |
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.
Hmm, branch coverage output for C code looks cryptic. Trycomplex([]), i.e. nbr != NULL, but it has no nb_float/index, that works for me.
| "complex() second arg can't be a string"); | ||
| return NULL; | ||
| } | ||
| PyObject *orig_r = r; | ||
| tmp = try_complex_special_method(r); |
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.
IIUIC, this logic will be dropped after a deprecation period as well. I'm not sure if this is obvious, maybe worth a comment.
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.
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.
| 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 && |
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.
No. Oh, I see: that should benbr == NULL condition. Socomplex(object()) will work too.
| 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); | ||
| } |
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.
| elseif (PyComplex_Check(arg)) { | |
| /*Notethatifargisofacomplexsubtype,we're only | |
| retainingitsreal&imagpartshere,andthereturn | |
| valueis (properly)ofthebuiltincomplextype.*/ | |
| Py_complexc= ((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).
ef01e95 intopython:mainUh oh!
There was an error while loading.Please reload this page.
…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.
…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.
Uh oh!
There was an error while loading.Please reload this page.
📚 Documentation preview 📚:https://cpython-previews--119620.org.readthedocs.build/