Remove comparison operator fallbacks

[gfmcknight]

Remove type comparison fallbacks for the '<', '>', '<=', and '>='
operators and throw a TypeError instead when no suitable comparator is
found between two types.

Listed in #33.

[slozier]

You picked a complicated issue to tackle, but I think this is a good first step!

[slozier]

Can we use this format instead?

with self.assertRaises(TypeError):
    eval('x ' + op + ' 3')

Also, I assume CPython also throws?

[gfmcknight]

Ah; this doesn't throw in CPython, and I think that the whole premise of the test doesn't apply anymore?

>>> global called
>>> class C:
...     for method, op in [('__eq__', '=='), ('__gt__', '>'), ('__lt__', '<'), ('__ge__', '>='), ('__le__', '<='), ('__ne__', '!=')]:
...         exec("""
... def %s(self, *args, **kwargs):
...     global called
...     called = '%s'
...     return True
... """ % (method, op))
...
>>> a = C()
>>> import _weakref
>>> x = _weakref.proxy(a)
>>> eval('x < 3')
True
>>> called
'<'
>>> eval('x == 3')
True
>>> called
'=='

[slozier]

Indeed. It seems like it calls all the underlying operators with Python 3? I guess the test should be inverted.

[gfmcknight]

Then let's change the test to look for the new behavior an add an expected failure annotation instead? This looks like a pretty major feature -- I wonder it's under any of the What's new in Python 3.x updates.

[slozier]

Agreed, let's update the test to the new behavior. We should file an issue about it and reference it in the test if possible.

I just tried with 3.0 and it looks like the operators "flow through" the proxy. Kind of makes sense since the proxy is supposed to be a stand-in for the actual value. So if it's in any of the release notes it would be in 3.0.

[gfmcknight]

I couldn't find it in any release notes, but I believe that it was discussed in this bug: https://bugs.python.org/issue9658 and the one linked at the bottom of it as well. It looks to me like operators in general have been added incrementally to the proxy implementation.

[slozier]

Any idea in what other cases MakeBinaryThrow is used? Just wondering if changing the error message to match CPython will break other error messages.

[gfmcknight]

MakeBinaryThrow is called for all binary operators I think; it looks like those also use the symbol in Python 3, so I just changed it here.

[slozier]

Ok, looks like the error message is slightly different (CPython 3.8) for the comparison operators:

>>> 1 ^ None
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: unsupported operand type(s) for ^: 'int' and 'NoneType'
>>> 1 < None
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: '<' not supported between instances of 'int' and 'NoneType'

Since the error messages basically mean the same thing I'm fine with diverging from CPython as long as there are no StdLib tests wanting to checking the actual message.

[slozier]

Not quite what I had in mind. I was thinking more like PythonTuple.MakeTuple(de.Key, de.Value). I'm surprised the test is passing! I guess tuple comparison needs work (e.g. (1,) < (None,) should throw).

[gfmcknight]

I guess because we use comparison in CompareArrays():

ironpython3/Src/IronPython/Runtime/Operations/PythonOps.cs

Lines 688 to 696 in fa2d397

	public static int CompareArrays(object[] data0, int size0, object[] data1, int size1) {
	int size = Math.Min(size0, size1);
	for (int i = 0; i < size; i++) {
	int c = PythonOps.Compare(data0[i], data1[i]);
	if (c != 0) return c;
	}
	if (size0 == size1) return 0;
	return size0 > size1 ? +1 : -1;
	}

[slozier]

Yeah, something for a separate PR I think.

[slozier]

Thanks for the PR!