downstream test: varnames fix on py3.14

[RonnyPfannschmidt]

Trigger downstream tests for the varnames/py3.14 fix branch.

Upstream PR: pytest-dev#632

Made with Cursor

Summary by Sourcery

Refine varnames handling for methods and annotations, add compatibility warnings for legacy hookspecs, and introduce a TOML/uv-based downstream testing framework with CI integration.

New Features:

• Add a legacy_noself option to varnames and HookSpec to support and warn about hookspec methods that omit a self parameter.
• Introduce a generic downstream testing driver using uv-managed virtual environments and TOML recipes for key dependent projects.
• Add a GitHub Actions workflow to run downstream test recipes on demand or for designated pull requests.

Bug Fixes:

• Fix varnames to handle bound and unbound methods, classmethods, staticmethods, and unresolvable string annotations correctly across Python versions, including 3.14.
• Ensure hookspec methods defined without self are detected and surfaced via FutureWarning instead of silently misbehaving.

Enhancements:

• Reimplement varnames using code objects instead of inspect.signature for more robust and efficient parameter introspection.
• Improve detection of implicit instance parameters, including PyPy-specific naming, when stripping method receivers.
• Extend tests and benchmarks to cover the new varnames semantics and compare against the legacy implementation.
• Refine hookspec tests to account for the new FutureWarning behavior without breaking existing coverage.

Build:

• Update the pre-commit mypy hook to version 2.0.0.

CI:

• Add a downstream GitHub Actions workflow that runs a matrix of downstream recipe checks under Python 3.12 with uv.

Documentation:

• Document the downstream testing tooling, recipes, and invocation instructions in the downstream README.
• Add changelog fragments for the varnames bug fix and downstream tooling changes.

Tests:

• Add comprehensive tests for varnames behavior with various method types, unconventional parameter names, hookspec patterns, and problematic annotations.
• Add benchmark tests comparing the new varnames implementation against the legacy variant across several callable forms.
• Extend warning tests to validate FutureWarning behavior for hookspecs missing self, with self, and using staticmethod.

[sourcery-ai]

Hey - I've found 1 security issue, and 2 other issues

Security issues:

• Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'. (link)

Prompt for AI Agents

Please address the comments from this code review:

## Individual Comments

### Comment 1
<location path="testing/test_warnings.py" line_range="53-62" />
<code_context>
     assert Path(wc.list[0].filename).name == "test_warnings.py"
+
+
+def test_hookspec_missing_self_warns(pm: PluginManager) -> None:
+    """A hookspec defined as a method without ``self`` emits a FutureWarning."""
+
+    class Api:
+        @hookspec
+        def my_hook(item, extra):
+            pass
+
+    with pytest.warns(
+        FutureWarning,
+        match=r"is a method but its first parameter 'item' is not 'self'",
+    ):
+        pm.add_hookspecs(Api)
+
+
</code_context>
<issue_to_address>
**suggestion (testing):** Also assert that hookspec registration still produces the expected hook argument names when the warning is emitted

This currently only checks that `pm.add_hookspecs(Api)` emits the `FutureWarning`. To also verify compatibility of `legacy_noself` with hook registration, please assert the resulting hook signature, e.g.:

```python
hook = pm.hook.my_hook
assert hook.spec is not None
assert hook.spec.argnames == ("item", "extra")
```

This ensures the warning path doesn’t alter or break the hook’s argument handling.
</issue_to_address>

### Comment 2
<location path="src/pluggy/_hooks.py" line_range="293" />
<code_context>
+_IMPLICIT_NAMES = ("self", "cls", "obj") if _PYPY else ("self", "cls")


-def varnames(func: object) -> tuple[tuple[str, ...], tuple[str, ...]]:
-    """Return tuple of positional and keywrord argument names for a function,
-    method, class or callable.
</code_context>
<issue_to_address>
**issue (complexity):** Consider extracting the callable normalization, argument splitting, and implicit-first-arg stripping from `varnames` into small helpers so the main function reads as a simple orchestration of clear steps.

You can keep all of the new behavior but reduce the cognitive load in `varnames` by extracting a few focused helpers. That lets you centralize the “what is this callable / is it a method / what are the args” logic instead of interleaving it.

### 1. Normalize callable + bound detection

Pull out the class / callable-object / unwrap / `ismethod` handling into a helper:

```python
def _normalize_callable(func: object) -> tuple[types.FunctionType, bool]:
    is_bound = False

    if inspect.isclass(func):
        try:
            func = func.__init__
        except AttributeError:  # pragma: no cover - pypy special case
            return (), (), False  # sentinel; see usage below
        is_bound = True
    elif not inspect.isroutine(func):
        try:
            func = getattr(func, "__call__", func)
        except Exception:  # pragma: no cover - pypy special case
            return (), (), False

    if inspect.ismethod(func):
        is_bound = True
    func = inspect.unwrap(func)  # type: ignore[arg-type]
    if inspect.ismethod(func):
        is_bound = True
        func = func.__func__

    return func, is_bound
```

Then `varnames` starts with:

```python
def varnames(func: object, *, legacy_noself: bool = False) -> tuple[tuple[str, ...], tuple[str, ...]]:
    func, is_bound = _normalize_callable(func)
    if not isinstance(func, types.FunctionType):
        return (), ()
    # continue with code/defaults handling...
```

(You can keep your existing early-return sentinels; the main point is isolating this logic.)

### 2. Split positional vs kw-defaults

Hide the `__code__` / `__defaults__` details:

```python
def _split_positional_and_kwdefaults(
    func: types.FunctionType,
) -> tuple[tuple[str, ...], tuple[str, ...], str]:
    try:
        code: types.CodeType = func.__code__          # type: ignore[attr-defined]
        defaults: tuple[object, ...] | None = func.__defaults__  # type: ignore[attr-defined]
        qualname: str = func.__qualname__            # type: ignore[attr-defined]
    except AttributeError:  # pragma: no cover
        return (), (), ""

    args: tuple[str, ...] = code.co_varnames[: code.co_argcount]

    if defaults:
        index = -len(defaults)
        return args[:index], args[index:], qualname
    return args, (), qualname
```

Then in `varnames`:

```python
    args, kwargs, qualname = _split_positional_and_kwdefaults(func)
    if not qualname:
        return (), ()
```

### 3. Encapsulate “looks like class method” logic

Move the `__qualname__` parsing and implicit-name decision into one place:

```python
def _looks_like_class_method(qualname: str) -> bool:
    tail = qualname.rsplit("<locals>.", maxsplit=1)[-1]
    return "." in tail

def _strip_implicit_first_arg(
    args: tuple[str, ...],
    *,
    is_bound: bool,
    qualname: str,
    legacy_noself: bool,
) -> tuple[str, ...]:
    if not args:
        return args

    if is_bound:
        return args[1:]

    if not _looks_like_class_method(qualname):
        return args

    if args[0] in _IMPLICIT_NAMES:
        return args[1:]

    if legacy_noself:
        warnings.warn(
            f"{qualname} is a method but its first parameter {args[0]!r} is not 'self'."
            " Add 'self' as the first parameter or use @staticmethod."
            " This will become an error in a future version of pluggy.",
            FutureWarning,
            stacklevel=2,
        )
    return args
```

Then `varnames` becomes:

```python
def varnames(func: object, *, legacy_noself: bool = False) -> tuple[tuple[str, ...], tuple[str, ...]]:
    func, is_bound = _normalize_callable(func)
    if not isinstance(func, types.FunctionType):
        return (), ()

    args, kwargs, qualname = _split_positional_and_kwdefaults(func)
    if not qualname:
        return (), ()

    args = _strip_implicit_first_arg(
        args, is_bound=is_bound, qualname=qualname, legacy_noself=legacy_noself
    )
    return args, kwargs
```

This keeps all the behavior you added (PyPy support, structural method detection, `legacy_noself` warning) but centralizes each concern in a small helper with a clear contract. The main `varnames` body becomes a straightforward orchestration of those steps, which should address the “intertwined concerns” and “spread across multiple checks” feedback without changing semantics.
</issue_to_address>

### Comment 3
<location path="downstream/run_downstream.py" line_range="145-150" />
<code_context>
    result = subprocess.run(
        list(argv),
        cwd=cwd,
        env=merged_env,
        check=False,
    )
</code_context>
<issue_to_address>
**security (python.lang.security.audit.dangerous-subprocess-use-audit):** Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.

*Source: opengrep*
</issue_to_address>

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[sourcery-ai]

suggestion (testing): Also assert that hookspec registration still produces the expected hook argument names when the warning is emitted

This currently only checks that pm.add_hookspecs(Api) emits the FutureWarning. To also verify compatibility of legacy_noself with hook registration, please assert the resulting hook signature, e.g.:

hook = pm.hook.my_hook
assert hook.spec is not None
assert hook.spec.argnames == ("item", "extra")

This ensures the warning path doesn’t alter or break the hook’s argument handling.

[sourcery-ai]

issue (complexity): Consider extracting the callable normalization, argument splitting, and implicit-first-arg stripping from varnames into small helpers so the main function reads as a simple orchestration of clear steps.

You can keep all of the new behavior but reduce the cognitive load in varnames by extracting a few focused helpers. That lets you centralize the “what is this callable / is it a method / what are the args” logic instead of interleaving it.

1. Normalize callable + bound detection

Pull out the class / callable-object / unwrap / ismethod handling into a helper:

def _normalize_callable(func: object) -> tuple[types.FunctionType, bool]:
    is_bound = False

    if inspect.isclass(func):
        try:
            func = func.__init__
        except AttributeError:  # pragma: no cover - pypy special case
            return (), (), False  # sentinel; see usage below
        is_bound = True
    elif not inspect.isroutine(func):
        try:
            func = getattr(func, "__call__", func)
        except Exception:  # pragma: no cover - pypy special case
            return (), (), False

    if inspect.ismethod(func):
        is_bound = True
    func = inspect.unwrap(func)  # type: ignore[arg-type]
    if inspect.ismethod(func):
        is_bound = True
        func = func.__func__

    return func, is_bound

Then varnames starts with:

def varnames(func: object, *, legacy_noself: bool = False) -> tuple[tuple[str, ...], tuple[str, ...]]:
    func, is_bound = _normalize_callable(func)
    if not isinstance(func, types.FunctionType):
        return (), ()
    # continue with code/defaults handling...

(You can keep your existing early-return sentinels; the main point is isolating this logic.)

2. Split positional vs kw-defaults

Hide the __code__ / __defaults__ details:

def _split_positional_and_kwdefaults(
    func: types.FunctionType,
) -> tuple[tuple[str, ...], tuple[str, ...], str]:
    try:
        code: types.CodeType = func.__code__          # type: ignore[attr-defined]
        defaults: tuple[object, ...] | None = func.__defaults__  # type: ignore[attr-defined]
        qualname: str = func.__qualname__            # type: ignore[attr-defined]
    except AttributeError:  # pragma: no cover
        return (), (), ""

    args: tuple[str, ...] = code.co_varnames[: code.co_argcount]

    if defaults:
        index = -len(defaults)
        return args[:index], args[index:], qualname
    return args, (), qualname

Then in varnames:

    args, kwargs, qualname = _split_positional_and_kwdefaults(func)
    if not qualname:
        return (), ()

3. Encapsulate “looks like class method” logic

Move the __qualname__ parsing and implicit-name decision into one place:

def _looks_like_class_method(qualname: str) -> bool:
    tail = qualname.rsplit("<locals>.", maxsplit=1)[-1]
    return "." in tail

def _strip_implicit_first_arg(
    args: tuple[str, ...],
    *,
    is_bound: bool,
    qualname: str,
    legacy_noself: bool,
) -> tuple[str, ...]:
    if not args:
        return args

    if is_bound:
        return args[1:]

    if not _looks_like_class_method(qualname):
        return args

    if args[0] in _IMPLICIT_NAMES:
        return args[1:]

    if legacy_noself:
        warnings.warn(
            f"{qualname} is a method but its first parameter {args[0]!r} is not 'self'."
            " Add 'self' as the first parameter or use @staticmethod."
            " This will become an error in a future version of pluggy.",
            FutureWarning,
            stacklevel=2,
        )
    return args

Then varnames becomes:

def varnames(func: object, *, legacy_noself: bool = False) -> tuple[tuple[str, ...], tuple[str, ...]]:
    func, is_bound = _normalize_callable(func)
    if not isinstance(func, types.FunctionType):
        return (), ()

    args, kwargs, qualname = _split_positional_and_kwdefaults(func)
    if not qualname:
        return (), ()

    args = _strip_implicit_first_arg(
        args, is_bound=is_bound, qualname=qualname, legacy_noself=legacy_noself
    )
    return args, kwargs

This keeps all the behavior you added (PyPy support, structural method detection, legacy_noself warning) but centralizes each concern in a small helper with a clear contract. The main varnames body becomes a straightforward orchestration of those steps, which should address the “intertwined concerns” and “spread across multiple checks” feedback without changing semantics.

[sourcery-ai]

security (python.lang.security.audit.dangerous-subprocess-use-audit): Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.

Source: opengrep

[Copilot]

Pull request overview

This PR has two distinct purposes bundled together: (1) the main upstream fix — reimplementing varnames() in pluggy._hooks to read positional argument names directly from __code__/__defaults__/__qualname__ instead of inspect.signature(), so that hookspec/hookimpl registration no longer fails on Python 3.14+ when annotations contain unresolvable forward references; and (2) a substantial overhaul of the downstream/ testing infrastructure, replacing per-project bash scripts with a single PEP 723 driver that consumes TOML "recipes", plus a new GitHub Actions workflow that runs those recipes.

Changes:

• Rewrite varnames() to be annotation-resolution-free, add a new legacy_noself flag with a FutureWarning for hookspec classes whose methods omit self, and add _IMPLICIT_NAMES (now including "cls").
• Replace downstream/*.sh scripts with TOML recipes plus a run_downstream.py driver (pydantic-validated), add a .github/workflows/downstream.yml, and document the new layout in downstream/README.md and RELEASING.rst.
• Add tests covering the new varnames behavior, a _varnames_legacy benchmark for comparison, a doc clarification about hookimpl defaults, and small documentation/spelling fixes plus three changelog entries.

Reviewed changes

Copilot reviewed 30 out of 30 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
src/pluggy/_hooks.py	Core varnames() rewrite; _IMPLICIT_NAMES adds cls; HookSpec.__init__ opts into legacy_noself for non-staticmethod class members.
testing/test_helpers.py	New unit tests for varnames across bound/unbound/classmethod/staticmethod/no-self/unresolvable-annotation cases.
testing/test_warnings.py	New tests asserting FutureWarning (or absence) for hookspec methods without self.
testing/test_hookcaller.py	Adds filterwarnings("ignore::FutureWarning") to a test that triggers the new warning.
testing/benchmark.py	Adds a _varnames_legacy reference impl and parametrized benchmarks comparing new vs old.
downstream/run_downstream.py	New pydantic+tomllib driver that clones a repo and runs a recipe's install/test steps under a uv venv (or delegates to a bash script).
downstream/recipes/*.toml, conda.bash	New TOML recipes plus a conda bootstrap script replacing the old per-project shell scripts.
downstream/*.sh (removed)	Deleted in favor of recipes.
downstream/README.md, RELEASING.rst	Document the new recipe-based workflow and fix a typo.
downstream/.gitignore	Ignore the new python-lsp-server/ clone directory.
.github/workflows/downstream.yml	New CI workflow that runs each recipe in a matrix on PRs whose branch name contains downstream or via manual dispatch.
docs/index.rst	Convert two broken intersphinx links to direct URLs (tox, Kedro) and document that hookimpl args with defaults are not passed.
changelog/186.doc.rst, 522.doc.rst, 629.bugfix.rst	Changelog entries for the doc fixes and the 3.14 hooks-registration fix.
.pre-commit-config.yaml	Bump mypy mirror from v1.20.2 to v2.0.0.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.