From 45b9294e9d3b8c679df0ae029de6b1792b183782 Mon Sep 17 00:00:00 2001 From: Jason Madden Date: Wed, 17 Mar 2021 11:11:19 -0500 Subject: [PATCH] Make Provides and ClassProvides ignore redundant interfaces like @implementer cf #207 --- CHANGES.rst | 8 ++ src/zope/interface/declarations.py | 17 +++- src/zope/interface/interface.py | 9 +- src/zope/interface/tests/test_declarations.py | 87 ++++++++++++++++++- 4 files changed, 115 insertions(+), 6 deletions(-) diff --git a/CHANGES.rst b/CHANGES.rst index 94f07b17..c791f505 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 + `_. + 5.2.0 (2020-11-05) ================== diff --git a/src/zope/interface/declarations.py b/src/zope/interface/declarations.py index 9a0146cd..d6875994 100644 --- a/src/zope/interface/declarations.py +++ b/src/zope/interface/declarations.py @@ -131,6 +131,19 @@ def __add__(self, other): __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 @@ def __init__(self, cls, metacls, *interfaces): 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 d100aae2..c5d18526 100644 --- a/src/zope/interface/interface.py +++ b/src/zope/interface/interface.py @@ -413,6 +413,13 @@ def __setBases(self, bases): __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 @@ def _calculate_sro(self): # 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 9ef192a7..29b75e9c 100644 --- a/src/zope/interface/tests/test_declarations.py +++ b/src/zope/interface/tests/test_declarations.py @@ -1280,7 +1280,7 @@ class Foo(object): 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 Foo(object): 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 @@ def test__repr__(self): ) +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 Foo(object): 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 @@ def test__repr__(self): "" % 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):