bpo-38302. rpow now called when ipow returns NotImplemented #16459

ashkop · 2019-09-28T11:57:00Z

Basically copied the behavior of binary_iop for the ternary operator. Also made sure that op_name is used in error message.

https://bugs.python.org/issue38302

brandtbucher

Thanks, this looks good! Just noticed a couple of small things:

brandtbucher · 2019-09-28T19:28:05Z

Objects/abstract.c

@@ -1216,7 +1235,7 @@ PyNumber_InPlacePower(PyObject *v, PyObject *w, PyObject *z)
 {
    if (v->ob_type->tp_as_number &&
        v->ob_type->tp_as_number->nb_inplace_power != NULL) {
-        return ternary_op(v, w, z, NB_SLOT(nb_inplace_power), "**=");
+        return ternary_iop(v, w, z, NB_SLOT(nb_inplace_power), NB_SLOT(nb_power), "**=");


PyNumber_InPlacePower can now just mirror the other PyNumber_InPlace* functions, right?

So this is the only line that's needed here.

Ah yes, thank you.

brandtbucher · 2019-09-28T19:31:08Z

Lib/test/test_descr.py

@@ -3900,6 +3900,18 @@ def __ipow__(self, other):
        a = C()
        a **= 2

+    def test_ipow_returns_not_implemented(self):
+        class A:
+            def __ipow__(self, other):


It looks like we're also lacking a test for when A doesn't define __ipow__. Do you mind adding one?

Sure, added the test

Could you add a test for the two fallbacks and ensure the same class is returning NotImplemented and implementing the regular power methods i.e something like:

class A: def __ipow__(self, other): return NotImplemented class B(A): def __pow__(self, other): return 1 class C(A): def __rpow__(self, other): return 2

The linked bug is only exhibited when a class has an ipow slot but when called it returns NotImplemented.

Not sure I understood correctly what you meant, but I updated the test to also test the __pow__ fallback.

Thank you, what you have now looks good. What I meant was that to catch the original bug/properly be a regression test, the class has to look like:

class A: def __ipow__(self, _): return NotImplemented def __pow__(self, other): ...

if the class looks like

class A: def __pow__(self, other): ...

then the old logic returns a false here and correctly goes to the non-inplace version of power anyway:

cpython/Objects/abstract.c

Lines 1162 to 1163 in 8e19c8b

if (Py_TYPE(v)->tp_as_number &&

Py_TYPE(v)->tp_as_number->nb_inplace_power != NULL) {

Ah yes, you are right. Thanks!

brandtbucher

@ashkop, one more small thing in the new test. Then it looks good!

brandtbucher · 2019-09-29T17:12:24Z

Lib/test/test_descr.py

+            def __rpow__(self, other):
+                return 1
+
+        a = A()


This can just be:

Suggested change

a = A()

a = object()

Then you don't need the class definition above!

serhiy-storchaka

LGTM. 👍

ammaraskar · 2020-08-24T22:16:54Z

Lib/test/test_descr.py

@@ -3900,6 +3900,18 @@ def __ipow__(self, other):
        a = C()
        a **= 2

+    def test_ipow_returns_not_implemented(self):
+        class A:
+            def __ipow__(self, other):


Could you add a test for the two fallbacks and ensure the same class is returning NotImplemented and implementing the regular power methods i.e something like:

class A: def __ipow__(self, other): return NotImplemented class B(A): def __pow__(self, other): return 1 class C(A): def __rpow__(self, other): return 2

The linked bug is only exhibited when a class has an ipow slot but when called it returns NotImplemented.

ammaraskar · 2020-08-24T22:23:54Z

Misc/NEWS.d/next/Core and Builtins/2019-09-28-12-23-23.bpo-38302.hsCNgX.rst

@@ -0,0 +1 @@
+If :func:`object.__ipow__` returns :const:`NotImplemented` :func:`object.__rpow__` will be called now as expected.


Just fyi, @brettcannon posted on the original bug pointing out that this would not call __pow__ either. I'd recommend changing this to

If :func:`object.__ipow__` returns :const:`NotImplemented`, the operator will correctly fall back to :func:`object.__pow__` and :func:`object.__rpow__` as expected.

It might also be worth mentioning this in whatsnew, even though it's obscure it is potentially a change in behavior that people should be informed about.

Updated the news entry, and also added a line to What's New

ammaraskar · 2020-08-24T22:36:10Z

Objects/abstract.c

@@ -965,8 +965,9 @@ ternary_op(PyObject *v,
    if (z == Py_None)
        PyErr_Format(
            PyExc_TypeError,
-            "unsupported operand type(s) for ** or pow(): "
+            "unsupported operand type(s) for %.100s: "


Thanks for fixing this! Really weird that op_name went unused so long. Would you mind adding a test for this like:

x = None with self.assertRaises(TypeError) as cm: x **= 2 self.assertIn('unsupported operand type(s) for **=', str(cm.exception))

Thanks, added a new test

ammaraskar · 2020-08-25T16:36:26Z

Lib/test/test_descr.py

@@ -3900,6 +3900,18 @@ def __ipow__(self, other):
        a = C()
        a **= 2

+    def test_ipow_returns_not_implemented(self):
+        class A:
+            def __ipow__(self, other):


Thank you, what you have now looks good. What I meant was that to catch the original bug/properly be a regression test, the class has to look like:

class A: def __ipow__(self, _): return NotImplemented def __pow__(self, other): ...

if the class looks like

class A: def __pow__(self, other): ...

then the old logic returns a false here and correctly goes to the non-inplace version of power anyway:

cpython/Objects/abstract.c

Lines 1162 to 1163 in 8e19c8b

if (Py_TYPE(v)->tp_as_number &&

Py_TYPE(v)->tp_as_number->nb_inplace_power != NULL) {

…8302-ipow-fix

ashkop · 2021-02-22T09:28:54Z

Merged upstream, added Py_TEST usage, removed TERNARY_OP definition since op_name is now used in the error message.

brettcannon · 2021-02-26T19:58:01Z

I also tested this PR against my own test suite that uncovered this issue and it passed!

bedevere-bot · 2021-02-26T19:58:41Z

@brettcannon: Please replace # with GH- in the commit message next time. Thanks!

brettcannon · 2021-02-26T19:59:26Z

@ashkop thanks for the PR!

…mented (python#16459)

the-knights-who-say-ni added the CLA signed label Sep 28, 2019

bedevere-bot added the awaiting review label Sep 28, 2019

brandtbucher requested changes Sep 28, 2019

View reviewed changes

bedevere-bot added awaiting core review and removed awaiting review labels Sep 28, 2019

brandtbucher added the type-bug An unexpected behavior, bug, or error label Sep 28, 2019

This comment has been minimized.

Sign in to view

brandtbucher requested changes Sep 29, 2019

View reviewed changes

brandtbucher approved these changes Sep 29, 2019

View reviewed changes

serhiy-storchaka approved these changes Aug 24, 2020

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting core review labels Aug 24, 2020

ammaraskar requested changes Aug 24, 2020

View reviewed changes

ashkop and others added 7 commits August 25, 2020 17:13

bpo-38302. __rpow__ now called when __ipow__ returns NotImplemented

75aba4a

📜🤖 Added by blurb_it.

c981f0d

Fix review comments

9005cce

Use object() in test

8a357bb

Add tests, update news entry

6426025

Add whatsnew entry

84241a2

Fix whitespace

25f95b5

ashkop force-pushed the bpo-38302-ipow-fix branch from a2476e7 to 25f95b5 Compare August 25, 2020 15:39

ammaraskar approved these changes Aug 25, 2020

View reviewed changes

ammaraskar mentioned this pull request Feb 20, 2021

bpo-38302: when __ipow__ returns NotImplemented, fall back to __pow__/__rpow__ #24587

Closed

ashkop added 2 commits February 22, 2021 10:36

Merge branch 'master' of https://github.com/python/cpython into bpo-3…

c00fb1a

…8302-ipow-fix

Remove TERNARY_OP since op_name is now used in error message

80cad6c

brettcannon self-requested a review February 23, 2021 01:15

brettcannon approved these changes Feb 26, 2021

View reviewed changes

brettcannon merged commit cc02b4f into python:master Feb 26, 2021

bedevere-bot removed the awaiting merge label Feb 26, 2021

adorilson pushed a commit to adorilson/cpython that referenced this pull request Mar 13, 2021

bpo-38302: __pow__/__rpow__ now called when __ipow__ returns NotImple…

3be4b90

…mented (python#16459)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bpo-38302. rpow now called when ipow returns NotImplemented #16459

bpo-38302. rpow now called when ipow returns NotImplemented #16459

ashkop commented Sep 28, 2019 •

edited by bedevere-bot

Loading

brandtbucher left a comment

brandtbucher Sep 28, 2019 •

edited

Loading

ashkop Sep 29, 2019

brandtbucher Sep 28, 2019

ashkop Sep 29, 2019

ammaraskar Aug 24, 2020

ashkop Aug 25, 2020

ammaraskar Aug 25, 2020

ashkop Aug 26, 2020

This comment has been minimized.

brandtbucher left a comment

brandtbucher Sep 29, 2019

ashkop Sep 29, 2019

serhiy-storchaka left a comment

ammaraskar Aug 24, 2020

ammaraskar Aug 24, 2020 •

edited

Loading

ashkop Aug 25, 2020

ammaraskar Aug 24, 2020

ashkop Aug 25, 2020

ammaraskar Aug 25, 2020

ashkop commented Feb 22, 2021

brettcannon commented Feb 26, 2021

bedevere-bot commented Feb 26, 2021

brettcannon commented Feb 26, 2021

	if (Py_TYPE(v)->tp_as_number &&
	Py_TYPE(v)->tp_as_number->nb_inplace_power != NULL) {

		@@ -0,0 +1 @@
		If :func:`object.__ipow__` returns :const:`NotImplemented` :func:`object.__rpow__` will be called now as expected.

bpo-38302. __rpow__ now called when __ipow__ returns NotImplemented #16459

bpo-38302. __rpow__ now called when __ipow__ returns NotImplemented #16459

Conversation

ashkop commented Sep 28, 2019 • edited by bedevere-bot Loading

brandtbucher left a comment

Choose a reason for hiding this comment

brandtbucher Sep 28, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

brandtbucher left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ammaraskar Aug 24, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ashkop commented Feb 22, 2021

brettcannon commented Feb 26, 2021

bedevere-bot commented Feb 26, 2021

brettcannon commented Feb 26, 2021

bpo-38302. rpow now called when ipow returns NotImplemented #16459

bpo-38302. rpow now called when ipow returns NotImplemented #16459

ashkop commented Sep 28, 2019 •

edited by bedevere-bot

Loading

brandtbucher Sep 28, 2019 •

edited

Loading

ammaraskar Aug 24, 2020 •

edited

Loading