Rich compasions of Int32-based ints and subclasses of int return NotImplemented

[slozier]

trafficstars

Calling a rich comparison directly on an instance of an Int32-based int returns NotImplemented for subclasses of int. In CPython, the comparison succeeds. However, it works properly for BigInteger. for For example:

import System

class test(int): pass

assert (0).__lt__(test(1)) is True

assert (0).ToBigInteger().__lt__(test(1)) is True

Seems to have broken somewhere between the alpha and the beta releases. @BCSharp could this be related to your work on https://github.com/IronLanguages/ironpython3/issues/52?

[BCSharp]

Possibly. Will look into it.

[BCSharp]

For example:

Hmm, the example given above works for me (after adding import System at the top) across all supported frameworks, using the latest commit from master. Am I missing something?

[slozier]

Ahh, sorry about that. Should have is True in the assert statements. Just edited the top post with the proper repro.

[BCSharp]

Thanks. First bad commit is 3ed501b6b2e74d9284ee213db2753614e102144b (it's mine indeed).

[BCSharp]

The cause for this regression is that in beta subclasses of int are derived from BigInteger, while it was Int32 in alpha. The issue is not specific to sublasses of int but affects any instances of BigInteger. E.g. (0).__lt__(1<<64) also returns NotImplemented. This was already the case in alpha.

Looking at the code, (0).__lt__(test(1)) returning NotImplemented is a deliberate performance optimization. It is meant to invoke test(1).__ge__(0), which is implemented. Is this a problem? I can add more overloads to Int32 to avoid it. But what about other operators, e.g. arithmetic (like __mul__), bitwise, etc.?

If this is just a matter of 100% CPython compatibility, then I assume it only applies to Int32 instances, and not to other CLR integer types.

[slozier]

I'm not sure if there are real world scenarios, but the one I ran into was basically this (from test_heapq):

class EvilClass(int):
    def __lt__(self, o):
        # do something
        return NotImplemented

EvilClass(1) < 2

which ends up throwing a TypeError because both sides return NotImplemented.

Looks like any reversible operator "implemented" like this would end up throwing so just adding the rich comparison overloads would not solve the issue.

Anyway, I don't think it's super critical - was just wondering if there was a quick fix.

[BCSharp]

The fix would be to modify generate_alltypes.py to generate overloads in Int32Ops accepting BigInteger, Int64, and UInt64 as the second argument for all reversible operators. I don't think it would be complicated. The biggest challenge is not to forget any operator.

[slozier]

Well, I guess it would save me having to patch test_heapq in 3.6. Although the tests appear to be failing for other reasons after they're patched so until I figure out and fix that other failure, I have no compelling reasons to push for this fix (beyond that it's a regression from the alpha).