Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 34k
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
Uh oh!
There was an error while loading. Please reload this page.
Changes from all commits
9035969c35779b607e2d5d7b2c2fd7a9120537cc4ee8b6ba9ddfed7b9e66399573b2cd14598f090029cf2a60cc8File filter
Filter by extension
Conversations
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Uh oh!
There was an error while loading. Please reload this page.
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)){ | ||||||||||||||||
Member 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 MemberAuthor 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.
Member 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.) Member 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 Member 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 && | ||||||||||||||||
Member 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: MemberAuthor 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.
Member 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 MemberAuthor 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 Member 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(); | ||||||||||||||||
Member 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. MemberAuthor 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); | ||||||||||||||||
Member 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){ | ||||||||||||||||
Member 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))) | ||||||||||||||||
Member 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. MemberAuthor 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. Member 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){ | ||||||||||||||||
Member
| ||||||||||||||||
| 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 Member 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)?