Merge a4ed056 into abf12ad

CHANGES.rst

@@ -2,10 +2,30 @@
  Changes
 =========
 
-5.0.3 (unreleased)
-==================
-
-- Nothing changed yet.
+5.1.0 (unreleased)
+==================
+
+- Remove all bare ``except:`` statements. Previously, when accessing
+  special attributes such as ``__provides__``, ``__providedBy__``,
+  ``__class__`` and ``__conform__``, this package wrapped such access
+  in a bare ``except:`` statement, meaning that many errors could pass
+  silently; typically this would result in a fallback path being taken
+  and sometimes (like with ``providedBy()``) the result would be
+  non-sensical. This is especially true when those attributes are
+  implemented with descriptors. Now, only ``AttributeError`` is
+  caught. This makes errors more obvious.
+
+  Obviously, this means that some exceptions will be propagated
+  differently than before. In particular, ``RuntimeError`` raised by
+  Acquisition in the case of circular containment will now be
+  propagated. Previously, when adapting such a broken object, a
+  ``TypeError`` would be the common result, but now it will be a more
+  informative ``RuntimeError``.
+
+  In addition, ZODB errors like ``POSKeyError`` could now be
+  propagated where previously they would ignored by this package.
+
+  See `issue 200 <https://github.com/zopefoundation/zope.interface/issues/200>`_.
 
 
 5.0.2 (2020-03-30)

src/zope/interface/_zope_interface_coptimizations.c

@@ -178,23 +178,46 @@ getObjectSpecification(PyObject *ignored, PyObject *ob)
   PyObject *cls, *result;
 
   result = PyObject_GetAttr(ob, str__provides__);
-  if (result != NULL && PyObject_TypeCheck(result, &SpecificationBaseType))
-    return result;
-
-
-  PyErr_Clear();
+  if (!result)
+  {
+      if (!PyErr_ExceptionMatches(PyExc_AttributeError))
+      {
+          /* Propagate non AttributeError exceptions. */
+          return NULL;
+      }
+      PyErr_Clear();
+  }
+  else
+  {
+      int is_instance = -1;
+      is_instance = PyObject_IsInstance(result, (PyObject*)&SpecificationBaseType);
+      if (is_instance < 0)
+      {
+          /* Propagate all errors */
+          return NULL;
+      }
+      if (is_instance)
+      {
+          return result;
+      }
+  }
 
   /* We do a getattr here so as not to be defeated by proxies */
   cls = PyObject_GetAttr(ob, str__class__);
   if (cls == NULL)
-    {
+  {
+      if (!PyErr_ExceptionMatches(PyExc_AttributeError))
+      {
+          /* Propagate non-AttributeErrors */
+          return NULL;
+      }
       PyErr_Clear();
       if (imported_declarations == 0 && import_declarations() < 0)
           return NULL;
 
       Py_INCREF(empty);
       return empty;
-    }
+  }
   result = implementedBy(NULL, cls);
   Py_DECREF(cls);
 
@@ -205,21 +228,36 @@ static PyObject *
 providedBy(PyObject *ignored, PyObject *ob)
 {
   PyObject *result, *cls, *cp;
-
+  int is_instance = -1;
   result = NULL;
 
-  if (PyObject_TypeCheck(ob, &PySuper_Type))
+  is_instance = PyObject_IsInstance(ob, (PyObject*)&PySuper_Type);
+  if (is_instance < 0)
+  {
+      if (!PyErr_ExceptionMatches(PyExc_AttributeError))
+      {
+          /* Propagate non-AttributeErrors */
+          return NULL;
+      }
+      PyErr_Clear();
+  }
+  if (is_instance)
   {
       return implementedBy(NULL, ob);
   }
 
   result = PyObject_GetAttr(ob, str__providedBy__);
 
   if (result == NULL)
-    {
+  {
+      if (!PyErr_ExceptionMatches(PyExc_AttributeError))
+      {
+          return NULL;
+      }
+
       PyErr_Clear();
       return getObjectSpecification(NULL, ob);
-    }
+  }
 
 
   /* We want to make sure we have a spec. We can't do a type check
@@ -737,63 +775,85 @@ static struct PyMethodDef ib_methods[] = {
 };
 
 /*
-        def __call__(self, obj, alternate=_marker):
-            conform = getattr(obj, '__conform__', None)
-            if conform is not None:
-                adapter = self._call_conform(conform)
-                if adapter is not None:
-                    return adapter
-
-            adapter = self.__adapt__(obj)
-
+    def __call__(self, obj, alternate=_marker):
+        try:
+            conform = obj.__conform__
+        except AttributeError: # pylint:disable=bare-except
+            conform = None
+
+        if conform is not None:
+            adapter = self._call_conform(conform)
             if adapter is not None:
                 return adapter
-            elif alternate is not _marker:
-                return alternate
-            else:
-                raise TypeError("Could not adapt", obj, self)
+
+        adapter = self.__adapt__(obj)
+
+        if adapter is not None:
+            return adapter
+        if alternate is not _marker:
+            return alternate
+        raise TypeError("Could not adapt", obj, self)
+
 */
 static PyObject *
 ib_call(PyObject *self, PyObject *args, PyObject *kwargs)
 {
-  PyObject *conform, *obj, *alternate=NULL, *adapter;
-
+  PyObject *conform, *obj, *alternate, *adapter;
   static char *kwlist[] = {"obj", "alternate", NULL};
+  conform = obj = alternate = adapter = NULL;
+
 
   if (!PyArg_ParseTupleAndKeywords(args, kwargs, "O|O", kwlist,
                                    &obj, &alternate))
     return NULL;
 
   conform = PyObject_GetAttr(obj, str__conform__);
-  if (conform != NULL)
-    {
+  if (conform == NULL)
+  {
+      if (!PyErr_ExceptionMatches(PyExc_AttributeError))
+      {
+          /* Propagate non-AttributeErrors */
+          return NULL;
+      }
+      PyErr_Clear();
+
+      Py_INCREF(Py_None);
+      conform = Py_None;
+  }
+
+  if (conform != Py_None)
+  {
       adapter = PyObject_CallMethodObjArgs(self, str_call_conform,
                                            conform, NULL);
       Py_DECREF(conform);
       if (adapter == NULL || adapter != Py_None)
-        return adapter;
+          return adapter;
       Py_DECREF(adapter);
-    }
+  }
   else
-    PyErr_Clear();
+  {
+      Py_DECREF(conform);
+  }
 
-  adapter = __adapt__(self, obj);
+  adapter = __adapt__(self, obj); // XXX: should be self.__adapt__.
   if (adapter == NULL || adapter != Py_None)
-    return adapter;
+  {
+      return adapter;
+  }
   Py_DECREF(adapter);
 
   if (alternate != NULL)
-    {
+  {
       Py_INCREF(alternate);
       return alternate;
-    }
+  }
 
   adapter = Py_BuildValue("sOO", "Could not adapt", obj, self);
   if (adapter != NULL)
-    {
+  {
       PyErr_SetObject(PyExc_TypeError, adapter);
       Py_DECREF(adapter);
-    }
+  }
   return NULL;
 }
 

src/zope/interface/declarations.py

@@ -984,6 +984,7 @@ def moduleProvides(*interfaces):
 
 # XXX:  is this a fossil?  Nobody calls it, no unit tests exercise it, no
 #       doctests import it, and the package __init__ doesn't import it.
+#       (Answer: Versions of zope.container prior to 4.4.0 called this.)
 def ObjectSpecification(direct, cls):
     """Provide object specifications
 
@@ -994,16 +995,17 @@ def ObjectSpecification(direct, cls):
 @_use_c_impl
 def getObjectSpecification(ob):
     try:
-        provides = getattr(ob, '__provides__', None)
-    except:
+        provides = ob.__provides__
+    except AttributeError:
         provides = None
+
     if provides is not None:
         if isinstance(provides, SpecificationBase):
             return provides
 
     try:
         cls = ob.__class__
-    except:
+    except AttributeError:
         # We can't get the class, so just consider provides
         return _empty
     return implementedBy(cls)
@@ -1028,7 +1030,7 @@ def providedBy(ob):
             return implementedBy(ob)
 
         r = ob.__providedBy__
-    except:
+    except AttributeError:
         # Not set yet. Fall back to lower-level thing that computes it
         return getObjectSpecification(ob)
 

src/zope/interface/interface.py

@@ -275,16 +275,10 @@ def __call__(self, obj, alternate=_marker):
         """Adapt an object to the interface
         """
         try:
-            conform = getattr(obj, '__conform__', None)
-        # XXX: Ideally, we don't want to catch BaseException. Things
-        # like MemoryError, KeyboardInterrupt, and other BaseExceptions should
-        # get through. Here, and everywhere we have bare ``except:`` clauses.
-        # The bare ``except:`` clauses exist for compatibility with the C code in
-        # ``_zope_interface_coptimizations.c`` (``ib_call()`` in this case). Both
-        # implementations would need to change. But beware of things like proxies and
-        # Acquisition wrappers. See https://github.com/zopefoundation/zope.interface/issues/163
-        except: # pylint:disable=bare-except
+            conform = obj.__conform__
+        except AttributeError:
             conform = None
+
         if conform is not None:
             adapter = self._call_conform(conform)
             if adapter is not None:

src/zope/interface/tests/__init__.py

@@ -32,6 +32,66 @@ def test_optimizations(self):
         else:
             self.assertIs(used, fallback)
 
+
+class MissingSomeAttrs(object):
+    """
+    Helper for tests that raises a specific exception
+    for attributes that are missing. This is usually not
+    an AttributeError, and this object is used to test that
+    those errors are not improperly caught and treated like
+    an AttributeError.
+    """
+
+    def __init__(self, exc_kind, **other_attrs):
+        self.__exc_kind = exc_kind
+        d = object.__getattribute__(self, '__dict__')
+        d.update(other_attrs)
+
+    def __getattribute__(self, name):
+        # Note that we ignore objects found in the class dictionary.
+        d = object.__getattribute__(self, '__dict__')
+        try:
+            return d[name]
+        except KeyError:
+            raise d['_MissingSomeAttrs__exc_kind'](name)
+
+    EXCEPTION_CLASSES = (
+        TypeError,
+        RuntimeError,
+        BaseException,
+        ValueError,
+    )
+
+    @classmethod
+    def test_raises(cls, unittest, test_func, expected_missing, **other_attrs):
+        """
+        Loop through various exceptions, calling *test_func* inside a ``assertRaises`` block.
+
+        :param test_func: A callable of one argument, the instance of this
+            class.
+        :param str expected_missing: The attribute that should fail with the exception.
+           This is used to ensure that we're testing the path we think we are.
+        :param other_attrs: Attributes that should be provided on the test object.
+           Must not contain *expected_missing*.
+        """
+        assert isinstance(expected_missing, str)
+        assert expected_missing not in other_attrs
+        for exc in cls.EXCEPTION_CLASSES:
+            ob = cls(exc, **other_attrs)
+            with unittest.assertRaises(exc) as ex:
+                test_func(ob)
+
+            unittest.assertEqual(ex.exception.args[0], expected_missing)
+
+        # Now test that the AttributeError for that expected_missing is *not* raised.
+        ob = cls(AttributeError, **other_attrs)
+        try:
+            test_func(ob)
+        except AttributeError as e:
+            unittest.assertNotIn(expected_missing, str(e))
+        except Exception: # pylint:disable=broad-except
+            pass
+
 # Be sure cleanup functionality is available; classes that use the adapter hook
 # need to be sure to subclass ``CleanUp``.
 #