Allow hookspecs to declare default values for arguments

changelog/170.feature.rst

@@ -0,0 +1,3 @@
+Hookspecs can now delcare default values for arguments, using the usual ``def myhook(myarg="default")`` syntax.
+This is mainly a backward compatibility feature, to allow a hookspec to add new parameters to an existing hook without breaking existing *callers*.
+See :ref:`hookspec_arg_default` for details.

docs/index.rst

@@ -563,6 +563,8 @@ behavior you can run a check with the
 to enforce requisite *hookspecs* but with certain exceptions for some hooks
 then make sure to mark those hooks as :ref:`optional <optionalhook>`.
 
+.. _opt_in_arguments:
+
 Opt-in arguments
 ^^^^^^^^^^^^^^^^
 To allow for *hookspecs* to evolve over the lifetime of a project,
@@ -608,6 +610,37 @@ whereas this is not:
     many arguments as its *hookimpls*) is the conventional :ref:`self <python:tut-remarks>` arg; this
     is always ignored when *hookimpls* are defined as :ref:`methods <python:tut-methodobjects>`.
 
+
+.. _hookspec_arg_default:
+
+Hookspec argument defaults
+^^^^^^^^^^^^^^^^^^^^^^^^^^
+A host program can specify hooks that are intended to be called by external
+plugins it has no control of, and not just by itself. This adds another
+dimension of backward compatibility to consider when evolving a hookspec.
+
+While :ref:`opt-in arguments <opt_in_arguments>` allows a hookspec to evolve
+while keeping existing *hookimpls* working, it doesn't help with existing *hook
+callers*. To handle this, pluggy allows a hookspec to declare a default value
+for an argument, using the usual ``def myhook(new_arg="default")`` Python syntax.
+
+If a caller omits this argument, pluggy will supply the default to the hookimpls.
+This allows the hookspec to add the new argument without breaking existing
+callers:
+
+.. code-block:: python
+
+    @hookspec
+    def myhook(config, args, new_arg=None):
+        pass
+
+
+    # Existing caller; hookimpls will get new_arg=None.
+    pm.hook.myhook(config=config, args=args)
+    # New caller; hookimpls will get new_arg="get this".
+    pm.hook.myhook(config=config, args=args, new_arg="get this")
+
+
 .. _firstresult:
 
 First result only

src/pluggy/_hooks.py

@@ -505,6 +505,11 @@ def _add_hookimpl(self, hookimpl: HookImpl) -> None:
     def __repr__(self) -> str:
         return f"<HookCaller {self.name!r}>"
 
+    def _apply_defaults(self, kwargs: Mapping[str, object]) -> Mapping[str, object]:
+        if self.spec is None or not self.spec.kwargdefaults:
+            return kwargs
+        return {**self.spec.kwargdefaults, **kwargs}
+
     def _verify_all_args_are_provided(self, kwargs: Mapping[str, object]) -> None:
         # This is written to avoid expensive operations when not needed.
         if self.spec:
@@ -539,10 +544,13 @@ def __call__(self, **kwargs: object) -> Any:
         assert not self.is_historic(), (
             "Cannot directly call a historic hook - use call_historic instead."
         )
-        self._verify_all_args_are_provided(kwargs)
+        call_kwargs = self._apply_defaults(kwargs)
+        self._verify_all_args_are_provided(call_kwargs)
         firstresult = self.spec.opts.get("firstresult", False) if self.spec else False
         # Copy because plugins may register other plugins during iteration (#438).
-        return self._hookexec(self.name, self._hookimpls.copy(), kwargs, firstresult)
+        return self._hookexec(
+            self.name, self._hookimpls.copy(), call_kwargs, firstresult
+        )
 
     def call_historic(
         self,
@@ -559,6 +567,7 @@ def call_historic(
         """
         assert self._call_history is not None
         kwargs = kwargs or {}
+        kwargs = self._apply_defaults(kwargs)
         self._verify_all_args_are_provided(kwargs)
         self._call_history.append((kwargs, result_callback))
         # Historizing hooks don't return results.
@@ -580,6 +589,7 @@ def call_extra(
         assert not self.is_historic(), (
             "Cannot directly call a historic hook - use call_historic instead."
         )
+        kwargs = self._apply_defaults(kwargs)
         self._verify_all_args_are_provided(kwargs)
         opts: HookimplOpts = {
             "wrapper": False,
@@ -729,6 +739,7 @@ class HookSpec:
     __slots__ = (
         "argnames",
         "function",
+        "kwargdefaults",
         "kwargnames",
         "name",
         "namespace",
@@ -747,6 +758,8 @@ def __init__(self, namespace: _Namespace, name: str, opts: HookspecOpts) -> None
         self.argnames, self.kwargnames = varnames(
             self.function, legacy_noself=legacy_noself
         )
+        defaults = inspect.unwrap(self.function).__defaults__
+        self.kwargdefaults = dict(zip(self.kwargnames, defaults or ()))
         self.opts = opts
         self.warn_on_impl = opts.get("warn_on_impl")
         self.warn_on_impl_args = opts.get("warn_on_impl_args")

src/pluggy/_manager.py

@@ -414,7 +414,9 @@ def _verify_hook(self, hook: HookCaller, hookimpl: HookImpl) -> None:
             _warn_for_function(hook.spec.warn_on_impl, hookimpl.function)
 
         # positional arg checking
-        notinspec = set(hookimpl.argnames) - set(hook.spec.argnames)
+        notinspec = set(hookimpl.argnames) - (
+            set(hook.spec.argnames) | set(hook.spec.kwargnames)
+        )
         if notinspec:
             raise PluginValidationError(
                 hookimpl.plugin,

testing/test_invocations.py

@@ -367,3 +367,193 @@ def wrap(self):
     pm.register(Plugin())
     with pytest.raises(RuntimeError, match="wrap_controller at 'wrap'.* did not yield"):
         pm.hook.wrap()
+
+
+class TestHookspecDefaultArgumentValue:
+    """Tests for the hookspec functionality of providing a default value for args."""
+
+    def test_basic(self, pm: PluginManager) -> None:
+        """Basic scenario with old and new impls."""
+        default = object()
+
+        class Api:
+            @hookspec
+            def hello(self, arg, new_arg=default):
+                pass
+
+        class NewPlugin:
+            @hookimpl
+            def hello(self, arg, new_arg):
+                return "new", arg, new_arg
+
+        class OldPlugin:
+            @hookimpl
+            def hello(self, arg):
+                return "old", arg
+
+        pm.add_hookspecs(Api)
+        pm.register(NewPlugin())
+        pm.register(OldPlugin())
+
+        assert pm.hook.hello(arg=1) == [("old", 1), ("new", 1, default)]
+
+        assert pm.hook.hello(arg=1, new_arg="call") == [("old", 1), ("new", 1, "call")]
+
+    def test_added_after_registration(self, pm: PluginManager) -> None:
+        """If spec is registered after plugin, the default still works."""
+
+        class Plugin:
+            @hookimpl
+            def hello(self, arg, new_arg):
+                return arg, new_arg
+
+        pm.register(Plugin())
+
+        class Api:
+            @hookspec
+            def hello(self, arg, new_arg="default"):
+                pass
+
+        pm.add_hookspecs(Api)
+
+        assert pm.hook.hello(arg=1) == [(1, "default")]
+
+    def test_old_spec_new_impl(self, pm: PluginManager) -> None:
+        """Test what happens when old spec version is used with new impl version.
+
+        The impl must provide its *own* default if it wants to keep supporting
+        old spec versions.
+        """
+
+        class Api:
+            @hookspec
+            def hello(self, arg):
+                pass
+
+        pm.add_hookspecs(Api)
+
+        class PluginWithoutImplDefault:
+            @hookimpl
+            def hello(self, arg, new_arg):
+                return arg, new_arg  # pragma: no cover
+
+        class PluginWithImplDefault:
+            @hookimpl
+            def hello(self, arg, new_arg="impl-default"):
+                return arg, new_arg
+
+        with pytest.raises(PluginValidationError):
+            pm.register(PluginWithoutImplDefault())
+        pm.register(PluginWithImplDefault())
+
+        assert pm.hook.hello(arg=1) == [(1, "impl-default")]
+
+    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.
+
+        NOTE: This is verifying existing behavior, but it's not necessarily what
+        we want (#442).
+        """
+
+        class Api:
+            @hookspec
+            def hello(self, arg, new_arg="spec-default"):
+                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) == ["impl-default"]
+        assert pm.hook.hello(arg=1, new_arg="call") == ["impl-default"]
+
+    def test_new_call_argument_is_not_delivered_by_old_spec(
+        self, pm: PluginManager
+    ) -> None:
+        """If call uses new spec, but both spec and plugin are old, the new arg
+        is just ignored."""
+
+        class Api:
+            @hookspec
+            def hello(self, arg):
+                pass
+
+        class Plugin:
+            @hookimpl
+            def hello(self, arg):
+                return arg
+
+        pm.add_hookspecs(Api)
+        pm.register(Plugin())
+
+        assert pm.hook.hello(arg=1, extra=2) == [1]
+
+    @pytest.mark.parametrize("value", [None, False, 0, ""])
+    def test_falsy_values_allowed_as_default(
+        self, pm: PluginManager, value: object
+    ) -> None:
+        """Make sure falsy values like None are allowed as default (e.g. not
+        used as special sentinels by the implementation)."""
+
+        class Api:
+            @hookspec
+            def hello(self, arg=value):
+                pass
+
+        class Plugin:
+            @hookimpl
+            def hello(self, arg):
+                return (arg,)
+
+        pm.add_hookspecs(Api)
+        pm.register(Plugin())
+
+        assert pm.hook.hello() == [(value,)]
+        assert pm.hook.hello(arg=value) == [(value,)]
+
+    def test_historic_replay(self, pm: PluginManager) -> None:
+        """Test historic calls respect defaults."""
+
+        class Api:
+            @hookspec(historic=True)
+            def hello(self, arg, new_arg="spec"):
+                pass
+
+        pm.add_hookspecs(Api)
+
+        pm.hook.hello.call_historic(kwargs={"arg": 1})
+        pm.hook.hello.call_historic(kwargs={"arg": 2, "new_arg": "call"})
+
+        seen = []
+
+        class Plugin:
+            @hookimpl
+            def hello(self, arg, new_arg):
+                seen.append((arg, new_arg))
+
+        pm.register(Plugin())
+
+        assert seen == [(1, "spec"), (2, "call")]
+
+    def test_call_extra(self, pm: PluginManager) -> None:
+        """Test calls with extras respect defaults."""
+
+        class Api:
+            @hookspec
+            def hello(self, arg, new_arg="spec"):
+                pass
+
+        pm.add_hookspecs(Api)
+
+        def hello(arg, new_arg):
+            return arg, new_arg
+
+        assert pm.hook.hello.call_extra([hello], {"arg": 1}) == [(1, "spec")]
+        assert pm.hook.hello.call_extra([hello], {"arg": 2, "new_arg": "call"}) == [
+            (2, "call")
+        ]