Deprecate keyword-only hook parameters

changelog/742.removal.rst

@@ -0,0 +1 @@
+Keyword-only parameters declared in a hookspec or hookimpl (with ``*``) now emit a :class:`DeprecationWarning`. pluggy currently ignores them: a hookimpl which requests one never receives the value passed by the hook caller, and gets its own default or a :exc:`TypeError` instead. Declare such parameters as regular ones; this will become an error in a future release (`#742 <https://github.com/pytest-dev/pluggy/issues/742>`_).

docs/index.rst

@@ -660,6 +660,25 @@ callers:
 .. versionadded:: 1.7
     Hookspec argument defaults.
 
+Keyword-only parameters
+^^^^^^^^^^^^^^^^^^^^^^^
+Keyword-only parameters, declared after a ``*``, are currently **ignored** by
+pluggy: they are not part of the *hookspec* signature, and they are never passed
+to a *hookimpl*. A *hookimpl* which requests one receives its own default (or
+raises :exc:`TypeError` if it has none), even when the caller passes a value.
+
+Declaring such a parameter in a *hookspec* or a *hookimpl* therefore emits a
+:class:`DeprecationWarning`. Declare the parameter as a regular one instead:
+
+.. code-block:: python
+
+    @hookspec
+    def myhook(config, args, kwonly=None):  # not `config, args, *, kwonly=None`
+        pass
+
+.. deprecated:: 1.7
+    Keyword-only parameters in hookspecs and hookimpls.
+
 .. _firstresult:
 
 First result only

src/pluggy/_decorators.py

@@ -325,13 +325,88 @@ def varnames(
     return args, kwargs
 
 
+def _callable_code(func: object) -> types.CodeType | None:
+    """Return the code object of ``func``, or None if there is none.
+
+    ``func`` is normalized the same way :func:`varnames` does it: a class
+    stands for its ``__init__``, a callable object for its ``__call__``, and
+    wrappers are unwrapped. This keeps both functions reporting the same
+    parameters for the same input.
+    """
+    if inspect.isclass(func):
+        func = getattr(func, "__init__", None)
+    elif not inspect.isroutine(func):
+        try:
+            # Not a `callable()` check: the `__call__` attribute itself is
+            # wanted, so that its code can be inspected below.
+            func = getattr(func, "__call__", func)  # noqa: B004
+        except Exception:  # pragma: no cover - pypy special case
+            return None
+    if func is None:
+        return None
+    try:
+        func = inspect.unwrap(func)  # type: ignore[arg-type]
+    except Exception:  # pragma: no cover - broken __wrapped__ chain
+        return None
+    if inspect.ismethod(func):
+        func = func.__func__
+    code: types.CodeType | None = getattr(func, "__code__", None)
+    return code
+
+
+def _kwonly_argnames(func: object) -> tuple[str, ...]:
+    """Return the names of the keyword-only parameters of a callable.
+
+    Keyword-only parameters are not part of ``argnames``/``kwargnames``:
+    :func:`varnames` only looks at ``co_argcount``, which excludes the
+    parameters after ``*``. This exposes them so that they can be reported
+    instead of silently ignored.
+    """
+    code = _callable_code(func)
+    if code is None:
+        return ()
+    start = code.co_argcount
+    return code.co_varnames[start : start + code.co_kwonlyargcount]
+
+
+def warn_keyword_only_parameters(
+    func: Callable[..., object],
+    *,
+    what: str,
+    stacklevel: int,
+) -> None:
+    """Warn when ``func`` declares keyword-only parameters.
+
+    pluggy does not pass keyword-only parameters to hook implementations, so a
+    parameter that is declared in both the hookspec and the hookimpl is
+    silently ignored. Warn so that it can be removed before it becomes an
+    error.
+
+    ``stacklevel`` is relative to the caller of this function, like
+    :func:`warnings.warn`.
+    """
+    kwonly = _kwonly_argnames(func)
+    if not kwonly:
+        return
+    names = ", ".join(repr(name) for name in kwonly)
+    warnings.warn(
+        f"{what} declares keyword-only parameter(s) {names}, which pluggy "
+        "currently ignores (they are never passed to hook implementations). "
+        "Declare them as regular parameters instead. This will become an "
+        "error in a future version of pluggy.",
+        DeprecationWarning,
+        stacklevel=stacklevel,
+    )
+
+
 @final
 class HookSpec:
     __slots__ = (
         "argnames",
         "function",
         "kwargdefaults",
         "kwargnames",
+        "kwonlyargnames",
         "name",
         "namespace",
         "opts",
@@ -349,6 +424,10 @@ def __init__(self, namespace: _Namespace, name: str, opts: HookspecOpts) -> None
         self.argnames, self.kwargnames = varnames(
             self.function, legacy_noself=legacy_noself
         )
+        #: The keyword-only parameter names of the hook specification. These
+        #: are not passed to hook implementations; see
+        #: :func:`warn_keyword_only_parameters`.
+        self.kwonlyargnames = _kwonly_argnames(self.function)
         defaults = inspect.unwrap(self.function).__defaults__
         self.kwargdefaults = dict(zip(self.kwargnames, defaults or ()))
         self.opts = opts

src/pluggy/_hooks.py

@@ -16,11 +16,13 @@
 from ._config import HookimplOpts
 from ._config import HookspecOpts
 from ._config import normalize_hookimpl_opts
+from ._decorators import _kwonly_argnames
 from ._decorators import _Namespace
 from ._decorators import HookimplMarker
 from ._decorators import HookSpec
 from ._decorators import HookspecMarker
 from ._decorators import varnames
+from ._decorators import warn_keyword_only_parameters
 from ._impl import _HookImplFunction
 from ._impl import _Plugin
 from ._impl import HookImpl
@@ -42,6 +44,8 @@
     "_Namespace",
     "_Plugin",
     "_SubsetHookCaller",
+    "_kwonly_argnames",
     "normalize_hookimpl_opts",
     "varnames",
+    "warn_keyword_only_parameters",
 ]

src/pluggy/_impl.py

@@ -12,6 +12,7 @@
 from typing import TypeVar
 
 from ._config import HookimplOpts
+from ._decorators import _kwonly_argnames
 from ._decorators import varnames
 from ._result import Result
 
@@ -31,6 +32,7 @@ class HookImpl:
         "function",
         "hookwrapper",
         "kwargnames",
+        "kwonlyargnames",
         "optionalhook",
         "opts",
         "plugin",
@@ -56,6 +58,9 @@ def __init__(
         #: The keyword parameter names of ``function`` which declare defaults.
         #: Keyword-only parameters are *not* included.
         self.kwargnames: Final = kwargnames
+        #: The keyword-only parameter names of ``function``. These are not
+        #: passed to the function; see :func:`warn_keyword_only_parameters`.
+        self.kwonlyargnames: Final = _kwonly_argnames(self.function)
         #: The plugin which defined this hook implementation.
         self.plugin: Final = plugin
         #: The :class:`HookimplOpts` used to configure this hook implementation.

src/pluggy/_manager.py

@@ -26,6 +26,7 @@
 from ._hooks import HookRelay
 from ._hooks import HookspecOpts
 from ._hooks import normalize_hookimpl_opts
+from ._hooks import warn_keyword_only_parameters
 from ._result import Result
 
 
@@ -358,6 +359,15 @@ def add_hookspecs(self, module_or_class: _Namespace) -> None:
                     hc.set_specification(module_or_class, spec_opts)
                     for hookfunction in hc.get_hookimpls():
                         self._verify_hook(hc, hookfunction)
+                assert hc.spec is not None
+                # Keyword-only parameters are never passed to hook
+                # implementations (#742), so warn even when no implementation
+                # requests this hook yet.
+                warn_keyword_only_parameters(
+                    hc.spec.function,
+                    what=f"hookspec {name!r}",
+                    stacklevel=3,
+                )
                 names.append(name)
 
         if not names:
@@ -438,6 +448,16 @@ def _verify_hook(self, hook: HookCaller, hookimpl: HookImpl) -> None:
         if hook.spec.warn_on_impl:
             _warn_for_function(hook.spec.warn_on_impl, hookimpl.function)
 
+        # Keyword-only parameters are never passed to the hookimpl (#742), so
+        # warn about any of them: one shared with the hookspec would be
+        # silently ignored.
+        if hook.spec.kwonlyargnames or hookimpl.kwonlyargnames:
+            warn_keyword_only_parameters(
+                hookimpl.function,
+                what=_formatdef(hookimpl.function),
+                stacklevel=4,
+            )
+
         # positional arg checking
         notinspec = set(hookimpl.argnames) - (
             set(hook.spec.argnames) | set(hook.spec.kwargnames)

testing/test_helpers.py

@@ -1,10 +1,16 @@
 from collections.abc import Callable
 from functools import wraps
+from pathlib import Path
 from typing import Any
 from typing import cast
 from typing import TypeVar
+import warnings
 
+import pytest
+
+from pluggy._hooks import _kwonly_argnames
 from pluggy._hooks import varnames
+from pluggy._hooks import warn_keyword_only_parameters
 from pluggy._manager import _formatdef
 
 
@@ -272,3 +278,89 @@ def func_with_bad_annotation(
         pass
 
     assert varnames(func_with_bad_annotation) == (("x", "y"), ())
+
+
+def test_kwonly_argnames_forms() -> None:
+    """``_kwonly_argnames`` normalizes callables the same way ``varnames`` does.
+
+    These parameters are not part of ``varnames`` results, so they need to be
+    reported separately (see ``test_warnings.py``).
+    """
+
+    def function(a, b=1, *, c, d=2) -> None:
+        pass  # pragma: no cover
+
+    class CallableObject:
+        def __call__(self, q, *, cobalt) -> None:
+            pass  # pragma: no cover
+
+    class Methods:
+        def method(self, a, *, kwonly, other=2) -> None:
+            pass  # pragma: no cover
+
+        @classmethod
+        def cm(cls, a, *, kwonly) -> None:
+            pass  # pragma: no cover
+
+        @staticmethod
+        def sm(a, *, kwonly) -> None:
+            pass  # pragma: no cover
+
+    F = TypeVar("F", bound=Callable[..., Any])
+
+    def decorator(func: F) -> F:
+        @wraps(func)
+        def wrapper(*args, **kwargs):
+            return func(*args, **kwargs)  # pragma: no cover
+
+        return cast(F, wrapper)
+
+    @decorator
+    def decorated(a, *, decorated_kwonly=3) -> None:
+        pass  # pragma: no cover
+
+    assert _kwonly_argnames(function) == ("c", "d")
+    assert _kwonly_argnames(lambda a: None) == ()
+    assert _kwonly_argnames(Methods.cm) == ("kwonly",)
+    assert _kwonly_argnames(Methods.sm) == ("kwonly",)
+    assert _kwonly_argnames(Methods.method) == ("kwonly", "other")
+    assert _kwonly_argnames(Methods().method) == ("kwonly", "other")
+    assert _kwonly_argnames(CallableObject()) == ("cobalt",)
+    assert _kwonly_argnames(decorated) == ("decorated_kwonly",)
+    # Callables without a code object, and classes whose __init__ has no
+    # keyword-only parameters.
+    assert _kwonly_argnames(len) == ()
+    assert _kwonly_argnames(type("Bare", (), {})) == ()
+
+    class RaisingCall:
+        def __getattr__(self, name):
+            if name == "__call__":
+                raise RuntimeError("boom")  # pragma: no cover
+            raise AttributeError(name)  # pragma: no cover
+
+    def broken_wrapped(*args, **kwargs) -> None:
+        pass  # pragma: no cover
+
+    broken_wrapped.__wrapped__ = 42  # type: ignore[attr-defined]
+
+    # Never raise on exotic callables, just report nothing.
+    assert _kwonly_argnames(RaisingCall()) == ()
+    assert _kwonly_argnames(broken_wrapped) == ()
+    assert _kwonly_argnames(5) == ()
+    assert _kwonly_argnames(None) == ()
+
+
+def test_warn_keyword_only_parameters_directly() -> None:
+    """The helper warns with the given description, pointing at the caller."""
+
+    def func(a, *, kwonly) -> None:
+        pass  # pragma: no cover
+
+    with pytest.warns(DeprecationWarning, match=r"func\(a, \*, kwonly\)") as wc:
+        warn_keyword_only_parameters(func, what="func(a, *, kwonly)", stacklevel=2)
+    assert len(wc.list) == 1
+    assert Path(wc.list[0].filename).name == "test_helpers.py"
+
+    with warnings.catch_warnings():
+        warnings.simplefilter("error")
+        warn_keyword_only_parameters(lambda a, b=1: None, what="lambda", stacklevel=2)