NotImplemented->TypeError in __add__ and __mul__

[arigo]

BPO	1390657
Nosy	@malemburg, @arigo, @nascheme
Files	concat_repeat_cleanup.diff: Dont' set sq_concat/sq_repeat slots. sequence_operations.diff: Make PySequence_Concat/Repeat() work more often.

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2006-02-20.10:21:36.000>
created_at = <Date 2005-12-26.16:09:50.000>
labels = ['interpreter-core']
title = 'NotImplemented->TypeError in __add__ and __mul__'
updated_at = <Date 2006-02-20.10:21:36.000>
user = 'https://github.com/arigo'

bugs.python.org fields:

activity = <Date 2006-02-20.10:21:36.000>
actor = 'arigo'
assignee = 'none'
closed = True
closed_date = None
closer = None
components = ['Interpreter Core']
creation = <Date 2005-12-26.16:09:50.000>
creator = 'arigo'
dependencies = []
files = ['6930', '6931']
hgrepos = []
issue_num = 1390657
keywords = ['patch']
message_count = 10.0
messages = ['49221', '49222', '49223', '49224', '49225', '49226', '49227', '49228', '49229', '49230']
nosy_count = 3.0
nosy_names = ['lemburg', 'arigo', 'nascheme']
pr_nums = []
priority = 'normal'
resolution = 'accepted'
stage = None
status = 'closed'
superseder = None
type = None
url = 'https://bugs.python.org/issue1390657'
versions = []

[arigo]

This is a fix for bug bpo-1355842, with a comprehensive
test checking that implementing a binary special method
but returning NotImplemented does indeed cause a
TypeError to be raised. In addition to the case
reported in bpo-1355842 (x+=y) the test found another case
where NotImplemented was just returned (A()*5), a case
where an OverflowError or ValueError was raised instead
of TypeError (A()*large_integer), and a case where an
unexpected AttributeError was raised (5*A()).

The proposed patch also reverts the changes done in svn
revisions 25556-25557 corresponding to bug bpo-516727,
because it makes that fix unnecessary by adopting a
more radical approach.

All the problems are due to hard-to-control
interactions between the various PyTypeObject slots for
addition and multiplication (nb_add vs sq_concat,
nb_multiply vs sq_repeat). The approach of the present
patch is to not define sq_concat, sq_repeat,
sq_inplace_concat and sq_inplace_repeat slots *at all*
for user-defined types, even if they have __add__ and
__mul__ methods. This is the only sane solution I
found, specially with respect to the
OverflowError/ValueError problem (caused by trying to
convert the integer to a C int in order to try to call
sq_repeat). It is also consistent with the behavior of
instances of old-style classes -- the InstanceType did
not define sq_concat and sq_repeat, either.

I'll propose a redefinition of operator.concat() and
operator.repeat() on python-dev.

[arigo]

Logged In: YES
user_id=4771

Attached a second patch, extending PySequence_Repeat() and
PySequence_Concat() to match more precisely their
documentation, which says that they should be equivalent
to multiplication and addition respectively, at least when
the proper arguments are sequences.

In 2.4 and HEAD, it works with new-style instances defining
__add__ or __mul__, but it does not work with old-style
instances. If the concat_repeat_cleanup.diff patch is
checked in, then it stops working for new-style instances
too. So the sequence_operations.diff patch cleans up the
situation by making this work in all cases.

Compatibility note: PySequence_Concat/Repeat() are mostly
never used in the core; they are essentially only used by
operator.concat() and operator.repeat(). Both patches
together should thus not break existing code and could be
considered as a bug fix. I'd recommend that we check them
in both HEAD and the 2.4 branch.

The patch also addresses PySequence_InPlaceConcat/Repeat()
but without testing them -- they can't be tested from
Python, so they are already completely untested.

[nascheme]

Logged In: YES
user_id=35752

Your change to abstract.c that adds "result =
binop_type_error(...)" is certainly correct.
Py_NotImplemented should not be returned.

Small nits: why not remove the SLOT1 lines from typeobject.c
instead of commenting them out? If you want to leave them
as comments then you should add an explaination as to why
they should not be there. Also, I don't personally like
having SF issue numbers in comments as I would rather have
the comment be self explanatory. Who knows, SF might go away.

The changes to PySequence_* look correct to me as well.

[arigo]

Logged In: YES
user_id=4771

Thanks for the comments about the comments, I will fix them.

I don't understand the note about abstract.c, though.
I think you are referring to the change in PyNumber_Add(),
but this change is basically a no-op. You might be reading
the diff in the wrong direction: it appears to *remove* a
check for Py_NotImplemented. However, this check was added
in r25556 and r25557 to fix another bug, and this is no
longer needed if the four SLOT1() lines go away.

[nascheme]

Logged In: YES
user_id=35752

I'm not sure what Py_NotImplemented check you are referring
to but I think the patches do the right thing and could be
checked in as is.

[malemburg]

Logged In: YES
user_id=38388

Haven't looked at the patches, but your comment "The patch
also addresses PySequence_InPlaceConcat/Repeat()
but without testing them -- they can't be tested from
Python, so they are already completely untested." is not
quite correct:

We have the _testcapimodule.c for doing C API tests.

Do you also address the problem I posted to python-dev ?

"""...the following code
could be the cause for leaking a NotImplemented singleton
reference:

#define SLOT1BINFULL(FUNCNAME, TESTFUNC, SLOTNAME, OPSTR,
ROPSTR) \
static PyObject * \
FUNCNAME(PyObject *self, PyObject *other) \
{ \
	static PyObject *cache_str, *rcache_str; \
	int do_other = self->ob_type != other->ob_type && \
	    other->ob_type->tp_as_number != NULL && \
	    other->ob_type->tp_as_number->SLOTNAME == TESTFUNC; \
	if (self->ob_type->tp_as_number != NULL && \
	    self->ob_type->tp_as_number->SLOTNAME == TESTFUNC) { \
		PyObject *r; \
		if (do_other && \
		    PyType_IsSubtype(other->ob_type, self->ob_type) && \
		    method_is_overloaded(self, other, ROPSTR)) { \
			r = call_maybe( \
				other, ROPSTR, &rcache_str, "(O)", self); \
			if (r != Py_NotImplemented) \
				return r; \
			Py_DECREF(r); \
			do_other = 0; \
		} \
		r = call_maybe( \
			self, OPSTR, &cache_str, "(O)", other); \
		if (r != Py_NotImplemented || \
		    other->ob_type == self->ob_type) \
                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
If both types are of the same type, then a NotImplemented
returng
value would be returned.

			return r; \
		Py_DECREF(r); \
	} \
	if (do_other) { \
		return call_maybe( \
			other, ROPSTR, &rcache_str, "(O)", self); \
	} \
	Py_INCREF(Py_NotImplemented); \
	return Py_NotImplemented; \
}

"""

[arigo]

Logged In: YES
user_id=4771

Thanks for the reminder! It doesn't change the fact that
PySequence_InPlace*() is not tested, but indeed now I know
where I should add a test for them.

About the python-dev post, see my python-dev answer: it's
expected of this function to be able to return
Py_NotImplemented, as is obvious from the last return
statement :-)

(Updated patch to come...)

[arigo]

Logged In: YES
user_id=4771

Improved the comments and checked in as r41842.

I have tested PySequence_InPlace*() "manually". I am
thinking about adding the in-place operations to the
operator module; this should make all these C functions
more easily testable.

[arigo]

Logged In: YES
user_id=4771

Checked in new built-ins operator.ixxx(), with docs and tests.

[arigo]

Logged In: YES
user_id=4771

Back-ported to 2.4 as r42511.