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

Incorrect result when using != comparison against Java {List, Set, Map} (fix #2639) #96

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@jamesmudd
Contributor

jamesmudd commented Nov 22, 2017

@jeff5

I think this proposal breaks how binary operations are delegated. See comments at the appropriate points, some on the removed (pink) code.

@@ -339,38 +371,17 @@ public PyObject __call__(PyObject key) {
private static final PyBuiltinMethodNarrow listEqProxy = new ListMethod("__eq__", 1) {
@Override
public PyObject __call__(PyObject other) {
List jList = asList();
if (other.getType().isSubType(PyList.TYPE)) {

This comment has been minimized.

@jeff5

jeff5 Nov 22, 2017

Member

One needs to appreciate the subtlety of the way Python binary operators give way to each other. See the note just following https://docs.python.org/2/reference/datamodel.html#object.__ror__ . Note especially the bit about subclasses.

When I look at this line, it may be trying to do that, although I expected to see it call other._eq(this) or some such. So wonder if the bug isn't related to that. However, if not, I'd look at the code further up the stack that calls this, as maybe mis-handling (not correctly negating) the return value, maybe when it is null -- see below. I haven't looked myself (yet).

@jeff5

jeff5 Nov 22, 2017

Member

One needs to appreciate the subtlety of the way Python binary operators give way to each other. See the note just following https://docs.python.org/2/reference/datamodel.html#object.__ror__ . Note especially the bit about subclasses.

When I look at this line, it may be trying to do that, although I expected to see it call other._eq(this) or some such. So wonder if the bug isn't related to that. However, if not, I'd look at the code further up the stack that calls this, as maybe mis-handling (not correctly negating) the return value, maybe when it is null -- see below. I haven't looked myself (yet).

This comment has been minimized.

@jamesmudd

jamesmudd Nov 29, 2017

Contributor

I don't think that's the intention, and maybe I should refactor this like JavaProxySet into an isPyList(other) method. I think this is just to check if we are being compared to a Python list.

On the point of how operators should give way to each other i'm not clear that should happen in the ne case. https://docs.python.org/2/reference/datamodel.html#object.__ne__ just below the doc states

when defining eq(), one should also define ne() so that the operators will behave as expected

I think thats the problem which needed to be fixed here. I did look at the code callling this and didn't see any mistakes in the logic, but another view on that would be good. I think some comments in this might help so I will add some.

@jamesmudd

jamesmudd Nov 29, 2017

Contributor

I don't think that's the intention, and maybe I should refactor this like JavaProxySet into an isPyList(other) method. I think this is just to check if we are being compared to a Python list.

On the point of how operators should give way to each other i'm not clear that should happen in the ne case. https://docs.python.org/2/reference/datamodel.html#object.__ne__ just below the doc states

when defining eq(), one should also define ne() so that the operators will behave as expected

I think thats the problem which needed to be fixed here. I did look at the code callling this and didn't see any mistakes in the logic, but another view on that would be good. I think some comments in this might help so I will add some.

This comment has been minimized.

@jeff5

jeff5 Dec 2, 2017

Member

You could define __ne__ in terms of __eq__, but you need to take account of a null return from __eq__, when you don't know whether it's equal or not. I'm surprised this isn't done in PyObject as the default implementation of __ne__, __ge__ and __le__, but maybe there's a good reason.

The other._eq(this) I was looking for is dealt with in PyObject._eq by re-entrancy.

@jeff5

jeff5 Dec 2, 2017

Member

You could define __ne__ in terms of __eq__, but you need to take account of a null return from __eq__, when you don't know whether it's equal or not. I'm surprised this isn't done in PyObject as the default implementation of __ne__, __ge__ and __le__, but maybe there's a good reason.

The other._eq(this) I was looking for is dealt with in PyObject._eq by re-entrancy.

}
return Py.True;
} else {
return null;

This comment has been minimized.

@jeff5

jeff5 Nov 22, 2017

Member

Any binary operation, and __eq__ is not different (I think) should be allowed to say "I don't know, ask the other guy". When you do this in Python, you return NotImplemented, but down here in the works you return null.

@jeff5

jeff5 Nov 22, 2017

Member

Any binary operation, and __eq__ is not different (I think) should be allowed to say "I don't know, ask the other guy". When you do this in Python, you return NotImplemented, but down here in the works you return null.

This comment has been minimized.

@jamesmudd

jamesmudd Nov 29, 2017

Contributor

Agree, but here I think we do know were not equal?

@jamesmudd

jamesmudd Nov 29, 2017

Contributor

Agree, but here I think we do know were not equal?

This comment has been minimized.

@jeff5

jeff5 Dec 2, 2017

Member

I think you only know it's a type you can't deal with.

@jeff5

jeff5 Dec 2, 2017

Member

I think you only know it's a type you can't deal with.

Show outdated Hide outdated src/org/python/core/JavaProxyList.java Outdated
Show outdated Hide outdated src/org/python/core/JavaProxyList.java Outdated
@jamesmudd

Thanks for taking a look at this still think I can tidy up a bit more and will add some more comments

Show outdated Hide outdated src/org/python/core/JavaProxyList.java Outdated
@@ -339,38 +371,17 @@ public PyObject __call__(PyObject key) {
private static final PyBuiltinMethodNarrow listEqProxy = new ListMethod("__eq__", 1) {
@Override
public PyObject __call__(PyObject other) {
List jList = asList();
if (other.getType().isSubType(PyList.TYPE)) {

This comment has been minimized.

@jamesmudd

jamesmudd Nov 29, 2017

Contributor

I don't think that's the intention, and maybe I should refactor this like JavaProxySet into an isPyList(other) method. I think this is just to check if we are being compared to a Python list.

On the point of how operators should give way to each other i'm not clear that should happen in the ne case. https://docs.python.org/2/reference/datamodel.html#object.__ne__ just below the doc states

when defining eq(), one should also define ne() so that the operators will behave as expected

I think thats the problem which needed to be fixed here. I did look at the code callling this and didn't see any mistakes in the logic, but another view on that would be good. I think some comments in this might help so I will add some.

@jamesmudd

jamesmudd Nov 29, 2017

Contributor

I don't think that's the intention, and maybe I should refactor this like JavaProxySet into an isPyList(other) method. I think this is just to check if we are being compared to a Python list.

On the point of how operators should give way to each other i'm not clear that should happen in the ne case. https://docs.python.org/2/reference/datamodel.html#object.__ne__ just below the doc states

when defining eq(), one should also define ne() so that the operators will behave as expected

I think thats the problem which needed to be fixed here. I did look at the code callling this and didn't see any mistakes in the logic, but another view on that would be good. I think some comments in this might help so I will add some.

}
return Py.True;
} else {
return null;

This comment has been minimized.

@jamesmudd

jamesmudd Nov 29, 2017

Contributor

Agree, but here I think we do know were not equal?

@jamesmudd

jamesmudd Nov 29, 2017

Contributor

Agree, but here I think we do know were not equal?

@jamesmudd

This comment has been minimized.

Show comment
Hide comment
@jamesmudd

jamesmudd Dec 3, 2017

Contributor

I have added back the support for returning null and added some comments and doc. When you get time have a look over it carefully, see if I have done anything stupid! Thanks

Contributor

jamesmudd commented Dec 3, 2017

I have added back the support for returning null and added some comments and doc. When you get time have a look over it carefully, see if I have done anything stupid! Thanks

@jeff5

This comment has been minimized.

Show comment
Hide comment
@jeff5

jeff5 Jan 3, 2018

Member

@jamesmudd: Thanks for this thorough job.

I've tried this on my machine and it seems good. One only has to single-step through something like this to be reminded just how much work Jython does under the covers of a superficially simple operation. It looks like the delegation now works as I understand it should.

Good call adapting the tests using the type2test idiom. I was thinking about the tests you specifically added for equality and inequality and their coverage. I see:

self.assertTrue(jmap == {'a': 1, 'b': 2, 'c': 3})

but in order to test both delegation paths and the "not a Python dict") arm of your implementation, don't we also need:

self.assertTrue({'a': 1, 'b': 2, 'c': 3} == jmap)
self.assertTrue(jmap == self.type2test({'a': 1, 'b': 2, 'c': 3}))

and so on in other types and assertions? They seem to work ok at the prompt:

>>> dr = dict(un='one', deux=2, trois=3+0j)
>>> da, db, dc = HashMap(dr), dict(dr), dict(dr)
>>> dc['deux'] = sys
>>> da == db and db == da and da == HashMap(db)
True
>>> da == dc or dc == da or da == HashMap(dc)
False
Member

jeff5 commented Jan 3, 2018

@jamesmudd: Thanks for this thorough job.

I've tried this on my machine and it seems good. One only has to single-step through something like this to be reminded just how much work Jython does under the covers of a superficially simple operation. It looks like the delegation now works as I understand it should.

Good call adapting the tests using the type2test idiom. I was thinking about the tests you specifically added for equality and inequality and their coverage. I see:

self.assertTrue(jmap == {'a': 1, 'b': 2, 'c': 3})

but in order to test both delegation paths and the "not a Python dict") arm of your implementation, don't we also need:

self.assertTrue({'a': 1, 'b': 2, 'c': 3} == jmap)
self.assertTrue(jmap == self.type2test({'a': 1, 'b': 2, 'c': 3}))

and so on in other types and assertions? They seem to work ok at the prompt:

>>> dr = dict(un='one', deux=2, trois=3+0j)
>>> da, db, dc = HashMap(dr), dict(dr), dict(dr)
>>> dc['deux'] = sys
>>> da == db and db == da and da == HashMap(db)
True
>>> da == dc or dc == da or da == HashMap(dc)
False
@jamesmudd

This comment has been minimized.

Show comment
Hide comment
@jamesmudd

jamesmudd Jan 4, 2018

Contributor

Thanks for the suggestion your right I should also test the comparisons the other way around. I have added that now map and list are ok, but some of the set ones are failing so I need to have a look at that.

Contributor

jamesmudd commented Jan 4, 2018

Thanks for the suggestion your right I should also test the comparisons the other way around. I have added that now map and list are ok, but some of the set ones are failing so I need to have a look at that.

@jamesmudd

This comment has been minimized.

Show comment
Hide comment
@jamesmudd

jamesmudd May 15, 2018

Contributor

I have finally got back to looking at this and think I have now fixed (the more comprehensive) tests.

@jeff5 If you could find time to have another look over this then I think it could be merged.

Contributor

jamesmudd commented May 15, 2018

I have finally got back to looking at this and think I have now fixed (the more comprehensive) tests.

@jeff5 If you could find time to have another look over this then I think it could be merged.

@jeff5 jeff5 changed the title from Fix for #2639 to Incorrect result when using != comparison against Java {List, Set, Map} (fix #2639) May 20, 2018

@jeff5

jeff5 approved these changes May 21, 2018

@jeff5

This comment has been minimized.

Show comment
Hide comment
@jeff5

jeff5 May 22, 2018

Member

@jamesmudd Hi. This looks good to me. I had problems with test_overflow when the list is a Java type, run under regtest on Windows:

test_overflow (test.test_list_jy.JavaLinkedListTestCase) ... test test_list_jy crashed --Exception in thread "MainThread"
Exception: java.lang.OutOfMemoryError thrown from the UncaughtExceptionHandler in thread "MainThread"
Exception in thread "Thread-1" java.lang.OutOfMemoryError: Java heap space
Exception in thread "Jython Shutdown Closer" java.lang.OutOfMemoryError: Java heap space

A PyList is an array type, so it attempts to allocate in one go and simply fails, whereas with the linked list we reach the cliff-edge (very) gradually are then unable to scrabble back to safety.

We've noticed before that Jython doesn't like running out of memory. I'll tweak the test to "pass" that (as several others have been). Otherwise I think this should go in as is, and thanks for working on this, which turned out rather long.

Member

jeff5 commented May 22, 2018

@jamesmudd Hi. This looks good to me. I had problems with test_overflow when the list is a Java type, run under regtest on Windows:

test_overflow (test.test_list_jy.JavaLinkedListTestCase) ... test test_list_jy crashed --Exception in thread "MainThread"
Exception: java.lang.OutOfMemoryError thrown from the UncaughtExceptionHandler in thread "MainThread"
Exception in thread "Thread-1" java.lang.OutOfMemoryError: Java heap space
Exception in thread "Jython Shutdown Closer" java.lang.OutOfMemoryError: Java heap space

A PyList is an array type, so it attempts to allocate in one go and simply fails, whereas with the linked list we reach the cliff-edge (very) gradually are then unable to scrabble back to safety.

We've noticed before that Jython doesn't like running out of memory. I'll tweak the test to "pass" that (as several others have been). Otherwise I think this should go in as is, and thanks for working on this, which turned out rather long.

@jamesmudd

This comment has been minimized.

Show comment
Hide comment
@jamesmudd

jamesmudd May 22, 2018

Contributor

Interesting, I haven't seen that, but mostly work on Linux. I wouldn't have though the platform would make much difference to this though. It makes sense there might be problems with other implementation classes though, that's why I added tests! Since this is a memory issue and its a failure of a new test I have added I agree that it should be merged as is.

Thanks for looking at it. Hope its right now, i'm pretty confident it is.

Contributor

jamesmudd commented May 22, 2018

Interesting, I haven't seen that, but mostly work on Linux. I wouldn't have though the platform would make much difference to this though. It makes sense there might be problems with other implementation classes though, that's why I added tests! Since this is a memory issue and its a failure of a new test I have added I agree that it should be merged as is.

Thanks for looking at it. Hope its right now, i'm pretty confident it is.

@jeff5

This comment has been minimized.

Show comment
Hide comment
@jeff5

jeff5 May 25, 2018

Member

@jamesmudd : I committed this, locally, not pushed yet, then I decided to beef up the tests to torture it a bit more. List stood up to it perfectly. Something is not quite right about sets:

>>> list_ref = [False, 1, 2, "3"]
>>> list_v = [0, True, 2L, u"3"]
>>> set(list_v) == set(list_ref)
True
>>> from java.util import LinkedHashSet, HashSet
>>> set(list_v) == HashSet(list_ref)
True
>>> HashSet(list_v) == set(list_ref)
False
>>> HashSet(list_v) == HashSet(list_ref)
False

And dictionaries:

>>> dict_ref = {False:0, 'a':1, u'b':2L, 3:"3"}
>>> dict_v = {0:False, u'a':True, 'b':2, 3:"3"}
>>> dict_ref == dict_v
True
>>> dict_v == dict_ref
True
>>> from java.util import HashMap, LinkedHashMap, Hashtable
>>> HashMap(dict_v) == dict_ref
True
>>> dict_v == HashMap(dict_ref)
True
>>> HashMap(dict_v) == HashMap(dict_ref)
False

I haven't dug into why. It may be to do with that tricky mutual delegation of the two classes we discussed. But I also notice that the values of Java collections repr() in a way that suggests the objects in them have been converted, in which case equality has not the same meaning.

That would turn this more into a question of what the behaviour ought to be, the kind of question I like to ask @jimbaker :).

Member

jeff5 commented May 25, 2018

@jamesmudd : I committed this, locally, not pushed yet, then I decided to beef up the tests to torture it a bit more. List stood up to it perfectly. Something is not quite right about sets:

>>> list_ref = [False, 1, 2, "3"]
>>> list_v = [0, True, 2L, u"3"]
>>> set(list_v) == set(list_ref)
True
>>> from java.util import LinkedHashSet, HashSet
>>> set(list_v) == HashSet(list_ref)
True
>>> HashSet(list_v) == set(list_ref)
False
>>> HashSet(list_v) == HashSet(list_ref)
False

And dictionaries:

>>> dict_ref = {False:0, 'a':1, u'b':2L, 3:"3"}
>>> dict_v = {0:False, u'a':True, 'b':2, 3:"3"}
>>> dict_ref == dict_v
True
>>> dict_v == dict_ref
True
>>> from java.util import HashMap, LinkedHashMap, Hashtable
>>> HashMap(dict_v) == dict_ref
True
>>> dict_v == HashMap(dict_ref)
True
>>> HashMap(dict_v) == HashMap(dict_ref)
False

I haven't dug into why. It may be to do with that tricky mutual delegation of the two classes we discussed. But I also notice that the values of Java collections repr() in a way that suggests the objects in them have been converted, in which case equality has not the same meaning.

That would turn this more into a question of what the behaviour ought to be, the kind of question I like to ask @jimbaker :).

@jamesmudd

This comment has been minimized.

Show comment
Hide comment
@jamesmudd

jamesmudd May 25, 2018

Contributor

@jeff5 Thanks for taking another look. It still looks broken to me. Guess I need to add some more tests and then take another look...

Contributor

jamesmudd commented May 25, 2018

@jeff5 Thanks for taking another look. It still looks broken to me. Guess I need to add some more tests and then take another look...

@jeff5

This comment has been minimized.

Show comment
Hide comment
@jeff5

jeff5 May 26, 2018

Member

I've the tests ok, and now I think I understand the problem to be that that equality between objects in the container is tested between the Java forms not the Python forms. I've just tried it in one branch of JavaSetProxy.isEqual and got what we need in the example. So currently:

                // Do element by element comparison, if any elements are not contained return false
                for (Object obj : selfSet) {
                    if (!otherPySet.contains(Py.java2py(obj))) {
                        return Py.False;
                    }
                }

instead of

                // Do element by element comparison, if any elements are not contained return false
                for (PyObject pyobj : otherPySet) {
                    if (!selfSet.contains(pyobj.__tojava__(Object.class))) {
                        return Py.False;
                    }
                }

This approach works for mixed container types. When it comes to comparing two Java container types, I'm somewhat torn. Do we want Python equals or Java Equals? I think Python equals on the argument that the == is a Python operator.

Member

jeff5 commented May 26, 2018

I've the tests ok, and now I think I understand the problem to be that that equality between objects in the container is tested between the Java forms not the Python forms. I've just tried it in one branch of JavaSetProxy.isEqual and got what we need in the example. So currently:

                // Do element by element comparison, if any elements are not contained return false
                for (Object obj : selfSet) {
                    if (!otherPySet.contains(Py.java2py(obj))) {
                        return Py.False;
                    }
                }

instead of

                // Do element by element comparison, if any elements are not contained return false
                for (PyObject pyobj : otherPySet) {
                    if (!selfSet.contains(pyobj.__tojava__(Object.class))) {
                        return Py.False;
                    }
                }

This approach works for mixed container types. When it comes to comparing two Java container types, I'm somewhat torn. Do we want Python equals or Java Equals? I think Python equals on the argument that the == is a Python operator.

@jeff5

This comment has been minimized.

Show comment
Hide comment
@jeff5

jeff5 May 26, 2018

Member

In terms of organising the code, this has got a bit messy now. (Sorry.) With hindsight, I ought to have done this under a git repo on my machine, then I'd be able to push to this PR branch (I think). Instead, here are my beefed-up tests. They have print statements that shouldn't go into the final.

List:

    def test_equality(self):
        jl = self.type2test()
        self.assertTrue(jl == [])
        self.assertTrue([] == jl)
        self.assertFalse(jl == [1])
        self.assertFalse([1] == jl)

        ref = [False, 1, 3**9, "3"]
        alt = [0, True, 3L**9, u"3"]
        self.assertEqual(ref, alt) # test assumption
        jref = self.type2test(ref)
        print                                    ##JA remove various print
        print '==', self.type2test, ref, alt
        for v in [ref, alt, jref]:
            print "    ", v
            self.assertTrue(jref == v)
            self.assertTrue(v == jref)
            self.assertTrue(jref == self.type2test(v))

        alt = [False, 1, 2e4, "3"]
        print '!='
        for v in [alt, ref[:-1], ref+[{}], []]:
            print "    ", v
            self.assertFalse(jref == v)
            self.assertFalse(v == jref)
            self.assertFalse(jref == self.type2test(v))

    def test_inequality(self):
        # Test for http://bugs.jython.org/issue2639
        # This is to test the != comparisons between Java and Python lists
        jl = self.type2test()
        self.assertFalse(jl != [])
        self.assertFalse([] != jl)
        self.assertTrue(jl != [1])
        self.assertTrue([1] != jl)

        ref = [False, 1, 3**9, "3"]
        alt = [0, True, 3L**9, u"3"]
        self.assertEqual(ref, alt) # test assumption
        jref = self.type2test(ref)
        print                                    ##JA remove various print
        print '==', self.type2test, ref, alt
        for v in [ref, alt, jref]:
            print "    ", v
            self.assertFalse(jref != v)
            self.assertFalse(v != jref)
            self.assertFalse(jref != self.type2test(v))

        alt = [False, 1, 2e4, "3"]
        print '!='
        for v in [alt, ref[:-1], ref+[{}], []]:
            print "    ", v
            self.assertTrue(jref != v)
            self.assertTrue(v != jref)
            self.assertTrue(jref != self.type2test(v))

Set:

    def test_equality(self):
        js = self.type2test()
        self.assertTrue(js == set())
        self.assertTrue(set() == js)
        self.assertFalse(js == set([1]))
        self.assertFalse(set([1]) == js)

        ref = {False, 1, 3**9, "3"}
        alt = {0, True, 3L**9, u"3"}
        self.assertEqual(ref, alt) # test assumption
        jref = self.type2test(ref)
        print                                    ##JA remove various print
        print '==', self.type2test, ref, alt
        for v in [ref, alt, jref]:
            print "    ", v
            self.assertTrue(jref == v)
            self.assertTrue(v == jref)
            self.assertTrue(jref == self.type2test(v))

        alt1 = {False, 1.01, 3**9, "3"}
        alt2 = {False, 1, "3"};
        alt3 = {False, 1, 3**9, "3", ""};
        print '!='
        for v in [alt1, alt2, alt3, set()]:
            print "    ", v
            self.assertFalse(jref == v)
            self.assertFalse(v == jref)
            self.assertFalse(jref == self.type2test(v))

    def test_inequality(self):
        # Test for http://bugs.jython.org/issue2639
        # This is to test the != comparisons between Java and Python lists
        js = self.type2test()
        self.assertFalse(js != set())
        self.assertFalse(set() != js)
        self.assertTrue(set([1]) != js)

        ref = {False, 1, 3**9, "3"}
        alt = {0, True, 3L**9, u"3"}
        self.assertEqual(ref, alt) # test assumption
        jref = self.type2test(ref)
        print                                    ##JA remove various print
        print '==', self.type2test, ref, alt
        for v in [ref, alt, jref]:
            print "    ", v
            self.assertFalse(jref != v)
            self.assertFalse(v != jref)
            self.assertFalse(jref != self.type2test(v))

        alt1 = {False, 1.01, 3**9, "3"}
        alt2 = {False, 1, "3"};
        alt3 = {False, 1, 3**9, "3", ""};
        print '!='
        for v in [alt1, alt2, alt3, set()]:
            print "    ", v
            self.assertTrue(jref != v)
            self.assertTrue(v != jref)
            self.assertTrue(jref != self.type2test(v))

Dictionary:

    def test_equality(self):
        jmap = self.type2test()
        self.assertTrue(jmap == {})
        self.assertTrue({} == jmap)
        self.assertFalse({'a': 1} == jmap)
        self.assertFalse(jmap == {'a': 1})

        ref = {False:0, 'a':1, u'b':2L, 3:"3"}
        alt = {0:False, u'a':True, 'b':2, 3:"3"}
        self.assertEqual(ref, alt) # test assumption
        jref = self.type2test(ref)
        print                                    ##JA remove various print
        print '==', self.type2test, ref, alt
        for v in [ref, alt, jref]:
            print "    ", v
            self.assertTrue(jref == v)
            self.assertTrue(v == jref)
            self.assertTrue(jref == self.type2test(v))

        alt1 = ref.copy(); alt1['a'] = 2;
        alt2 = ref.copy(); del alt2['a'];
        alt3 = ref.copy(); alt3['c'] = [];
        print '!='
        for v in [alt1, alt2, alt3, {}]:
            print "    ", v
            self.assertFalse(jref == v)
            self.assertFalse(v == jref)
            self.assertFalse(jref == self.type2test(v))

    def test_inequality(self):
        # Test for http://bugs.jython.org/issue2639
        # This is to test the != comparisons between Java and Python lists
        jmap = self.type2test()
        self.assertFalse(jmap != {})
        self.assertFalse({} != jmap)
        self.assertTrue(jmap != {'a': 1})
        self.assertTrue({'a': 1} != jmap)

        ref = {False:0, 'a':1, u'b':2L, 3:"3"}
        alt = {0:False, u'a':True, 'b':2, 3:"3"}
        self.assertEqual(ref, alt) # test assumption
        jref = self.type2test(ref)
        print                                    ##JA remove various print
        print '==', self.type2test, ref, alt
        for v in [ref, alt, jref]:
            print "    ", v
            self.assertFalse(jref != v)
            self.assertFalse(v != jref)
            self.assertFalse(jref != self.type2test(v))

        alt1 = ref.copy(); alt1['a'] = 2;
        alt2 = ref.copy(); del alt2['a'];
        alt3 = ref.copy(); alt3['c'] = [];
        print '!='
        for v in [alt1, alt2, alt3, {}]:
            print "    ", v
            self.assertTrue(jref != v)
            self.assertTrue(v != jref)
            self.assertTrue(jref != self.type2test(v))
Member

jeff5 commented May 26, 2018

In terms of organising the code, this has got a bit messy now. (Sorry.) With hindsight, I ought to have done this under a git repo on my machine, then I'd be able to push to this PR branch (I think). Instead, here are my beefed-up tests. They have print statements that shouldn't go into the final.

List:

    def test_equality(self):
        jl = self.type2test()
        self.assertTrue(jl == [])
        self.assertTrue([] == jl)
        self.assertFalse(jl == [1])
        self.assertFalse([1] == jl)

        ref = [False, 1, 3**9, "3"]
        alt = [0, True, 3L**9, u"3"]
        self.assertEqual(ref, alt) # test assumption
        jref = self.type2test(ref)
        print                                    ##JA remove various print
        print '==', self.type2test, ref, alt
        for v in [ref, alt, jref]:
            print "    ", v
            self.assertTrue(jref == v)
            self.assertTrue(v == jref)
            self.assertTrue(jref == self.type2test(v))

        alt = [False, 1, 2e4, "3"]
        print '!='
        for v in [alt, ref[:-1], ref+[{}], []]:
            print "    ", v
            self.assertFalse(jref == v)
            self.assertFalse(v == jref)
            self.assertFalse(jref == self.type2test(v))

    def test_inequality(self):
        # Test for http://bugs.jython.org/issue2639
        # This is to test the != comparisons between Java and Python lists
        jl = self.type2test()
        self.assertFalse(jl != [])
        self.assertFalse([] != jl)
        self.assertTrue(jl != [1])
        self.assertTrue([1] != jl)

        ref = [False, 1, 3**9, "3"]
        alt = [0, True, 3L**9, u"3"]
        self.assertEqual(ref, alt) # test assumption
        jref = self.type2test(ref)
        print                                    ##JA remove various print
        print '==', self.type2test, ref, alt
        for v in [ref, alt, jref]:
            print "    ", v
            self.assertFalse(jref != v)
            self.assertFalse(v != jref)
            self.assertFalse(jref != self.type2test(v))

        alt = [False, 1, 2e4, "3"]
        print '!='
        for v in [alt, ref[:-1], ref+[{}], []]:
            print "    ", v
            self.assertTrue(jref != v)
            self.assertTrue(v != jref)
            self.assertTrue(jref != self.type2test(v))

Set:

    def test_equality(self):
        js = self.type2test()
        self.assertTrue(js == set())
        self.assertTrue(set() == js)
        self.assertFalse(js == set([1]))
        self.assertFalse(set([1]) == js)

        ref = {False, 1, 3**9, "3"}
        alt = {0, True, 3L**9, u"3"}
        self.assertEqual(ref, alt) # test assumption
        jref = self.type2test(ref)
        print                                    ##JA remove various print
        print '==', self.type2test, ref, alt
        for v in [ref, alt, jref]:
            print "    ", v
            self.assertTrue(jref == v)
            self.assertTrue(v == jref)
            self.assertTrue(jref == self.type2test(v))

        alt1 = {False, 1.01, 3**9, "3"}
        alt2 = {False, 1, "3"};
        alt3 = {False, 1, 3**9, "3", ""};
        print '!='
        for v in [alt1, alt2, alt3, set()]:
            print "    ", v
            self.assertFalse(jref == v)
            self.assertFalse(v == jref)
            self.assertFalse(jref == self.type2test(v))

    def test_inequality(self):
        # Test for http://bugs.jython.org/issue2639
        # This is to test the != comparisons between Java and Python lists
        js = self.type2test()
        self.assertFalse(js != set())
        self.assertFalse(set() != js)
        self.assertTrue(set([1]) != js)

        ref = {False, 1, 3**9, "3"}
        alt = {0, True, 3L**9, u"3"}
        self.assertEqual(ref, alt) # test assumption
        jref = self.type2test(ref)
        print                                    ##JA remove various print
        print '==', self.type2test, ref, alt
        for v in [ref, alt, jref]:
            print "    ", v
            self.assertFalse(jref != v)
            self.assertFalse(v != jref)
            self.assertFalse(jref != self.type2test(v))

        alt1 = {False, 1.01, 3**9, "3"}
        alt2 = {False, 1, "3"};
        alt3 = {False, 1, 3**9, "3", ""};
        print '!='
        for v in [alt1, alt2, alt3, set()]:
            print "    ", v
            self.assertTrue(jref != v)
            self.assertTrue(v != jref)
            self.assertTrue(jref != self.type2test(v))

Dictionary:

    def test_equality(self):
        jmap = self.type2test()
        self.assertTrue(jmap == {})
        self.assertTrue({} == jmap)
        self.assertFalse({'a': 1} == jmap)
        self.assertFalse(jmap == {'a': 1})

        ref = {False:0, 'a':1, u'b':2L, 3:"3"}
        alt = {0:False, u'a':True, 'b':2, 3:"3"}
        self.assertEqual(ref, alt) # test assumption
        jref = self.type2test(ref)
        print                                    ##JA remove various print
        print '==', self.type2test, ref, alt
        for v in [ref, alt, jref]:
            print "    ", v
            self.assertTrue(jref == v)
            self.assertTrue(v == jref)
            self.assertTrue(jref == self.type2test(v))

        alt1 = ref.copy(); alt1['a'] = 2;
        alt2 = ref.copy(); del alt2['a'];
        alt3 = ref.copy(); alt3['c'] = [];
        print '!='
        for v in [alt1, alt2, alt3, {}]:
            print "    ", v
            self.assertFalse(jref == v)
            self.assertFalse(v == jref)
            self.assertFalse(jref == self.type2test(v))

    def test_inequality(self):
        # Test for http://bugs.jython.org/issue2639
        # This is to test the != comparisons between Java and Python lists
        jmap = self.type2test()
        self.assertFalse(jmap != {})
        self.assertFalse({} != jmap)
        self.assertTrue(jmap != {'a': 1})
        self.assertTrue({'a': 1} != jmap)

        ref = {False:0, 'a':1, u'b':2L, 3:"3"}
        alt = {0:False, u'a':True, 'b':2, 3:"3"}
        self.assertEqual(ref, alt) # test assumption
        jref = self.type2test(ref)
        print                                    ##JA remove various print
        print '==', self.type2test, ref, alt
        for v in [ref, alt, jref]:
            print "    ", v
            self.assertFalse(jref != v)
            self.assertFalse(v != jref)
            self.assertFalse(jref != self.type2test(v))

        alt1 = ref.copy(); alt1['a'] = 2;
        alt2 = ref.copy(); del alt2['a'];
        alt3 = ref.copy(); alt3['c'] = [];
        print '!='
        for v in [alt1, alt2, alt3, {}]:
            print "    ", v
            self.assertTrue(jref != v)
            self.assertTrue(v != jref)
            self.assertTrue(jref != self.type2test(v))

jamesmudd added some commits Nov 7, 2017

#2639 Add test coverage of != comparisons to Java {List, Set, Map}
Also tests with more Java types and test the comparisons in both
directions.
#2639 Fix incorrect evaluation of != on Java {List, Set, Map}
Adds __ne__ method to JavaProxy{List, Set, Map}.

Changes BaseSet __eq__ and __ne__ methods to use item by item comparison
where needed.

Also refactors methods for testing equality and inequality to be very
similar across JavaProxy{List, Set, Map}. Some minor improvements e.g.
use entrySet for map comparison.
@jamesmudd

This comment has been minimized.

Show comment
Hide comment
@jamesmudd

jamesmudd Jun 4, 2018

Contributor

Ok have had another look at this. Think I have improved the tests to cover the broken cases. I have then fixed the bug. I have implemented Python ==/!= event when comparing Java to Java, I think this is the most reasonable approach? In the case where a Java {List, Set, Map} is compare to another the Set or Map is converted into Python then the comparison made again. This allows for Python style element by element equality checks.

This change has become quite large. I think I have improved the tests enough that i'm fairly confident its right, but wouldn't be surprised if there are more edge cases I have missed!

Anyway it needs careful review let me know if there is anything that could be better. Or maybe a totally different solution.

Contributor

jamesmudd commented Jun 4, 2018

Ok have had another look at this. Think I have improved the tests to cover the broken cases. I have then fixed the bug. I have implemented Python ==/!= event when comparing Java to Java, I think this is the most reasonable approach? In the case where a Java {List, Set, Map} is compare to another the Set or Map is converted into Python then the comparison made again. This allows for Python style element by element equality checks.

This change has become quite large. I think I have improved the tests enough that i'm fairly confident its right, but wouldn't be surprised if there are more edge cases I have missed!

Anyway it needs careful review let me know if there is anything that could be better. Or maybe a totally different solution.

@jeff5

This comment has been minimized.

Show comment
Hide comment
@jeff5

jeff5 Jun 5, 2018

Member

Thanks James. The apparently effortless consistency of Jython on the surface is hard-won in the Java underneath. I'll take a look.

Member

jeff5 commented Jun 5, 2018

Thanks James. The apparently effortless consistency of Jython on the surface is hard-won in the Java underneath. I'll take a look.

@jeff5

This comment has been minimized.

Show comment
Hide comment
@jeff5

jeff5 Jul 11, 2018

Member

Thanks for the hard work on this James. It took a little stitching, but I've made this into one changeset: https://hg.python.org/jython/rev/e546ad3d85bd .

I found that test_overflow on linked lists would crash the test with the sort of irrecoverable OutOfMemory error we've remarked on before, so I added another override-pass. Then because these look like proper passing tests I decided to skip them formally with an annotation. I don't think I changed any real code.

I tweaked a bit of formatting, some of which may not have been yours anyway (and my editor enforces a few things too). If your editor has a right-margin, it should be at 100 for Java.

Member

jeff5 commented Jul 11, 2018

Thanks for the hard work on this James. It took a little stitching, but I've made this into one changeset: https://hg.python.org/jython/rev/e546ad3d85bd .

I found that test_overflow on linked lists would crash the test with the sort of irrecoverable OutOfMemory error we've remarked on before, so I added another override-pass. Then because these look like proper passing tests I decided to skip them formally with an annotation. I don't think I changed any real code.

I tweaked a bit of formatting, some of which may not have been yours anyway (and my editor enforces a few things too). If your editor has a right-margin, it should be at 100 for Java.

@jeff5 jeff5 closed this Jul 11, 2018

@jamesmudd jamesmudd deleted the jamesmudd:2639 branch Jul 12, 2018

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