Make Provides and ClassProvides ignore redundant interfaces like @implementerissue207

cf #207

4 files changed, 115 insertions, 6 deletions

diff --git a/CHANGES.rst b/CHANGES.rst
index 94f07b1..c791f50 100644
--- a/CHANGES.rst
+++ b/CHANGES.rst
@@ -36,6 +36,14 @@
   object itself (this might happen to persistent ``Components``
   implementations in the face of bugs).
 
+- Fix the ``Provides`` and ``ClassProvides`` descriptors to stop
+  allowing redundant interfaces (those already implemented by the
+  underlying class or meta class) to produce an inconsistent
+  resolution order. This is similar to the change in ``@implementer``
+  in 5.1.0, and resolves inconsistent resolution orders with
+  ``zope.proxy`` and ``zope.location``. See `issue 207
+  <https://github.com/zopefoundation/zope.interface/issues/207>`_.
+
 5.2.0 (2020-11-05)
 ==================
 
diff --git a/src/zope/interface/declarations.py b/src/zope/interface/declarations.py
index 9a0146c..d687599 100644
--- a/src/zope/interface/declarations.py
+++ b/src/zope/interface/declarations.py
@@ -131,6 +131,19 @@ class Declaration(Specification):
 
     __radd__ = __add__
 
+    @staticmethod
+    def _add_interfaces_to_cls(interfaces, cls):
+        # Strip redundant interfaces already provided
+        # by the cls so we don't produce invalid
+        # resolution orders.
+        implemented_by_cls = implementedBy(cls)
+        interfaces = tuple([
+            iface
+            for iface in interfaces
+            if not implemented_by_cls.isOrExtends(iface)
+        ])
+        return interfaces + (implemented_by_cls,)
+
 
 class _ImmutableDeclaration(Declaration):
     # A Declaration that is immutable. Used as a singleton to
@@ -747,7 +760,7 @@ class Provides(Declaration):  # Really named ProvidesClass
     def __init__(self, cls, *interfaces):
         self.__args = (cls, ) + interfaces
         self._cls = cls
-        Declaration.__init__(self, *(interfaces + (implementedBy(cls), )))
+        Declaration.__init__(self, *self._add_interfaces_to_cls(interfaces, cls))
 
     def __repr__(self):
         return "<%s.%s for %s>" % (
@@ -890,7 +903,7 @@ class ClassProvides(Declaration, ClassProvidesBase):
         self._cls = cls
         self._implements = implementedBy(cls)
         self.__args = (cls, metacls, ) + interfaces
-        Declaration.__init__(self, *(interfaces + (implementedBy(metacls), )))
+        Declaration.__init__(self, *self._add_interfaces_to_cls(interfaces, metacls))
 
     def __repr__(self):
         return "<%s.%s for %s>" % (
diff --git a/src/zope/interface/interface.py b/src/zope/interface/interface.py
index d100aae..c5d1852 100644
--- a/src/zope/interface/interface.py
+++ b/src/zope/interface/interface.py
@@ -413,6 +413,13 @@ class Specification(SpecificationBase):
         __setBases,
         )
 
+    # This method exists for tests to override the way we call
+    # ro.calculate_ro(), usually by adding extra kwargs. We don't
+    # want to have a mutable dictionary as a class member that we pass
+    # ourself because mutability is bad, and passing **kw is slower than
+    # calling the bound function.
+    _do_calculate_ro = calculate_ro
+
     def _calculate_sro(self):
         """
         Calculate and return the resolution order for this object, using its ``__bases__``.
@@ -448,7 +455,7 @@ class Specification(SpecificationBase):
         # This requires that by the time this method is invoked, our bases
         # have settled their SROs. Thus, ``changed()`` must first
         # update itself before telling its descendents of changes.
-        sro = calculate_ro(self, base_mros={
+        sro = self._do_calculate_ro(base_mros={
             b: b.__sro__
             for b in self.__bases__
         })
diff --git a/src/zope/interface/tests/test_declarations.py b/src/zope/interface/tests/test_declarations.py
index 9ef192a..29b75e9 100644
--- a/src/zope/interface/tests/test_declarations.py
+++ b/src/zope/interface/tests/test_declarations.py
@@ -1280,7 +1280,7 @@ class ProvidesClassTests(unittest.TestCase):
             pass
         spec = self._makeOne(Foo, IFoo)
         klass, args = spec.__reduce__()
-        self.assertTrue(klass is Provides)
+        self.assertIs(klass, Provides)
         self.assertEqual(args, (Foo, IFoo))
 
     def test___get___class(self):
@@ -1290,7 +1290,7 @@ class ProvidesClassTests(unittest.TestCase):
             pass
         spec = self._makeOne(Foo, IFoo)
         Foo.__provides__ = spec
-        self.assertTrue(Foo.__provides__ is spec)
+        self.assertIs(Foo.__provides__, spec)
 
     def test___get___instance(self):
         from zope.interface.interface import InterfaceClass
@@ -1312,6 +1312,43 @@ class ProvidesClassTests(unittest.TestCase):
         )
 
 
+class ProvidesClassStrictTests(ProvidesClassTests):
+    # Tests that require the strict C3 resolution order.
+
+    def _getTargetClass(self):
+        ProvidesClass = super(ProvidesClassStrictTests, self)._getTargetClass()
+        class StrictProvides(ProvidesClass):
+            def _do_calculate_ro(self, base_mros):
+                return ProvidesClass._do_calculate_ro(self, base_mros=base_mros, strict=True)
+        return StrictProvides
+
+    def test__repr__(self):
+        self.skipTest("Not useful for the subclass.")
+
+    def test_overlapping_interfaces_corrected(self):
+        # Giving Provides(cls, IFace), where IFace is already
+        # provided by cls, doesn't produce invalid resolution orders.
+        from zope.interface import implementedBy
+        from zope.interface import Interface
+        from zope.interface import implementer
+
+        class IBase(Interface):
+            pass
+
+        @implementer(IBase)
+        class Base(object):
+            pass
+
+        spec = self._makeOne(Base, IBase)
+        self.assertEqual(spec.__sro__, (
+            spec,
+            implementedBy(Base),
+            IBase,
+            implementedBy(object),
+            Interface
+        ))
+
+
 class Test_Provides(unittest.TestCase):
 
     def _callFUT(self, *args, **kw):
@@ -1580,7 +1617,7 @@ class ClassProvidesTests(unittest.TestCase):
             pass
         cp = Foo.__provides__ = self._makeOne(Foo, type(Foo), IBar)
         self.assertEqual(cp.__reduce__(),
-                         (self._getTargetClass(), (Foo, type(Foo), IBar)))
+                         (type(cp), (Foo, type(Foo), IBar)))
 
     def test__repr__(self):
         inst = self._makeOne(type(self), type)
@@ -1589,6 +1626,50 @@ class ClassProvidesTests(unittest.TestCase):
             "<zope.interface.declarations.ClassProvides for %r>"  % type(self)
         )
 
+
+class ClassProvidesStrictTests(ClassProvidesTests):
+    # Tests that require the strict C3 resolution order.
+
+    def _getTargetClass(self):
+        ClassProvides = super(ClassProvidesStrictTests, self)._getTargetClass()
+        class StrictClassProvides(ClassProvides):
+            def _do_calculate_ro(self, base_mros):
+                return ClassProvides._do_calculate_ro(self, base_mros=base_mros, strict=True)
+        return StrictClassProvides
+
+    def test__repr__(self):
+        self.skipTest("Not useful for the subclass.")
+
+    def test_overlapping_interfaces_corrected(self):
+        # Giving ClassProvides(cls, metaclass, IFace), where IFace is already
+        # provided by metacls, doesn't produce invalid resolution orders.
+        from zope.interface import implementedBy
+        from zope.interface import Interface
+        from zope.interface import implementer
+
+        class IBase(Interface):
+            pass
+
+        @implementer(IBase)
+        class metaclass(type):
+            pass
+
+        cls = metaclass(
+            'cls',
+            (object,),
+            {}
+        )
+
+        spec = self._makeOne(cls, metaclass, IBase)
+        self.assertEqual(spec.__sro__, (
+            spec,
+            implementedBy(metaclass),
+            IBase,
+            implementedBy(type),
+            implementedBy(object),
+            Interface
+        ))
+
 class Test_directlyProvidedBy(unittest.TestCase):
 
     def _callFUT(self, *args, **kw):