Merge pull request #203 from zopefoundation/issue199

Make @implementer and classImplements not add redundant interfaces

3 files changed, 246 insertions, 147 deletions

diff --git a/src/zope/interface/declarations.py b/src/zope/interface/declarations.py
index d77c824..9a0146c 100644
--- a/src/zope/interface/declarations.py
+++ b/src/zope/interface/declarations.py
@@ -33,6 +33,7 @@ from types import ModuleType
 import weakref
 
 from zope.interface.advice import addClassAdvisor
+from zope.interface.interface import Interface
 from zope.interface.interface import InterfaceClass
 from zope.interface.interface import SpecificationBase
 from zope.interface.interface import Specification
@@ -430,18 +431,26 @@ def implementedBy(cls): # pylint:disable=too-many-return-statements,too-many-bra
 
 
 def classImplementsOnly(cls, *interfaces):
-    """Declare the only interfaces implemented by instances of a class
+    """
+    Declare the only interfaces implemented by instances of a class
 
-      The arguments after the class are one or more interfaces or interface
-      specifications (`~zope.interface.interfaces.IDeclaration` objects).
+    The arguments after the class are one or more interfaces or interface
+    specifications (`~zope.interface.interfaces.IDeclaration` objects).
 
-      The interfaces given (including the interfaces in the specifications)
-      replace any previous declarations.
+    The interfaces given (including the interfaces in the specifications)
+    replace any previous declarations, *including* inherited definitions. If you
+    wish to preserve inherited declarations, you can pass ``implementedBy(cls)``
+    in *interfaces*. This can be used to alter the interface resolution order.
     """
     spec = implementedBy(cls)
+    # Clear out everything inherited. It's important to
+    # also clear the bases right now so that we don't improperly discard
+    # interfaces that are already implemented by *old* bases that we're
+    # about to get rid of.
     spec.declared = ()
     spec.inherit = None
-    classImplements(cls, *interfaces)
+    spec.__bases__ = ()
+    _classImplements_ordered(spec, interfaces, ())
 
 
 def classImplements(cls, *interfaces):
@@ -460,6 +469,14 @@ def classImplements(cls, *interfaces):
        beginning or end of the list of interfaces declared for *cls*,
        based on inheritance, in order to try to maintain a consistent
        resolution order. Previously, all interfaces were added to the end.
+    .. versionchanged:: 5.1.0
+       If *cls* is already declared to implement an interface (or derived interface)
+       in *interfaces* through inheritance, the interface is ignored. Previously, it
+       would redundantly be made direct base of *cls*, which often produced inconsistent
+       interface resolution orders. Now, the order will be consistent, but may change.
+       Also, if the ``__bases__`` of the *cls* are later changed, the *cls* will no
+       longer be considered to implement such an interface (changing the ``__bases__`` of *cls*
+       has never been supported).
     """
     spec = implementedBy(cls)
     interfaces = tuple(_normalizeargs(interfaces))
@@ -495,13 +512,30 @@ def classImplementsFirst(cls, iface):
 
 
 def _classImplements_ordered(spec, before=(), after=()):
+    # Elide everything already inherited.
+    # Except, if it is the root, and we don't already declare anything else
+    # that would imply it, allow the root through. (TODO: When we disallow non-strict
+    # IRO, this part of the check can be removed because it's not possible to re-declare
+    # like that.)
+    before = [
+        x
+        for x in before
+        if not spec.isOrExtends(x) or (x is Interface and not spec.declared)
+    ]
+    after = [
+        x
+        for x in after
+        if not spec.isOrExtends(x) or (x is Interface and not spec.declared)
+    ]
+
     # eliminate duplicates
     new_declared = []
     seen = set()
-    for b in before + spec.declared + after:
-        if b not in seen:
-            new_declared.append(b)
-            seen.add(b)
+    for l in before, spec.declared, after:
+        for b in l:
+            if b not in seen:
+                new_declared.append(b)
+                seen.add(b)
 
     spec.declared = tuple(new_declared)
 
@@ -526,33 +560,38 @@ def _implements_advice(cls):
 
 
 class implementer(object):
-    """Declare the interfaces implemented by instances of a class.
+    """
+    Declare the interfaces implemented by instances of a class.
 
-      This function is called as a class decorator.
+    This function is called as a class decorator.
 
-      The arguments are one or more interfaces or interface
-      specifications (`~zope.interface.interfaces.IDeclaration` objects).
+    The arguments are one or more interfaces or interface
+    specifications (`~zope.interface.interfaces.IDeclaration`
+    objects).
 
-      The interfaces given (including the interfaces in the
-      specifications) are added to any interfaces previously
-      declared.
+    The interfaces given (including the interfaces in the
+    specifications) are added to any interfaces previously declared,
+    unless the interface is already implemented.
 
-      Previous declarations include declarations for base classes
-      unless implementsOnly was used.
+    Previous declarations include declarations for base classes unless
+    implementsOnly was used.
 
-      This function is provided for convenience. It provides a more
-      convenient way to call `classImplements`. For example::
+    This function is provided for convenience. It provides a more
+    convenient way to call `classImplements`. For example::
 
         @implementer(I1)
         class C(object):
             pass
 
-      is equivalent to calling::
+    is equivalent to calling::
 
         classImplements(C, I1)
 
-      after the class has been created.
-      """
+    after the class has been created.
+
+    .. seealso:: `classImplements`
+       The change history provided there applies to this function too.
+    """
     __slots__ = ('interfaces',)
 
     def __init__(self, *interfaces):
@@ -607,10 +646,10 @@ class implementer_only(object):
             # on a method or function....
             raise ValueError('The implementer_only decorator is not '
                              'supported for methods or functions.')
-        else:
-            # Assume it's a class:
-            classImplementsOnly(ob, *self.interfaces)
-            return ob
+
+        # Assume it's a class:
+        classImplementsOnly(ob, *self.interfaces)
+        return ob
 
 def _implements(name, interfaces, do_classImplements):
     # This entire approach is invalid under Py3K.  Don't even try to fix
@@ -631,30 +670,35 @@ def _implements(name, interfaces, do_classImplements):
     addClassAdvisor(_implements_advice, depth=3)
 
 def implements(*interfaces):
-    """Declare interfaces implemented by instances of a class
+    """
+    Declare interfaces implemented by instances of a class.
 
-      This function is called in a class definition.
+    .. deprecated:: 5.0
+        This only works for Python 2. The `implementer` decorator
+        is preferred for all versions.
 
-      The arguments are one or more interfaces or interface
-      specifications (`~zope.interface.interfaces.IDeclaration` objects).
+    This function is called in a class definition.
 
-      The interfaces given (including the interfaces in the
-      specifications) are added to any interfaces previously
-      declared.
+    The arguments are one or more interfaces or interface
+    specifications (`~zope.interface.interfaces.IDeclaration`
+    objects).
 
-      Previous declarations include declarations for base classes
-      unless `implementsOnly` was used.
+    The interfaces given (including the interfaces in the
+    specifications) are added to any interfaces previously declared.
 
-      This function is provided for convenience. It provides a more
-      convenient way to call `classImplements`. For example::
+    Previous declarations include declarations for base classes unless
+    `implementsOnly` was used.
+
+    This function is provided for convenience. It provides a more
+    convenient way to call `classImplements`. For example::
 
         implements(I1)
 
-      is equivalent to calling::
+    is equivalent to calling::
 
         classImplements(C, I1)
 
-      after the class has been created.
+    after the class has been created.
     """
     # This entire approach is invalid under Py3K.  Don't even try to fix
     # the coverage for this block there. :(
diff --git a/src/zope/interface/tests/test_declarations.py b/src/zope/interface/tests/test_declarations.py
index fc2ab68..c8678b5 100644
--- a/src/zope/interface/tests/test_declarations.py
+++ b/src/zope/interface/tests/test_declarations.py
@@ -805,27 +805,58 @@ class Test_implementedBy(Test_implementedByFallback,
         return implementedBy
 
 
-class Test_classImplementsOnly(unittest.TestCase):
+class _ImplementsTestMixin(object):
+    FUT_SETS_PROVIDED_BY = True
 
-    def _callFUT(self, *args, **kw):
-        from zope.interface.declarations import classImplementsOnly
-        return classImplementsOnly(*args, **kw)
+    def _callFUT(self, cls, iface):
+        # Declare that *cls* implements *iface*; return *cls*
+        raise NotImplementedError
 
-    def test_no_existing(self):
+    def _check_implementer(self, Foo,
+                           orig_spec=None,
+                           spec_name=__name__ + '.Foo',
+                           inherit="not given"):
         from zope.interface.declarations import ClassProvides
         from zope.interface.interface import InterfaceClass
+        IFoo = InterfaceClass('IFoo')
+
+        returned = self._callFUT(Foo, IFoo)
+
+        self.assertIs(returned, Foo)
+        spec = Foo.__implemented__
+        if orig_spec is not None:
+            self.assertIs(spec, orig_spec)
+
+        self.assertEqual(spec.__name__,
+                         spec_name)
+        inherit = Foo if inherit == "not given" else inherit
+        self.assertIs(spec.inherit, inherit)
+        self.assertIs(Foo.__implemented__, spec)
+        if self.FUT_SETS_PROVIDED_BY:
+            self.assertIsInstance(Foo.__providedBy__, ClassProvides)
+            self.assertIsInstance(Foo.__provides__, ClassProvides)
+            self.assertEqual(Foo.__provides__, Foo.__providedBy__)
+
+        return Foo, IFoo
+
+    def test_oldstyle_class(self):
+        # This only matters on Python 2
+        class Foo:
+            pass
+        self._check_implementer(Foo)
+
+    def test_newstyle_class(self):
         class Foo(object):
             pass
-        ifoo = InterfaceClass('IFoo')
-        self._callFUT(Foo, ifoo)
-        spec = Foo.__implemented__ # pylint:disable=no-member
-        self.assertEqual(spec.__name__,
-                         'zope.interface.tests.test_declarations.Foo')
-        self.assertIsNone(spec.inherit)
-        self.assertIs(Foo.__implemented__, spec) # pylint:disable=no-member
-        self.assertIsInstance(Foo.__providedBy__, ClassProvides) # pylint:disable=no-member
-        self.assertIsInstance(Foo.__provides__, ClassProvides) # pylint:disable=no-member
-        self.assertEqual(Foo.__provides__, Foo.__providedBy__) # pylint:disable=no-member
+        self._check_implementer(Foo)
+
+class Test_classImplementsOnly(_ImplementsTestMixin, unittest.TestCase):
+    FUT_SETS_PROVIDED_BY = False
+
+    def _callFUT(self, cls, iface):
+        from zope.interface.declarations import classImplementsOnly
+        classImplementsOnly(cls, iface)
+        return cls
 
     def test_w_existing_Implements(self):
         from zope.interface.declarations import Implements
@@ -843,28 +874,97 @@ class Test_classImplementsOnly(unittest.TestCase):
         self.assertEqual(impl.inherit, None)
         self.assertEqual(impl.declared, (IBar,))
 
+    def test_oldstyle_class(self):
+        from zope.interface.declarations import Implements
+        from zope.interface.interface import InterfaceClass
+        IBar = InterfaceClass('IBar')
+        old_spec = Implements(IBar)
 
-class Test_classImplements(unittest.TestCase):
+        class Foo:
+            __implemented__ = old_spec
+        self._check_implementer(Foo, old_spec, '?', inherit=None)
 
-    def _callFUT(self, *args, **kw):
+    def test_newstyle_class(self):
+        from zope.interface.declarations import Implements
+        from zope.interface.interface import InterfaceClass
+        IBar = InterfaceClass('IBar')
+        old_spec = Implements(IBar)
+
+        class Foo(object):
+            __implemented__ = old_spec
+        self._check_implementer(Foo, old_spec, '?', inherit=None)
+
+
+    def test_redundant_with_super_still_implements(self):
+        Base, IBase = self._check_implementer(
+            type('Foo', (object,), {}),
+            inherit=None,
+        )
+
+        class Child(Base):
+            pass
+
+        self._callFUT(Child, IBase)
+        self.assertTrue(IBase.implementedBy(Child))
+
+
+class Test_classImplements(_ImplementsTestMixin, unittest.TestCase):
+
+    def _callFUT(self, cls, iface):
         from zope.interface.declarations import classImplements
-        return classImplements(*args, **kw)
+        result = classImplements(cls, iface) # pylint:disable=assignment-from-no-return
+        self.assertIsNone(result)
+        return cls
+
+    def __check_implementer_redundant(self, Base):
+        # If we @implementer exactly what was already present, we write
+        # no declared attributes on the parent (we still set everything, though)
+        Base, IBase = self._check_implementer(Base)
+
+        class Child(Base):
+            pass
+
+        returned = self._callFUT(Child, IBase)
+        self.assertIn('__implemented__', returned.__dict__)
+        self.assertNotIn('__providedBy__', returned.__dict__)
+        self.assertIn('__provides__', returned.__dict__)
+
+        spec = Child.__implemented__
+        self.assertEqual(spec.declared, ())
+        self.assertEqual(spec.inherit, Child)
+
+        self.assertTrue(IBase.providedBy(Child()))
+
+    def test_redundant_implementer_empty_class_declarations_newstyle(self):
+        self.__check_implementer_redundant(type('Foo', (object,), {}))
+
+    def test_redundant_implementer_empty_class_declarations_oldstyle(self):
+        # This only matters on Python 2
+        class Foo:
+            pass
+        self.__check_implementer_redundant(Foo)
+
+    def test_redundant_implementer_Interface(self):
+        from zope.interface import Interface
+        from zope.interface import implementedBy
+        from zope.interface import ro
+        from zope.interface.tests.test_ro import C3Setting
 
-    def test_no_existing(self):
-        from zope.interface.declarations import ClassProvides
-        from zope.interface.interface import InterfaceClass
         class Foo(object):
             pass
-        IFoo = InterfaceClass('IFoo')
-        self._callFUT(Foo, IFoo)
-        spec = Foo.__implemented__ # pylint:disable=no-member
-        self.assertEqual(spec.__name__,
-                         'zope.interface.tests.test_declarations.Foo')
-        self.assertIs(spec.inherit, Foo)
-        self.assertIs(Foo.__implemented__, spec) # pylint:disable=no-member
-        self.assertIsInstance(Foo.__providedBy__, ClassProvides) # pylint:disable=no-member
-        self.assertIsInstance(Foo.__provides__, ClassProvides) # pylint:disable=no-member
-        self.assertEqual(Foo.__provides__, Foo.__providedBy__) # pylint:disable=no-member
+
+        with C3Setting(ro.C3.STRICT_IRO, False):
+            self._callFUT(Foo, Interface)
+            self.assertEqual(list(implementedBy(Foo)), [Interface])
+
+            class Baz(Foo):
+                pass
+
+            self._callFUT(Baz, Interface)
+            self.assertEqual(list(implementedBy(Baz)), [Interface])
+
+    def _order_for_two(self, applied_first, applied_second):
+        return (applied_first, applied_second)
 
     def test_w_existing_Implements(self):
         from zope.interface.declarations import Implements
@@ -878,9 +978,10 @@ class Test_classImplements(unittest.TestCase):
         impl.inherit = Foo
         self._callFUT(Foo, IBar)
         # Same spec, now different values
-        self.assertTrue(Foo.__implemented__ is impl)
+        self.assertIs(Foo.__implemented__, impl)
         self.assertEqual(impl.inherit, Foo)
-        self.assertEqual(impl.declared, (IFoo, IBar,))
+        self.assertEqual(impl.declared,
+                         self._order_for_two(IFoo, IBar))
 
     def test_w_existing_Implements_w_bases(self):
         from zope.interface.declarations import Implements
@@ -907,8 +1008,22 @@ class Test_classImplements(unittest.TestCase):
         # Same spec, now different values
         self.assertIs(ExtendsRoot.__implemented__, impl_extends_root)
         self.assertEqual(impl_extends_root.inherit, ExtendsRoot)
-        self.assertEqual(impl_extends_root.declared, (IExtendsRoot, ISecondRoot,))
-        self.assertEqual(impl_extends_root.__bases__, (IExtendsRoot, ISecondRoot, impl_root))
+        self.assertEqual(impl_extends_root.declared,
+                         self._order_for_two(IExtendsRoot, ISecondRoot,))
+        self.assertEqual(impl_extends_root.__bases__,
+                         self._order_for_two(IExtendsRoot, ISecondRoot) + (impl_root,))
+
+
+class Test_classImplementsFirst(Test_classImplements):
+
+    def _callFUT(self, cls, iface):
+        from zope.interface.declarations import classImplementsFirst
+        result = classImplementsFirst(cls, iface) # pylint:disable=assignment-from-no-return
+        self.assertIsNone(result)
+        return cls
+
+    def _order_for_two(self, applied_first, applied_second):
+        return (applied_second, applied_first)
 
 
 class Test__implements_advice(unittest.TestCase):
@@ -930,7 +1045,7 @@ class Test__implements_advice(unittest.TestCase):
         self.assertEqual(list(Foo.__implemented__), [IFoo]) # pylint:disable=no-member
 
 
-class Test_implementer(unittest.TestCase):
+class Test_implementer(Test_classImplements):
 
     def _getTargetClass(self):
         from zope.interface.declarations import implementer
@@ -939,42 +1054,9 @@ class Test_implementer(unittest.TestCase):
     def _makeOne(self, *args, **kw):
         return self._getTargetClass()(*args, **kw)
 
-    def test_oldstyle_class(self):
-        # TODO Py3 story
-        from zope.interface.declarations import ClassProvides
-        from zope.interface.interface import InterfaceClass
-        IFoo = InterfaceClass('IFoo')
-        class Foo:
-            pass
-        decorator = self._makeOne(IFoo)
-        returned = decorator(Foo)
-        self.assertTrue(returned is Foo)
-        spec = Foo.__implemented__ # pylint:disable=no-member
-        self.assertEqual(spec.__name__,
-                         'zope.interface.tests.test_declarations.Foo')
-        self.assertIs(spec.inherit, Foo)
-        self.assertIs(Foo.__implemented__, spec) # pylint:disable=no-member
-        self.assertIsInstance(Foo.__providedBy__, ClassProvides) # pylint:disable=no-member
-        self.assertIsInstance(Foo.__provides__, ClassProvides) # pylint:disable=no-member
-        self.assertEqual(Foo.__provides__, Foo.__providedBy__) # pylint:disable=no-member
-
-    def test_newstyle_class(self):
-        from zope.interface.declarations import ClassProvides
-        from zope.interface.interface import InterfaceClass
-        IFoo = InterfaceClass('IFoo')
-        class Foo(object):
-            pass
-        decorator = self._makeOne(IFoo)
-        returned = decorator(Foo)
-        self.assertTrue(returned is Foo)
-        spec = Foo.__implemented__ # pylint:disable=no-member
-        self.assertEqual(spec.__name__,
-                         'zope.interface.tests.test_declarations.Foo')
-        self.assertIs(spec.inherit, Foo)
-        self.assertIs(Foo.__implemented__, spec) # pylint:disable=no-member
-        self.assertIsInstance(Foo.__providedBy__, ClassProvides) # pylint:disable=no-member
-        self.assertIsInstance(Foo.__provides__, ClassProvides) # pylint:disable=no-member
-        self.assertEqual(Foo.__provides__, Foo.__providedBy__) # pylint:disable=no-member
+    def _callFUT(self, cls, *ifaces):
+        decorator = self._makeOne(*ifaces)
+        return decorator(cls)
 
     def test_nonclass_cannot_assign_attr(self):
         from zope.interface.interface import InterfaceClass
@@ -997,7 +1079,8 @@ class Test_implementer(unittest.TestCase):
         self.assertIs(foo.__implemented__, spec) # pylint:disable=no-member
 
 
-class Test_implementer_only(unittest.TestCase):
+
+class Test_implementer_only(Test_classImplementsOnly):
 
     def _getTargetClass(self):
         from zope.interface.declarations import implementer_only
@@ -1006,6 +1089,10 @@ class Test_implementer_only(unittest.TestCase):
     def _makeOne(self, *args, **kw):
         return self._getTargetClass()(*args, **kw)
 
+    def _callFUT(self, cls, iface):
+        decorator = self._makeOne(iface)
+        return decorator(cls)
+
     def test_function(self):
         from zope.interface.interface import InterfaceClass
         IFoo = InterfaceClass('IFoo')
@@ -1023,38 +1110,6 @@ class Test_implementer_only(unittest.TestCase):
                 raise NotImplementedError()
         self.assertRaises(ValueError, decorator, Bar._method)
 
-    def test_oldstyle_class(self):
-        # TODO Py3 story
-        from zope.interface.declarations import Implements
-        from zope.interface.interface import InterfaceClass
-        IFoo = InterfaceClass('IFoo')
-        IBar = InterfaceClass('IBar')
-        old_spec = Implements(IBar)
-        class Foo:
-            __implemented__ = old_spec
-        decorator = self._makeOne(IFoo)
-        returned = decorator(Foo)
-        self.assertTrue(returned is Foo)
-        spec = Foo.__implemented__
-        self.assertEqual(spec.__name__, '?')
-        self.assertTrue(spec.inherit is None)
-        self.assertTrue(Foo.__implemented__ is spec)
-
-    def test_newstyle_class(self):
-        from zope.interface.declarations import Implements
-        from zope.interface.interface import InterfaceClass
-        IFoo = InterfaceClass('IFoo')
-        IBar = InterfaceClass('IBar')
-        old_spec = Implements(IBar)
-        class Foo(object):
-            __implemented__ = old_spec
-        decorator = self._makeOne(IFoo)
-        returned = decorator(Foo)
-        self.assertTrue(returned is Foo)
-        spec = Foo.__implemented__
-        self.assertEqual(spec.__name__, '?')
-        self.assertTrue(spec.inherit is None)
-        self.assertTrue(Foo.__implemented__ is spec)
 
 
 # Test '_implements' by way of 'implements{,Only}', its only callers.
diff --git a/src/zope/interface/tests/test_registry.py b/src/zope/interface/tests/test_registry.py
index 2df4ae8..cb1fdf9 100644
--- a/src/zope/interface/tests/test_registry.py
+++ b/src/zope/interface/tests/test_registry.py
@@ -873,7 +873,7 @@ class ComponentsTests(unittest.TestCase):
         ibar = IFoo('IBar')
         _info = u'info'
         _name = u'name'
-        _to_reg = object()
+
         class _Factory(object):
             pass