Make Declaration.__add__ try harder to produce consistent resolution orders.issue193

By moving things from the RHS to the front of the list if they already extend something from the LHS.

Fixes #193

4 files changed, 101 insertions, 24 deletions

diff --git a/CHANGES.rst b/CHANGES.rst
index 600fbb8..5b57ddb 100644
--- a/CHANGES.rst
+++ b/CHANGES.rst
@@ -12,6 +12,14 @@
   shown as ``classImplements(list, IMutableSequence, IIterable)``. See
   `issue 236 <https://github.com/zopefoundation/zope.interface/issues/236>`_.
 
+- Make ``Declaration.__add__`` (as in ``implementedBy(Cls) +
+  ISomething``) try harder to preserve a consistent resolution order
+  when the two arguments share overlapping pieces of the interface
+  inheritance hierarchy. Previously, the right hand side was always
+  put at the end of the resolution order, which could easily produce
+  invalid orders. See `issue 193
+  <https://github.com/zopefoundation/zope.interface/issues/193>`_.
+
 5.3.0 (2020-03-21)
 ==================
 
diff --git a/docs/api/declarations.rst b/docs/api/declarations.rst
index cdbf815..4327c72 100644
--- a/docs/api/declarations.rst
+++ b/docs/api/declarations.rst
@@ -763,34 +763,38 @@ Exmples for :meth:`Declaration.__add__`:
 .. doctest::
 
    >>> from zope.interface import Interface
-   >>> class I1(Interface): pass
+   >>> class IRoot1(Interface): pass
    ...
-   >>> class I2(I1): pass
+   >>> class IDerived1(IRoot1): pass
    ...
-   >>> class I3(Interface): pass
+   >>> class IRoot2(Interface): pass
    ...
-   >>> class I4(I3): pass
+   >>> class IDerived2(IRoot2): pass
    ...
    >>> spec = Declaration()
    >>> [iface.getName() for iface in spec]
    []
-   >>> [iface.getName() for iface in spec+I1]
-   ['I1']
-   >>> [iface.getName() for iface in I1+spec]
-   ['I1']
+   >>> [iface.getName() for iface in spec+IRoot1]
+   ['IRoot1']
+   >>> [iface.getName() for iface in IRoot1+spec]
+   ['IRoot1']
    >>> spec2 = spec
-   >>> spec += I1
+   >>> spec += IRoot1
    >>> [iface.getName() for iface in spec]
-   ['I1']
+   ['IRoot1']
    >>> [iface.getName() for iface in spec2]
    []
-   >>> spec2 += Declaration(I3, I4)
+   >>> spec2 += Declaration(IRoot2, IDerived2)
    >>> [iface.getName() for iface in spec2]
-   ['I3', 'I4']
+   ['IDerived2', 'IRoot2']
    >>> [iface.getName() for iface in spec+spec2]
-   ['I1', 'I3', 'I4']
+   ['IRoot1', 'IDerived2', 'IRoot2']
    >>> [iface.getName() for iface in spec2+spec]
-   ['I3', 'I4', 'I1']
+   ['IDerived2', 'IRoot2', 'IRoot1']
+   >>> [iface.getName() for iface in (spec+spec2).__bases__]
+   ['IRoot1', 'IDerived2', 'IRoot2']
+   >>> [iface.getName() for iface in (spec2+spec).__bases__]
+   ['IDerived2', 'IRoot2', 'IRoot1']
 
 
 
diff --git a/src/zope/interface/declarations.py b/src/zope/interface/declarations.py
index 935b026..de90074 100644
--- a/src/zope/interface/declarations.py
+++ b/src/zope/interface/declarations.py
@@ -115,20 +115,35 @@ class Declaration(Specification):
         ])
 
     def __add__(self, other):
-        """Add two specifications or a specification and an interface
         """
-        seen = {}
-        result = []
-        for i in self.interfaces():
-            seen[i] = 1
-            result.append(i)
+        Add two specifications or a specification and an interface
+        and produce a new declaration.
+
+        .. versionchanged:: 5.4.0
+           Now tries to preserve a consistent resolution order. Interfaces
+           being added to this object are added to the front of the resulting resolution
+           order if they already extend an interface in this object. Previously,
+           they were always added to the end of the order, which easily resulted in
+           invalid orders.
+        """
+        before = []
+        result = list(self.interfaces())
+        seen = set(result)
         for i in other.interfaces():
-            if i not in seen:
-                seen[i] = 1
+            if i in seen:
+                continue
+            seen.add(i)
+            if any(i.extends(x) for x in result):
+                # It already extends us, e.g., is a subclass,
+                # so it needs to go at the front of the RO.
+                before.append(i)
+            else:
                 result.append(i)
+        return Declaration(*(before + result))
 
-        return Declaration(*result)
-
+    # XXX: Is __radd__ needed? No tests break if it's removed.
+    # If it is needed, does it need to handle the C3 ordering differently?
+    # I (JAM) don't *think* it does.
     __radd__ = __add__
 
     @staticmethod
diff --git a/src/zope/interface/tests/test_declarations.py b/src/zope/interface/tests/test_declarations.py
index 8efe2d8..04a0dca 100644
--- a/src/zope/interface/tests/test_declarations.py
+++ b/src/zope/interface/tests/test_declarations.py
@@ -289,6 +289,56 @@ class DeclarationTests(EmptyDeclarationTests):
         after = before + other
         self.assertEqual(list(after), [IFoo, IBar, IBaz])
 
+    def test___add___overlapping_interface(self):
+        # The derived interfaces end up with higher priority, and
+        # don't produce a C3 resolution order violation. This
+        # example produced a C3 error, and the resulting legacy order
+        # used to be wrong ([IBase, IDerived] instead of
+        # the other way).
+        from zope.interface import Interface
+        from zope.interface.interface import InterfaceClass
+        from zope.interface.tests.test_ro import C3Setting
+        from zope.interface import ro
+
+        IBase = InterfaceClass('IBase')
+        IDerived = InterfaceClass('IDerived', (IBase,))
+
+        with C3Setting(ro.C3.STRICT_IRO, True):
+            base = self._makeOne(IBase)
+            after = base + IDerived
+
+        self.assertEqual(after.__iro__, (IDerived, IBase, Interface))
+        self.assertEqual(after.__bases__, (IDerived, IBase))
+        self.assertEqual(list(after), [IDerived, IBase])
+
+    def test___add___overlapping_interface_implementedBy(self):
+        # Like test___add___overlapping_interface, but pulling
+        # in a realistic example. This one previously produced a
+        # C3 error, but the resulting legacy order was (somehow)
+        # correct.
+        from zope.interface import Interface
+        from zope.interface import implementedBy
+        from zope.interface import implementer
+        from zope.interface.tests.test_ro import C3Setting
+        from zope.interface import ro
+
+        class IBase(Interface):
+            pass
+
+        class IDerived(IBase):
+            pass
+
+        @implementer(IBase)
+        class Base(object):
+            pass
+
+        with C3Setting(ro.C3.STRICT_IRO, True):
+            after = implementedBy(Base) + IDerived
+
+        self.assertEqual(after.__sro__, (after, IDerived, IBase, Interface))
+        self.assertEqual(after.__bases__, (IDerived, IBase))
+        self.assertEqual(list(after), [IDerived, IBase])
+
 
 class TestImmutableDeclaration(EmptyDeclarationTests):