Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
slozier opened this issue Sep 9, 2022 · 8 comments

Comments

@slozier
Copy link
Contributor

slozier commented Sep 9, 2022

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 #52?

@BCSharp
Copy link
Member

BCSharp commented Sep 9, 2022

Possibly. Will look into it.

@BCSharp
Copy link
Member

BCSharp commented Sep 12, 2022

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
Copy link
Contributor Author

slozier commented Sep 12, 2022

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

@BCSharp
Copy link
Member

BCSharp commented Sep 12, 2022

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

@BCSharp
Copy link
Member

BCSharp commented Sep 13, 2022

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
Copy link
Contributor Author

slozier commented Sep 13, 2022

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
Copy link
Member

BCSharp commented Sep 13, 2022

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
Copy link
Contributor Author

slozier commented Sep 13, 2022

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants