Repository: jpype-project/jpype (language: python)

Issue:
__eq__ for comparable should use equals()?
Currently JPype's implementation of `__eq__` uses `compareTo()` if one of the objects implements `Comparable`. However, this leads to problems when comparing objects of two different classes, as `compareTo()` usually does not handle this case, e.g. for Enums which implement `compareTo()` only among objects of the same class.

Also, in Java it is only recommended - but not strictly required - that `compareTo()` is consistent with `equals()`.

Is there a reason why not to use `equals()` for comparing any object?

Example:
```python
import jpype as jp
jp.startJVM()
jp.java.time.temporal.ChronoUnit.SECONDS.equals(jp.java.util.concurrent.TimeUnit.SECONDS)
# >>> False
jp.java.time.temporal.ChronoUnit.SECONDS == jp.java.util.concurrent.TimeUnit.SECONDS
```
The last line crashes with:
```
Traceback (most recent call last):
  File "Enum.java", line 55, in java.lang.Enum.compareTo
Exception: Java Exception
The above exception was the direct cause of the following exception:
Traceback (most recent call last):
  File "<input>", line 1, in <module>
java.lang.java.lang.ClassCastException: java.lang.ClassCastException
```


Discussion / hints:
Hmm.  seems like an issue.   So should we always use ``equals()``?  I though that equals was supposed to be strict and ``compareTo`` was supposed to be more lenient but it looks like this is the opposite case.   Looking at the documentation I would not have expected the ClassCastException, but I think that fixing it to always use equals (or !equals) would be safest.


Java `Object` defines `equals(Object)`, which is supposed to accept any other object to test for equality. If the two objects are of different classes, `equals()` shall not throw but instead just return `false` (or `true` in case they are of different classes but nevertheless considered "equal" e.g. different implementations of `Set` containing the same elements).

On the other hand, `Comparable<T>` implements `compareTo(T)` which is only expecting other objects of type `T`. Often enough, it is uses as in `class Foo implements Comparable<Foo>`, i.e. only allowing comparisons with itself. In Java this is checked at compile time...

So yes, I'm tempted to say that `__eq__` should always use `equals()` (and `__ne__` likewise). `compareTo()` should only be used for `__lt__`, `__le__`, `__gt__` and `__ge__`. At least, that's what I would naively expect.

Fix patch:
diff --git a/doc/CHANGELOG.rst b/doc/CHANGELOG.rst
index 4c5684a58..c4e08c0ab 100644
--- a/doc/CHANGELOG.rst
+++ b/doc/CHANGELOG.rst
@@ -5,11 +5,20 @@ This changelog *only* contains changes from the *first* pypi release (0.5.4.3) o
 
 Latest Changes:
 - **1.5.0_dev0 - 2023-04-03**
-- **1.4.2_dev0 - 2022-10-26**
+
+  - Switched ``__eq__`` and ``__ne__`` operator to use ``equals`` rather than
+    ``compareTo`` for comparable objects to avoid exception when comparing
+    object of different types.
+
+  - Fixed segmentation fault when comparing Java Comparable to primitives.
+
+  - Java exceptions that occur in inequality comparisons now map to Python
+    TypeError.
 
   - Fixed crash when calling subscript on JArray.
 
-  - Fixed direct byte buffers not reporting nbytes correctly when cast to memoryview.
+  - Fixed direct byte buffers not reporting nbytes correctly when cast to
+    memoryview.
 
   - Expand the defintion for Functional interface to include classes without
     FunctionInterface annotation.
diff --git a/native/common/jp_javaframe.cpp b/native/common/jp_javaframe.cpp
index d05b0480a..42d5739e9 100644
--- a/native/common/jp_javaframe.cpp
+++ b/native/common/jp_javaframe.cpp
@@ -1194,7 +1194,13 @@ jint JPJavaFrame::compareTo(jobject obj, jobject obj2)
 {
 	jvalue v;
 	v.l = obj2;
-	return CallIntMethodA(obj, m_Context->m_CompareToID, &v);
+	jint ret = m_Env->CallIntMethodA(obj, m_Context->m_CompareToID, &v);
+	if (m_Env->ExceptionOccurred())
+	{
+		m_Env->ExceptionClear();
+		JP_RAISE(PyExc_TypeError, "Unable to compare")
+	}
+	return ret;
 }
 
 jthrowable JPJavaFrame::getCause(jthrowable th)
diff --git a/native/python/pyjp_object.cpp b/native/python/pyjp_object.cpp
index 494175f12..0e47f4fb6 100644
--- a/native/python/pyjp_object.cpp
+++ b/native/python/pyjp_object.cpp
@@ -55,8 +55,6 @@ static PyObject *PyJPObject_compare(PyObject *self, PyObject *other, int op)
 	if (op == Py_NE)
 	{
 		PyObject *ret = PyJPObject_compare(self, other, Py_EQ);
-		if (ret == NULL)
-			return NULL;
 		int rc = (ret == Py_False);
 		Py_DECREF(ret);
 		return PyBool_FromLong(rc);
@@ -120,7 +118,7 @@ static PyObject *PyJPComparable_compare(PyObject *self, PyObject *other, int op)
 	bool null1 = false;
 
 	// First slot is Null
-	if (self == Py_None || javaSlot0 == NULL ||
+	if (javaSlot0 == NULL ||
 			(!javaSlot0->getClass()->isPrimitive() && javaSlot0->getValue().l == NULL))
 		null0  = true;
 	if (other == Py_None || (javaSlot1 != NULL &&
@@ -153,8 +151,16 @@ static PyObject *PyJPComparable_compare(PyObject *self, PyObject *other, int op)
 			Py_INCREF(out);
 			return out;
 		}
+		if (match.type < JPMatch::Type::_implicit)
+		{
+			if (op == Py_EQ || op == Py_NE)
+				return  PyBool_FromLong(op == Py_NE);
+			PyObject *out = Py_NotImplemented;
+			Py_INCREF(out);
+			return out;
+		}
 		obj1 = match.convert().l;
-	} else if (!null1 && javaSlot1 != NULL)
+	} else if (!null1 && javaSlot1 != NULL && !javaSlot1->getClass()->isPrimitive())
 		obj1 = javaSlot1->getValue().l;
 
 	switch (op)
@@ -164,14 +170,13 @@ static PyObject *PyJPComparable_compare(PyObject *self, PyObject *other, int op)
 				Py_RETURN_TRUE;
 			if (null0 || null1)
 				Py_RETURN_FALSE;
-			return PyBool_FromLong(frame.compareTo(obj0, obj1) == 0);
-
+			return PyBool_FromLong(frame.equals(obj0, obj1));
 		case Py_NE:
 			if (null0 && null1)
 				Py_RETURN_FALSE;
 			if (null0 || null1)
 				Py_RETURN_TRUE;
-			return PyBool_FromLong(frame.compareTo(obj0, obj1) != 0);
+			return PyBool_FromLong(!frame.equals(obj0, obj1));
 		case Py_LT:
 			if (null0 || null1)
 				break;


Test patch:
diff --git a/test/jpypetest/test_comparable.py b/test/jpypetest/test_comparable.py
index 273bdc2de..ce54b223f 100644
--- a/test/jpypetest/test_comparable.py
+++ b/test/jpypetest/test_comparable.py
@@ -79,3 +79,43 @@ def testComparableNull(self):
             print(i3 > i3)
         with self.assertRaises(ValueError):
             print(i3 >= i3)
+
+    def testComparableObj(self):
+        C1 = jpype.JClass("java.time.temporal.ChronoUnit")
+        C2 = jpype.JClass("java.util.concurrent.TimeUnit")
+        O1 = C1.SECONDS
+        O2 = C2.SECONDS
+        N1 = jpype.JObject(None, C1)
+        N2 = jpype.JObject(None, C2)
+        V = jpype.JInt(1)
+        # Test dissimilar objects
+        self.assertTrue(O1 != O2)
+        self.assertFalse(O1 == O2)
+        # test Nulls
+        self.assertTrue(N1 == N2)
+        self.assertFalse(N1 != N2)
+        self.assertTrue(N1 == None)
+        self.assertFalse(N1 != None)
+        # test primitives
+        self.assertFalse(O1 == V)
+        self.assertFalse(V == O1)
+        # test null to primitives
+        self.assertFalse(N1 == V)
+        self.assertFalse(V == N1)
+
+        self.assertFalse(1 == O1)
+        self.assertFalse("M" == O1)
+        self.assertFalse(O1 == 1)
+        self.assertFalse(O1 == "M")
+
+        self.assertTrue(1 != O1)
+        self.assertTrue("M" != O1)
+        self.assertTrue(O1 != 1)
+        self.assertTrue(O1 != "M")
+
+        with self.assertRaises(TypeError):
+            self.assertTrue(O1 > 1)
+        with self.assertRaises(TypeError):
+            self.assertTrue(1 > O1)
+        with self.assertRaises(TypeError):
+            self.assertTrue(O1 > O2)
diff --git a/test/jpypetest/test_objectwrapper.py b/test/jpypetest/test_objectwrapper.py
index 80f18c7eb..f6a64cf85 100644
--- a/test/jpypetest/test_objectwrapper.py
+++ b/test/jpypetest/test_objectwrapper.py
@@ -113,6 +113,37 @@ class Fred(object):
         with self.assertRaises(TypeError):
             jpype.JObject(Fred())
 
+    def testObjectEq(self):
+        C1 = jpype.JClass("java.lang.StringBuffer")
+        C2 = jpype.JClass("java.lang.Exception")
+        O1 = C1()
+        O2 = C2()
+        N1 = jpype.JObject(None, C1)
+        N2 = jpype.JObject(None, C2)
+        V = jpype.JInt(1)
+        # Test dissimilar objects
+        self.assertTrue(O1 != O2)
+        self.assertFalse(O1 == O2)
+        self.assertTrue(O2 != O1)
+        self.assertFalse(O2 == O1)
+        # test Nulls
+        self.assertTrue(N1 == N2)
+        self.assertFalse(N1 != N2)
+        self.assertTrue(N1 == None)
+        self.assertFalse(N1 != None)
+        self.assertTrue(None == N1)
+        self.assertFalse(None != N1)
+        # test primitives
+        self.assertFalse(O1 == V)
+        self.assertFalse(V == O1)
+        self.assertFalse(O2 == V)
+        self.assertFalse(V == O2)
+        # test null to primitives
+        self.assertFalse(N1 == V)
+        self.assertFalse(V == N1)
+        self.assertFalse(N2 == V)
+        self.assertFalse(V == N2)
+
 
 #     def testMakeSureWeCanLoadAllClasses(self):
 #         def get_system_jars():