Skip to content

Deprecate positional-only hook implementation parameters - #775

Open
adenzhou1350 wants to merge 7 commits into
pytest-dev:mainfrom
adenzhou1350:codex/fix-posonly-hook-defaults
Open

adenzhou1350 wants to merge 7 commits into
pytest-dev:mainfrom
adenzhou1350:codex/fix-posonly-hook-defaults

Conversation

@adenzhou1350

@adenzhou1350 adenzhou1350 commented Oct 11, 2026 •

Copy link
Copy Markdown

Refs #774. Revised to follow the agreed deprecation policy: #774 (comment)

Emit DeprecationWarning when constructing a hook implementation with positional-only parameters. Exclude bound self/cls receivers and leave hookspecs unchanged. The warning points to the implementation definition and mentions pluggy 2.0.

The earlier argument-binding fix is removed: _execution.py is identical to current main. Required positional-only calls still work; defaulted positional-only calls retain their existing behavior, including the existing TypeError when supplied by keyword.

Tests cover plain hooks, both wrapper styles, methods, class/static methods, decorated and callable call_extra implementations, and receiver-only controls. The new matrix is 12 failures/1 passing control on unmodified main; all 13 pass with the warning.

Validation: full suite 232 passed on Windows CPython 3.14 and Linux CPython 3.12, using exact source/import checks and existing private dependency environments (pytest 9.0.3/9.1.1), not the complete uv-locked matrix. Mypy and the cached non-Ruff pre-commit checks pass. Local Ruff remains blocked by Windows policy; current hosted checks are pending.

AI-assisted with OpenAI Codex. No performance claim.

Fixes pytest-dev#774

Assisted-by: OpenAI Codex
Signed-off-by: Xucheng Zhou <aden1350@outlook.com>
Assisted-by: OpenAI Codex
Signed-off-by: Xucheng Zhou <aden1350@outlook.com>

@RonnyPfannschmidt RonnyPfannschmidt left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

Approving. I measured the runtime cost against main before approving. The setup was CPython 3.13, a micro-benchmark, 2 rounds, and the best of 15 runs for each case:

Case main this PR Δ
Hook call, 10 impls, no defaulted args ~4.3 µs ~4.35 µs noise
Hook call, 10 impls with b=None, c=None ~7.2 µs ~7.9 µs +8–12% (~60–80 ns per impl)
Hook call, 10 impls with positional-only defaults TypeError ~15.2 µs new path, ~1.2 µs per impl
HookImpl() construction ~2.55 µs ~4.35 µs +70% (~1.8 µs per impl)
  • Impls without defaulted args are unaffected, because all the new code is inside if hook_impl.kwargnames:.
  • Impls with defaulted args now pay on every call for the _posonlydefaults truthiness check. They also pay for a not in _posonlydefaults lookup for each kwarg inside the kwargs comprehension.
  • The registration cost is one-time and negligible in absolute terms.

Follow-up (not blocking, will be folded into a refactoring PR stack): move the split to HookImpl.__init__. That means precomputing a kwargnames tuple without the positional-only names for the kwargs comprehension, plus a precomputed (name, default) sequence for the positional-only fill. The call path then only touches the positional-only branch when it is non-empty. With that, impls with ordinary defaults should cost the same as on main, and only positional-only impls pay extra.


Generated by Claude Code

@bluetech bluetech left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This fix is too complicated and puts the code in the wrong place.

I think it can be done better, but another option is to just disallow positional-only. I'll leave a comment on the issue.

@adenzhou1350

Copy link
Copy Markdown
Author

Thanks. I'll hold off on further changes here until the positional-only policy is settled on #774. If rejection is preferred, I can replace this with registration-time validation rather than extend the call path.

aden and others added 3 commits October 15, 2026 15:34
Keep the existing call behavior as requested in pytest-dev#774. Remove the previous binding-path change; warn only for actual implementation parameters, not a bound self or cls receiver.

Assisted-by: OpenAI Codex
Signed-off-by: Xucheng Zhou <aden1350@outlook.com>
@adenzhou1350 adenzhou1350 changed the title Deliver defaulted positional-only hook arguments positionally Deprecate positional-only hook implementation parameters Oct 15, 2026
Assisted-by: OpenAI Codex
Signed-off-by: Xucheng Zhou <aden1350@outlook.com>
@adenzhou1350

Copy link
Copy Markdown
Author

Updated to the policy agreed on #774: warn for positional-only implementation parameters, keep existing call behavior, and leave the call loop unchanged. Windows/Linux full suites pass 232 tests; the new warning tests fail on unmodified main. The PR description now reflects the narrower scope and validation limits.

@adenzhou1350
adenzhou1350 requested a review from bluetech October 15, 2026 07:50
Assisted-by: OpenAI Codex
Signed-off-by: Xucheng Zhou <aden1350@outlook.com>
@adenzhou1350

Copy link
Copy Markdown
Author

The remaining Read the Docs failure happens before the documentation build starts: build 35160460 fails at git checkout cd600f4 with "reference is not a tree". GitHub refs/pull/775/head currently resolves to the full cd600f4 commit, which I verified with git ls-remote. The test/package/benchmark and pre-commit checks are green. Could a maintainer retry the external docs build? Log: https://app.readthedocs.org/projects/pluggy/builds/35160460/

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