diff --git a/changelog/629.bugfix.rst b/changelog/629.bugfix.rst new file mode 100644 index 00000000..88c96c1f --- /dev/null +++ b/changelog/629.bugfix.rst @@ -0,0 +1 @@ +Fix hooks failing to register on Python 3.14+ when type annotations use forward references. diff --git a/src/pluggy/_hooks.py b/src/pluggy/_hooks.py index 3d232870..d62dff19 100644 --- a/src/pluggy/_hooks.py +++ b/src/pluggy/_hooks.py @@ -11,6 +11,7 @@ from collections.abc import Set import inspect import sys +import types from types import ModuleType from typing import Any from typing import Final @@ -287,70 +288,86 @@ def normalize_hookimpl_opts(opts: HookimplOpts) -> None: opts.setdefault("specname", None) -_PYPY = hasattr(sys, "pypy_version_info") +_PYPY = sys.implementation.name == "pypy" +_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. +def varnames( + func: object, *, legacy_noself: bool = False +) -> tuple[tuple[str, ...], tuple[str, ...]]: + """Return tuple of positional and keyword parameter names for a callable. In case of a class, its ``__init__`` method is considered. - For methods the ``self`` parameter is not included. + For bound methods, the already-bound first parameter is not included. + For unbound methods with a dotted ``__qualname__``, the first parameter is + stripped only if its name is a known implicit name (``self``, ``cls``). + Keyword-only parameters are not included. + + :param legacy_noself: + If ``True``, support hookspec classes whose methods omit ``self``. + When the function looks like a class method but has no implicit first + parameter, a :class:`FutureWarning` is emitted. """ + is_bound = False if inspect.isclass(func): try: func = func.__init__ except AttributeError: # pragma: no cover - pypy special case return (), () + is_bound = True elif not inspect.isroutine(func): # callable object? try: func = getattr(func, "__call__", func) except Exception: # pragma: no cover - pypy special case return (), () + # Track bound methods before unwrapping, since __func__ loses that info. + 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__ + try: - # func MUST be a function or method here or we won't parse any args. - sig = inspect.signature( - func.__func__ if inspect.ismethod(func) else func # type:ignore[arg-type] - ) - except TypeError: # pragma: no cover + 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 (), () - _valid_param_kinds = ( - inspect.Parameter.POSITIONAL_ONLY, - inspect.Parameter.POSITIONAL_OR_KEYWORD, - ) - _valid_params = { - name: param - for name, param in sig.parameters.items() - if param.kind in _valid_param_kinds - } - args = tuple(_valid_params) - defaults = ( - tuple( - param.default - for param in _valid_params.values() - if param.default is not param.empty - ) - or None - ) + # Get positional argument names (positional-only + positional-or-keyword) + args: tuple[str, ...] = code.co_varnames[: code.co_argcount] + # Determine which args have defaults + kwargs: tuple[str, ...] if defaults: index = -len(defaults) - args, kwargs = args[:index], tuple(args[index:]) + args, kwargs = args[:index], args[index:] else: kwargs = () - # strip any implicit instance arg - # pypy3 uses "obj" instead of "self" for default dunder methods - if not _PYPY: - implicit_names: tuple[str, ...] = ("self",) - else: - implicit_names = ("self", "obj") + # Strip implicit instance/class arg. + # Check if this looks like a method defined in a class by examining the + # qualname after the last "." segment (if any). A remaining dot + # means it's a class method (e.g. "MyClass.method" or + # "func..MyClass.method"), not just a nested function. + _tail = qualname.rsplit(".", maxsplit=1)[-1] + _is_class_method = "." in _tail if args: - qualname: str = getattr(func, "__qualname__", "") - if inspect.ismethod(func) or ("." in qualname and args[0] in implicit_names): + if is_bound: args = args[1:] + elif _is_class_method and args[0] in _IMPLICIT_NAMES: + args = args[1:] + elif _is_class_method and legacy_noself: + warnings.warn( + f"{qualname} is a method but its first parameter" + f" {args[0]!r} is not 'self'." + f" Add 'self' as the first parameter or use @staticmethod." + f" This will become an error in a future version of pluggy.", + FutureWarning, + stacklevel=2, + ) return args, kwargs @@ -708,9 +725,14 @@ class HookSpec: def __init__(self, namespace: _Namespace, name: str, opts: HookspecOpts) -> None: self.namespace = namespace - self.function: Callable[..., object] = getattr(namespace, name) self.name = name - self.argnames, self.kwargnames = varnames(self.function) + self.function: Callable[..., object] = getattr(namespace, name) + legacy_noself = inspect.isclass(namespace) and not isinstance( + inspect.getattr_static(namespace, name), staticmethod + ) + self.argnames, self.kwargnames = varnames( + self.function, legacy_noself=legacy_noself + ) self.opts = opts self.warn_on_impl = opts.get("warn_on_impl") self.warn_on_impl_args = opts.get("warn_on_impl_args") diff --git a/testing/benchmark.py b/testing/benchmark.py index cc3be4eb..81823edd 100644 --- a/testing/benchmark.py +++ b/testing/benchmark.py @@ -2,6 +2,8 @@ Benchmarking and performance tests. """ +import inspect +import sys from typing import Any import pytest @@ -11,6 +13,66 @@ from pluggy import PluginManager from pluggy._callers import _multicall from pluggy._hooks import HookImpl +from pluggy._hooks import varnames + + +def _varnames_legacy(func: object) -> tuple[tuple[str, ...], tuple[str, ...]]: + """Pre-structural-detection implementation using inspect.signature and + name-based heuristics for comparison.""" + if inspect.isclass(func): + try: + func = func.__init__ + except AttributeError: + return (), () + elif not inspect.isroutine(func): + try: + func = getattr(func, "__call__", func) + except Exception: + return (), () + + try: + sig = inspect.signature( + func.__func__ if inspect.ismethod(func) else func # type: ignore[arg-type] + ) + except TypeError: + return (), () + + _valid_param_kinds = ( + inspect.Parameter.POSITIONAL_ONLY, + inspect.Parameter.POSITIONAL_OR_KEYWORD, + ) + _valid_params = { + name: param + for name, param in sig.parameters.items() + if param.kind in _valid_param_kinds + } + args = tuple(_valid_params) + defaults = ( + tuple( + param.default + for param in _valid_params.values() + if param.default is not param.empty + ) + or None + ) + + if defaults: + index = -len(defaults) + args, kwargs = args[:index], tuple(args[index:]) + else: + kwargs = () + + _pypy = hasattr(sys, "pypy_version_info") + if not _pypy: + implicit_names: tuple[str, ...] = ("self",) + else: + implicit_names = ("self", "obj") + if args: + qualname: str = getattr(func, "__qualname__", "") + if inspect.ismethod(func) or ("." in qualname and args[0] in implicit_names): + args = args[1:] + + return args, kwargs hookspec = HookspecMarker("example") @@ -106,3 +168,30 @@ def fun(self): pm.register(PluginWrap(i), name=f"wrap_plug_{i}") benchmark(pm.hook.fun, hooks=pm.hook, nesting=nesting) + + +def _plain_func(x: int, y: str, z: float = 1.0) -> None: + pass + + +class _MethodHolder: + def method(self, x: int, y: str, z: float = 1.0) -> None: + pass + + +_varnames_funcs = [ + pytest.param(_plain_func, id="plain_function"), + pytest.param(_MethodHolder.method, id="unbound_method"), + pytest.param(_MethodHolder().method, id="bound_method"), + pytest.param(_MethodHolder, id="class"), +] + + +@pytest.mark.parametrize("func", _varnames_funcs) +def test_varnames(benchmark, func: object) -> None: + benchmark(varnames, func) + + +@pytest.mark.parametrize("func", _varnames_funcs) +def test_varnames_legacy(benchmark, func: object) -> None: + benchmark(_varnames_legacy, func) diff --git a/testing/test_helpers.py b/testing/test_helpers.py index a08e3d7a..c0d64b3e 100644 --- a/testing/test_helpers.py +++ b/testing/test_helpers.py @@ -112,5 +112,141 @@ def example_method(self, x, y=1) -> None: ex_inst = Example() assert varnames(example) == (("a",), ("b",)) + # Unbound: self is stripped because it's in _IMPLICIT_NAMES and qualname is dotted. assert varnames(Example.example_method) == (("x",), ("y",)) + # Bound: self is already consumed. assert varnames(ex_inst.example_method) == (("x",), ("y",)) + + +def test_varnames_bound_method_from_module_function() -> None: + """A module-level function assigned to a class attribute becomes a bound + method when accessed on an instance, but its __qualname__ has no dot. + varnames must still strip the first parameter.""" + + def standalone(self, x) -> None: + pass # pragma: no cover + + class MyClass: + method = standalone + + assert varnames(MyClass().method) == (("x",), ()) + + +def test_varnames_unconventional_first_param_name() -> None: + """Bound methods strip unconditionally, but unbound methods with + non-standard first parameter names preserve all arguments.""" + + class MyClass: + def method(this, x) -> None: + pass # pragma: no cover + + # Bound: stripped regardless of name. + assert varnames(MyClass().method) == (("x",), ()) + # Unbound with dotted qualname but non-implicit name: NOT stripped. + assert varnames(MyClass.method) == (("this", "x"), ()) + + +def test_varnames_classmethod() -> None: + class MyClass: + @classmethod + def cm(cls, x, y=1) -> None: + pass # pragma: no cover + + # Classmethods are always bound (even from the class). + assert varnames(MyClass.cm) == (("x",), ("y",)) + assert varnames(MyClass().cm) == (("x",), ("y",)) + + +def test_varnames_staticmethod() -> None: + class MyClass: + @staticmethod + def sm(x, y=1) -> None: + pass # pragma: no cover + + # Staticmethods have no implicit first arg. + assert varnames(MyClass.sm) == (("x",), ("y",)) + assert varnames(MyClass().sm) == (("x",), ("y",)) + + +def test_varnames_hookspec_without_self() -> None: + """Hookspec-style class methods without self/cls preserve all parameters. + + This is the convention used by projects like pytest-timeout where hookspec + classes define methods without ``self`` since they serve as pure signatures. + By default varnames does not warn; the warning is emitted when + ``legacy_noself=True`` is passed (as HookSpec.__init__ does). + """ + + class MySpecs: + def my_hook(item, extra) -> None: + pass # pragma: no cover + + # Accessed as unbound: first arg is not an implicit name, keep it. + assert varnames(MySpecs.my_hook) == (("item", "extra"), ()) + # Accessed as bound (via instance): first arg is stripped. + assert varnames(MySpecs().my_hook) == (("extra",), ()) + + +def test_varnames_legacy_noself_warns() -> None: + """With ``legacy_noself=True``, varnames warns when it encounters a + class method whose first parameter is not an implicit name.""" + import warnings + + class MySpecs: + def my_hook(item, extra) -> None: + pass # pragma: no cover + + with warnings.catch_warnings(record=True) as w: + warnings.simplefilter("always") + result = varnames(MySpecs.my_hook, legacy_noself=True) + assert result == (("item", "extra"), ()) + assert len(w) == 1 + assert issubclass(w[0].category, FutureWarning) + assert "'item' is not 'self'" in str(w[0].message) + + +def test_varnames_legacy_noself_no_warn_with_self() -> None: + """With ``legacy_noself=True``, no warning when the method has ``self``.""" + import warnings + + class MySpecs: + def my_hook(self, item, extra) -> None: + pass # pragma: no cover + + with warnings.catch_warnings(record=True) as w: + warnings.simplefilter("always") + result = varnames(MySpecs.my_hook, legacy_noself=True) + assert result == (("item", "extra"), ()) + assert len(w) == 0 + + +def test_varnames_no_legacy_noself_no_warn() -> None: + """Without ``legacy_noself``, no warning even for class methods without self.""" + import warnings + + class MySpecs: + def my_hook(item, extra) -> None: + pass # pragma: no cover + + with warnings.catch_warnings(record=True) as w: + warnings.simplefilter("always") + result = varnames(MySpecs.my_hook) + assert result == (("item", "extra"), ()) + assert len(w) == 0 + + +def test_varnames_unresolvable_annotation() -> None: + """Test that varnames works with annotations that cannot be resolved. + + In Python 3.14+, inspect.signature() tries to resolve string annotations + by default, which can fail if the annotation refers to a type that isn't + importable. Using __code__ directly avoids this issue. + """ + + def func_with_bad_annotation( + x: "NonExistentType", # type: ignore[name-defined] # noqa: F821 + y, + ) -> None: + pass + + assert varnames(func_with_bad_annotation) == (("x", "y"), ()) diff --git a/testing/test_hookcaller.py b/testing/test_hookcaller.py index f9855814..36dea7b7 100644 --- a/testing/test_hookcaller.py +++ b/testing/test_hookcaller.py @@ -301,6 +301,7 @@ def m13() -> None: ... ] +@pytest.mark.filterwarnings("ignore::FutureWarning") def test_hookspec(pm: PluginManager) -> None: class HookSpec: @hookspec() diff --git a/testing/test_warnings.py b/testing/test_warnings.py index 4f5454be..e14e829f 100644 --- a/testing/test_warnings.py +++ b/testing/test_warnings.py @@ -1,4 +1,5 @@ from pathlib import Path +import warnings import pytest @@ -47,3 +48,45 @@ def my_hook(self): pm.hook.my_hook() assert len(wc.list) == 1 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) + + +def test_hookspec_with_self_no_warning(pm: PluginManager) -> None: + """A hookspec with ``self`` does not emit a FutureWarning.""" + + class Api: + @hookspec + def my_hook(self, item, extra): + pass + + with warnings.catch_warnings(): + warnings.simplefilter("error") + pm.add_hookspecs(Api) + + +def test_hookspec_staticmethod_no_warning(pm: PluginManager) -> None: + """A hookspec using @staticmethod does not emit a FutureWarning.""" + + class Api: + @staticmethod + @hookspec + def my_hook(item, extra) -> None: + pass + + with warnings.catch_warnings(): + warnings.simplefilter("error") + pm.add_hookspecs(Api)