Skip to content

Deprecate keyword-only hook parameters - #803

Closed
simpleqt wants to merge 1 commit into
pytest-dev:mainfrom
simpleqt:deprecate-keyword-only-hook-parameters
Closed

simpleqt wants to merge 1 commit into
pytest-dev:mainfrom
simpleqt:deprecate-keyword-only-hook-parameters

Conversation

@simpleqt

Copy link
Copy Markdown

Refs #742 — "How to handle keyword-only arguments". No competing PR: the open PRs touching adjacent ground are #775 (positional-only parameters, #774) and #777 (plugin identity, #760), neither of which changes how keyword-only parameters are handled.

Problem

pluggy ignores keyword-only parameters. varnames() only reads co_argcount, so a parameter declared after * lands in neither argnames nor kwargnames. A hookimpl that requests one is never handed the value the caller passed — it silently receives its own default, or raises a bare TypeError when it has none.

@hookspec
def configure(name, *, verbosity="spec-default"): ...

@hookimpl
def configure(name, *, verbosity="impl-default"):
    return (name, verbosity)
>>> pm.hook.configure(name="cfg", verbosity="debug")
[('cfg', 'impl-default')]        # 'debug' silently dropped
>>> pm.hook.go(arg=1, required_kwonly=2)
TypeError: RequiredPlugin.go() missing 1 required keyword-only argument: 'required_kwonly'

The hookspec reports argnames=('name',), kwargnames=() — the keyword-only parameter is invisible to the manager, so _verify_hook cannot reject it and _multicall cannot pass it. The options listed in #742 are "support them" or "reject them"; either way the current behaviour — accept the declaration, then ignore it — is the worst of the two. This follows the project's existing deprecation pattern for hookspec methods without self (#632, pluggy 1.6): warn now, error later.

Repro

pluggy-keyword-only-repro.py (attached) against unmodified main (0224f79):

$ .venv/Scripts/python.exe pluggy-keyword-only-repro.py
=== 1. does pluggy warn about the keyword-only parameter? ===
  (no warning)

=== 2. what does the hookspec report? ===
  argnames       : ('name',)
  kwargnames     : ()
  kwonlyargnames : <absent>

=== 3. is the caller's value delivered to the hookimpl? ===
  caller passed  : verbosity='debug'
  hook returned  : [('cfg', 'impl-default')]
  hookimpl saw   : ['impl-default']
  -> value dropped

=== 4. a *required* keyword-only parameter ===
  TypeError: RequiredPlugin.go() missing 1 required keyword-only argument: 'required_kwonly'

Expected: pluggy says something, rather than delivering a silently wrong value.

Root cause

src/pluggy/_decorators.py::varnames derives parameters from code.co_varnames[:code.co_argcount]. co_kwonlyargcount is never consulted, so keyword-only parameters are dropped on the floor; HookSpec and HookImpl then have no record of them, and neither registration-time validation nor _multicall can act on them.

Fix

Keep varnames()'s documented contract intact (test_varnames_keyword_only asserts keyword-only parameters are excluded, and HookCaller/_multicall are keyword-passing by design), and expose the dropped names alongside it:

  • _kwonly_argnames() — reads co_kwonlyargcount, normalizing callables the same way varnames does (class → __init__, callable object → __call__, unwrap functools.wraps, bound methods → __func__); never raises on exotic callables.
  • warn_keyword_only_parameters() — emits DeprecationWarning naming the parameters, with an explicit stacklevel so the warning is attributed to the code defining the hookspec/hookimpl.
  • HookSpec.kwonlyargnames / HookImpl.kwonlyargnames — new attributes recording the names.
  • PluginManager.add_hookspecs warns for a hookspec declaring them, even when no implementation requests that hook yet.
  • PluginManager._verify_hook warns for a hookimpl declaring them, whether or not the hookspec repeats them.

Plus a docs section (Keyword-only parameters) and a changelog fragment.

Tests

testing/test_warnings.py (behaviour, public API only) — 4 new tests:

  • hookspec with keyword-only parameters warns once with the parameter names, points at the defining file, and reports them via spec.kwonlyargnames;
  • hookspec without them does not warn;
  • hookimpl requesting one warns at registration and the test pins the motivating behaviour (pm.hook.my_hook(arg=1, kwonly=2) == [(1, 1)]);
  • hookimpl warns even when the hookspec does not repeat the parameter.

testing/test_helpers.py (helper level) — 2 new tests: _kwonly_argnames over plain functions, classes, callable objects, bound/unbound methods, class/static methods, decorated functions, len, and callables that raise from __call__ or have a broken __wrapped__; and warn_keyword_only_parameters directly.

Evidence

RED — unmodified main, new tests present:

FAILED testing/test_warnings.py::test_hookspec_keyword_only_warns        - DID NOT WARN
FAILED testing/test_warnings.py::test_hookspec_without_keyword_only_no_warning
FAILED testing/test_warnings.py::test_hookimpl_keyword_only_warns        - DID NOT WARN
FAILED testing/test_warnings.py::test_hookimpl_keyword_only_warns_even_if_spec_has_none
4 failed, 5 passed

testing/test_helpers.py cannot even import _kwonly_argnames before the fix (collection error), as that helper is new.

GREEN — with the fix:

$ pytest testing/test_warnings.py testing/test_helpers.py -q   # 27 passed
$ pytest testing -q                                           # 232 passed

Full pre-commit run --files <changed files>: ruff check, ruff format, autoflake, trailing-whitespace, end-of-files, rst code two-backticks, pyupgrade, blacken-docs, mypy — all pass. (rst was skipped: pygments/restructuredtext_lint are not installed here.)

Two notes on scope:

  • The repro still shows step 3 delivering 'impl-default' after the fix; the warning is the change. Making keyword-only parameters actually work is the "support them" option How to handle keyword-only arguments #742 rules out on performance grounds, and rejecting them outright would be a breaking change this deprecation exists to avoid.
  • varnames() return values are unchanged, so no existing caller of it is affected.

The final commit is signed off and the patch applies cleanly to a pristine clone of main (verified there: 232 passed).

AI assistance

AI-assisted: finding, reproduction, implementation, tests and evidence were prepared with an AI agent (DeepSeek Harness, dsv41) and reviewed against the reported issue and the project's existing deprecation precedent (#632). No performance claims; the added work happens only at registration time.

pluggy ignores keyword-only parameters: `varnames` only reads
`co_argcount`, so a parameter declared after `*` ends up in neither
`argnames` nor `kwargnames`. A hookimpl that requests one is therefore
never handed the value the caller passed -- it silently receives its own
default, or raises a bare `TypeError` when it has none:

    @hookspec
    def configure(name, *, verbosity="spec-default"): ...

    @hookimpl
    def configure(name, *, verbosity="impl-default"): ...
        return (name, verbosity)          # always ("cfg", "impl-default")

    pm.hook.configure(name="cfg", verbosity="debug")  # "debug" is dropped

Report such parameters instead of ignoring them, following the
DeprecationWarning + future error pattern already used for hookspec
methods without `self` (pytest-dev#632):

- `varnames` is left untouched (its documented contract excludes
  keyword-only parameters); `_kwonly_argnames` exposes them, and
  `warn_keyword_only_parameters` emits the warning pointing at the code
  which defined the hookspec or hookimpl.
- `HookSpec` and `HookImpl` record the names in a new `kwonlyargnames`
  attribute.
- `add_hookspecs` warns for a hookspec declaring them, even when no
  implementation requests that hook yet; `_verify_hook` warns for a
  hookimpl declaring them, whether or not the hookspec repeats them.

Tests cover both directions and the helper's normalization of classes,
callable objects, bound/unbound methods, class/static methods, wrapped
functions, and callables without a code object. Existing tests are
unaffected: `test_varnames_keyword_only` still documents that `varnames`
excludes keyword-only parameters.

Refs pytest-dev#742.

Signed-off-by: simpleqt <2015900818@qq.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

An organization owner can choose the repository organization as the billing source for reviews requested by members with a paid Copilot license. Learn about Copilot code review billing.

Copy link
Copy Markdown
Member

🤖 Written by Claude Opus 5.5 via Claude Code for the pluggy maintainers; I prompted it, it did the work, I read it.

Closing. We don't accept PRs from unattended AI agents. This account opened 30+ PRs across unrelated projects today. Also, #742 has an assignee and the approach is still undecided, so a deprecation PR is premature.


Generated by Claude Code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants