Fix the bad IROs in the bundled ABC interfaces, and implement a way t…

…o get warnings or errors.

src/zope/interface/common/__init__.py

@@ -125,10 +125,9 @@ def __init__(self, name, bases, attrs):
         if 'abc' not in attrs:
             # Something like ``IList(ISequence)``: We're extending
             # abc interfaces but not an ABC interface ourself.
-            self.__class__ = InterfaceClass
             InterfaceClass.__init__(self, name, bases, attrs)
-            for cls in extra_classes:
-                classImplements(cls, self)
+            ABCInterfaceClass.__register_classes(self, extra_classes)
+            self.__class__ = InterfaceClass
             return
 
         based_on = attrs.pop('abc')
@@ -216,11 +215,11 @@ def __method_from_function(self, function, name):
         method.positional = method.positional[1:]
         return method
 
-    def __register_classes(self):
+    def __register_classes(self, conformers=None):
         # Make the concrete classes already present in our ABC's registry
         # declare that they implement this interface.
-
-        for cls in self.getRegisteredConformers():
+        conformers = conformers if conformers is not None else self.getRegisteredConformers()
+        for cls in conformers:
             classImplements(cls, self)
 
     def getABC(self):

src/zope/interface/common/builtins.py

@@ -37,7 +37,6 @@
 ]
 
 # pylint:disable=no-self-argument
-
 class IList(collections.IMutableSequence):
     """
     Interface for :class:`list`

src/zope/interface/common/tests/__init__.py

@@ -50,6 +50,7 @@ def predicate(iface):
 
 
 def add_verify_tests(cls, iface_classes_iter):
+    cls.maxDiff = None
     for iface, registered_classes in iface_classes_iter:
         for stdlib_class in registered_classes:
 
@@ -59,15 +60,33 @@ def test(self, stdlib_class=stdlib_class, iface=iface):
 
                 self.assertTrue(self.verify(iface, stdlib_class))
 
-            name = 'test_auto_' + stdlib_class.__name__ + '_' + iface.__name__
+            suffix = stdlib_class.__name__ + '_' + iface.__name__
+            name = 'test_auto_' + suffix
             test.__name__ = name
             assert not hasattr(cls, name)
             setattr(cls, name, test)
 
+            def test_ro(self, stdlib_class=stdlib_class, iface=iface):
+                from zope.interface import ro
+                from zope.interface import implementedBy
+                self.assertEqual(
+                    tuple(ro.ro(iface, strict=True)),
+                    iface.__sro__)
+                implements = implementedBy(stdlib_class)
+                strict = stdlib_class not in self.NON_STRICT_RO
+                self.assertEqual(
+                    tuple(ro.ro(implements, strict=strict)),
+                    implements.__sro__)
+
+            name = 'test_auto_ro_' + suffix
+            test_ro.__name__ = name
+            assert not hasattr(cls, name)
+            setattr(cls, name, test_ro)
 
 class VerifyClassMixin(unittest.TestCase):
     verifier = staticmethod(verifyClass)
     UNVERIFIABLE = ()
+    NON_STRICT_RO = ()
 
     def _adjust_object_before_verify(self, iface, x):
         return x

src/zope/interface/common/tests/test_collections.py

@@ -17,6 +17,7 @@
 except ImportError:
     import collections as abc
 from collections import deque
+from collections import OrderedDict
 
 
 try:
@@ -118,6 +119,9 @@ def test_non_iterable_UserDict(self):
             type({}.viewitems()),
             type({}.viewkeys()),
         })
+        NON_STRICT_RO = {
+            OrderedDict
+        }
 
 add_abc_interface_tests(TestVerifyClass, collections.ISet.__module__)
 

src/zope/interface/declarations.py

@@ -458,7 +458,20 @@ def classImplements(cls, *interfaces):
       are added to any interfaces previously declared.
     """
     spec = implementedBy(cls)
-    spec.declared += tuple(_normalizeargs(interfaces))
+    interfaces = tuple(_normalizeargs(interfaces))
+    append = True
+    if len(interfaces) == 1:
+        # In the common case of a single interface, take steps to try
+        # to avoid producing an invalid resolution order, while
+        # still allowing for BWC (in the past, we always appended)
+        for b in spec.declared:
+            if interfaces[0].extends(b):
+                append = False
+                break
+    if append:
+        spec.declared += interfaces
+    else:
+        spec.declared = interfaces + spec.declared
 
     # compute the bases
     bases = []

src/zope/interface/ro.py

@@ -80,6 +80,22 @@ def _legacy_ro(ob):
 # 1200 Implements, 1100 ClassProvides objects)
 ###
 
+# Allow changing during runtime for tests.
+STRICT_RO = False
+TRACK_BAD_IRO = None
+BAD_IROS = None
+
+class InconsistentResolutionOrderWarning(PendingDeprecationWarning):
+    """
+    The warning issued when an invalid IRO is requested.
+    """
+
+class InconsistentResolutionOrderError(TypeError):
+    """
+    The error raised when an invalid IRO is requested in strict mode.
+    """
+
+
 def _can_choose_base(base, base_tree_remaining):
     # From C3:
     # nothead = [s for s in nonemptyseqs if cand in s[1:]]
@@ -91,15 +107,39 @@ def _can_choose_base(base, base_tree_remaining):
                 return False
     return True
 
+
 def _nonempty_bases_ignoring(base_tree, ignoring):
     return list(filter(None, (
         [b for b in bases if b is not ignoring]
         for bases
         in base_tree
     )))
 
-def _find_next_base(C, base_tree_remaining, ignore_first):
+
+def _warn_iro():
+    import warnings
+    warnings.warn(
+        "An inconsistent resolution order is being requested. "
+        "(Interfaces should follow the Python class rules known as C3.) "
+        "For backwards compatibility, zope.interface will allow this, "
+        "making the best guess it can to produce as meaningful an order as possible. "
+        "In the future this might be an error. Set the warning filter to error, or set "
+        "the environment variable 'ZOPE_INTERFACE_TRACK_BAD_IRO' to '1' and examine "
+        "ro.BAD_IROS to debug.",
+        InconsistentResolutionOrderWarning,
+        # Using a stack level higher than the default tends to produce
+        # lots of noise in some applications. At this writing, 9 was the one
+        # that caught most module-level issues and showed good line numbers.
+        #stacklevel=9
+    )
+
+
+def _find_next_base(C, base_tree_remaining, memo, ignore_first=True, strict=False):
+    global TRACK_BAD_IRO
+    global BAD_IROS
+
     base = None
+    warn_iro = _warn_iro
     for bases in base_tree_remaining:
         base = bases[0]
         can_choose_base = _can_choose_base(base, base_tree_remaining)
@@ -136,28 +176,37 @@ def _find_next_base(C, base_tree_remaining, ignore_first):
         # When we do this, we also need to remove it from any other
         # place that it may still appear both transiently and then
         # permanently (that part is handled by ``_merge``).
+        if TRACK_BAD_IRO is None:
+            import os
+            import weakref
+            TRACK_BAD_IRO = os.environ.get('ZOPE_INTERFACE_TRACK_BAD_IRO', '') == "1"
+            BAD_IROS = weakref.WeakKeyDictionary()
 
-        # TODO: Have a way (environment variable) for a user to opt-in
-        # to having ``zope.interface`` track and/or warn about
-        # locations of inconsistent IROs. A user would know to turn
-        # this on because we would issue a simple warning just for the
-        # first inconsistent case we saw (so no worrying about
-        # stacklevels or extra messaging). The problem there,
-        # naturally, is that some of the common interfaces shipped in
-        # ``zope.interface`` itself exhibit inconsistent IROs right
-        # now, so until that's changed, everyone would always get a
-        # warning. (The problem classes are list, dict, tuple, str,
-        # bytes, and OSError).
+        if strict or TRACK_BAD_IRO:
+            catch = () if strict else InconsistentResolutionOrderError
+            try:
+                raise InconsistentResolutionOrderError(
+                    "Invalid IRO", C,
+                    [[C]] + [ro(base, memo) for base in C.__bases__] + [list(C.__bases__)]
+                )
+            except catch as ex:
+                BAD_IROS[C] = ex
+                del ex
+
+        if ignore_first:
+            warn_iro()
+            warn_iro = lambda: None
         if ignore_first and len(base_tree_remaining[0]) > 1:
             first_base = base_tree_remaining[0][0]
             ignoring_first = _nonempty_bases_ignoring(base_tree_remaining, first_base)
-            base = _find_next_base(C, ignoring_first, False)
+            base = _find_next_base(C, ignoring_first, memo, ignore_first=False)
 
         if base is None:
+            warn_iro()
             base = base_tree_remaining[0][0]
     return base
 
-def _merge(C, base_tree):
+def _merge(C, base_tree, memo, strict):
     # Returns a merged *list*.
     result = []
     base_tree_remaining = base_tree
@@ -172,11 +221,12 @@ def _merge(C, base_tree):
         if not base_tree_remaining:
             return result
 
-        base = _find_next_base(C, base_tree_remaining, True)
+        base = _find_next_base(C, base_tree_remaining, memo, strict=strict)
 
         result.append(base)
 
-def ro(C, _memo=None):
+
+def ro(C, strict=None, _memo=None):
     """
     Compute the precedence list (mro) according to C3.
 
@@ -190,6 +240,7 @@ def ro(C, _memo=None):
 
     :return: A fresh `list` object.
     """
+    strict = STRICT_RO if strict is None else strict
     memo = _memo if _memo is not None else {}
     try:
         return list(memo[C])
@@ -199,6 +250,8 @@ def ro(C, _memo=None):
     result = _merge(
         C,
         [[C]] + [ro(base, memo) for base in C.__bases__] + [list(C.__bases__)],
+        memo,
+        strict
     )
 
     memo[C] = tuple(result)

src/zope/interface/tests/test_ro.py

@@ -78,10 +78,10 @@ class Qux(Bar, Baz):
 
 
 class Test_ro(unittest.TestCase):
-
-    def _callFUT(self, ob):
+    maxDiff = None
+    def _callFUT(self, ob, **kwargs):
         from zope.interface.ro import _legacy_ro
-        return _legacy_ro(ob)
+        return _legacy_ro(ob, **kwargs)
 
     def test_w_empty_bases(self):
         class Foo(object):
@@ -175,9 +175,9 @@ class ExtendsFooImplementsNothing(ExtendsFoo, ImplementsNothing):
 
 
 class Test_c3_ro(Test_ro):
-    def _callFUT(self, ob):
+    def _callFUT(self, ob, **kwargs):
         from zope.interface.ro import ro
-        return ro(ob)
+        return ro(ob, **kwargs)
 
     def test_complex_diamond(self, base=object):
         # https://github.com/zopefoundation/zope.interface/issues/21
@@ -220,10 +220,39 @@ def test_OSError_IOError(self):
         self.assertEqual(
             list(providedBy(OSError()).flattened()),
             [
-                interfaces.IIOError,
                 interfaces.IOSError,
+                interfaces.IIOError,
                 interfaces.IEnvironmentError,
                 interfaces.IStandardError,
                 interfaces.IException,
                 interfaces.Interface,
             ])
+
+    def test_non_orderable(self):
+        import warnings
+        from zope.interface import ro
+        try:
+            # If we've already warned, we must reset that state.
+            del ro.__warningregistry__
+        except AttributeError:
+            pass
+
+        with warnings.catch_warnings():
+            warnings.simplefilter('error')
+
+            with self.assertRaises(ro.InconsistentResolutionOrderWarning):
+                super(Test_c3_ro, self).test_non_orderable()
+
+        IOErr, _ = self._make_IOErr()
+        with self.assertRaises(ro.InconsistentResolutionOrderError):
+            self._callFUT(IOErr, strict=True)
+
+        old_val = ro.TRACK_BAD_IRO
+        try:
+            ro.TRACK_BAD_IRO = True
+            with warnings.catch_warnings():
+                warnings.simplefilter('ignore')
+                self._callFUT(IOErr)
+            self.assertIn(IOErr, ro.BAD_IROS)
+        finally:
+            ro.TRACK_BAD_IRO = old_val