-
-
Notifications
You must be signed in to change notification settings - Fork 31.9k
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
9035969
c35779b
607e2d5
d7b2c2f
d7a9120
537cc4e
e8b6ba9
ddfed7b
9e66399
573b2cd
14598f0
90029cf
2a60cc8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
:func:`complex` accepts now a string only as a positional argument. Passing | ||
a complex number as the "real" or "imag" argument is deprecated; it should | ||
only be passed as a single positional argument. |
Original file line number | Diff line number | Diff line change | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -894,8 +894,8 @@ complex_subtype_from_string(PyTypeObject *type, PyObject *v) | |||||||||||||||
} | ||||||||||||||||
else { | ||||||||||||||||
PyErr_Format(PyExc_TypeError, | ||||||||||||||||
"complex() argument must be a string or a number, not '%.200s'", | ||||||||||||||||
Py_TYPE(v)->tp_name); | ||||||||||||||||
"complex() argument must be a string or a number, not %T", | ||||||||||||||||
v); | ||||||||||||||||
return NULL; | ||||||||||||||||
} | ||||||||||||||||
|
||||||||||||||||
|
@@ -905,6 +905,77 @@ complex_subtype_from_string(PyTypeObject *type, PyObject *v) | |||||||||||||||
return result; | ||||||||||||||||
} | ||||||||||||||||
|
||||||||||||||||
/* The constructor should only accept a string as a positional argument, | ||||||||||||||||
* not as by the 'real' keyword. But Argument Clinic does not allow | ||||||||||||||||
* to distinguish between argument passed positionally and by keyword. | ||||||||||||||||
* So the constructor must be split into two parts: actual_complex_new() | ||||||||||||||||
* handles the case of no arguments and one positional argument, and calls | ||||||||||||||||
* complex_new(), implemented with Argument Clinic, to handle the remaining | ||||||||||||||||
* cases: 'real' and 'imag' arguments. This separation is well suited | ||||||||||||||||
* for different constructor roles: convering a string or number to a complex | ||||||||||||||||
* number and constructing a complex number from real and imaginary parts. | ||||||||||||||||
*/ | ||||||||||||||||
static PyObject * | ||||||||||||||||
actual_complex_new(PyTypeObject *type, PyObject *args, PyObject *kwargs) | ||||||||||||||||
{ | ||||||||||||||||
PyObject *res = NULL; | ||||||||||||||||
PyNumberMethods *nbr; | ||||||||||||||||
|
||||||||||||||||
if (PyTuple_GET_SIZE(args) > 1 || (kwargs != NULL && PyDict_GET_SIZE(kwargs))) { | ||||||||||||||||
return complex_new(type, args, kwargs); | ||||||||||||||||
} | ||||||||||||||||
if (!PyTuple_GET_SIZE(args)) { | ||||||||||||||||
return complex_subtype_from_doubles(type, 0, 0); | ||||||||||||||||
} | ||||||||||||||||
|
||||||||||||||||
PyObject *arg = PyTuple_GET_ITEM(args, 0); | ||||||||||||||||
/* Special-case for a single argument when type(arg) is complex. */ | ||||||||||||||||
if (PyComplex_CheckExact(arg) && type == &PyComplex_Type) { | ||||||||||||||||
/* Note that we can't know whether it's safe to return | ||||||||||||||||
a complex *subclass* instance as-is, hence the restriction | ||||||||||||||||
to exact complexes here. If either the input or the | ||||||||||||||||
output is a complex subclass, it will be handled below | ||||||||||||||||
as a non-orthogonal vector. */ | ||||||||||||||||
return Py_NewRef(arg); | ||||||||||||||||
} | ||||||||||||||||
if (PyUnicode_Check(arg)) { | ||||||||||||||||
return complex_subtype_from_string(type, arg); | ||||||||||||||||
} | ||||||||||||||||
PyObject *tmp = try_complex_special_method(arg); | ||||||||||||||||
if (tmp) { | ||||||||||||||||
Py_complex c = ((PyComplexObject*)tmp)->cval; | ||||||||||||||||
res = complex_subtype_from_doubles(type, c.real, c.imag); | ||||||||||||||||
Py_DECREF(tmp); | ||||||||||||||||
} | ||||||||||||||||
else if (PyErr_Occurred()) { | ||||||||||||||||
return NULL; | ||||||||||||||||
} | ||||||||||||||||
else if (PyComplex_Check(arg)) { | ||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 by There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
That seems to be an implementation detail. I wonder if we could add nb_complex slot: there is a reserved slot right now anyway.
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()).
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 commentThe reason will be displayed to describe this comment to others. Learn more. |
||||||||||||||||
/* 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); | ||||||||||||||||
} | ||||||||||||||||
Comment on lines
+953
to
+959
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
That seems redundant (and untested). Complex subclasses have |
||||||||||||||||
else if ((nbr = Py_TYPE(arg)->tp_as_number) != NULL && | ||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. One branch missed:
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, branch coverage output for C code looks cryptic. Try There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No. Oh, I see: that should be |
||||||||||||||||
(nbr->nb_float != NULL || nbr->nb_index != NULL)) | ||||||||||||||||
{ | ||||||||||||||||
/* The argument really is entirely real, and contributes | ||||||||||||||||
nothing in the imaginary direction. | ||||||||||||||||
Just treat it as a double. */ | ||||||||||||||||
double r = PyFloat_AsDouble(arg); | ||||||||||||||||
if (r != -1.0 || !PyErr_Occurred()) { | ||||||||||||||||
res = complex_subtype_from_doubles(type, r, 0); | ||||||||||||||||
} | ||||||||||||||||
} | ||||||||||||||||
else { | ||||||||||||||||
PyErr_Format(PyExc_TypeError, | ||||||||||||||||
"complex() argument must be a string or a number, not %T", | ||||||||||||||||
arg); | ||||||||||||||||
} | ||||||||||||||||
return res; | ||||||||||||||||
} | ||||||||||||||||
|
||||||||||||||||
/*[clinic input] | ||||||||||||||||
@classmethod | ||||||||||||||||
complex.__new__ as complex_new | ||||||||||||||||
|
@@ -930,32 +1001,10 @@ 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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. It is now covered by a new test added in #119635 for |
||||||||||||||||
} | ||||||||||||||||
PyObject *orig_r = r; | ||||||||||||||||
|
||||||||||||||||
/* Special-case for a single argument when type(arg) is complex. */ | ||||||||||||||||
if (PyComplex_CheckExact(r) && i == NULL && | ||||||||||||||||
type == &PyComplex_Type) { | ||||||||||||||||
/* Note that we can't know whether it's safe to return | ||||||||||||||||
a complex *subclass* instance as-is, hence the restriction | ||||||||||||||||
to exact complexes here. If either the input or the | ||||||||||||||||
output is a complex subclass, it will be handled below | ||||||||||||||||
as a non-orthogonal vector. */ | ||||||||||||||||
return Py_NewRef(r); | ||||||||||||||||
} | ||||||||||||||||
if (PyUnicode_Check(r)) { | ||||||||||||||||
if (i != NULL) { | ||||||||||||||||
PyErr_SetString(PyExc_TypeError, | ||||||||||||||||
"complex() can't take second arg" | ||||||||||||||||
" if first is a string"); | ||||||||||||||||
return NULL; | ||||||||||||||||
} | ||||||||||||||||
return complex_subtype_from_string(type, r); | ||||||||||||||||
} | ||||||||||||||||
if (i != NULL && PyUnicode_Check(i)) { | ||||||||||||||||
PyErr_SetString(PyExc_TypeError, | ||||||||||||||||
"complex() second arg can't be a string"); | ||||||||||||||||
return NULL; | ||||||||||||||||
} | ||||||||||||||||
|
||||||||||||||||
/* DEPRECATED: The call of try_complex_special_method() for the "real" | ||||||||||||||||
* part will be dropped after the end of the deprecation period. */ | ||||||||||||||||
tmp = try_complex_special_method(r); | ||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||||||||||||||||
if (tmp) { | ||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. else if (PyErr_Occurred()) branch is not tested. |
||||||||||||||||
r = tmp; | ||||||||||||||||
|
@@ -970,9 +1019,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 commentThe 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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. Now it's better, yet one branch seems to be missed:
Looks like it's |
||||||||||||||||
{ | ||||||||||||||||
PyErr_Format(PyExc_TypeError, | ||||||||||||||||
"complex() first argument must be a string or a number, " | ||||||||||||||||
"not '%.200s'", | ||||||||||||||||
Py_TYPE(r)->tp_name); | ||||||||||||||||
"complex() argument 'real' must be a real number, not %T", | ||||||||||||||||
r); | ||||||||||||||||
if (own_r) { | ||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually, 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 commentThe 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 commentThe 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. |
||||||||||||||||
Py_DECREF(r); | ||||||||||||||||
} | ||||||||||||||||
|
@@ -984,9 +1032,8 @@ complex_new_impl(PyTypeObject *type, PyObject *r, PyObject *i) | |||||||||||||||
(nbi->nb_float == NULL && nbi->nb_index == NULL && !PyComplex_Check(i))) | ||||||||||||||||
{ | ||||||||||||||||
PyErr_Format(PyExc_TypeError, | ||||||||||||||||
"complex() second argument must be a number, " | ||||||||||||||||
"not '%.200s'", | ||||||||||||||||
Py_TYPE(i)->tp_name); | ||||||||||||||||
"complex() argument 'imag' must be a real number, not %T", | ||||||||||||||||
i); | ||||||||||||||||
if (own_r) { | ||||||||||||||||
Py_DECREF(r); | ||||||||||||||||
} | ||||||||||||||||
|
@@ -998,6 +1045,7 @@ complex_new_impl(PyTypeObject *type, PyObject *r, PyObject *i) | |||||||||||||||
both be treated as numbers, and the constructor should return a | ||||||||||||||||
complex number equal to (real + imag*1j). | ||||||||||||||||
|
||||||||||||||||
The following is DEPRECATED: | ||||||||||||||||
Note that we do NOT assume the input to already be in canonical | ||||||||||||||||
form; the "real" and "imag" parts might themselves be complex | ||||||||||||||||
numbers, which slightly complicates the code below. */ | ||||||||||||||||
|
@@ -1008,19 +1056,27 @@ complex_new_impl(PyTypeObject *type, PyObject *r, PyObject *i) | |||||||||||||||
cr = ((PyComplexObject*)r)->cval; | ||||||||||||||||
cr_is_complex = 1; | ||||||||||||||||
if (own_r) { | ||||||||||||||||
/* r was a newly created complex number, rather | ||||||||||||||||
than the original "real" argument. */ | ||||||||||||||||
Py_DECREF(r); | ||||||||||||||||
} | ||||||||||||||||
nbr = Py_TYPE(orig_r)->tp_as_number; | ||||||||||||||||
if (nbr == NULL || | ||||||||||||||||
(nbr->nb_float == NULL && nbr->nb_index == NULL)) | ||||||||||||||||
Comment on lines
+1064
to
+1065
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This also looks untested:
|
||||||||||||||||
{ | ||||||||||||||||
if (PyErr_WarnFormat(PyExc_DeprecationWarning, 1, | ||||||||||||||||
"complex() argument 'real' must be a real number, not %T", | ||||||||||||||||
orig_r)) { | ||||||||||||||||
return NULL; | ||||||||||||||||
} | ||||||||||||||||
} | ||||||||||||||||
} | ||||||||||||||||
else { | ||||||||||||||||
/* The "real" part really is entirely real, and contributes | ||||||||||||||||
nothing in the imaginary direction. | ||||||||||||||||
Just treat it as a double. */ | ||||||||||||||||
tmp = PyNumber_Float(r); | ||||||||||||||||
if (own_r) { | ||||||||||||||||
/* r was a newly created complex number, rather | ||||||||||||||||
than the original "real" argument. */ | ||||||||||||||||
Py_DECREF(r); | ||||||||||||||||
} | ||||||||||||||||
assert(!own_r); | ||||||||||||||||
if (tmp == NULL) | ||||||||||||||||
return NULL; | ||||||||||||||||
assert(PyFloat_Check(tmp)); | ||||||||||||||||
|
@@ -1032,6 +1088,11 @@ complex_new_impl(PyTypeObject *type, PyObject *r, PyObject *i) | |||||||||||||||
ci.real = cr.imag; | ||||||||||||||||
} | ||||||||||||||||
else if (PyComplex_Check(i)) { | ||||||||||||||||
if (PyErr_WarnFormat(PyExc_DeprecationWarning, 1, | ||||||||||||||||
"complex() argument 'imag' must be a real number, not %T", | ||||||||||||||||
i)) { | ||||||||||||||||
return NULL; | ||||||||||||||||
} | ||||||||||||||||
ci = ((PyComplexObject*)i)->cval; | ||||||||||||||||
ci_is_complex = 1; | ||||||||||||||||
} else { | ||||||||||||||||
|
@@ -1131,6 +1192,6 @@ PyTypeObject PyComplex_Type = { | |||||||||||||||
0, /* tp_dictoffset */ | ||||||||||||||||
0, /* tp_init */ | ||||||||||||||||
PyType_GenericAlloc, /* tp_alloc */ | ||||||||||||||||
complex_new, /* tp_new */ | ||||||||||||||||
actual_complex_new, /* tp_new */ | ||||||||||||||||
PyObject_Del, /* tp_free */ | ||||||||||||||||
}; |
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 from
complex_new
)?