Repository navigation
Conversation
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>
Contributor
There was a problem hiding this comment.
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.
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 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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 readsco_argcount, so a parameter declared after*lands in neitherargnamesnorkwargnames. A hookimpl that requests one is never handed the value the caller passed — it silently receives its own default, or raises a bareTypeErrorwhen it has none.The hookspec reports
argnames=('name',),kwargnames=()— the keyword-only parameter is invisible to the manager, so_verify_hookcannot reject it and_multicallcannot 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 withoutself(#632, pluggy 1.6): warn now, error later.Repro
pluggy-keyword-only-repro.py(attached) against unmodifiedmain(0224f79):Expected: pluggy says something, rather than delivering a silently wrong value.
Root cause
src/pluggy/_decorators.py::varnamesderives parameters fromcode.co_varnames[:code.co_argcount].co_kwonlyargcountis never consulted, so keyword-only parameters are dropped on the floor;HookSpecandHookImplthen have no record of them, and neither registration-time validation nor_multicallcan act on them.Fix
Keep
varnames()'s documented contract intact (test_varnames_keyword_onlyasserts keyword-only parameters are excluded, andHookCaller/_multicallare keyword-passing by design), and expose the dropped names alongside it:_kwonly_argnames()— readsco_kwonlyargcount, normalizing callables the same wayvarnamesdoes (class →__init__, callable object →__call__, unwrapfunctools.wraps, bound methods →__func__); never raises on exotic callables.warn_keyword_only_parameters()— emitsDeprecationWarningnaming the parameters, with an explicitstacklevelso the warning is attributed to the code defining the hookspec/hookimpl.HookSpec.kwonlyargnames/HookImpl.kwonlyargnames— new attributes recording the names.PluginManager.add_hookspecswarns for a hookspec declaring them, even when no implementation requests that hook yet.PluginManager._verify_hookwarns 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:spec.kwonlyargnames;pm.hook.my_hook(arg=1, kwonly=2) == [(1, 1)]);testing/test_helpers.py(helper level) — 2 new tests:_kwonly_argnamesover 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__; andwarn_keyword_only_parametersdirectly.Evidence
RED — unmodified
main, new tests present:testing/test_helpers.pycannot even import_kwonly_argnamesbefore the fix (collection error), as that helper is new.GREEN — with the fix:
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. (rstwas skipped:pygments/restructuredtext_lintare not installed here.)Two notes on scope:
'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.