Skip to content

Fix handler wrapping for keyword-only parameters#3084

Open
rohan-patnaik wants to merge 2 commits into
microsoft:mainfrom
rohan-patnaik:fix-handler-keyword-only-signature
Open

Fix handler wrapping for keyword-only parameters#3084
rohan-patnaik wants to merge 2 commits into
microsoft:mainfrom
rohan-patnaik:fix-handler-keyword-only-signature

Conversation

@rohan-patnaik
Copy link
Copy Markdown

Fixes #3067

Summary

This fixes handler wrapping so keyword-only parameters are not counted as positional handler arguments.

Why

Before this change, a handler like locator.blur was treated as if it accepted the triggering locator because it has a keyword-only timeout option. Playwright then passed the locator positionally and crashed with TypeError.

Test plan

  • python3 -m pytest tests/common/test_impl_to_api_mapping.py
  • python3 -m black --check playwright/_impl/_impl_to_api_mapping.py tests/common/test_impl_to_api_mapping.py
  • python3 -m mypy playwright/_impl/_impl_to_api_mapping.py

@rohan-patnaik
Copy link
Copy Markdown
Author

@microsoft-github-policy-service agree

Copy link
Copy Markdown

@biswajeetdev biswajeetdev left a comment

Choose a reason for hiding this comment

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

The fix correctly identifies the problem: inspect.signature(handler).parameters includes KEYWORD_ONLY and VAR_KEYWORD entries in its count, so a handler like def blur(self, *, timeout=None) was getting counted as having 2 parameters and receiving a spurious positional arg it could not accept.

The new logic — counting only POSITIONAL_ONLY and POSITIONAL_OR_KEYWORD params, and falling back to len(args) when *args is present — is exactly right.

Two small suggestions:

1. Add a test for the *args / has_varargs branch

The current test covers keyword-only params, but the has_varargs path is not exercised:

def test_wrap_handler_passes_all_args_for_varargs_handler() -> None:
    calls = []

    def handler(*args):
        calls.append(args)

    ImplToApiMapping().wrap_handler(handler)("a", "b", "c")
    assert len(calls[0]) == 3

2. POSITIONAL_ONLY params in practice

Python's POSITIONAL_ONLY kind (params before /) is rare in pure-Python code but common in C-extension signatures and some inspect-wrapped callables. The fix correctly includes them — just noting it is a good edge case to keep in mind if future test fixtures include C-extension handlers.

@rohan-patnaik
Copy link
Copy Markdown
Author

Addressed the review suggestion by adding coverage for the *args / has_varargs path in wrap_handler.

Verified with:

  • python3 -m pytest tests/common/test_impl_to_api_mapping.py
  • python3 -m black --check playwright/_impl/_impl_to_api_mapping.py tests/common/test_impl_to_api_mapping.py
  • python3 -m mypy playwright/_impl/_impl_to_api_mapping.py

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

Labels

None yet

Projects

None yet

2 participants