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: open Resolution: remind
Dependencies: Superseder:
Assigned To: Nosy List: bmvanwyk, tomluk, zyasoft
Priority: high Keywords:

Created on 2015-12-29.13:20:54 by tomluk, last changed 2017-09-12.23:42:52 by zyasoft.

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.
History
Date User Action Args
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