Merge pull request #201 from zopefoundation/issue200

Remove the bare except: statements.

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)

benchmarks/micro.py

@@ -37,6 +37,21 @@ class DeepestInheritance(object):
     pass
 classImplements(DeepestInheritance, deep_ifaces[-1])
 
+
+class ImplementsNothing(object):
+    pass
+
+
+class HasConformReturnNone(object):
+    def __conform__(self, iface):
+        return None
+
+
+class HasConformReturnObject(object):
+    def __conform__(self, iface):
+        return self
+
+
 def make_implementer(iface):
     c = type('Implementer' + iface.__name__, (object,), {})
     classImplements(c, iface)
@@ -77,16 +92,17 @@ def bench_sort(loops, objs):
     return pyperf.perf_counter() - t0
 
 def bench_query_adapter(loops, components, objs=providers):
+    components_queryAdapter = components.queryAdapter
     # One time through to prime the caches
     for iface in ifaces:
         for provider in providers:
-            components.queryAdapter(provider, iface)
+            components_queryAdapter(provider, iface)
 
     t0 = pyperf.perf_counter()
     for _ in range(loops):
         for iface in ifaces:
             for provider in objs:
-                components.queryAdapter(provider, iface)
+                components_queryAdapter(provider, iface)
     return pyperf.perf_counter() - t0
 
 
@@ -106,8 +122,106 @@ def bench_getattr(loops, name, get=getattr):
             get(Interface, name) # 10
     return pyperf.perf_counter() - t0
 
+
+def bench_iface_call_no_conform_no_alternate_not_provided(loops):
+    inst = ImplementsNothing()
+    t0 = pyperf.perf_counter()
+    for _ in range(loops):
+        for _ in range(INNER):
+            for iface in ifaces:
+                try:
+                    iface(inst)
+                except TypeError:
+                    pass
+                else:
+                    raise TypeError("Should have failed")
+    return pyperf.perf_counter() - t0
+
+
+def bench_iface_call_no_conform_w_alternate_not_provided(loops):
+    inst = ImplementsNothing()
+    t0 = pyperf.perf_counter()
+    for _ in range(loops):
+        for _ in range(INNER):
+            for iface in ifaces:
+                iface(inst, 42)
+    return pyperf.perf_counter() - t0
+
+
+def bench_iface_call_w_conform_return_none_not_provided(loops):
+    inst = HasConformReturnNone()
+    t0 = pyperf.perf_counter()
+    for _ in range(loops):
+        for _ in range(INNER):
+            for iface in ifaces:
+                iface(inst, 42)
+    return pyperf.perf_counter() - t0
+
+
+def bench_iface_call_w_conform_return_non_none_not_provided(loops):
+    inst = HasConformReturnObject()
+    t0 = pyperf.perf_counter()
+    for _ in range(loops):
+        for _ in range(INNER):
+            for iface in ifaces:
+                iface(inst)
+    return pyperf.perf_counter() - t0
+
+def _bench_iface_call_simple(loops, inst):
+    t0 = pyperf.perf_counter()
+    for _ in range(loops):
+        for _ in range(INNER):
+            for iface in ifaces:
+                iface(inst)
+    return pyperf.perf_counter() - t0
+
+
+def bench_iface_call_no_conform_provided_wide(loops):
+    return _bench_iface_call_simple(loops, WideInheritance())
+
+
+def bench_iface_call_no_conform_provided_deep(loops):
+    return _bench_iface_call_simple(loops, DeepestInheritance())
+
+
 runner = pyperf.Runner()
 
+runner.bench_time_func(
+    'call interface (provides; deep)',
+    bench_iface_call_no_conform_provided_deep,
+    inner_loops=INNER * len(ifaces)
+)
+
+runner.bench_time_func(
+    'call interface (provides; wide)',
+    bench_iface_call_no_conform_provided_wide,
+    inner_loops=INNER * len(ifaces)
+)
+
+runner.bench_time_func(
+    'call interface (no alternate, no conform, not provided)',
+    bench_iface_call_no_conform_no_alternate_not_provided,
+    inner_loops=INNER * len(ifaces)
+)
+
+runner.bench_time_func(
+    'call interface (alternate, no conform, not provided)',
+    bench_iface_call_no_conform_w_alternate_not_provided,
+    inner_loops=INNER * len(ifaces)
+)
+
+runner.bench_time_func(
+    'call interface (no alternate, valid conform, not provided)',
+    bench_iface_call_w_conform_return_non_none_not_provided,
+    inner_loops=INNER * len(ifaces)
+)
+
+runner.bench_time_func(
+    'call interface (alternate, invalid conform, not provided)',
+    bench_iface_call_w_conform_return_none_not_provided,
+    inner_loops=INNER * len(ifaces)
+)
+
 runner.bench_time_func(
     'read __module__', # stored in C, accessed through __getattribute__
     bench_getattr,

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;
 }