Merge pull request #464 from bluetech/hookwrapper-teardown-warning

Warn when old-style hookwrapper raises during teardown

Author: bluetech

changelog/463.feature.rst

@@ -0,0 +1,2 @@
+A warning :class:`~pluggy.PluggyTeardownRaisedWarning` is now issued when an old-style hookwrapper raises an exception during teardown.
+See the warning documentation for more details.

docs/api_reference.rst

@@ -47,3 +47,15 @@ API Reference
 .. autoclass:: pluggy.HookimplOpts()
     :show-inheritance:
     :members:
+
+
+Warnings
+--------
+
+Custom warnings generated in some situations such as improper usage or deprecated features.
+
+.. autoclass:: pluggy.PluggyWarning()
+    :show-inheritance:
+
+.. autoclass:: pluggy.PluggyTeardownRaisedWarning()
+    :show-inheritance:

src/pluggy/__init__.py

@@ -18,6 +18,8 @@
     "HookspecMarker",
     "HookimplMarker",
     "Result",
+    "PluggyWarning",
+    "PluggyTeardownRaisedWarning",
 ]
 
 from ._manager import PluginManager, PluginValidationError
@@ -31,3 +33,7 @@
     HookimplOpts,
     HookImpl,
 )
+from ._warnings import (
+    PluggyWarning,
+    PluggyTeardownRaisedWarning,
+)

src/pluggy/_callers.py

@@ -3,27 +3,52 @@
 """
 from __future__ import annotations
 
+import warnings
 from typing import cast
 from typing import Generator
 from typing import Mapping
+from typing import NoReturn
 from typing import Sequence
 from typing import Tuple
 from typing import Union
 
 from ._hooks import HookImpl
-from ._result import _raise_wrapfail
 from ._result import HookCallError
 from ._result import Result
+from ._warnings import PluggyTeardownRaisedWarning
 
 
 # Need to distinguish between old- and new-style hook wrappers.
-# Wrapping one a singleton tuple is the fastest type-safe way I found to do it.
+# Wrapping with a tuple is the fastest type-safe way I found to do it.
 Teardown = Union[
-    Tuple[Generator[None, Result[object], None]],
+    Tuple[Generator[None, Result[object], None], HookImpl],
     Generator[None, object, object],
 ]
 
 
+def _raise_wrapfail(
+    wrap_controller: (
+        Generator[None, Result[object], None] | Generator[None, object, object]
+    ),
+    msg: str,
+) -> NoReturn:
+    co = wrap_controller.gi_code
+    raise RuntimeError(
+        "wrap_controller at %r %s:%d %s"
+        % (co.co_name, co.co_filename, co.co_firstlineno, msg)
+    )
+
+
+def _warn_teardown_exception(
+    hook_name: str, hook_impl: HookImpl, e: BaseException
+) -> None:
+    msg = "A plugin raised an exception during an old-style hookwrapper teardown.\n"
+    msg += f"Plugin: {hook_impl.plugin_name}, Hook: {hook_name}\n"
+    msg += f"{type(e).__name__}: {e}\n"
+    msg += "For more information see https://pluggy.readthedocs.io/en/stable/api_reference.html#pluggy.PluggyTeardownRaisedWarning"  # noqa: E501
+    warnings.warn(PluggyTeardownRaisedWarning(msg), stacklevel=5)
+
+
 def _multicall(
     hook_name: str,
     hook_impls: Sequence[HookImpl],
@@ -60,7 +85,7 @@ def _multicall(
                         res = hook_impl.function(*args)
                         wrapper_gen = cast(Generator[None, Result[object], None], res)
                         next(wrapper_gen)  # first yield
-                        teardowns.append((wrapper_gen,))
+                        teardowns.append((wrapper_gen, hook_impl))
                     except StopIteration:
                         _raise_wrapfail(wrapper_gen, "did not yield")
                 elif hook_impl.wrapper:
@@ -128,9 +153,13 @@ def _multicall(
                 if isinstance(teardown, tuple):
                     try:
                         teardown[0].send(outcome)
-                        _raise_wrapfail(teardown[0], "has second yield")
                     except StopIteration:
                         pass
+                    except BaseException as e:
+                        _warn_teardown_exception(hook_name, teardown[1], e)
+                        raise
+                    else:
+                        _raise_wrapfail(teardown[0], "has second yield")
                 else:
                     try:
                         if outcome._exception is not None:

src/pluggy/_result.py

@@ -7,9 +7,7 @@
 from typing import Callable
 from typing import cast
 from typing import final
-from typing import Generator
 from typing import Generic
-from typing import NoReturn
 from typing import Optional
 from typing import Tuple
 from typing import Type
@@ -20,19 +18,6 @@
 ResultType = TypeVar("ResultType")
 
 
-def _raise_wrapfail(
-    wrap_controller: (
-        Generator[None, Result[ResultType], None] | Generator[None, object, object]
-    ),
-    msg: str,
-) -> NoReturn:
-    co = wrap_controller.gi_code
-    raise RuntimeError(
-        "wrap_controller at %r %s:%d %s"
-        % (co.co_name, co.co_filename, co.co_firstlineno, msg)
-    )
-
-
 class HookCallError(Exception):
     """Hook was called incorrectly."""
 

src/pluggy/_warnings.py

@@ -0,0 +1,27 @@
+from typing import final
+
+
+class PluggyWarning(UserWarning):
+    """Base class for all warnings emitted by pluggy."""
+
+    __module__ = "pluggy"
+
+
+@final
+class PluggyTeardownRaisedWarning(PluggyWarning):
+    """A plugin raised an exception during an :ref:`old-style hookwrapper
+    <old_style_hookwrappers>` teardown.
+
+    Such exceptions are not handled by pluggy, and may cause subsequent
+    teardowns to be executed at unexpected times, or be skipped entirely.
+
+    This is an issue in the plugin implementation.
+
+    If the exception is unintended, fix the underlying cause.
+
+    If the exception is intended, switch to :ref:`new-style hook wrappers
+    <hookwrappers>`, or use :func:`result.force_exception()
+    <pluggy.Result.force_exception>` to set the exception instead of raising.
+    """
+
+    __module__ = "pluggy"

testing/test_warnings.py

@@ -0,0 +1,49 @@
+from pathlib import Path
+
+import pytest
+
+from pluggy import HookimplMarker
+from pluggy import HookspecMarker
+from pluggy import PluggyTeardownRaisedWarning
+from pluggy import PluginManager
+
+
+hookspec = HookspecMarker("example")
+hookimpl = HookimplMarker("example")
+
+
+def test_teardown_raised_warning(pm: PluginManager) -> None:
+    class Api:
+        @hookspec
+        def my_hook(self):
+            raise NotImplementedError()
+
+    pm.add_hookspecs(Api)
+
+    class Plugin1:
+        @hookimpl
+        def my_hook(self):
+            pass
+
+    class Plugin2:
+        @hookimpl(hookwrapper=True)
+        def my_hook(self):
+            yield
+            1 / 0
+
+    class Plugin3:
+        @hookimpl(hookwrapper=True)
+        def my_hook(self):
+            yield
+
+    pm.register(Plugin1(), "plugin1")
+    pm.register(Plugin2(), "plugin2")
+    pm.register(Plugin3(), "plugin3")
+    with pytest.warns(
+        PluggyTeardownRaisedWarning,
+        match=r"\bplugin2\b.*\bmy_hook\b.*\n.*ZeroDivisionError",
+    ) as wc:
+        with pytest.raises(ZeroDivisionError):
+            pm.hook.my_hook()
+    assert len(wc.list) == 1
+    assert Path(wc.list[0].filename).name == "test_warnings.py"