Issue2456

[DESCRIPTION]

Consider this simple example that works in Jython 2.5.2:

linux> jython2.5
Jython 2.5.2 (Release_2_5_2:7206, Mar 2 2011, 23:12:06) 
[Java HotSpot(TM) 64-Bit Server VM (Oracle Corporation)] on java1.8.0_60
Type "help", "copyright", "credits" or "license" for more information.
>>> import java              
>>> a = java.util.ArrayList()
>>> a.add("a")               
True
>>> a.remove(0)              
u'a'

The element with index 0 has been removed.

The same code fails in Jython 2.7.0:

linux> jython2.7
Jython 2.7.0 (default:9987c746f838, Apr 29 2015, 02:25:11) 
[Java HotSpot(TM) 64-Bit Server VM (Oracle Corporation)] on java1.8.0_60
Type "help", "copyright", "credits" or "license" for more information.
>>> import java
>>> a = java.util.ArrayList()
>>> a.add("a")
True
>>> a.remove(0)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ValueError: 0 is not in list


[DIAGNOSIS]

The exception is thrown by JavaProxyList.listRemoveOverrideProxy, which covers the method List.remove(Object), but not List.remove(int).


[SOLUTION]

Fix JavaProxyList.listRemoveOverrideProxy to cope with removal by index.

So the problem here is that Python's list.remove (https://docs.python.org/2/library/stdtypes.html#mutable-sequence-types) has different semantics than java.util.List (https://docs.oracle.com/javase/7/docs/api/java/util/List.html#remove(int)), but of course the same as remove(Object) when the List contains integers...

See this note in NEWS (https://github.com/jythontools/jython/blob/master/NEWS#L166)

  Potentially backwards breaking changes, removing silent errors:

    - remove method on proxied List objects now follows Python
      sematics: a ValueError is raised if the item is not found in the
      java.util.List. In the past, Java semantics were used, with a
      boolean returned indicating if the item was removed or
      not. Given how this interacted with Jython, such remove
      invocations were in the past silent, and perhaps were actually a
      bug.

I'm not sure how we can reconcile, other than to document that the equivalent Python operation would be list.pop(0), which is available:

$ bin/jython 
Jython 2.7.1b2 (default:f58d86b21716, Jan 26 2016, 14:07:09) 
[Java HotSpot(TM) 64-Bit Server VM (Oracle Corporation)] on java1.7.0_80
Type "help", "copyright", "credits" or "license" for more information.
>>> from java.util import ArrayList
>>> x = ArrayList()
>>> x.add(42)
True
>>> x.remove(0)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ValueError: 0 is not in list
>>> y = list()
>>> y.append(
y.append(   
>>> y.append(42)
>>> y.remove(0)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ValueError: list.remove(x): x not in list
>>> from java.lang import Integer
>>> y.remove((Integer(0))
... )
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ValueError: list.remove(x): x not in list
>>> y.remove(Integer(0))
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ValueError: list.remove(x): x not in list
>>> x.remove(Integer(0))
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ValueError: 0 is not in list
>>> x.pop(0)
42
>>> x
[]

Don't you think that taking away a valid call to the java interface (List.remove(int)) on a Java object and only document it does not seem to be a valid approach either?

Patching JavaProxyList.listRemoveOverrideProxy like this works for us:

private static final PyBuiltinMethodNarrow listRemoveOverrideProxy = new ListMethod("remove", 1) {
    @Override
    public PyObject __call__(PyObject object) {
        List jlist = asList();
        for (int i = 0; i < jlist.size(); i++) {
            Object jobj = jlist.get(i);
            if (Py.java2py(jobj)._eq(object).__nonzero__()) {
                jlist.remove(i);
                return Py.None;
            }
        }
        // START PATCH FOR BUG 2456
        if (object instanceof PyInteger) {
            jlist.remove(((PyInteger)object).getValue());
            return Py.None;
        }
        // END PATCH FOR BUG 2456
        throw Py.ValueError(object.toString() + " is not in list");
    }
};

It would be nice if such a change is incorporated in the library, as we consider this as another regression in Jython 2.7.0 with respect to Jython 2.5.2.

The suggested override in msg10689 sounds good to me.

But we should also

1. Use PyObject#__index__ if available (check with PyObject#isIndex, which is fast for PyInteger)

2. Implement corresponding tests, so as to prevent future regressions!

Fixed as of https://hg.python.org/jython/rev/01ef8d1c6d0e

Many thanks!