Support comparison operators #1347

christabella · 2021-01-06T15:48:46Z

What does this implement/fix? Explain your changes.

Continuation of https://github.com/pythonnet/pythonnet/pull/1324/files but specifically for comparison operators e.g. >, >=, <, == etc. based on a discussion with @tminka --- in a nutshell, we want to check each C# class for any comparison operator methods when calling ClassBase.tp_richcompare, before proceeding with the usual logic (see #294) which handles C# classes that implement an IComparable interface.

Would appreciate a review from @lostmsu who helped with the previous PR that this builds on. Thanks!

Does this close any currently open issues?

Closes #1312
More concrete examples can also be found in other Infer.NET tutorials e.g. having to use op_GreaterThan https://github.com/dotnet/infer/blob/67b4f80d97018460bcb817f76ec874d0f33f1651/test/TestPython/test_tutorials.py#L31

Any other comments?

Some remaining tasks could be:

Add tests for derived objects, and for other edge cases (e.g. if truly no operator methods defined, asserting good error messages)
When C# class defines operator ==(), the class should also define .Equals and .GetHashCode() https://stackoverflow.com/questions/10790370/whats-wrong-with-defining-operator-but-not-defining-equals-or-gethashcode

Checklist

Check all those that are applicable and complete.

Make sure to include one or more tests for your change
If an enhancement PR, please create docs and at best an example
Add yourself to AUTHORS
Updated the CHANGELOG

The tests pass in net472 but not on netcoreapp3.1

lostmsu · 2021-01-07T18:33:55Z

src/runtime/classbase.cs

@@ -21,6 +21,7 @@ internal class ClassBase : ManagedType
        [NonSerialized]
        internal List<string> dotNetMembers;
        internal Indexer indexer;
+        internal Hashtable richcompare;


Why not have it as Dictionary<int, MethodObject>, mapping directly from {Py_EQ, ...} to corresponding operator implementation?

lostmsu · 2021-01-07T18:46:05Z

src/runtime/classbase.cs

+            // otherwise fallback to checking if an IComparable interface is handled.
+            if (PyToCilOpMap.ContainsKey(op)) {
+                string CilOp = PyToCilOpMap[op];
+                if (cls.richcompare.Contains(CilOp)) {


Generally, you'd do cls.richcompare.TryGetValue(op, out var methodObject) instead of having separate ContainsKey check.

lostmsu · 2021-01-07T18:52:05Z

src/runtime/classbase.cs

+                    var methodObject = (MethodObject)cls.richcompare[CilOp];
+                    IntPtr args = other;
+                    var free = false;
+                    if (!Runtime.PyTuple_Check(other))


I don't think the code inside this if should ever be skipped. How would a > (1,2) work if a was a .NET object with > operator? Because (1,2) is a tuple, they'd be passed to Invoke as two args instead of 1.

Consequently, free will alwasy be true and not needed.

Probably a good idea to add a test. I believe you'd need an operator >(SomeClass a, PyObject b) defined and to check b indeed receives the tuple (1,2).

Indeed if b were a tuple this if would be skipped. Once removing the if, the operator method does receive b as PyObject, but I get an AccessViolationException : Attempted to read or write protected memory. so I think b is wrongly parsed? I suppose a tuple when converted into a PyObject should probably not look like this:

I think that error message might have been a flaky bug, because now the variable states are the same but the error is:

Message: Python.Runtime.PythonException : TypeError : '>=' not supported between instances of 'int' and 'tuple'

This is because to work with PyObject instances in C# callbacks you must hold GIL. E.g. the method body must be inside using (Py.GIL()) { ... block. It applies to debugging too.

Thanks, I didn't know about that. I put the operator >(Obj a, PyObject b) body in a Py.GIL block and can now see that b has a value of {(1, 2)}

I added 2/6 of the tuple comparison tests but it's done in a bit of a strange way. In the test I'm assuming that the PyObject is a tuple; is that okay?

(Below is resolved)

~~However, the same error message persists, and when I try to debug~~

pythonnet/src/runtime/methodbinder.cs

Line 847 in d6c0081

result = binding.info.Invoke(binding.inst, BindingFlags.Default, null, binding.args, null);

I get this error

Maybe I'm missing something simple again, should the GIL block should also be wrapping that statement? But that would make the change bigger than I thought. Are tuples commonly used enough for comparison operators, or can I put the tuple test in a different PR?

I don't think you need to add a test with a tuple for each comparison operator. One is enough.

The failure on the screenshot is only happening because you have debugger attached. It tries to call ToString on some PyObject to show in Watches window, but it does not work without Py.GIL

lostmsu · 2021-01-07T19:03:57Z

src/runtime/operatormethod.cs

+                ["op_Equality"] = new SlotDefinition("__eq__", TypeOffset.tp_richcompare),
+                ["op_Inequality"] = new SlotDefinition("__ne__", TypeOffset.tp_richcompare),
+                ["op_LessThanOrEqual"] = new SlotDefinition("__le__", TypeOffset.tp_richcompare),
+                ["op_GreaterThanOrEqual"] = new SlotDefinition("__ge__", TypeOffset.tp_richcompare),
+                ["op_LessThan"] = new SlotDefinition("__lt__", TypeOffset.tp_richcompare),
+                ["op_GreaterThan"] = new SlotDefinition("__gt__", TypeOffset.tp_richcompare),


I think this is error-prone. op_LessThan does not really implement tp_richcompare. It would be better to have a separate ComparisonOpMap, string -> string, and fix IsOperatorMethod correspondingly.

It would also remove the need to change FixupSlots below.

Makes sense, thanks! Should that ComparisonOpMap also be merged with the other map for op_LessThan -> Py_LT?

I added this OperatorMethod.ComparisonOpMap in the latest commit, without merging it with the ClassBase.CilToPyOpMap, although they both have the same keys so perhaps they should be together in OperatorMethod.

lostmsu · 2021-01-07T19:04:34Z

src/runtime/operatormethod.cs

@@ -86,7 +99,7 @@ public static void FixupSlots(IntPtr pyType, Type clrType)
            Debug.Assert(_opType != null);
            foreach (var method in clrType.GetMethods(flags))
            {
-                if (!IsOperatorMethod(method))
+                if (!IsOperatorMethod(method) || IsComparisonOp(method))  // We don't want to override ClassBase.tp_richcompare.


Direct comment to the reason why we don't want to override, otherwise it does not explain anything and simply says what code in this method does (which is generally not very useful).

Something like "comparison operators are handled by ClassBase.tp_richcompare"

tminka · 2021-01-08T08:25:25Z

src/embed_tests/TestOperator.cs

@@ -25,6 +25,17 @@ public class OperableObject
        {
            public int Num { get; set; }

+            public override int GetHashCode()
+            {
+                return 159832395 + Num.GetHashCode();


unchecked(159832395 + Num.GetHashCode())

src/embed_tests/TestOperator.cs

lostmsu · 2021-01-11T19:56:10Z

src/runtime/classbase.cs

+                if (true)
+                {


Please, remove unnecessary if and free boolean (always true)

lostmsu · 2021-01-11T20:11:20Z

src/runtime/operatormethod.cs

-                if (!IsOperatorMethod(method))
+                // We don't want to override slots for either non-operators or
+                // comparison operators, which are handled by ClassBase.tp_richcompare.
+                if (!IsOperatorMethod(method) || IsComparisonOp(method))


NIT: !OpMethodMap .ContainsKey(method)

Of course, thanks!

christabella marked this pull request as draft January 6, 2021 15:55

christabella force-pushed the feat/comparison-operators branch from 6196140 to 3c4ea26 Compare January 7, 2021 07:13

christabella marked this pull request as ready for review January 7, 2021 07:45

christabella mentioned this pull request Jan 7, 2021

Operator overloads support #1324

Merged

4 tasks

lostmsu requested changes Jan 7, 2021

View reviewed changes

tminka reviewed Jan 8, 2021

View reviewed changes

christabella commented Jan 11, 2021

View reviewed changes

src/embed_tests/TestOperator.cs Show resolved Hide resolved

lostmsu requested changes Jan 11, 2021

View reviewed changes

christabella added 12 commits January 12, 2021 09:19

Add comparison operators

9a8cb96

Add tests for logical (bool) comparison operators

bcbace3

Finish comparison operator impl and add tests

31f3ed1

Add eq and ineq operators, disallow reverse for comp

ae7e774

Minor formatting (whitespace, ordering)

5685727

Minor: use out var instead of declaring separately.

03a73c0

Implement .Equals and .GetHashCode to fix warning

67928e1

Style (TryGetValue), formatting, and minor changes

6a64678

Use a separate map for comparison operators

9dd9162

Add tuple comparison operator tests (2/6)

a8c6591

Add reverse case for tuple comparison op test

6d4dec7

Simplify PyObj casting, logic of if blocks.

7a2b5e2

christabella force-pushed the feat/comparison-operators branch from a9e28ba to 7a2b5e2 Compare January 12, 2021 08:00

lostmsu approved these changes Jan 12, 2021

View reviewed changes

lostmsu merged commit e44aa46 into pythonnet:master Jan 12, 2021

christabella deleted the feat/comparison-operators branch January 13, 2021 08:15

Support comparison operators #1347

Support comparison operators #1347

Uh oh!

Conversation

christabella commented Jan 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this implement/fix? Explain your changes.

Does this close any currently open issues?

Any other comments?

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lostmsu Jan 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

christabella Jan 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

christabella Jan 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lostmsu Jan 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

christabella Jan 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lostmsu Jan 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lostmsu Jan 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

christabella Jan 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

christabella commented Jan 6, 2021 •

edited

Loading

lostmsu Jan 7, 2021 •

edited

Loading

christabella Jan 8, 2021 •

edited

Loading

christabella Jan 8, 2021 •

edited

Loading

lostmsu Jan 9, 2021 •

edited

Loading

christabella Jan 11, 2021 •

edited

Loading

lostmsu Jan 11, 2021 •

edited

Loading

lostmsu Jan 7, 2021 •

edited

Loading

christabella Jan 8, 2021 •

edited

Loading