Make the resolution order of all fields consistent.issue80

And test this. Fixes #80.

Add Python 3.8, drop Python 3.4.

10 files changed, 175 insertions, 49 deletions

diff --git a/.gitignore b/.gitignore
index a063dff..f79af92 100644
--- a/.gitignore
+++ b/.gitignore
@@ -1,4 +1,5 @@
 *.pyc
+*.pyo
 __pycache__
 .installed.cfg
 bin
diff --git a/.travis.yml b/.travis.yml
index 6e19505..e8ad1da 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -2,21 +2,17 @@ language: python
 sudo: false
 python:
   - 2.7
-  - 3.4
   - 3.5
   - 3.6
+  - 3.7
+  - 3.8
   - pypy
   - pypy3
-matrix:
-    include:
-        - python: "3.7"
-          dist: xenial
-          sudo: true
 
 script:
   - coverage run -m zope.testrunner --test-path=src
-  - sphinx-build                         -b html    -d docs/_build/doctrees docs docs/_build/html
-  - coverage run -a `which sphinx-build` -b doctest -d docs/_build/doctrees docs docs/_build/doctest
+  - sphinx-build              -b html    -d docs/_build/doctrees docs docs/_build/html
+  - coverage run -a -m sphinx -b doctest -d docs/_build/doctrees docs docs/_build/doctest
 
 after_success:
   - coveralls
diff --git a/CHANGES.rst b/CHANGES.rst
index e14f522..6ebf24a 100644
--- a/CHANGES.rst
+++ b/CHANGES.rst
@@ -2,11 +2,19 @@
  Changes
 =========
 
-5.0.2 (unreleased)
+6.0.0 (unreleased)
 ==================
 
-- Nothing changed yet.
+- Require zope.interface 5.0.
 
+- Ensure the resolution orders of all fields are consistent and make
+  sense. In particular, ``Bool`` fields now correctly implement
+  ``IBool`` before ``IFromUnicode``. See `issue 80
+  <https://github.com/zopefoundation/zope.schema/issues/80>`_.
+
+- Add support for Python 3.8.
+
+- Drop support for Python 3.4.
 
 5.0.1 (2020-03-06)
 ==================
diff --git a/setup.py b/setup.py
index e1010e6..443922e 100644
--- a/setup.py
+++ b/setup.py
@@ -29,7 +29,7 @@ def read(*rnames):
 
 REQUIRES = [
     'setuptools',
-    'zope.interface >= 3.6.0',
+    'zope.interface >= 5.0.0',
     'zope.event',
 ]
 
@@ -64,10 +64,10 @@ setup(
         "Programming Language :: Python :: 2",
         "Programming Language :: Python :: 2.7",
         "Programming Language :: Python :: 3",
-        "Programming Language :: Python :: 3.4",
         "Programming Language :: Python :: 3.5",
         "Programming Language :: Python :: 3.6",
         "Programming Language :: Python :: 3.7",
+        "Programming Language :: Python :: 3.8",
         "Programming Language :: Python :: Implementation :: CPython",
         "Programming Language :: Python :: Implementation :: PyPy",
         "Framework :: Zope :: 3",
diff --git a/src/zope/schema/_field.py b/src/zope/schema/_field.py
index cf0f7fe..aab68ec 100644
--- a/src/zope/schema/_field.py
+++ b/src/zope/schema/_field.py
@@ -29,7 +29,9 @@ import re
 
 
 from zope.interface import classImplements
+from zope.interface import classImplementsFirst
 from zope.interface import implementer
+from zope.interface import implementedBy
 from zope.interface.interfaces import IInterface
 
 
@@ -135,23 +137,42 @@ classImplements(Field, IField)
 MinMaxLen.min_length = FieldProperty(IMinMaxLen['min_length'])
 MinMaxLen.max_length = FieldProperty(IMinMaxLen['max_length'])
 
-classImplements(Text, IText)
-classImplements(TextLine, ITextLine)
-classImplements(Password, IPassword)
-classImplements(Bool, IBool)
-classImplements(Iterable, IIterable)
-classImplements(Container, IContainer)
-
-classImplements(Number, INumber)
-classImplements(Complex, IComplex)
-classImplements(Real, IReal)
-classImplements(Rational, IRational)
-classImplements(Integral, IIntegral)
-classImplements(Int, IInt)
-classImplements(Decimal, IDecimal)
-
-classImplements(Object, IObject)
-
+classImplementsFirst(Text, IText)
+classImplementsFirst(TextLine, ITextLine)
+classImplementsFirst(Password, IPassword)
+classImplementsFirst(Bool, IBool)
+classImplementsFirst(Iterable, IIterable)
+classImplementsFirst(Container, IContainer)
+
+classImplementsFirst(Number, INumber)
+classImplementsFirst(Complex, IComplex)
+classImplementsFirst(Real, IReal)
+classImplementsFirst(Rational, IRational)
+classImplementsFirst(Integral, IIntegral)
+classImplementsFirst(Int, IInt)
+classImplementsFirst(Decimal, IDecimal)
+
+classImplementsFirst(Object, IObject)
+
+
+class implementer_if_needed(object):
+    # Helper to make sure we don't redundantly implement
+    # interfaces already inherited. Doing so tends to produce
+    # problems with the C3 order. This is used when we cannot
+    # statically determine if we need the interface or not, e.g,
+    # because we're picking different base classes under some circumstances.
+    def __init__(self, *ifaces):
+        self._ifaces = ifaces
+
+    def __call__(self, cls):
+        ifaces_needed = []
+        implemented = implementedBy(cls)
+        ifaces_needed = [
+            iface
+            for iface in self._ifaces
+            if not implemented.isOrExtends(iface)
+        ]
+        return implementer(*ifaces_needed)(cls)
 
 
 @implementer(ISourceText)
@@ -176,7 +197,7 @@ class Bytes(MinMaxLen, Field):
         return value
 
 
-@implementer(INativeString, IFromUnicode, IFromBytes)
+@implementer_if_needed(INativeString, IFromUnicode, IFromBytes)
 class NativeString(Text if PY3 else Bytes):
     """
     A native string is always the type `str`.
@@ -219,7 +240,7 @@ class BytesLine(Bytes):
         return b'\n' not in value
 
 
-@implementer(INativeStringLine, IFromUnicode, IFromBytes)
+@implementer_if_needed(INativeStringLine, IFromUnicode, IFromBytes)
 class NativeStringLine(TextLine if PY3 else BytesLine):
     """
     A native string is always the type `str`; this field excludes
@@ -462,7 +483,10 @@ class Choice(Field):
             raise ConstraintNotSatisfied(value, self.__name__).with_field_and_value(self, value)
 
 
-@implementer(IFromUnicode, IFromBytes)
+# Both of these are inherited from the parent; re-declaring them
+# here messes with the __sro__ of subclasses, causing them to be
+# inconsistent with C3.
+# @implementer(IFromUnicode, IFromBytes)
 class _StrippedNativeStringLine(NativeStringLine):
 
     _invalid_exc_type = None
diff --git a/src/zope/schema/accessors.py b/src/zope/schema/accessors.py
index 983c1cd..decf3aa 100644
--- a/src/zope/schema/accessors.py
+++ b/src/zope/schema/accessors.py
@@ -36,25 +36,59 @@ specifications.  Write accessors are solely method specifications.
 
 from zope.interface import providedBy, implementedBy
 from zope.interface.interface import Method
+from zope.interface.declarations import Declaration
 
 
 class FieldReadAccessor(Method):
     """Field read accessor
     """
 
-    # A read field accessor is a method and a field.
-    # A read accessor is a decorator of a field, using the given
-    # fields properties to provide meta data.
-
-    def __provides__(self):
-        return providedBy(self.field) + implementedBy(FieldReadAccessor)
-    __provides__ = property(__provides__)
-
     def __init__(self, field):
         self.field = field
         Method.__init__(self, '')
         self.__doc__ = 'get %s' % field.__doc__
 
+    # A read field accessor is a method and a field.
+    # A read accessor is a decorator of a field, using the given
+    # field's properties to provide meta data.
+
+    @property
+    def __provides__(self):
+        provided = providedBy(self.field)
+        implemented = implementedBy(FieldReadAccessor)
+
+        # Declaration.__add__ is not very smart in zope.interface 5.0.0.
+        # It's very easy to produce C3 inconsistent orderings using
+        # it, because it uses itself plus any new interfaces from the
+        # second argument as the ``__bases__``, ignoring their
+        # relative order.
+        #
+        # Here, we can easily work around that. We know that ``field``
+        # will be some sub-class of Attribute, just as we are
+        # (FieldReadAccessor <- Method <- Attribute). So there will be
+        # overlap, and commonly only IMethod would be added to the end
+        # of the list of bases; but since IMethod extends IAttribute,
+        # having IAttribute earlier in the bases will be inconsistent.
+        # The fix here is to remove those duplicates from the first
+        # element so that we don't get into that situation.
+        provided_list = list(provided)
+        for iface in implemented:
+            if iface in provided_list:
+                provided_list.remove(iface)
+        provided = Declaration(*provided_list)
+        try:
+            return provided + implemented
+        except BaseException as e: # pragma: no cover pylint:disable=broad-except
+            # Sadly, zope.interface catches and silently ignores
+            # any exceptions raised in ``__providedBy__``,
+            # which is the class descriptor that invokes ``__provides__``.
+            # So, for example, if we're in strict C3 mode and fail to produce
+            # a resolution order, that gets ignored and we fallback to just what's
+            # implemented by the class.
+            # That's not good. Do our best to propagate the exception by returning it.
+            # There will be downstream errors later.
+            return e
+
     def getSignatureString(self):
         return '()'
 
diff --git a/src/zope/schema/tests/test__bootstrapfields.py b/src/zope/schema/tests/test__bootstrapfields.py
index 027944c..58e7e31 100644
--- a/src/zope/schema/tests/test__bootstrapfields.py
+++ b/src/zope/schema/tests/test__bootstrapfields.py
@@ -17,8 +17,9 @@ import unittest
 import unicodedata
 
 # pylint:disable=protected-access,inherit-non-class,blacklisted-name
+# pylint:disable=attribute-defined-outside-init
 
-class EqualityTestsMixin(object):
+class InterfaceConformanceTestsMixin(object):
 
     def _getTargetClass(self):
         raise NotImplementedError
@@ -54,6 +55,26 @@ class EqualityTestsMixin(object):
             verifyObject(iface, instance)
         return verifyObject
 
+    def test_iface_is_first_in_sro(self):
+        from zope.interface import implementedBy
+        implemented = implementedBy(self._getTargetClass())
+        __traceback_info__ = implemented.__sro__
+        self.assertIs(implemented, implemented.__sro__[0])
+        self.assertIs(self._getTargetInterface(), implemented.__sro__[1])
+
+    def test_implements_consistent__sro__(self):
+        from zope.interface import ro
+        from zope.interface import implementedBy
+        __traceback_info__ = implementedBy(self._getTargetClass()).__sro__
+        self.assertTrue(ro.is_consistent(implementedBy(self._getTargetClass())))
+
+    def test_iface_consistent_ro(self):
+        from zope.interface import ro
+        __traceback_info__ = self._getTargetInterface().__iro__
+        self.assertTrue(ro.is_consistent(self._getTargetInterface()))
+
+class EqualityTestsMixin(InterfaceConformanceTestsMixin):
+
     def test_is_hashable(self):
         field = self._makeOne()
         hash(field)  # doesn't raise
diff --git a/src/zope/schema/tests/test__field.py b/src/zope/schema/tests/test__field.py
index 2e78b8e..13600d5 100644
--- a/src/zope/schema/tests/test__field.py
+++ b/src/zope/schema/tests/test__field.py
@@ -711,7 +711,7 @@ class ChoiceTests(EqualityTestsMixin,
         from zope.schema.vocabulary import setVocabularyRegistry
 
         class Reg(object):
-            def get(*args):
+            def get(self, *args):
                 raise LookupError
 
         setVocabularyRegistry(Reg())
@@ -1369,7 +1369,7 @@ class SequenceTests(WrongTypeTestsMixin,
         from zope.schema._field import abc
 
         class MutableSequence(abc.MutableSequence):
-            def insert(self, item, ix):
+            def insert(self, index, value):
                 raise AssertionError("not implemented")
             def __getitem__(self, name):
                 raise AssertionError("not implemented")
@@ -1754,6 +1754,21 @@ class NativeStringLineTests(EqualityTestsMixin,
         self.assertEqual(field.fromUnicode(u'DEADBEEF'), 'DEADBEEF')
 
 
+class StrippedNativeStringLineTests(NativeStringLineTests):
+
+    def _getTargetClass(self):
+        from zope.schema._field import _StrippedNativeStringLine
+        return _StrippedNativeStringLine
+
+    def test_strips(self):
+        field = self._makeOne()
+        self.assertEqual(field.fromBytes(b' '), '')
+        self.assertEqual(field.fromUnicode(u' '), '')
+
+    def test_iface_is_first_in_sro(self):
+        self.skipTest("Not applicable; we inherit implementation but have no interface")
+
+
 def _makeSampleVocabulary():
     from zope.interface import implementer
     from zope.schema.interfaces import IVocabulary
@@ -1784,7 +1799,7 @@ def _makeDummyRegistry(v):
             VocabularyRegistry.__init__(self)
             self._vocabulary = vocabulary
 
-        def get(self, object, name):
+        def get(self, context, name):
             return self._vocabulary
     return DummyRegistry(v)
 
diff --git a/src/zope/schema/tests/test_accessors.py b/src/zope/schema/tests/test_accessors.py
index 1512766..dbd3a48 100644
--- a/src/zope/schema/tests/test_accessors.py
+++ b/src/zope/schema/tests/test_accessors.py
@@ -15,6 +15,7 @@
 """
 import unittest
 
+# pylint:disable=inherit-non-class
 
 class FieldReadAccessorTests(unittest.TestCase):
 
@@ -59,15 +60,41 @@ class FieldReadAccessorTests(unittest.TestCase):
     def test___provides___w_field_w_provides(self):
         from zope.interface import implementedBy
         from zope.interface import providedBy
+        from zope.interface.interfaces import IAttribute
+        from zope.interface.interfaces import IMethod
         from zope.schema import Text
+
+        # When wrapping a field that provides stuff,
+        # we provide the same stuff, with the addition of
+        # IMethod at the correct spot in the IRO (just before
+        # IAttribute).
         field = Text()
         field_provides = list(providedBy(field))
         wrapped = self._makeOne(field)
         wrapped_provides = list(providedBy(wrapped))
-        self.assertEqual(wrapped_provides[:len(field_provides)],
-                         list(providedBy(field)))
+
+        index_of_attribute = field_provides.index(IAttribute)
+        expected = list(field_provides)
+        expected.insert(index_of_attribute, IMethod)
+        self.assertEqual(expected, wrapped_provides)
         for iface in list(implementedBy(self._getTargetClass())):
-            self.assertTrue(iface in wrapped_provides)
+            self.assertIn(iface, wrapped_provides)
+
+    def test___provides___w_field_w_provides_strict(self):
+        from zope.interface import ro
+        attr = 'STRICT_IRO'
+        try:
+            getattr(ro.C3, attr)
+        except AttributeError:
+            # https://github.com/zopefoundation/zope.interface/issues/194
+            # zope.interface 5.0.0 used this incorrect spelling.
+            attr = 'STRICT_RO'
+            getattr(ro.C3, attr)
+        setattr(ro.C3, attr, True)
+        try:
+            self.test___provides___w_field_w_provides()
+        finally:
+            setattr(ro.C3, attr, getattr(ro.C3, 'ORIG_' + attr))
 
     def test_getSignatureString(self):
         wrapped = self._makeOne()
@@ -180,7 +207,7 @@ class FieldReadAccessorTests(unittest.TestCase):
             pass
 
         writer = Writer()
-        writer.__name__ = 'setMe'
+        writer.__name__ = 'setMe' # pylint:disable=attribute-defined-outside-init
         getter.writer = writer
 
         class Foo(object):
diff --git a/tox.ini b/tox.ini
index 184397e..feb5397 100644
--- a/tox.ini
+++ b/tox.ini
@@ -3,7 +3,7 @@ envlist =
 # Jython support pending 2.7 support, due 2012-07-15 or so.  See:
 # http://fwierzbicki.blogspot.com/2012/03/adconion-to-fund-jython-27.html
 #   py27,jython,pypy,coverage
-    py27,py34,py35,py36,py37,pypy,pypy3,coverage,docs
+    py27,py35,py36,py37,py38,pypy,pypy3,coverage,docs
 
 [testenv]
 deps =