Fix verification for methods of builtin types with pseudo-default arguments on Pypyissue118

On PyPy2, they are ignored (like on CPython), but on PyPy3 they can
actually be validated.

Fixes #118

7 files changed, 177 insertions, 46 deletions

diff --git a/CHANGES.rst b/CHANGES.rst
index bf23808..efb7b88 100644
--- a/CHANGES.rst
+++ b/CHANGES.rst
@@ -81,6 +81,21 @@
   `issue 153
   <https://github.com/zopefoundation/zope.interface/issues/153>`_.
 
+- Fix ``verifyClass`` and ``verifyObject`` for builtin types like
+  ``dict`` that have methods taking an optional, unnamed argument with
+  no default value like ``dict.pop``. On PyPy3, the verification is
+  strict, but on PyPy2 (as on all versions of CPython) those methods
+  cannot be verified and are ignored. See `issue 118
+  <https://github.com/zopefoundation/zope.interface/issues/118>`_.
+
+- Update the common interfaces ``IEnumerableMapping``,
+  ``IExtendedReadMapping``, ``IExtendedWriteMapping``,
+  ``IReadSequence`` and ``IUniqueMemberWriteSequence`` to no longer
+  require methods that were removed from Python 3 on Python 3, such as
+  ``__setslice___``. Now, ``dict``, ``list`` and ``tuple`` properly
+  verify as ``IFullMapping``, ``ISequence`` and ``IReadSequence,``
+  respectively on all versions of Python.
+
 4.7.1 (2019-11-11)
 ==================
 
diff --git a/src/zope/interface/_compat.py b/src/zope/interface/_compat.py
index f8b7887..a57951a 100644
--- a/src/zope/interface/_compat.py
+++ b/src/zope/interface/_compat.py
@@ -54,6 +54,9 @@ else:
     PYTHON3 = True
     PYTHON2 = False
 
+PYPY = hasattr(sys, 'pypy_version_info')
+PYPY2 = PYTHON2 and PYPY
+
 def _skip_under_py3k(test_method):
     import unittest
     return unittest.skipIf(sys.version_info[0] >= 3, "Only on Python 2")(test_method)
diff --git a/src/zope/interface/common/mapping.py b/src/zope/interface/common/mapping.py
index 1c5661a..a005aed 100644
--- a/src/zope/interface/common/mapping.py
+++ b/src/zope/interface/common/mapping.py
@@ -17,6 +17,7 @@ Importing this module does *not* mark any standard classes
 as implementing any of these interfaces.
 """
 from zope.interface import Interface
+from zope.interface._compat import PYTHON2 as PY2
 
 class IItemMapping(Interface):
     """Simplest readable mapping object
@@ -89,14 +90,15 @@ class IIterableMapping(IEnumerableMapping):
     without copying.
     """
 
-    def iterkeys():
-        "iterate over keys; equivalent to ``__iter__``"
+    if PY2:
+        def iterkeys():
+            "iterate over keys; equivalent to ``__iter__``"
 
-    def itervalues():
-        "iterate over values"
+        def itervalues():
+            "iterate over values"
 
-    def iteritems():
-        "iterate over items"
+        def iteritems():
+            "iterate over items"
 
 class IClonableMapping(Interface):
     """Something that can produce a copy of itself.
@@ -115,8 +117,9 @@ class IExtendedReadMapping(IIterableMapping):
     in Python 3.
     """
 
-    def has_key(key):
-        """Tell if a key exists in the mapping; equivalent to ``__contains__``"""
+    if PY2:
+        def has_key(key):
+            """Tell if a key exists in the mapping; equivalent to ``__contains__``"""
 
 class IExtendedWriteMapping(IWriteMapping):
     """Additional mutation methods.
@@ -133,12 +136,16 @@ class IExtendedWriteMapping(IWriteMapping):
     def setdefault(key, default=None):
         "D.setdefault(k[,d]) -> D.get(k,d), also set D[k]=d if k not in D"
 
-    def pop(k, *args):
-        """Remove specified key and return the corresponding value.
+    def pop(k, default=None):
+        """
+        pop(k[,default]) -> value
+
+        Remove specified key and return the corresponding value.
 
-        ``*args`` may contain a single default value, or may not be supplied.
-        If key is not found, default is returned if given, otherwise
-        `KeyError` is raised"""
+        If key is not found, *default* is returned if given, otherwise
+        `KeyError` is raised. Note that *default* must not be passed by
+        name.
+        """
 
     def popitem():
         """remove and return some (key, value) pair as a
diff --git a/src/zope/interface/common/sequence.py b/src/zope/interface/common/sequence.py
index 393918e..c36d615 100644
--- a/src/zope/interface/common/sequence.py
+++ b/src/zope/interface/common/sequence.py
@@ -19,6 +19,7 @@ as implementing any of these interfaces.
 
 __docformat__ = 'restructuredtext'
 from zope.interface import Interface
+from zope.interface._compat import PYTHON2 as PY2
 
 class IMinimalSequence(Interface):
     """Most basic sequence interface.
@@ -79,13 +80,14 @@ class IReadSequence(IFiniteSequence):
     def __rmul__(n):
         """``x.__rmul__(n) <==> n * x``"""
 
-    def __getslice__(i, j):
-        """``x.__getslice__(i, j) <==> x[i:j]``
+    if PY2:
+        def __getslice__(i, j):
+            """``x.__getslice__(i, j) <==> x[i:j]``
 
-        Use of negative indices is not supported.
+            Use of negative indices is not supported.
 
-        Deprecated since Python 2.0 but still a part of `UserList`.
-        """
+            Deprecated since Python 2.0 but still a part of `UserList`.
+            """
 
 class IExtendedReadSequence(IReadSequence):
     """Full read interface for lists"""
@@ -116,21 +118,23 @@ class IUniqueMemberWriteSequence(Interface):
         supports slice objects.
         """
 
-    def __setslice__(i, j, other):
-        """``x.__setslice__(i, j, other) <==> x[i:j] = other``
+    if PY2:
+        def __setslice__(i, j, other):
+            """``x.__setslice__(i, j, other) <==> x[i:j] = other``
 
-        Use of negative indices is not supported.
+            Use of negative indices is not supported.
 
-        Deprecated since Python 2.0 but still a part of `UserList`.
-        """
+            Deprecated since Python 2.0 but still a part of `UserList`.
+            """
 
-    def __delslice__(i, j):
-        """``x.__delslice__(i, j) <==> del x[i:j]``
+        def __delslice__(i, j):
+            """``x.__delslice__(i, j) <==> del x[i:j]``
 
-        Use of negative indices is not supported.
+            Use of negative indices is not supported.
+
+            Deprecated since Python 2.0 but still a part of `UserList`.
+            """
 
-        Deprecated since Python 2.0 but still a part of `UserList`.
-        """
     def __iadd__(y):
         """``x.__iadd__(y) <==> x += y``"""
 
diff --git a/src/zope/interface/interface.py b/src/zope/interface/interface.py
index 1681c75..b5d0e92 100644
--- a/src/zope/interface/interface.py
+++ b/src/zope/interface/interface.py
@@ -698,11 +698,18 @@ def fromFunction(func, interface=None, imlevel=0, name=None):
     defaults = getattr(func, '__defaults__', None) or ()
     code = func.__code__
     # Number of positional arguments
-    na = code.co_argcount-imlevel
+    na = code.co_argcount - imlevel
     names = code.co_varnames[imlevel:]
     opt = {}
     # Number of required arguments
-    nr = na-len(defaults)
+    defaults_count = len(defaults)
+    if not defaults_count:
+        # PyPy3 uses ``__defaults_count__`` for builtin methods
+        # like ``dict.pop``. Surprisingly, these don't have recorded
+        # ``__defaults__``
+        defaults_count = getattr(func, '__defaults_count__', 0)
+
+    nr = na - defaults_count
     if nr < 0:
         defaults = defaults[-nr:]
         nr = 0
diff --git a/src/zope/interface/tests/test_verify.py b/src/zope/interface/tests/test_verify.py
index 5ad8bff..65e390a 100644
--- a/src/zope/interface/tests/test_verify.py
+++ b/src/zope/interface/tests/test_verify.py
@@ -15,12 +15,29 @@
 """
 import unittest
 
+# pylint:disable=inherit-non-class,no-method-argument,no-self-argument
 
 class Test_verifyClass(unittest.TestCase):
 
-    def _callFUT(self, iface, klass):
+    verifier = None
+
+    @classmethod
+    def setUpClass(cls):
+        # zope.testrunner doesn't call setUpClass, so if you get
+        # 'NoneType is not callable', that's why.
+        cls.verifier = staticmethod(cls._get_FUT())
+
+    @classmethod
+    def _get_FUT(cls):
         from zope.interface.verify import verifyClass
-        return verifyClass(iface, klass)
+        return verifyClass
+
+    _adjust_object_before_verify = lambda self, x: x
+
+    def _callFUT(self, iface, klass, **kwargs):
+        return self.verifier(iface,
+                             self._adjust_object_before_verify(klass),
+                             **kwargs)
 
     def test_class_doesnt_implement(self):
         from zope.interface import Interface
@@ -54,7 +71,8 @@ class Test_verifyClass(unittest.TestCase):
         from zope.interface.exceptions import BrokenImplementation
 
         class ICurrent(Interface):
-            def method(): pass
+            def method():
+                pass
 
         @implementer(ICurrent)
         class Current(object):
@@ -68,7 +86,8 @@ class Test_verifyClass(unittest.TestCase):
         from zope.interface import implementer
 
         class ICurrent(Interface):
-            def method(): pass
+            def method():
+                pass
 
         @implementer(ICurrent)
         class Current(object):
@@ -514,13 +533,38 @@ class Test_verifyClass(unittest.TestCase):
 
         self._callFUT(ICurrent, Current)
 
+    def test_dict_IFullMapping(self):
+        # A dict should be an IFullMapping, but this exposes two
+        # issues. First, on CPython, methods of builtin types are
+        # "method_descriptor" objects, and are harder to introspect.
+        # Second, on PyPy, the signatures can be just plain wrong,
+        # specifying as required arguments that are actually optional.
+        # See https://github.com/zopefoundation/zope.interface/issues/118
+        from zope.interface.common.mapping import IFullMapping
+        self._callFUT(IFullMapping, dict, tentative=True)
+
+    def test_list_ISequence(self):
+        # As for test_dict_IFullMapping
+        from zope.interface.common.sequence import ISequence
+        self._callFUT(ISequence, list, tentative=True)
+
+    def test_tuple_IReadSequence(self):
+        # As for test_dict_IFullMapping
+        from zope.interface.common.sequence import IReadSequence
+        self._callFUT(IReadSequence, tuple, tentative=True)
+
+
 class Test_verifyObject(Test_verifyClass):
 
-    def _callFUT(self, iface, target):
+    @classmethod
+    def _get_FUT(cls):
         from zope.interface.verify import verifyObject
+        return verifyObject
+
+    def _adjust_object_before_verify(self, target):
         if isinstance(target, (type, type(OldSkool))):
             target = target()
-        return verifyObject(iface, target)
+        return target
 
     def test_class_misses_attribute_for_attribute(self):
         # This check *fails* for verifyObject
diff --git a/src/zope/interface/verify.py b/src/zope/interface/verify.py
index d43e49f..722bb4f 100644
--- a/src/zope/interface/verify.py
+++ b/src/zope/interface/verify.py
@@ -13,8 +13,13 @@
 ##############################################################################
 """Verify interface implementations
 """
+from __future__ import print_function
+import inspect
 import sys
-from types import FunctionType, MethodType
+from types import FunctionType
+from types import MethodType
+
+from zope.interface._compat import PYPY2
 
 from zope.interface.exceptions import BrokenImplementation, DoesNotImplement
 from zope.interface.exceptions import BrokenMethodImplementation
@@ -30,21 +35,21 @@ __all__ = [
 MethodTypes = (MethodType, )
 
 
-def _verify(iface, candidate, tentative=0, vtype=None):
-    """Verify that 'candidate' might correctly implements 'iface'.
+def _verify(iface, candidate, tentative=False, vtype=None):
+    """Verify that *candidate* might correctly implement *iface*.
 
     This involves:
 
-      o Making sure the candidate defines all the necessary methods
+      - Making sure the candidate defines all the necessary methods
 
-      o Making sure the methods have the correct signature
+      - Making sure the methods have the correct signature
 
-      o Making sure the candidate asserts that it implements the interface
+      - Making sure the candidate asserts that it implements the interface
 
     Note that this isn't the same as verifying that the class does
     implement the interface.
 
-    If optional tentative is true, suppress the "is implemented by" test.
+    If  *tentative* is true (not the default), suppress the "is implemented by" test.
     """
 
     if vtype == 'c':
@@ -71,6 +76,20 @@ def _verify(iface, candidate, tentative=0, vtype=None):
             # If it's not a method, there's nothing else we can test
             continue
 
+        if inspect.ismethoddescriptor(attr) or inspect.isbuiltin(attr):
+            # The first case is what you get for things like ``dict.pop``
+            # on CPython (e.g., ``verifyClass(IFullMapping, dict))``). The
+            # second case is what you get for things like ``dict().pop`` on
+            # CPython (e.g., ``verifyObject(IFullMapping, dict()))``.
+            # In neither case can we get a signature, so there's nothing
+            # to verify. Even the inspect module gives up and raises
+            # ValueError: no signature found. The ``__text_signature__`` attribute
+            # isn't typically populated either.
+            #
+            # Note that on PyPy 2 or 3 (up through 7.3 at least), these are
+            # not true for things like ``dict.pop`` (but might be true for C extensions?)
+            continue
+
         if isinstance(attr, FunctionType):
             if sys.version_info[0] >= 3 and isinstance(candidate, type) and vtype == 'c':
                 # This is an "unbound method" in Python 3.
@@ -103,23 +122,55 @@ def _verify(iface, candidate, tentative=0, vtype=None):
 
         mess = _incompat(desc, meth)
         if mess:
+            if PYPY2 and _pypy2_false_positive(mess, candidate, vtype):
+                continue
             raise BrokenMethodImplementation(name, mess)
 
     return True
 
-def verifyClass(iface, candidate, tentative=0):
+def verifyClass(iface, candidate, tentative=False):
     return _verify(iface, candidate, tentative, vtype='c')
 
-def verifyObject(iface, candidate, tentative=0):
+def verifyObject(iface, candidate, tentative=False):
     return _verify(iface, candidate, tentative, vtype='o')
 
+
+_MSG_TOO_MANY = 'implementation requires too many arguments'
+_KNOWN_PYPY2_FALSE_POSITIVES = frozenset((
+    _MSG_TOO_MANY,
+))
+
+
+def _pypy2_false_positive(msg, candidate, vtype):
+    # On PyPy2, builtin methods and functions like
+    # ``dict.pop`` that take pseudo-optional arguments
+    # (those with no default, something you can't express in Python 2
+    # syntax; CPython uses special internal APIs to implement these methods)
+    # return false failures because PyPy2 doesn't expose any way
+    # to detect this pseudo-optional status. PyPy3 doesn't have this problem
+    # because of __defaults_count__, and CPython never gets here because it
+    # returns true for ``ismethoddescriptor`` or ``isbuiltin``.
+    #
+    # We can't catch all such cases, but we can handle the common ones.
+    #
+    if msg not in _KNOWN_PYPY2_FALSE_POSITIVES:
+        return False
+
+    known_builtin_types = vars(__builtins__).values()
+    candidate_type = candidate if vtype == 'c' else type(candidate)
+    if candidate_type in known_builtin_types:
+        return True
+
+    return False
+
+
 def _incompat(required, implemented):
     #if (required['positional'] !=
     #    implemented['positional'][:len(required['positional'])]
     #    and implemented['kwargs'] is None):
     #    return 'imlementation has different argument names'
     if len(implemented['required']) > len(required['required']):
-        return 'implementation requires too many arguments'
+        return _MSG_TOO_MANY
     if ((len(implemented['positional']) < len(required['positional']))
         and not implemented['varargs']):
         return "implementation doesn't allow enough arguments"