Change precedence of hook call value, hookspec default, hookimpl default

changelog/442.bugfix.rst

@@ -0,0 +1,4 @@
+With the addition of default values for hookspec arguments, we also changed the precedence of default values for *hookimpl* arguments.
+Previously, if a hookimpl function declared a default for an argument, it was always used, even if the *hook call* passed an explicit value.
+Now the precedence is changed to: hook call passed value > hookspec default value > hookimpl default value.
+See :ref:`hookimpl_arg_defaults`.

docs/index.rst

@@ -502,6 +502,29 @@ method.
     :py:meth:`~pluggy.Result.force_exception` to adjust the
     exception.
 
+.. _hookimpl_arg_defaults:
+
+Hookimpl argument defaults
+^^^^^^^^^^^^^^^^^^^^^^^^^^
+A hookimpl can declare default values for its arguments, as in ``version=1`` in
+this example plugin::
+
+    class CompatiblePlugin:
+        @hookimpl
+        def setup_project(self, config, args, version=1):
+            ...
+
+This can be helpful if the host application evolved the hookspec to add a new
+argument, but the plugin still wants to support an old version of the host
+application which didn't pass this argument.
+
+If the hook caller passes a value for the parameter, it is used. Otherwise the
+default declared by the hookimpl is used.
+
+.. versionchanged:: 1.7
+    Previous versions had a different, unhelpful, behavior of *always* using the
+    hookimpl-declared default.
+
 .. _specs:
 
 Specifications
@@ -640,6 +663,8 @@ callers:
     # New caller; hookimpls will get new_arg="get this".
     pm.hook.myhook(config=config, args=args, new_arg="get this")
 
+.. versionadded:: 1.7
+    Hookspec argument defaults.
 
 .. _firstresult:
 

src/pluggy/_execution.py

@@ -25,15 +25,22 @@
 
 
 def run_old_style_hookwrapper(
-    hook_impl: HookImpl, hook_name: str, args: Sequence[object]
+    hook_impl: HookImpl,
+    hook_name: str,
+    args: Sequence[object],
+    kwargs: Mapping[str, object] | None,
 ) -> Teardown:
     """
     backward compatibility wrapper to run a old style hookwrapper as a wrapper
     """
+    if kwargs is None:
+        res = hook_impl.function(*args)
+    else:
+        res = hook_impl.function(*args, **kwargs)
     if TYPE_CHECKING:
-        teardown = cast(Teardown, hook_impl.function(*args))
+        teardown = cast(Teardown, res)
     else:
-        teardown = hook_impl.function(*args)
+        teardown = res
     try:
         next(teardown)
     except StopIteration:
@@ -98,19 +105,32 @@ def _multicall(
         for hook_impl in reversed(hook_impls):
             try:
                 args = [caller_kwargs[argname] for argname in hook_impl.argnames]
+                if hook_impl.kwargnames:
+                    kwargs = {
+                        argname: caller_kwargs[argname]
+                        for argname in hook_impl.kwargnames
+                        if argname in caller_kwargs
+                    } or None
+                else:
+                    kwargs = None
             except KeyError as e:
                 raise HookCallError(
                     f"hook call must provide argument {e.args[0]!r}"
                 ) from e
 
             if hook_impl.hookwrapper:
-                function_gen = run_old_style_hookwrapper(hook_impl, hook_name, args)
+                function_gen = run_old_style_hookwrapper(
+                    hook_impl, hook_name, args, kwargs
+                )
 
                 next(function_gen)  # first yield
                 teardowns.append(function_gen)
 
             elif hook_impl.wrapper:
-                res = hook_impl.function(*args)
+                if kwargs is None:
+                    res = hook_impl.function(*args)
+                else:
+                    res = hook_impl.function(*args, **kwargs)
                 # If this cast is not valid, a type error is raised below,
                 # which is the desired response.
                 if TYPE_CHECKING:
@@ -123,7 +143,10 @@ def _multicall(
                     _raise_wrapfail(function_gen, "did not yield")
                 teardowns.append(function_gen)
             else:
-                res = hook_impl.function(*args)
+                if kwargs is None:
+                    res = hook_impl.function(*args)
+                else:
+                    res = hook_impl.function(*args, **kwargs)
                 if res is not None:
                     results.append(res)
                     if firstresult:  # halt further impl calls

src/pluggy/_impl.py

@@ -51,9 +51,10 @@ def __init__(
         #: The hook implementation function.
         self.function: Final = function
         argnames, kwargnames = varnames(self.function)
-        #: The positional parameter names of ``function```.
+        #: The positional parameter names of ``function``.
         self.argnames: Final = argnames
-        #: The keyword parameter names of ``function```.
+        #: The keyword parameter names of ``function`` which declare defaults.
+        #: Keyword-only parameters are *not* included.
         self.kwargnames: Final = kwargnames
         #: The plugin which defined this hook implementation.
         self.plugin: Final = plugin

testing/benchmark.py

@@ -91,6 +91,11 @@ def wrapper(arg1, arg2, arg3):
     return (yield)
 
 
+@hookimpl
+def hook_with_default_argument(arg1, arg2=2):
+    return arg1, arg2
+
+
 @pytest.fixture(params=[10, 100], ids="hooks={}".format)
 def hooks(request: Any) -> list[object]:
     return [hook for i in range(request.param)]
@@ -112,7 +117,32 @@ def setup():
         firstresult = False
         return (hook_name, hook_impls, caller_kwargs, firstresult), {}
 
-    benchmark.pedantic(_multicall, setup=setup, rounds=10)
+    benchmark.pedantic(_multicall, setup=setup, rounds=100)
+
+
+@pytest.mark.parametrize("impl_count", [10, 100])
+@pytest.mark.parametrize(
+    "pass_arg", [False, True], ids=["impl-default", "caller-value"]
+)
+def test_hookimpl_with_default_speed(
+    benchmark, impl_count: int, pass_arg: bool
+) -> None:
+    def setup():
+        hook_impls = [
+            HookImpl(
+                None,
+                "<temp>",
+                hook_with_default_argument,
+                hook_with_default_argument.example_impl,  # type: ignore[attr-defined]
+            )
+            for _ in range(impl_count)
+        ]
+        caller_kwargs = {"arg1": 1}
+        if pass_arg:
+            caller_kwargs["arg2"] = 2
+        return ("foo", hook_impls, caller_kwargs, False), {}
+
+    benchmark.pedantic(_multicall, setup=setup, rounds=100)
 
 
 @pytest.mark.parametrize(

testing/test_invocations.py

@@ -446,15 +446,72 @@ def hello(self, arg, new_arg="impl-default"):
             pm.register(PluginWithoutImplDefault())
         pm.register(PluginWithImplDefault())
 
+        # Old spec, new impl, old call.
+        # The normal case for impl default.
         assert pm.hook.hello(arg=1) == [(1, "impl-default")]
+        # Old spec, new impl, new call.
+        # Less common, but the call arg is still passed.
+        assert pm.hook.hello(arg=1, new_arg="call") == [(1, "call")]
 
-    def test_does_not_override_hookimpl_default(self, pm: PluginManager) -> None:
-        """If an impl provides its own default, it takes precedence over both
-        the spec default and call value.
+    def test_call_value_overrides_hookimpl_default(self, pm: PluginManager) -> None:
+        """A call value takes precedence over an hookimpl default."""
 
-        NOTE: This is verifying existing behavior, but it's not necessarily what
-        we want (#442).
-        """
+        class Api:
+            @hookspec
+            def hello(self, arg, new_arg):
+                pass
+
+        class Plugin:
+            @hookimpl
+            def hello(self, arg, new_arg="impl-default"):
+                return new_arg
+
+        pm.add_hookspecs(Api)
+        pm.register(Plugin())
+
+        assert pm.hook.hello(arg=1, new_arg="call") == ["call"]
+
+    def test_call_value_overrides_hookimpl_default_in_wrappers(
+        self, pm: PluginManager
+    ) -> None:
+        class Api:
+            @hookspec
+            def hello(self, arg, new_arg):
+                pass
+
+        seen = []
+
+        class Plugin:
+            @hookimpl
+            def hello(self, arg, new_arg="impl-default"):
+                seen.append(new_arg)
+                return new_arg
+
+        class WrapperPlugin:
+            @hookimpl(wrapper=True)
+            def hello(self, arg, new_arg="impl-default"):
+                seen.append(new_arg)
+                result = yield
+                return result
+
+        class HookwrapperPlugin:
+            @hookimpl(hookwrapper=True)
+            def hello(self, arg, new_arg="impl-default"):
+                seen.append(new_arg)
+                yield
+
+        pm.add_hookspecs(Api)
+        pm.register(Plugin())
+        pm.register(WrapperPlugin())
+        pm.register(HookwrapperPlugin())
+
+        assert pm.hook.hello(arg=1, new_arg="call") == ["call"]
+        assert seen == ["call", "call", "call"]
+
+    def test_hookspec_default_and_call_override_hookimpl_default(
+        self, pm: PluginManager
+    ) -> None:
+        """Hookspec defaults take precedence over hookimpl defaults."""
 
         class Api:
             @hookspec
@@ -469,8 +526,8 @@ def hello(self, arg, new_arg="impl-default"):
         pm.add_hookspecs(Api)
         pm.register(Plugin())
 
-        assert pm.hook.hello(arg=1) == ["impl-default"]
-        assert pm.hook.hello(arg=1, new_arg="call") == ["impl-default"]
+        assert pm.hook.hello(arg=1) == ["spec-default"]
+        assert pm.hook.hello(arg=1, new_arg="call") == ["call"]
 
     def test_new_call_argument_is_not_delivered_by_old_spec(
         self, pm: PluginManager