diff --git a/changelog/742.removal.rst b/changelog/742.removal.rst new file mode 100644 index 00000000..996ed322 --- /dev/null +++ b/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 `_). diff --git a/docs/index.rst b/docs/index.rst index f2aaf957..210959e8 100644 --- a/docs/index.rst +++ b/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 diff --git a/src/pluggy/_decorators.py b/src/pluggy/_decorators.py index c5574a67..0996db4e 100644 --- a/src/pluggy/_decorators.py +++ b/src/pluggy/_decorators.py @@ -325,6 +325,80 @@ 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__ = ( @@ -332,6 +406,7 @@ class HookSpec: "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 diff --git a/src/pluggy/_hooks.py b/src/pluggy/_hooks.py index b57aae66..e301ecd8 100644 --- a/src/pluggy/_hooks.py +++ b/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", ] diff --git a/src/pluggy/_impl.py b/src/pluggy/_impl.py index 1b7182f1..841e876f 100644 --- a/src/pluggy/_impl.py +++ b/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. diff --git a/src/pluggy/_manager.py b/src/pluggy/_manager.py index 2697705b..e88b58cf 100644 --- a/src/pluggy/_manager.py +++ b/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) diff --git a/testing/test_helpers.py b/testing/test_helpers.py index 3c58ecee..7067bc6c 100644 --- a/testing/test_helpers.py +++ b/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) diff --git a/testing/test_warnings.py b/testing/test_warnings.py index 72fd8817..2e6feae4 100644 --- a/testing/test_warnings.py +++ b/testing/test_warnings.py @@ -92,3 +92,125 @@ def my_hook(item, extra) -> None: with warnings.catch_warnings(): warnings.simplefilter("error") pm.add_hookspecs(Api) + + +def test_hookspec_keyword_only_warns(pm: PluginManager) -> None: + """Keyword-only hookspec parameters are ignored, so they are deprecated. + + See https://github.com/pytest-dev/pluggy/issues/742. + """ + + class Api: + @hookspec + def my_hook(self, arg, *, required_kwonly, defaulted_kwonly=1) -> None: + pass + + with pytest.warns( + DeprecationWarning, + match=r"hookspec 'my_hook' declares keyword-only parameter\(s\) " + r"'required_kwonly', 'defaulted_kwonly'", + ) as wc: + pm.add_hookspecs(Api) + assert len(wc.list) == 1 + # The warning points at the code which defined the hookspec. + assert Path(wc.list[0].filename).name == "test_warnings.py" + + # They are not part of the specification's parameters. + spec = pm.hook.my_hook.spec + assert spec is not None + assert spec.argnames == ("arg",) + assert spec.kwargnames == () + assert spec.kwonlyargnames == ("required_kwonly", "defaulted_kwonly") + + +def test_hookspec_without_keyword_only_no_warning(pm: PluginManager) -> None: + """A hookspec without keyword-only parameters does not warn.""" + + class Api: + @hookspec + def my_hook(self, arg, other=1) -> None: + pass + + with warnings.catch_warnings(): + warnings.simplefilter("error") + pm.add_hookspecs(Api) + spec = pm.hook.my_hook.spec + assert spec is not None + assert spec.kwonlyargnames == () + + +def test_hookimpl_keyword_only_warns(pm: PluginManager) -> None: + """A hookimpl requesting a keyword-only parameter is deprecated. + + pluggy never passes such a parameter, so an implementation of this hook + which requests one gets its default (or fails) instead of the call value. + """ + + class Api: + @hookspec + def my_hook(self, arg, *, kwonly=1) -> None: + pass + + with pytest.warns(DeprecationWarning, match=r"hookspec 'my_hook'"): + pm.add_hookspecs(Api) + + class Plugin: + @hookimpl + def my_hook(self, arg, *, kwonly=1): + return (arg, kwonly) + + with pytest.warns( + DeprecationWarning, + match=r"my_hook\(arg, \*, kwonly=1\) declares keyword-only " + r"parameter\(s\) 'kwonly'", + ) as wc: + pm.register(Plugin(), "plugin") + assert len(wc.list) == 1 + assert Path(wc.list[0].filename).name == "test_warnings.py" + + # Document the behaviour which motivates the warning: the value passed by + # the caller is dropped and the implementation sees its own default. + assert pm.hook.my_hook(arg=1, kwonly=2) == [(1, 1)] + + +def test_hookimpl_without_keyword_only_no_warning(pm: PluginManager) -> None: + """A hookimpl without keyword-only parameters does not warn.""" + + class Api: + @hookspec + def my_hook(self, arg, kwonly=1) -> None: + pass + + pm.add_hookspecs(Api) + + class Plugin: + @hookimpl + def my_hook(self, arg, kwonly=1): + return (arg, kwonly) + + with warnings.catch_warnings(): + warnings.simplefilter("error") + pm.register(Plugin(), "plugin") + assert pm.hook.my_hook(arg=1, kwonly=2) == [(1, 2)] + + +def test_hookimpl_keyword_only_warns_even_if_spec_has_none( + pm: PluginManager, +) -> None: + """An unrequestable hookimpl parameter is still worth warning about.""" + + class Api: + @hookspec + def my_hook(self, arg) -> None: + pass + + pm.add_hookspecs(Api) + + class Plugin: + @hookimpl + def my_hook(self, arg, *, kwonly=1): + return (arg, kwonly) + + with pytest.warns(DeprecationWarning, match=r"declares keyword-only"): + pm.register(Plugin(), "plugin") + assert pm.hook.my_hook(arg=1) == [(1, 1)]