only lookup __[set/get/del]slice__ for slice AST nodes

[undingen]

only lookup __[set/get/del]slice__ for slice AST nodes
before this change
C()[slice(1,2)] = "a"
would call into __setslice__ because it got handled as
C()[1,2] = "a"
the problems only appeared after the guarding change in setitem because before we called mp_ass_subscript even when it was slot_mp_ass_subscript when we encountered before a type which had a != slot_mp_ass_subscript (we missed the guard)

I'm not sure if there is not a much nicer way to resolve this which I missed..

[Daetalus]

I know this problem was caused by Pyston try to call __setslice__ when it should call __setitem__. And this problem was always there. But why the first commit can trigger it. And why it still there even if I make a clean build?

[undingen]

the first commit does not introduce the problem it just makes it more likely to hit it because it introduces stricter mp_ass_subscript guarding.
Before we were sometimes calling into mp_ass_subscript even though it was set to slot_mp_ass_subscript. (this could happen in a rewrite path where we first encountered a type with != slot_mp_ass_subscript slot and rewrote it too a call to the setitemHelper function. slot_mp_ass_subscript will not lookup __setslice__ so we did not encounter the problem. )
Now we will never call into mp_ass_subscript if it is set to slot_mp_ass_subscript but this means we are not allowed to do the __setslice__ lookup if the AST node is not of type slice.

Hope this made it clearer.

[kmod]

Oh interesting, I didn't really notice that mp_ass_subscript block. It seems weird/wrong that setitem() will check for mp_ass_subscript first without considering whether there was a sq_ass_slice (at least for slice cases).

It looks like the if (m && m->mp_ass_subscript && m->mp_ass_subscript != slot_mp_ass_subscript) { block is an optimization and it instead should be something like

if (should_check_slice && sq_ass_slice) {
  if (sq_ass_slice != slot_sq_ass_slice)
    rewriter->call(sq_ass_slice);
  // else fall through to callItemOrSliceAttr
} else if (mp_ass_subscript && mp_ass_subscript != slot_mp_ass_subscript {
  rewriter->call(mp_ass_subscript);
}

I think the != slot_mp_ass_subscript check will basically end up skipping this block for any Python-defined classes, but if there is a C type that has both a mp_ass_subscript and a sq_ass_slice, we will end up calling mp_ass_subscript which I think is wrong. I was able to reproduce this by taking our SlotsTesterMap type, making it subclassable and then running this test:

import slots_test
class D(slots_test.SlotsTesterMap):
    def __setslice__(self, *args):
        print "setslice", args

d = D(1)
for i in xrange(10):
    print i
    d[1:2] = 1

which ends up calling mp_ass_subscript instead of the __setslice__ like it is supposed to.

[undingen]

So I did modify this patch heavily but think this is now much cleaner
I split the slice and normal getitem handling like cpython does and added a rewriter path for the most important path in the new functions.
Ready for review 👍