GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-109218: Deprecate weird cases in the complex() constructor by serhiy-storchaka · Pull Request #119620 · python/cpython

Conversation

@serhiy-storchaka

@serhiy-storchaka serhiy-storchaka commented May 27, 2024 •
edited by github-actions Bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member
  • 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/

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@serhiy-storchaka
serhiy-storchaka requested a review from mdickinson May 27, 2024 17:09
@bedevere-app bedevere-app Bot added the awaiting core review label May 27, 2024
@bedevere-app bedevere-app Bot mentioned this pull request May 27, 2024
Invalid "equivalents" of the complex type constructor in docs #109218
Closed
@serhiy-storchaka
pythongh-109218: Deprecate weird cases in the complex() constructor
9035969 
* 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
serhiy-storchaka force-pushed the complex-constructor branch from 9ec7587 to 9035969 Compare May 27, 2024 18:06
@serhiy-storchaka
pythongh-109218: Refactor tests for the complex() constructor
c35779b 
* 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
skirpichev reviewed May 28, 2024
View reviewed changes

@skirpichev skirpichev left a comment

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Objects/complexobject.c
@@ -930,31 +998,23 @@ complex_new_impl(PyTypeObject *type, PyObject *r, PyObject *i)
if (r == NULL) {
r = _PyLong_GetZero();

@skirpichev skirpichev May 28, 2024

Copy link
Copy Markdown
Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
serhiy-storchaka reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

@serhiy-storchaka serhiy-storchaka May 28, 2024

Copy link
Copy Markdown
Member Author

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 for complex(imag=1.5).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Objects/complexobject.c Outdated
if (PyErr_WarnFormat(PyExc_DeprecationWarning, 1,
"complex() argument 'real' must be a real number, not %T",
r)) {
return NULL;

@skirpichev skirpichev May 28, 2024

Copy link
Copy Markdown
Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
serhiy-storchaka reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
Comment thread Objects/complexobject.c Outdated

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

@skirpichev skirpichev May 28, 2024

Copy link
Copy Markdown
Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
serhiy-storchaka reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

@serhiy-storchaka serhiy-storchaka May 28, 2024

Copy link
Copy Markdown
Member Author

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Objects/complexobject.c
}

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

@skirpichev skirpichev May 28, 2024

Copy link
Copy Markdown
Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
serhiy-storchaka reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
Comment thread 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)))

@skirpichev skirpichev May 28, 2024

Copy link
Copy Markdown
Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
serhiy-storchaka reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

@serhiy-storchaka serhiy-storchaka May 28, 2024

Copy link
Copy Markdown
Member Author

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@skirpichev skirpichev May 28, 2024

Copy link
Copy Markdown
Member

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's nbr!=NULL.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Objects/complexobject.c
Py_TYPE(r)->tp_name);
"complex() argument 'real' must be a real number, not %T",
r);
if (own_r) {

@skirpichev skirpichev May 28, 2024 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@serhiy-storchaka serhiy-storchaka May 28, 2024

Copy link
Copy Markdown
Member Author

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@serhiy-storchaka serhiy-storchaka May 28, 2024

Copy link
Copy Markdown
Member Author

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
serhiy-storchaka added 6 commits May 28, 2024 10:19
@serhiy-storchaka
Merge branch 'main' into complex-constructor-tests
607e2d5
@serhiy-storchaka
Merge branch 'main' into complex-constructor
d7b2c2f
@serhiy-storchaka
Merge branch 'complex-constructor-tests' into complex-constructor
d7a9120
@serhiy-storchaka
Address review comments.
537cc4e
@serhiy-storchaka
Add more tests.
e8b6ba9
@serhiy-storchaka
Merge branch 'complex-constructor-tests' into complex-constructor
ddfed7b
@serhiy-storchaka

serhiy-storchaka commented May 28, 2024

Copy link
Copy Markdown
Member Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
skirpichev
skirpichev reviewed May 28, 2024
View reviewed changes
Comment thread Objects/complexobject.c
Comment on lines +1072 to +1073
if (nbr == NULL ||
(nbr->nb_float == NULL && nbr->nb_index == NULL))

@skirpichev skirpichev May 28, 2024

Copy link
Copy Markdown
Member

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))

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
skirpichev
skirpichev reviewed May 28, 2024
View reviewed changes
Comment thread 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 &&

@skirpichev skirpichev May 28, 2024

Copy link
Copy Markdown
Member

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))

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@serhiy-storchaka serhiy-storchaka May 28, 2024

Copy link
Copy Markdown
Member Author

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())

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@skirpichev skirpichev May 28, 2024

Copy link
Copy Markdown
Member

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. Try complex([]), i.e. nbr != NULL, but it has no nb_float/index, that works for me.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@serhiy-storchaka serhiy-storchaka May 28, 2024

Copy link
Copy Markdown
Member Author

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 think complex(MyInt()) (where MyInt has the __int__ method already covers this).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@skirpichev skirpichev May 29, 2024

Copy link
Copy Markdown
Member

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 be nbr == NULL condition. So complex(object()) will work too.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@serhiy-storchaka
Simplify some code.
9e66399
@skirpichev skirpichev mentioned this pull request May 28, 2024
gh-109802: removed inaccessible code from complexobject.c #109642
Closed
mdickinson
mdickinson reviewed May 28, 2024
View reviewed changes
Comment thread Objects/complexobject.c
return result;
}

static PyObject *

@mdickinson mdickinson May 28, 2024

Copy link
Copy Markdown
Member

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)?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
serhiy-storchaka reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
mdickinson
mdickinson approved these changes May 28, 2024
View reviewed changes

@mdickinson mdickinson left a comment

Copy link
Copy Markdown
Member

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. :-)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bedevere-app bedevere-app Bot added awaiting merge and removed awaiting core review labels May 28, 2024
skirpichev
skirpichev reviewed May 28, 2024
View reviewed changes
Comment thread Objects/complexobject.c
else if (PyErr_Occurred()) {
return NULL;
}
else if (PyComplex_Check(arg)) {

@skirpichev skirpichev May 28, 2024

Copy link
Copy Markdown
Member

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 by try_complex_special_method(), c.f. PyNumber_Float().

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@serhiy-storchaka serhiy-storchaka May 29, 2024

Copy link
Copy Markdown
Member Author

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(), 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__.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@skirpichev skirpichev May 29, 2024

Copy link
Copy Markdown
Member

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.

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.)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@skirpichev skirpichev Jun 26, 2024

Copy link
Copy Markdown
Member

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, discourse thread: https://discuss.python.org/t/use-nb-reserved-slot-of-pynumbermethods-to-support-complex-method/56620

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread 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 &&

@skirpichev skirpichev May 28, 2024

Copy link
Copy Markdown
Member

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. Try complex([]), i.e. nbr != NULL, but it has no nb_float/index, that works for me.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Objects/complexobject.c
}
PyObject *orig_r = r;

tmp = try_complex_special_method(r);

@skirpichev skirpichev May 28, 2024

Copy link
Copy Markdown
Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
serhiy-storchaka reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@serhiy-storchaka
Add comments.
573b2cd
@skirpichev skirpichev mentioned this pull request May 29, 2024
gh-109218: Improve documentation for the complex() constructor #119687
Merged
skirpichev
skirpichev approved these changes May 29, 2024
View reviewed changes

@skirpichev skirpichev left a comment

Copy link
Copy Markdown
Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread 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 &&

@skirpichev skirpichev May 29, 2024

Copy link
Copy Markdown
Member

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 be nbr == NULL condition. So complex(object()) will work too.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Objects/complexobject.c
Comment on lines +953 to +959
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);
}

@skirpichev skirpichev May 29, 2024

Copy link
Copy Markdown
Member

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.

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).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
serhiy-storchaka added 3 commits May 29, 2024 10:22
@serhiy-storchaka
Polishing.
14598f0
@serhiy-storchaka
Rename Mock to With.
90029cf
@serhiy-storchaka
Merge branch 'main' into complex-constructor
2a60cc8
@serhiy-storchaka
serhiy-storchaka merged commit ef01e95 into python:main May 30, 2024
@bedevere-app bedevere-app Bot removed the awaiting merge label May 30, 2024
@serhiy-storchaka
serhiy-storchaka deleted the complex-constructor branch May 30, 2024 20:31
estyxx pushed a commit to estyxx/cpython that referenced this pull request Jul 17, 2024
@serhiy-storchaka @estyxx
pythongh-109218: Deprecate weird cases in the complex() constructor (p…
a6d8d0f 
…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.
@hugovk hugovk mentioned this pull request Jul 24, 2024
gh-122085: Use include files for whatsnew/3.14.rst deprecations #122242
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@skirpichev skirpichev skirpichev approved these changes
+1 more reviewer
@mdickinson mdickinson mdickinson approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

3 participants

@serhiy-storchaka @mdickinson @skirpichev
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.