Repository: pytest-dev/pluggy
Pull request #282 (2020-06-30 07:00:36)

Validate that a hookwrapper's function is a generator function during registration
Previously this error was only reported when calling the hook. Now it is reported during registration.

Suggested by @maxnikulin in https://github.com/pytest-dev/pluggy/pull/257#discussion_r447416628.

maxnikulin: Thank you.

There is `test_hookwrapper_not_yield` in ` testing/test_multicall.py` that makes me surprised that some test configurations have passed.
bluetech: This is because it is not registered.

We can perhaps move this check to the `@hookimpl` decorator itself but up to now all validations are performed during registration so I stuck to that.
maxnikulin: I do not think that moving check to `hookimpl` will improve diagnostic. Vice versa it would be nice to get (in the compiler style) from a single run as mush errors as possible, namely if several hooks have problems, report all detected errors. Consider it as another idea, Of course, I do not expect so great changes in this pull request.
bluetech: Good point @RonnyPfannschmidt, this will break it.

I had a look at pytest+corpus of plugins, tox and devpi and didn't find a case that would break by this, except one legitimately broken [README example](https://github.com/pytest-dev/pytest-rerunfailures/#development) (which I'll send a PR to fix).

So overall I think the advantage outweigh the possible breakage from this, if any.

I enhanced to changelog entry to explain how to migrate (instead of returning a generator, `yield from` it).
RonnyPfannschmidt: Im totally happy with the change, i just wanted to make sure we communicate it right and bump the version right 

Patch:
diff --git a/changelog/282.feature.rst b/changelog/282.feature.rst
new file mode 100644
index 0000000..d5f2b9b
--- /dev/null
+++ b/changelog/282.feature.rst
@@ -0,0 +1,28 @@
+When registering a hookimpl which is declared as ``hookwrapper=True`` but whose
+function is not a generator function, a ``PluggyValidationError`` exception is
+now raised.
+
+Previously this problem would cause an error only later, when calling the hook.
+
+In the unlikely case that you have a hookwrapper that *returns* a generator
+instead of yielding directly, for example:
+
+.. code-block:: python
+
+    def my_hook_real_implementation(arg):
+        print("before")
+        yield
+        print("after")
+
+
+    @hookimpl(hookwrapper=True)
+    def my_hook(arg):
+        return my_hook_implementation(arg)
+
+change it to use ``yield from`` instead:
+
+.. code-block:: python
+
+    @hookimpl(hookwrapper=True)
+    def my_hook(arg):
+        yield from my_hook_implementation(arg)
diff --git a/src/pluggy/manager.py b/src/pluggy/manager.py
index e7acd3d..62b999f 100644
--- a/src/pluggy/manager.py
+++ b/src/pluggy/manager.py
@@ -221,8 +221,10 @@ class PluginManager:
                 "Plugin %r\nhook %r\nhistoric incompatible to hookwrapper"
                 % (hookimpl.plugin_name, hook.name),
             )
+
         if hook.spec.warn_on_impl:
             _warn_for_function(hook.spec.warn_on_impl, hookimpl.function)
+
         # positional arg checking
         notinspec = set(hookimpl.argnames) - set(hook.spec.argnames)
         if notinspec:
@@ -239,6 +241,14 @@ class PluginManager:
                 ),
             )
 
+        if 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"
+                % (hookimpl.plugin_name, hook.name, _formatdef(hookimpl.function)),
+            )
+
     def check_pending(self):
         """ Verify that all hooks which have not been verified against
         a hook specification are optional, otherwise raise :py:class:`.PluginValidationError`."""


Test patch:
diff --git a/testing/test_details.py b/testing/test_details.py
index dc2cec7..08f82e1 100644
--- a/testing/test_details.py
+++ b/testing/test_details.py
@@ -21,7 +21,7 @@ def test_parse_hookimpl_override():
 
         @hookimpl(hookwrapper=True, tryfirst=True)
         def x1meth2(self):
-            pass
+            yield  # pragma: no cover
 
     class Spec:
         @hookspec
diff --git a/testing/test_pluginmanager.py b/testing/test_pluginmanager.py
index 2ee7512..6eb5045 100644
--- a/testing/test_pluginmanager.py
+++ b/testing/test_pluginmanager.py
@@ -135,6 +135,19 @@ def test_register_mismatch_arg(he_pm):
     assert excinfo.value.plugin is plugin
 
 
+def test_register_hookwrapper_not_a_generator_function(he_pm):
+    class hello:
+        @hookimpl(hookwrapper=True)
+        def he_method1(self):
+            pass  # pragma: no cover
+
+    plugin = hello()
+
+    with pytest.raises(PluginValidationError, match="generator function") as excinfo:
+        he_pm.register(plugin)
+    assert excinfo.value.plugin is plugin
+
+
 def test_register(pm):
     class MyPlugin:
         pass