Warn from varnames for hookspec methods missing self

Move the missing-self warning into varnames where the ambiguity
actually lives, instead of bolting it onto HookSpec.__init__.

Add a legacy_noself parameter to varnames: when True and the
function looks like a class method but lacks self/cls as its
first parameter, emit a FutureWarning. HookSpec.__init__ passes
legacy_noself=True for class-based non-static hookspecs to
support the legacy pattern while warning about it.

Co-authored-by: Cursor AI <ai@cursor.sh>
Co-authored-by: Anthropic Claude Opus 4 <claude@anthropic.com>

Author: 3 people

src/pluggy/_hooks.py

@@ -288,32 +288,42 @@ def normalize_hookimpl_opts(opts: HookimplOpts) -> None:
     opts.setdefault("specname", None)
 
 
-_PYPY = hasattr(sys, "pypy_version_info")
-# pypy3 uses "obj" instead of "self" for default dunder methods
-_IMPLICIT_NAMES = ("self", "obj") if _PYPY else ("self",)
+_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 keyword parameter 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.
 
-    Keyword-only parameter names are not included.
     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.
-    is_bound = inspect.ismethod(func)
+    if inspect.ismethod(func):
+        is_bound = True
     func = inspect.unwrap(func)  # type: ignore[arg-type]
     if inspect.ismethod(func):
         is_bound = True
@@ -337,10 +347,27 @@ def varnames(func: object) -> tuple[tuple[str, ...], tuple[str, ...]]:
     else:
         kwargs = ()
 
-    # Strip implicit instance arg (self/obj for methods)
-    if args and args[0] in _IMPLICIT_NAMES:
-        if is_bound or "." in qualname:
+    # Strip implicit instance/class arg.
+    # Check if this looks like a method defined in a class by examining the
+    # qualname after the last "<locals>." segment (if any). A remaining dot
+    # means it's a class method (e.g. "MyClass.method" or
+    # "func.<locals>.MyClass.method"), not just a nested function.
+    _tail = qualname.rsplit("<locals>.", maxsplit=1)[-1]
+    _is_class_method = "." in _tail
+    if args:
+        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
 
@@ -698,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")

testing/benchmark.py

@@ -16,11 +16,9 @@
 from pluggy._hooks import varnames
 
 
-_PYPY = hasattr(sys, "pypy_version_info")
-
-
 def _varnames_legacy(func: object) -> tuple[tuple[str, ...], tuple[str, ...]]:
-    """Pre-PEP 649 implementation using inspect.signature for comparison."""
+    """Pre-structural-detection implementation using inspect.signature and
+    name-based heuristics for comparison."""
     if inspect.isclass(func):
         try:
             func = func.__init__
@@ -64,7 +62,8 @@ def _varnames_legacy(func: object) -> tuple[tuple[str, ...], tuple[str, ...]]:
     else:
         kwargs = ()
 
-    if not _PYPY:
+    _pypy = hasattr(sys, "pypy_version_info")
+    if not _pypy:
         implicit_names: tuple[str, ...] = ("self",)
     else:
         implicit_names = ("self", "obj")

testing/test_helpers.py

@@ -112,14 +112,16 @@ 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 ``self``."""
+    varnames must still strip the first parameter."""
 
     def standalone(self, x) -> None:
         pass  # pragma: no cover
@@ -130,6 +132,109 @@ class MyClass:
     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.
 

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()

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)