Make new-style wrappers use explicit wrapper=True

Previously new-style wrappers were implied by the function being a
generator, however that broke a use case of a non-wrapper impl being a
generator as a way to return an Iterator result. This is legitimate, and
used at least by conda (see #403).

Therefore, change to require an explicit `wrapper=True`. Also adds a
test for the iterator-returning case.

Fix #405.

Author: bluetech

.coveragerc

@@ -23,5 +23,7 @@ exclude_lines =
 
     if __name__ == .__main__.:
 
+    raise NotImplementedError
+
     # Ignore coverage on lines solely with `...`
     ^\s*\.\.\.\s*$

CHANGELOG.rst

@@ -26,7 +26,9 @@ pluggy 1.1.0 (YANKED)
 
 .. note::
 
-  This release was yanked because unfortunately the new-style hook wrappers broke some downstream projects. See `#403 <https://github.com/pytest-dev/pluggy/issues/403>`__ for more information.
+  This release was yanked because unfortunately the implicit new-style hook wrappers broke some downstream projects.
+  See `#403 <https://github.com/pytest-dev/pluggy/issues/403>`__ for more information.
+  This was rectified in the 1.2.0 release.
 
 Deprecations and Removals
 -------------------------

changelog/405.feature.rst

@@ -0,0 +1 @@
+The new-style hook wrappers, added in the yanked 1.1.0 release, now require an explicit ``wrapper=True`` designation in the ``@hookimpl()`` decorator.

docs/index.rst

@@ -366,19 +366,25 @@ Wrappers
 .. note::
     This section describes "new-style hook wrappers", which were added in Pluggy
     1.1. For earlier versions, see the "old-style hook wrappers" section below.
-    The two styles are fully interoperable.
 
-A *hookimpl* can be a generator function, which indicates that the function will
-be called to *wrap* (or surround) all other normal *hookimpl* calls. A *hook
-wrapper* can thus execute some code ahead and after the execution of all
-corresponding non-wrappper *hookimpls*.
+    New-style hooks wrappers are declared with ``wrapper=True``, while
+    old-style hook wrappers are declared with ``hookwrapper=True``.
+
+    The two styles are fully interoperable between plugins using different
+    styles. However within the same plugin we recommend using only one style,
+    for consistency.
+
+A *hookimpl* can be marked with the ``"wrapper"`` option, which indicates
+that the function will be called to *wrap* (or surround) all other normal
+*hookimpl* calls. A *hook wrapper* can thus execute some code ahead and after the
+execution of all corresponding non-wrappper *hookimpls*.
 
 Much in the same way as a :py:func:`@contextlib.contextmanager <python:contextlib.contextmanager>`,
 *hook wrappers* must be implemented as generator function with a single ``yield`` in its body:
 
 .. code-block:: python
 
-    @hookimpl
+    @hookimpl(wrapper=True)
     def setup_project(config, args):
         """Wrap calls to ``setup_project()`` implementations which
         should return json encoded config options.
@@ -837,7 +843,7 @@ re-raised at the hook invocation point:
             return 3
 
 
-    @hookimpl
+    @hookimpl(wrapper=True)
     def myhook(self, args):
         try:
             return (yield)

src/pluggy/_callers.py

@@ -66,7 +66,7 @@ def _multicall(
                         teardowns.append((wrapper_gen,))
                     except StopIteration:
                         _raise_wrapfail(wrapper_gen, "did not yield")
-                elif hook_impl.isgeneratorfunction:
+                elif hook_impl.wrapper:
                     try:
                         # If this cast is not valid, a type error is raised below,
                         # which is the desired response.

src/pluggy/_hooks.py

@@ -45,6 +45,7 @@ class _HookSpecOpts(TypedDict):
         warn_on_impl: Warning | None
 
     class _HookImplOpts(TypedDict):
+        wrapper: bool
         hookwrapper: bool
         optionalhook: bool
         tryfirst: bool
@@ -145,6 +146,7 @@ def __call__(
         tryfirst: bool = ...,
         trylast: bool = ...,
         specname: str | None = ...,
+        wrapper: bool = ...,
     ) -> _F:
         ...
 
@@ -157,6 +159,7 @@ def __call__(  # noqa: F811
         tryfirst: bool = ...,
         trylast: bool = ...,
         specname: str | None = ...,
+        wrapper: bool = ...,
     ) -> Callable[[_F], _F]:
         ...
 
@@ -168,6 +171,7 @@ def __call__(  # noqa: F811
         tryfirst: bool = False,
         trylast: bool = False,
         specname: str | None = None,
+        wrapper: bool = False,
     ) -> _F | Callable[[_F], _F]:
         """If passed a function, directly sets attributes on the function
         which will make it discoverable to :meth:`PluginManager.register`.
@@ -185,8 +189,7 @@ def __call__(  # noqa: F811
         If ``trylast`` is ``True``, this hook implementation will run as late as
         possible in the chain of N hook implementations.
 
-        If the hook implementation is a generator function, and ``hookwrapper``
-        is ``False`` or not set ("new-style hook wrapper"), the hook
+        If ``wrapper`` is ``True``("new-style hook wrapper"), the hook
         implementation needs to execute exactly one ``yield``. The code before
         the ``yield`` is run early before any non-hook-wrapper function is run.
         The code after the ``yield`` is run after all non-hook-wrapper functions
@@ -201,18 +204,19 @@ def __call__(  # noqa: F811
         The code after the ``yield`` is run after all non-hook-wrapper function
         have run  The ``yield`` receives a :class:`_Result` object representing
         the exception or result outcome of the inner calls (including earlier
-        hook wrapper calls).
+        hook wrapper calls). This option is mutually exclusive with ``wrapper``.
 
         If ``specname`` is provided, it will be used instead of the function
         name when matching this hook implementation to a hook specification
         during registration.
 
-        .. versionadded:: 1.1
-            New-style hook wrappers.
+        .. versionadded:: 1.2.0
+            The ``wrapper`` parameter.
         """
 
         def setattr_hookimpl_opts(func: _F) -> _F:
             opts: _HookImplOpts = {
+                "wrapper": wrapper,
                 "hookwrapper": hookwrapper,
                 "optionalhook": optionalhook,
                 "tryfirst": tryfirst,
@@ -231,6 +235,7 @@ def setattr_hookimpl_opts(func: _F) -> _F:
 def normalize_hookimpl_opts(opts: _HookImplOpts) -> None:
     opts.setdefault("tryfirst", False)
     opts.setdefault("trylast", False)
+    opts.setdefault("wrapper", False)
     opts.setdefault("hookwrapper", False)
     opts.setdefault("optionalhook", False)
     opts.setdefault("specname", None)
@@ -373,12 +378,12 @@ def get_hookimpls(self) -> list[HookImpl]:
     def _add_hookimpl(self, hookimpl: HookImpl) -> None:
         """Add an implementation to the callback chain."""
         for i, method in enumerate(self._hookimpls):
-            if method.hookwrapper or method.isgeneratorfunction:
+            if method.hookwrapper or method.wrapper:
                 splitpoint = i
                 break
         else:
             splitpoint = len(self._hookimpls)
-        if hookimpl.hookwrapper or hookimpl.isgeneratorfunction:
+        if hookimpl.hookwrapper or hookimpl.wrapper:
             start, end = splitpoint, len(self._hookimpls)
         else:
             start, end = 0, splitpoint
@@ -457,6 +462,7 @@ def call_extra(
         ), "Cannot directly call a historic hook - use call_historic instead."
         self._verify_all_args_are_provided(kwargs)
         opts: _HookImplOpts = {
+            "wrapper": False,
             "hookwrapper": False,
             "optionalhook": False,
             "trylast": False,
@@ -471,7 +477,7 @@ def call_extra(
             while (
                 i >= 0
                 and hookimpls[i].tryfirst
-                and not (hookimpls[i].hookwrapper or hookimpls[i].isgeneratorfunction)
+                and not (hookimpls[i].hookwrapper or hookimpls[i].wrapper)
             ):
                 i -= 1
             hookimpls.insert(i + 1, hookimpl)
@@ -545,7 +551,7 @@ class HookImpl:
         "plugin",
         "opts",
         "plugin_name",
-        "isgeneratorfunction",
+        "wrapper",
         "hookwrapper",
         "optionalhook",
         "tryfirst",
@@ -564,7 +570,7 @@ def __init__(
         self.plugin = plugin
         self.opts = hook_impl_opts
         self.plugin_name = plugin_name
-        self.isgeneratorfunction = inspect.isgeneratorfunction(self.function)
+        self.wrapper = hook_impl_opts["wrapper"]
         self.hookwrapper = hook_impl_opts["hookwrapper"]
         self.optionalhook = hook_impl_opts["optionalhook"]
         self.tryfirst = hook_impl_opts["tryfirst"]

src/pluggy/_manager.py

@@ -290,12 +290,10 @@ def get_name(self, plugin: _Plugin) -> str | None:
         return None
 
     def _verify_hook(self, hook: _HookCaller, hookimpl: HookImpl) -> None:
-        if hook.is_historic() and (
-            hookimpl.hookwrapper or hookimpl.isgeneratorfunction
-        ):
+        if hook.is_historic() and (hookimpl.hookwrapper or hookimpl.wrapper):
             raise PluginValidationError(
                 hookimpl.plugin,
-                "Plugin %r\nhook %r\nhistoric incompatible with yield/hookwrapper"
+                "Plugin %r\nhook %r\nhistoric incompatible with yield/wrapper/hookwrapper"
                 % (hookimpl.plugin_name, hook.name),
             )
 
@@ -319,11 +317,22 @@ def _verify_hook(self, hook: _HookCaller, hookimpl: HookImpl) -> None:
                 ),
             )
 
-        if hookimpl.hookwrapper and not inspect.isgeneratorfunction(hookimpl.function):
+        if (
+            hookimpl.wrapper or hookimpl.hookwrapper
+        ) and not inspect.isgeneratorfunction(hookimpl.function):
             raise PluginValidationError(
                 hookimpl.plugin,
                 "Plugin %r for hook %r\nhookimpl definition: %s\n"
-                "Declared as hookwrapper=True but function is not a generator function"
+                "Declared as wrapper=True or hookwrapper=True "
+                "but function is not a generator function"
+                % (hookimpl.plugin_name, hook.name, _formatdef(hookimpl.function)),
+            )
+
+        if hookimpl.wrapper and hookimpl.hookwrapper:
+            raise PluginValidationError(
+                hookimpl.plugin,
+                "Plugin %r for hook %r\nhookimpl definition: %s\n"
+                "The wrapper=True and hookwrapper=True options are mutually exclusive"
                 % (hookimpl.plugin_name, hook.name, _formatdef(hookimpl.function)),
             )
 

testing/benchmark.py

@@ -19,7 +19,7 @@ def hook(arg1, arg2, arg3):
     return arg1, arg2, arg3
 
 
-@hookimpl
+@hookimpl(wrapper=True)
 def wrapper(arg1, arg2, arg3):
     return (yield)
 
@@ -91,7 +91,7 @@ def __init__(self, num: int) -> None:
         def __repr__(self) -> str:
             return f"<PluginWrap {self.num}>"
 
-        @hookimpl
+        @hookimpl(wrapper=True)
         def fun(self):
             return (yield)
 

testing/test_details.py

@@ -25,7 +25,7 @@ def x1meth(self):
         def x1meth2(self):
             yield  # pragma: no cover
 
-        @hookimpl(trylast=True)
+        @hookimpl(wrapper=True, trylast=True)
         def x1meth3(self):
             return (yield)  # pragma: no cover
 
@@ -49,21 +49,21 @@ def x1meth3(self):
     hookimpls = pm.hook.x1meth.get_hookimpls()
     assert len(hookimpls) == 1
     assert not hookimpls[0].hookwrapper
-    assert not hookimpls[0].isgeneratorfunction
+    assert not hookimpls[0].wrapper
     assert not hookimpls[0].tryfirst
     assert not hookimpls[0].trylast
     assert not hookimpls[0].optionalhook
 
     hookimpls = pm.hook.x1meth2.get_hookimpls()
     assert len(hookimpls) == 1
     assert hookimpls[0].hookwrapper
-    assert hookimpls[0].isgeneratorfunction
+    assert not hookimpls[0].wrapper
     assert hookimpls[0].tryfirst
 
     hookimpls = pm.hook.x1meth3.get_hookimpls()
     assert len(hookimpls) == 1
     assert not hookimpls[0].hookwrapper
-    assert hookimpls[0].isgeneratorfunction
+    assert hookimpls[0].wrapper
     assert not hookimpls[0].tryfirst
     assert hookimpls[0].trylast
 

testing/test_hookcaller.py

@@ -35,10 +35,19 @@ def __init__(self, hc: _HookCaller) -> None:
         self.hc = hc
 
     def __call__(
-        self, tryfirst: bool = False, trylast: bool = False, hookwrapper: bool = False
+        self,
+        tryfirst: bool = False,
+        trylast: bool = False,
+        hookwrapper: bool = False,
+        wrapper: bool = False,
     ) -> Callable[[FuncT], FuncT]:
         def wrap(func: FuncT) -> FuncT:
-            hookimpl(tryfirst=tryfirst, trylast=trylast, hookwrapper=hookwrapper)(func)
+            hookimpl(
+                tryfirst=tryfirst,
+                trylast=trylast,
+                hookwrapper=hookwrapper,
+                wrapper=wrapper,
+            )(func)
             self.hc._add_hookimpl(
                 HookImpl(None, "<temp>", func, func.example_impl),  # type: ignore[attr-defined]
             )
@@ -150,7 +159,7 @@ def test_adding_wrappers_ordering(hc: _HookCaller, addmeth: AddMeth) -> None:
     def he_method1():
         yield
 
-    @addmeth()
+    @addmeth(wrapper=True)
     def he_method1_fun():
         yield
 
@@ -184,7 +193,7 @@ def he_method1():
     def he_method2():
         yield
 
-    @addmeth(tryfirst=True)
+    @addmeth(wrapper=True, tryfirst=True)
     def he_method3():
         yield
 
@@ -218,7 +227,7 @@ def m4() -> None:
 
     assert funcs(hc.get_hookimpls()) == [m3, m2, m1, m4]
 
-    @addmeth(tryfirst=True)
+    @addmeth(wrapper=True, tryfirst=True)
     def m5():
         yield
 
@@ -236,7 +245,7 @@ def m7() -> None:
 
     assert funcs(hc.get_hookimpls()) == [m3, m2, m7, m6, m1, m4, m5]
 
-    @addmeth()
+    @addmeth(wrapper=True)
     def m8():
         yield
 
@@ -260,7 +269,7 @@ def m11() -> None:
 
     assert funcs(hc.get_hookimpls()) == [m9, m3, m2, m7, m6, m10, m11, m1, m4, m8, m5]
 
-    @addmeth()
+    @addmeth(wrapper=True)
     def m12():
         yield