Issue2445

classification
Title: Eclipse's DelegatingFeatureMap has MRO conflict
Type: crash Severity: normal
Components: Core Versions: Jython 2.7
Milestone: Jython 2.7.2
process
Status: pending Resolution: fixed
Dependencies: Superseder:
Assigned To: jeff.allen Nosy List: bmvanwyk, jeff.allen, tomluk, wfouche2, zyasoft
Priority: high Keywords:

Created on 2015-12-29.13:20:54 by tomluk, last changed 2019-03-18.06:26:25 by jeff.allen.

Files
File name Uploaded Description Edit Remove
jython MRO problem.zip tomluk, 2015-12-29.13:20:54 extract this, paste standalone version of jython into extracted folder, edit runMe.bat to use new jython.jar, run runMe.bat
Messages
msg10561 (view) Author: Tomas Lukosius (tomluk) Date: 2015-12-29.13:30:19
using: 
Windows 8.1 64 bit OS
java 1.8u66 64 bit

Tryed to execute script with jython v2.5.4rc1. Import was succesful.
msg10565 (view) Author: Jim Baker (zyasoft) Date: 2015-12-29.14:51:15
First, please submit your bug with some more explanatory text so we don't have to dig through a zip file to triage.

Related bug is #1878

I see you also tried this on Jython 2.7, so there's some additional diagnostic output to consider:

TypeError: Supertypes that share a modified attribute have an MRO conflict[
attribute=remove,
supertypes=[
<type 'org.eclipse.emf.ecore.util.DelegatingNotifyingInternalEListImpl'>,
<type 'java.util.AbstractList'>,
<type 'org.eclipse.emf.common.util.DelegatingEList'>,
<type 'org.eclipse.emf.common.util.AbstractEList'>,
<type 'org.eclipse.emf.common.notify.NotifyingList'>,
<type 'org.eclipse.emf.common.notify.impl.DelegatingNotifyingListImpl'>,
<type 'org.eclipse.emf.common.util.EList'>,
<type 'java.util.List'>,
<type 'org.eclipse.emf.ecore.util.InternalEList'>
],
type=DelegatingFeatureMap]

It looks like the same problem we saw in #1878 - DelegatingFeatureMap is a Map that implements Iterable
msg10566 (view) Author: Tomas Lukosius (tomluk) Date: 2015-12-29.15:23:15
Thank you Jim for your quick response. Sorry for that skinny report, forgot to paste output i got from jython. But you did it for me.
msg10567 (view) Author: Jim Baker (zyasoft) Date: 2015-12-29.16:08:05
Tom, no worries about your initial "skinny" report :)

The underlying problem may be this line of code in PyJavaType:

if (method.equals("__iter__") && proxySet.equals(Generic.set(Iterable.class, Map.class))) {
    continue;
}

Mostly likely this equality test is too restricted and it should be instead testing it the proxy set is a subset:

if (method.equals("__iter__") && proxySet.containsAll(Generic.set(Iterable.class, Map.class))) {
    continue;
}
msg10568 (view) Author: Jim Baker (zyasoft) Date: 2015-12-29.17:45:19
So the problem is not what I thought it could be in msg10567. Define a test class like so:

// example simplified from
// org.eclipse.emf.ecore.util.DelegatingFeatureMap in the Eclipse
// Modeling Framework

// used as a test of http://bugs.jython.org/issue2445

package javatests;

import java.io.Serializable;
import java.util.AbstractList;
import java.util.AbstractSequentialList;
import java.util.Iterator;
import java.util.ListIterator;

// The following tag interfaces duplicate the interface/abstract class supertypes of
// org.eclipse.emf.ecore.util.DelegatingFeatureMap
// NOTE this class/interface hierarchy is sort of crazy, isn't it?

interface EList<E> extends java.util.List<E> {}
interface InternalEList<E> extends EList<E> {
    interface Unsettable<E> extends InternalEList<E> {}
}

abstract class AbstractSequentialInternalEList<E> extends AbstractSequentialList<E> implements InternalEList<E> {}

abstract class EContentsEList<E> extends AbstractSequentialInternalEList<E> implements EList<E>, InternalEList<E> {
    interface FeatureListIterator<E> extends FeatureIterator<E>, ListIterator<E> {}
    interface FeatureIterator<E> extends Iterator<E> {}
}

interface FeatureMap extends EList<FeatureMap.Entry> {
    interface Entry {
        interface Internal extends Entry {
        }
    }
    interface ValueListIterator<E> extends EContentsEList.FeatureListIterator<E> {}
    interface Internal extends FeatureMap, InternalEList<Entry>, EStructuralFeature.Setting {
        interface Wrapper {}
    }
}

abstract class AbstractEList<E> extends AbstractList<E> implements EList<E> {}
abstract class DelegatingEList<E> extends AbstractEList<E> implements Cloneable, Serializable {}
interface NotifyingList<E> extends EList<E> {}
abstract class DelegatingNotifyingListImpl<E> extends DelegatingEList<E> implements NotifyingList<E> {}
abstract class DelegatingNotifyingInternalEListImpl<E> extends DelegatingNotifyingListImpl<E> implements InternalEList<E> {}
interface Notifier {}
interface EObject extends Notifier {}
interface EModelElement extends EObject {}
interface ENamedElement extends EModelElement {}
interface ETypedElement extends ENamedElement {}
interface EStructuralFeature extends ETypedElement {
    interface Setting {}
}
abstract class DelegatingEcoreEList<E> extends DelegatingNotifyingInternalEListImpl<E>
        implements InternalEList.Unsettable<E>, EStructuralFeature.Setting {}

public abstract class DelegatingFeatureMapMRO extends DelegatingEcoreEList<FeatureMap.Entry>
  implements FeatureMap.Internal, FeatureMap.Internal.Wrapper {}

Then

>>> from javatests import DelegatingFeatureMapMRO
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: Supertypes that share a modified attribute have an MRO conflict[attribute=remove, supertypes=[<type 'java.util.List'>, <type 'java.util.AbstractList'>], type=DelegatingFeatureMapMRO]

Given that java.util.AbstractList implements java.util.List, there should be no conflict.

Note that as in #1878, some inheritance complexity is required to demonstrate the bug. A simple test class like so presents no problems:

package javatests;

import java.util.AbstractList;
import java.util.List;

abstract class DiamondAbstractList extends AbstractList implements List {}

>>> from javatests import DiamondAbstractList
>>>

Presumably in between the two test classes is one of the appropriate complexity to trigger this problem.
msg10569 (view) Author: Jim Baker (zyasoft) Date: 2015-12-29.17:54:50
Possibly related to #2391, which maintains ordering in attributes. In particular I'm suspicious of the code in PyJavaType#handleMroError.
msg10576 (view) Author: Tomas Lukosius (tomluk) Date: 2015-12-30.07:36:19
After some debugging i found out that might be some problem in method org.python.core.PyType#computeMro(org.python.core.PyType.MROMergeState[], java.util.List<org.python.core.PyObject>) when merging mro's, but i'am not fully sure
msg10642 (view) Author: Jim Baker (zyasoft) Date: 2016-01-13.18:52:48
Not related to the recently fixed #2391

Tomas, right, it's just part of the overall merge mechanism, and what appears to be overeager conflict detection when none actually exists.
msg10650 (view) Author: Tomas Lukosius (tomluk) Date: 2016-01-15.14:51:33
Is there an expected date, when this bug could be fixed?
msg10653 (view) Author: Jim Baker (zyasoft) Date: 2016-01-15.15:25:39
We should be able to fix in 2.7.2. The timeline for that release is not yet fixed, although I would expect approximately 6 months from now, given that it's best to keep our schedules roughly aligned with PyCon.
msg10944 (view) Author: Barry-Michael van Wyk (bmvanwyk) Date: 2016-09-07.14:12:11
This issue can easily be triggered by importing the IBM MQ Java class:

import com.ibm.mq.jms.MQQueue

TypeError: Supertypes that share a modified attribute have an MRO conflict[attribute=get, supertypes=[<type 'java.util.Map'>, <type 'com.ibm.msg.client.jms.admin.JmsJndiDestinationImpl'>, <type 'com.ibm.msg.client.jms.admin.JmsDestinationImpl'>, <type 'com.ibm.msg.client.jms.internal.JmsPropertyContextImpl'>, <type 'com.ibm.msg.client.jms.JmsPropertyContext'>, <type 'com.ibm.msg.client.jms.JmsDestination'>, <type 'com.ibm.mq.jms.MQDestination'>, <type 'com.ibm.msg.client.jms.JmsQueue'>], type=MQQueue]

Works fine on Jython 2.5, but not on Jython 2.7
msg10945 (view) Author: Jim Baker (zyasoft) Date: 2016-09-08.23:38:56
We were hoping 2.7.2 would be released by now (msg10653), but we are obviously still working on getting 2.7.1 released. I still see this as a 2.7.2 priority.
msg11581 (view) Author: Jim Baker (zyasoft) Date: 2017-09-09.22:39:10
Still a problem in Jython 2.7.x latest, using the test class from msg10568
msg11591 (view) Author: Jim Baker (zyasoft) Date: 2017-09-11.22:26:21
So likely the solution here is to add a type unification step to PyJavaType#handleMroError, such that if for some pairing of types T and U, if type T is reported to conflict with type U, but T is a subclass or subinterface of U, then ignore as a MRO conflict error (because it's not). For some reason, the MRO conflict detection reports false positives, and we already have one workaround for this with respect to __iter__. I suspect these all seem to revolve around methods that are part of the Python integration support seen in JavaProxyList, etc. In particular, remove was added to this support in 2.7; it was not part of 2.5, so this adds further evidence to why this conflict now exists.

This type test can be readily done with https://docs.oracle.com/javase/7/docs/api/java/lang/Class.html#isAssignableFrom(java.lang.Class)
msg11592 (view) Author: Jim Baker (zyasoft) Date: 2017-09-12.23:40:47
My suggestion in msg11591 works as expected. Need to look at some additional testing, but it should be part of 2.7.2:

diff -r 8d52ad2559f5 src/org/python/core/PyJavaType.java
--- a/src/org/python/core/PyJavaType.java	Sat Sep 09 14:02:01 2017 -0600
+++ b/src/org/python/core/PyJavaType.java	Tue Sep 12 17:38:22 2017 -0600
@@ -187,7 +187,15 @@
                     PyList types = new PyList();
                     Set<Class<?>> proxySet = Generic.set();
                     for (PyJavaType othertype : conflictedAttributes) {
-                        if (othertype.modified != null && othertype.modified.contains(method)) {
+                        /* Ignore any pairings of types that are in a superclass/superinterface
+                         * relationship with each other. This problem is a false positive that happens
+                         * because of the automatic addition of methods so that Java classes behave
+                         * more like their corresponding Python types, such as adding sort or remove.
+                         * See http://bugs.jython.org/issue2445
+                         */
+                        if (othertype.modified != null && othertype.modified.contains(method) &&
+                            !(othertype.getProxyType().isAssignableFrom(type.getProxyType()) ||
+                                type.getProxyType().isAssignableFrom(othertype.getProxyType()))) {
                             types.add(othertype);
                             proxySet.add(othertype.getProxyType());
                         }
@@ -199,13 +207,16 @@
                      * __iter__. Annoying but necessary logic. See http://bugs.jython.org/issue1878
                      */
                     if (method.equals("__iter__")
-                            && proxySet.equals(Generic.set(Iterable.class, Map.class))) {
+                            && Generic.set(Iterable.class, Map.class).containsAll(proxySet)) {
                         continue;
                     }
-                    throw Py.TypeError(String.format(
-                            "Supertypes that share a modified attribute "
-                                    + "have an MRO conflict[attribute=%s, supertypes=%s, type=%s]",
-                            method, types, this.getName()));
+
+                    if (types.size() > 0) {
+                        throw Py.TypeError(String.format(
+                                "Supertypes that share a modified attribute "
+                                        + "have an MRO conflict[attribute=%s, supertypes=%s, type=%s]",
+                                method, types, this.getName()));
+                    }
                 }
             }
         }
msg11593 (view) Author: Jim Baker (zyasoft) Date: 2017-09-12.23:42:52
In particular, this type unification seems to work as expected with the preceding diff. No additional computational complexity is seen here in the pairwise conflict detection (O(n^2)), although perhaps we could reduce to O(n) via union-find for type unification. But in practice we have not seen this blow up, and I will hand wave to state that the sets of types in conflict that are seen in practice, even with Eclispe or Clojure, are small, maybe on the order of 2 or 3 elements.
msg11746 (view) Author: Jeff Allen (jeff.allen) Date: 2018-03-06.07:35:48
Seems like a lot of work has gone into getting this to the 90% mark. Any chance you could do the other 90% in time for 2.7.2? (Not that we have a specific time in mind yet.)
msg12062 (view) Author: Werner Fouché (wfouche2) Date: 2018-07-23.18:14:03
Importing IBM MQ Java client libraries continue to fail with the latest Jython source code updated to change https://hg.python.org/jython/rev/86b859301121

Example on Windows:

set CLASSPATH=com.ibm.mqjms-7.0.1.jar;jms-1.1.jar;com.ibm.mq.jmqi-7.0.1.jar

jython -V
Jython 2.7.2a1+

jython -c "import com.ibm.mq.jms.MQQueue"

TypeError: Supertypes that share a modified attribute have an MRO conflict[attribute=get, supertypes=[<type 'com.ibm.msg.client.jms.internal.JmsPropertyContextImpl'>, <type 'com.ibm.msg.client.jms.JmsPropertyContext'>, <type 'com.ibm.msg.client.jms.admin.JmsJndiDestinationImpl'>, <type 'com.ibm.msg.client.jms.admin.JmsDestinationImpl'>, <type 'com.ibm.msg.client.jms.JmsQueue'>, <type 'java.util.Map'>, <type 'com.ibm.mq.jms.MQDestination'>, <type 'com.ibm.msg.client.jms.JmsDestination'>], type=MQQueue]

What remains to do be done to implement a 100% fix?
msg12063 (view) Author: Werner Fouché (wfouche2) Date: 2018-07-23.19:11:34
The IBM MQ client libraries are available from: https://developer.ibm.com/messaging/learn-mq/mq-tutorials/develop-mq-jms/

New example on Windows:

set CLASSPATH=com.ibm.mq.allclient-9.0.4.0.jar;javax.jms-api-2.0.1.jar

jython -V
Jython 2.7.2a1+

jython -c "import com.ibm.mq.jms.MQQueue"

TypeError: Supertypes that share a modified attribute have an MRO conflict[attribute=values, supertypes=[<type 'com.ibm.msg.client.jms.admin.JmsJndiDestinationImpl'>, <type 'java.util.Map'>, <type 'com.ibm.msg.client.jms.internal.JmsPropertyContextImpl'>, <type 'com.ibm.msg.client.jms.JmsQueue'>, <type 'com.ibm.msg.client.jms.JmsPropertyContext'>, <type 'com.ibm.msg.client.jms.admin.JmsDestinationImpl'>, <type 'com.ibm.mq.jms.MQDestination'>, <type 'com.ibm.msg.client.jms.JmsDestination'>], type=MQQueue]
msg12364 (view) Author: Jeff Allen (jeff.allen) Date: 2019-03-17.09:03:04
I intend to try Jim's fix for this, but first to create a failing test.

We seem only to lack a concise test that does not depend on external products. Jim has given us an independent test, but it lacks conciseness. I'll start with that and try tofigure out what minimal subset properly exercises the defect.
msg12366 (view) Author: Werner Fouché (wfouche2) Date: 2019-03-18.06:12:54
https://bugs.jython.org/issue2445
msg12368 (view) Author: Jeff Allen (jeff.allen) Date: 2019-03-18.06:26:25
I derived a simplified (but still failing) equivalent and made it the basis of an addition to test_java_integration. Jim's patch as posted fixes that without my having to understand too much!

Now in at: https://hg.python.org/jython/rev/3c4f2c14b808 (Thanks Jim.)
History
Date User Action Args
2019-03-18 06:26:25jeff.allensetstatus: open -> pending
resolution: accepted -> fixed
messages: + msg12368
2019-03-18 06:12:55wfouche2setmessages: + msg12366
2019-03-17 09:03:04jeff.allensetassignee: jeff.allen
resolution: remind -> accepted
messages: + msg12364
2018-07-23 19:11:34wfouche2setmessages: + msg12063
2018-07-23 18:14:04wfouche2setnosy: + wfouche2
messages: + msg12062
2018-03-06 07:35:49jeff.allensetnosy: + jeff.allen
messages: + msg11746
2017-09-12 23:42:52zyasoftsetmessages: + msg11593
2017-09-12 23:40:48zyasoftsetmessages: + msg11592
2017-09-11 22:26:22zyasoftsetmessages: + msg11591
2017-09-09 22:39:10zyasoftsetresolution: remind
messages: + msg11581
2016-09-08 23:38:57zyasoftsetmessages: + msg10945
2016-09-07 14:12:12bmvanwyksetnosy: + bmvanwyk
messages: + msg10944
2016-01-15 15:25:39zyasoftsetpriority: high
messages: + msg10653
milestone: Jython 2.7.2
2016-01-15 14:51:33tomluksetmessages: + msg10650
2016-01-13 18:52:49zyasoftsetmessages: + msg10642
2015-12-30 07:36:19tomluksetmessages: + msg10576
2015-12-29 17:54:50zyasoftsetmessages: + msg10569
2015-12-29 17:45:19zyasoftsetmessages: + msg10568
2015-12-29 16:08:05zyasoftsetmessages: + msg10567
title: Eclipse's DelegatingFeatureMap is reported as having a MRO conflict -> Eclipse's DelegatingFeatureMap has MRO conflict
2015-12-29 15:23:16tomluksetmessages: + msg10566
2015-12-29 14:51:15zyasoftsetnosy: + zyasoft
messages: + msg10565
title: MRO conflict when importing class from library -> Eclipse's DelegatingFeatureMap is reported as having a MRO conflict
2015-12-29 13:30:19tomluksetmessages: + msg10561
2015-12-29 13:20:54tomlukcreate