From cbf58d5c37c8cabff682282665df8b4424cc7fc7 Mon Sep 17 00:00:00 2001 From: Nick Sweeting Date: Thu, 26 Sep 2024 20:39:13 -0700 Subject: [PATCH 01/11] Avoid triggering property methods when inspecting plugin attribute signatures --- src/pluggy/_manager.py | 14 ++++++++++++++ testing/test_pluginmanager.py | 32 ++++++++++++++++++++++++++++++++ 2 files changed, 46 insertions(+) diff --git a/src/pluggy/_manager.py b/src/pluggy/_manager.py index 325388a8..d7c019a9 100644 --- a/src/pluggy/_manager.py +++ b/src/pluggy/_manager.py @@ -169,6 +169,20 @@ def parse_hookimpl_opts(self, plugin: _Plugin, name: str) -> HookimplOpts | None customize how hook implementation are picked up. By default, returns the options for items decorated with :class:`HookimplMarker`. """ + + # IMPORTANT: @property methods can have side effects, and are never hookimpl + # if attr is a property, skip it in advance + plugin_class = plugin if inspect.isclass(plugin) else type(plugin) + if isinstance(getattr(plugin_class, name, None), property): + return None + + # pydantic model fields are like attrs and also can never be hookimpls + plugin_is_pydantic_obj = hasattr(plugin, "__pydantic_core_schema__") + if plugin_is_pydantic_obj and name in getattr(plugin, "model_fields", {}): + # pydantic models mess with the class and attr __signature__ + # so inspect.isroutine(...) throws exceptions and cant be used + return None + method: object = getattr(plugin, name) if not inspect.isroutine(method): return None diff --git a/testing/test_pluginmanager.py b/testing/test_pluginmanager.py index dd395950..1b9ecbd0 100644 --- a/testing/test_pluginmanager.py +++ b/testing/test_pluginmanager.py @@ -4,6 +4,8 @@ import importlib.metadata from typing import Any +from typing import Dict +from typing import List from typing import cast import pytest @@ -125,6 +127,36 @@ class A: assert pm.register(A(), "somename") +def test_register_skips_properties(he_pm: PluginManager) -> None: + class ClassWithProperties: + property_was_executed: bool = False + + @property + def some_func(self): + self.property_was_executed = True + return None + + test_plugin = ClassWithProperties() + he_pm.register(test_plugin) + assert not test_plugin.property_was_executed + + +def test_register_skips_pydantic_fields(he_pm: PluginManager) -> None: + class PydanticModelClass: + # stub to make object look like a pydantic model + model_fields: Dict[str, bool] = {"some_attr": True} + + def __pydantic_core_schema__(self): ... + + @hookimpl + def some_attr(self): ... + + test_plugin = PydanticModelClass() + he_pm.register(test_plugin) + with pytest.raises(AttributeError): + he_pm.hook.some_attr.get_hookimpls() + + def test_register_mismatch_method(he_pm: PluginManager) -> None: class hello: @hookimpl From 6dcbcbce42e3a7304d16c83c7d6a1fc041da8da4 Mon Sep 17 00:00:00 2001 From: Nick Sweeting Date: Thu, 26 Sep 2024 23:01:53 -0700 Subject: [PATCH 02/11] Add exception handler to deal with proxy object attrs as well --- src/pluggy/_manager.py | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/src/pluggy/_manager.py b/src/pluggy/_manager.py index d7c019a9..53cfc0b4 100644 --- a/src/pluggy/_manager.py +++ b/src/pluggy/_manager.py @@ -179,11 +179,16 @@ def parse_hookimpl_opts(self, plugin: _Plugin, name: str) -> HookimplOpts | None # pydantic model fields are like attrs and also can never be hookimpls plugin_is_pydantic_obj = hasattr(plugin, "__pydantic_core_schema__") if plugin_is_pydantic_obj and name in getattr(plugin, "model_fields", {}): - # pydantic models mess with the class and attr __signature__ - # so inspect.isroutine(...) throws exceptions and cant be used return None - method: object = getattr(plugin, name) + method: object + try: + method = getattr(plugin, name) + except AttributeError: + # AttributeError: '__signature__' attribute of 'Plugin' is class-only + # can happen for some special objects (e.g. proxies, pydantic, etc.) + method = getattr(type(plugin), name) # use class sig instead + if not inspect.isroutine(method): return None try: From b2d1c4c5ea423ac629fefa9c45ffe052f7b8d08e Mon Sep 17 00:00:00 2001 From: Nick Sweeting Date: Tue, 1 Oct 2024 13:17:55 -0700 Subject: [PATCH 03/11] remove pydantic-specific logic and support hookspecs too MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit updates: - [github.com/astral-sh/ruff-pre-commit: v0.6.7 → v0.6.8](https://github.com/astral-sh/ruff-pre-commit/compare/v0.6.7...v0.6.8) Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- src/pluggy/_manager.py | 38 ++++++++++++++++++++++++----------- testing/test_pluginmanager.py | 23 +++------------------ 2 files changed, 29 insertions(+), 32 deletions(-) diff --git a/src/pluggy/_manager.py b/src/pluggy/_manager.py index 53cfc0b4..f46b5a4d 100644 --- a/src/pluggy/_manager.py +++ b/src/pluggy/_manager.py @@ -50,6 +50,17 @@ def _warn_for_function(warning: Warning, function: Callable[..., object]) -> Non ) +def _attr_is_property(obj: Any, name: str) -> bool: + """Check if a given attr is a @property on a module, class, or object""" + if inspect.ismodule(obj): + return False # modules can never have @property methods + + base_class = obj if inspect.isclass(obj) else type(obj) + if isinstance(getattr(base_class, name, None), property): + return True + return False + + class PluginValidationError(Exception): """Plugin failed validation. @@ -170,23 +181,16 @@ def parse_hookimpl_opts(self, plugin: _Plugin, name: str) -> HookimplOpts | None options for items decorated with :class:`HookimplMarker`. """ - # IMPORTANT: @property methods can have side effects, and are never hookimpl - # if attr is a property, skip it in advance - plugin_class = plugin if inspect.isclass(plugin) else type(plugin) - if isinstance(getattr(plugin_class, name, None), property): - return None - - # pydantic model fields are like attrs and also can never be hookimpls - plugin_is_pydantic_obj = hasattr(plugin, "__pydantic_core_schema__") - if plugin_is_pydantic_obj and name in getattr(plugin, "model_fields", {}): + if _attr_is_property(plugin, name): + # @property methods can have side effects, and are never hookimpls return None method: object try: method = getattr(plugin, name) except AttributeError: - # AttributeError: '__signature__' attribute of 'Plugin' is class-only - # can happen for some special objects (e.g. proxies, pydantic, etc.) + # AttributeError: '__signature__' attribute of 'plugin' is class-only + # can happen if plugin is a proxy object wrapping a class/module method = getattr(type(plugin), name) # use class sig instead if not inspect.isroutine(method): @@ -293,7 +297,17 @@ def parse_hookspec_opts( customize how hook specifications are picked up. By default, returns the options for items decorated with :class:`HookspecMarker`. """ - method = getattr(module_or_class, name) + if _attr_is_property(module_or_class, name): + # @property methods can have side effects, and are never hookspecs + return None + + method: object + try: + method = getattr(module_or_class, name) + except AttributeError: + # AttributeError: '__signature__' attribute of is class-only + # can happen if module_or_class is a proxy obj wrapping a class/module + method = getattr(type(module_or_class), name) # use class sig instead opts: HookspecOpts | None = getattr(method, self.project_name + "_spec", None) return opts diff --git a/testing/test_pluginmanager.py b/testing/test_pluginmanager.py index 1b9ecbd0..a6d4c7d7 100644 --- a/testing/test_pluginmanager.py +++ b/testing/test_pluginmanager.py @@ -4,7 +4,6 @@ import importlib.metadata from typing import Any -from typing import Dict from typing import List from typing import cast @@ -127,36 +126,20 @@ class A: assert pm.register(A(), "somename") -def test_register_skips_properties(he_pm: PluginManager) -> None: +def test_register_ignores_properties(he_pm: PluginManager) -> None: class ClassWithProperties: property_was_executed: bool = False @property def some_func(self): - self.property_was_executed = True - return None + self.property_was_executed = True # pragma: no cover + return None # pragma: no cover test_plugin = ClassWithProperties() he_pm.register(test_plugin) assert not test_plugin.property_was_executed -def test_register_skips_pydantic_fields(he_pm: PluginManager) -> None: - class PydanticModelClass: - # stub to make object look like a pydantic model - model_fields: Dict[str, bool] = {"some_attr": True} - - def __pydantic_core_schema__(self): ... - - @hookimpl - def some_attr(self): ... - - test_plugin = PydanticModelClass() - he_pm.register(test_plugin) - with pytest.raises(AttributeError): - he_pm.hook.some_attr.get_hookimpls() - - def test_register_mismatch_method(he_pm: PluginManager) -> None: class hello: @hookimpl From 4503678bc0041548b447dc1182f948d0bd2d7742 Mon Sep 17 00:00:00 2001 From: Nick Sweeting Date: Tue, 1 Oct 2024 13:41:35 -0700 Subject: [PATCH 04/11] explicitly test that properties are ignored on both classes and instances --- testing/test_pluginmanager.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/testing/test_pluginmanager.py b/testing/test_pluginmanager.py index a6d4c7d7..e58206d3 100644 --- a/testing/test_pluginmanager.py +++ b/testing/test_pluginmanager.py @@ -135,6 +135,9 @@ def some_func(self): self.property_was_executed = True # pragma: no cover return None # pragma: no cover + # test registering it as a class + he_pm.register(ClassWithProperties) + # test registering it as an instance test_plugin = ClassWithProperties() he_pm.register(test_plugin) assert not test_plugin.property_was_executed From c2899cceea15a72425123e6af44c23f2e303c5d0 Mon Sep 17 00:00:00 2001 From: Nick Sweeting Date: Thu, 3 Oct 2024 10:49:43 -0400 Subject: [PATCH 05/11] swap _attr_is_property arg Any type to object type Co-authored-by: Ran Benita --- src/pluggy/_manager.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/pluggy/_manager.py b/src/pluggy/_manager.py index f46b5a4d..7b0d969e 100644 --- a/src/pluggy/_manager.py +++ b/src/pluggy/_manager.py @@ -50,7 +50,7 @@ def _warn_for_function(warning: Warning, function: Callable[..., object]) -> Non ) -def _attr_is_property(obj: Any, name: str) -> bool: +def _attr_is_property(obj: object, name: str) -> bool: """Check if a given attr is a @property on a module, class, or object""" if inspect.ismodule(obj): return False # modules can never have @property methods From b2e3e23f72b62bc9fe277c458253b552208a784a Mon Sep 17 00:00:00 2001 From: Nick Sweeting Date: Thu, 24 Oct 2024 17:44:31 -0400 Subject: [PATCH 06/11] Return None instead of falling back to getattr(type(plugin)) --- src/pluggy/_manager.py | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/pluggy/_manager.py b/src/pluggy/_manager.py index 7b0d969e..856bf728 100644 --- a/src/pluggy/_manager.py +++ b/src/pluggy/_manager.py @@ -190,8 +190,9 @@ def parse_hookimpl_opts(self, plugin: _Plugin, name: str) -> HookimplOpts | None method = getattr(plugin, name) except AttributeError: # AttributeError: '__signature__' attribute of 'plugin' is class-only - # can happen if plugin is a proxy object wrapping a class/module - method = getattr(type(plugin), name) # use class sig instead + # can be raised when trying to access some descriptor/proxied fields + # https://github.com/pytest-dev/pluggy/pull/536#discussion_r1786431032 + return None if not inspect.isroutine(method): return None @@ -306,8 +307,9 @@ def parse_hookspec_opts( method = getattr(module_or_class, name) except AttributeError: # AttributeError: '__signature__' attribute of is class-only - # can happen if module_or_class is a proxy obj wrapping a class/module - method = getattr(type(module_or_class), name) # use class sig instead + # can be raised when trying to access some descriptor/proxied fields + # https://github.com/pytest-dev/pluggy/pull/536#discussion_r1786431032 + return None opts: HookspecOpts | None = getattr(method, self.project_name + "_spec", None) return opts From 09676eef593ee970fbb11ce0e2e7c35ee9d82b88 Mon Sep 17 00:00:00 2001 From: Ronny Pfannschmidt Date: Tue, 21 Jul 2026 09:59:09 +0200 Subject: [PATCH 07/11] Address review: soften AttributeError comments and add descriptor test Document the generic descriptor/proxy AttributeError skip and cover it with a minimal raising-descriptor plugin so the path is tested without pydantic. Co-authored-by: Cursor AI Co-authored-by: Cursor Grok 4.5 --- src/pluggy/_manager.py | 8 ++------ testing/test_pluginmanager.py | 27 +++++++++++++++++++++++++-- 2 files changed, 27 insertions(+), 8 deletions(-) diff --git a/src/pluggy/_manager.py b/src/pluggy/_manager.py index 856bf728..0d09f669 100644 --- a/src/pluggy/_manager.py +++ b/src/pluggy/_manager.py @@ -189,9 +189,7 @@ def parse_hookimpl_opts(self, plugin: _Plugin, name: str) -> HookimplOpts | None try: method = getattr(plugin, name) except AttributeError: - # AttributeError: '__signature__' attribute of 'plugin' is class-only - # can be raised when trying to access some descriptor/proxied fields - # https://github.com/pytest-dev/pluggy/pull/536#discussion_r1786431032 + # May be raised when trying to access some descriptor/proxied fields. return None if not inspect.isroutine(method): @@ -306,9 +304,7 @@ def parse_hookspec_opts( try: method = getattr(module_or_class, name) except AttributeError: - # AttributeError: '__signature__' attribute of is class-only - # can be raised when trying to access some descriptor/proxied fields - # https://github.com/pytest-dev/pluggy/pull/536#discussion_r1786431032 + # May be raised when trying to access some descriptor/proxied fields. return None opts: HookspecOpts | None = getattr(method, self.project_name + "_spec", None) return opts diff --git a/testing/test_pluginmanager.py b/testing/test_pluginmanager.py index e58206d3..57901a5f 100644 --- a/testing/test_pluginmanager.py +++ b/testing/test_pluginmanager.py @@ -135,14 +135,37 @@ def some_func(self): self.property_was_executed = True # pragma: no cover return None # pragma: no cover - # test registering it as a class + # Registering the class is harmless (getattr returns the property object). he_pm.register(ClassWithProperties) - # test registering it as an instance + # Registering an instance must not evaluate the property getter. test_plugin = ClassWithProperties() he_pm.register(test_plugin) assert not test_plugin.property_was_executed +def test_register_ignores_raising_descriptors(he_pm: PluginManager) -> None: + """Names in dir() whose getattr raises AttributeError are skipped.""" + + class RaisingDescriptor: + def __get__(self, obj: object, owner: type | None = None) -> object: + raise AttributeError("descriptor access failed") + + class PluginWithRaisingDescriptor: + weird_attr = RaisingDescriptor() + + @hookimpl + def he_method1(self, arg: object) -> list[object]: + return [arg] + + plugin = PluginWithRaisingDescriptor() + assert "weird_attr" in dir(plugin) + with pytest.raises(AttributeError): + getattr(plugin, "weird_attr") + + he_pm.register(plugin) + assert he_pm.hook.he_method1(arg=1) == [[1]] + + def test_register_mismatch_method(he_pm: PluginManager) -> None: class hello: @hookimpl From c1c39fee9484b806271e74d7024a7ecb53c468cf Mon Sep 17 00:00:00 2001 From: Ronny Pfannschmidt Date: Tue, 21 Jul 2026 10:26:43 +0200 Subject: [PATCH 08/11] Discover hooks via inspect.getattr_static instead of getattr Only treat functions, classmethods, staticmethods, and bound methods as hookable, so properties and other descriptors are never executed during registration. Also discovers @hookimpl applied above classmethod/staticmethod. Co-authored-by: Cursor AI Co-authored-by: Cursor Grok 4.5 --- src/pluggy/_manager.py | 117 +++++++++++++++++++++------------- testing/test_pluginmanager.py | 45 ++++++++++++- 2 files changed, 117 insertions(+), 45 deletions(-) diff --git a/src/pluggy/_manager.py b/src/pluggy/_manager.py index 0d09f669..32c3ff3d 100644 --- a/src/pluggy/_manager.py +++ b/src/pluggy/_manager.py @@ -15,7 +15,6 @@ from . import _tracing from ._callers import _multicall -from ._hooks import _HookImplFunction from ._hooks import _Namespace from ._hooks import _Plugin from ._hooks import _SubsetHookCaller @@ -50,15 +49,58 @@ def _warn_for_function(warning: Warning, function: Callable[..., object]) -> Non ) -def _attr_is_property(obj: object, name: str) -> bool: - """Check if a given attr is a @property on a module, class, or object""" - if inspect.ismodule(obj): - return False # modules can never have @property methods +def _get_hookable(obj: object, name: str) -> Callable[..., object] | None: + """Return a hookable callable for ``name`` without triggering descriptors. - base_class = obj if inspect.isclass(obj) else type(obj) - if isinstance(getattr(base_class, name, None), property): - return True - return False + Only plain functions, :class:`classmethod`, :class:`staticmethod`, and + already-bound :class:`~types.MethodType` objects are supported. Properties, + ``cached_property``, and other descriptors are intentionally skipped -- + hooks provided via such descriptors (or only via ``__getattr__``) are not + supported. + """ + static: object = inspect.getattr_static(obj, name, None) + if isinstance(static, staticmethod): + return static.__func__ + if isinstance(static, classmethod): + owner = obj if inspect.isclass(obj) else type(obj) + return cast(Callable[..., object], static.__get__(owner, owner)) + if isinstance(static, types.MethodType): + return static + if inspect.isfunction(static): + if inspect.isclass(obj) or inspect.ismodule(obj): + return static + return static.__get__(obj, type(obj)) + return None + + +def _hook_marker_holder(obj: object, name: str) -> object | None: + """Return the object that may carry hookimpl/hookspec marker options. + + Marker attributes may live on a ``classmethod``/``staticmethod`` wrapper + (``@hookimpl`` applied above them) or on the underlying function + (``@hookimpl`` applied below them). + """ + static: object = inspect.getattr_static(obj, name, None) + if isinstance(static, (classmethod, staticmethod)): + return static + if isinstance(static, types.MethodType): + return static.__func__ + if inspect.isfunction(static): + return static + return None + + +def _get_marker_opts(holder: object, attrname: str) -> dict[str, Any] | None: + """Read marker options from ``holder``, falling back to ``__func__``.""" + opts = getattr(holder, attrname, None) + if opts is not None: + return opts if isinstance(opts, dict) else None + func = getattr(holder, "__func__", None) + if func is not None: + opts = getattr(func, attrname, None) + if opts is not None: + return opts if isinstance(opts, dict) else None + return None class PluginValidationError(Exception): @@ -156,7 +198,8 @@ def register(self, plugin: _Plugin, name: str | None = None) -> str | None: hookimpl_opts = self.parse_hookimpl_opts(plugin, attr_name) if hookimpl_opts is not None: normalize_hookimpl_opts(hookimpl_opts) - method: _HookImplFunction[object] = getattr(plugin, attr_name) + method = _get_hookable(plugin, attr_name) + assert method is not None hookimpl = HookImpl(plugin, plugin_name, method, hookimpl_opts) hook_name = hookimpl_opts.get("specname") or attr_name hook: HookCaller | None = getattr(self.hook, hook_name, None) @@ -179,31 +222,18 @@ def parse_hookimpl_opts(self, plugin: _Plugin, name: str) -> HookimplOpts | None This method can be overridden by ``PluginManager`` subclasses to customize how hook implementation are picked up. By default, returns the options for items decorated with :class:`HookimplMarker`. - """ - - if _attr_is_property(plugin, name): - # @property methods can have side effects, and are never hookimpls - return None - - method: object - try: - method = getattr(plugin, name) - except AttributeError: - # May be raised when trying to access some descriptor/proxied fields. - return None - if not inspect.isroutine(method): + Discovery uses :func:`inspect.getattr_static` so properties and other + descriptors are not executed. Only functions, classmethods, + staticmethods, and bound methods are considered. + """ + holder = _hook_marker_holder(plugin, name) + if holder is None: return None - try: - res: HookimplOpts | None = getattr( - method, self.project_name + "_impl", None - ) - except Exception: # pragma: no cover - res = {} # type: ignore[assignment] #pragma: no cover - if res is not None and not isinstance(res, dict): - # false positive - res = None # type:ignore[unreachable] #pragma: no cover - return res + return cast( + HookimplOpts | None, + _get_marker_opts(holder, self.project_name + "_impl"), + ) def unregister( self, plugin: _Plugin | None = None, name: str | None = None @@ -295,19 +325,18 @@ def parse_hookspec_opts( This method can be overridden by ``PluginManager`` subclasses to customize how hook specifications are picked up. By default, returns the options for items decorated with :class:`HookspecMarker`. - """ - if _attr_is_property(module_or_class, name): - # @property methods can have side effects, and are never hookspecs - return None - method: object - try: - method = getattr(module_or_class, name) - except AttributeError: - # May be raised when trying to access some descriptor/proxied fields. + Discovery uses :func:`inspect.getattr_static` so properties and other + descriptors are not executed. Only functions, classmethods, + staticmethods, and bound methods are considered. + """ + holder = _hook_marker_holder(module_or_class, name) + if holder is None: return None - opts: HookspecOpts | None = getattr(method, self.project_name + "_spec", None) - return opts + return cast( + HookspecOpts | None, + _get_marker_opts(holder, self.project_name + "_spec"), + ) def get_plugins(self) -> set[Any]: """Return a set of all registered plugin objects.""" diff --git a/testing/test_pluginmanager.py b/testing/test_pluginmanager.py index 57901a5f..652c665f 100644 --- a/testing/test_pluginmanager.py +++ b/testing/test_pluginmanager.py @@ -143,8 +143,25 @@ def some_func(self): assert not test_plugin.property_was_executed +def test_register_ignores_cached_property(he_pm: PluginManager) -> None: + from functools import cached_property + + class ClassWithCachedProperty: + cached_was_executed: bool = False + + @cached_property + def some_func(self) -> None: + self.cached_was_executed = True # pragma: no cover + return None # pragma: no cover + + test_plugin = ClassWithCachedProperty() + he_pm.register(test_plugin) + assert not test_plugin.cached_was_executed + assert "some_func" not in test_plugin.__dict__ + + def test_register_ignores_raising_descriptors(he_pm: PluginManager) -> None: - """Names in dir() whose getattr raises AttributeError are skipped.""" + """Descriptor attrs are skipped without accessing them via getattr.""" class RaisingDescriptor: def __get__(self, obj: object, owner: type | None = None) -> object: @@ -166,6 +183,32 @@ def he_method1(self, arg: object) -> list[object]: assert he_pm.hook.he_method1(arg=1) == [[1]] +def test_register_hookimpl_above_classmethod(he_pm: PluginManager) -> None: + """@hookimpl applied above @classmethod is discoverable via static lookup.""" + + class Plugin: + @hookimpl + @classmethod + def he_method1(cls, arg: object) -> list[object]: + return [arg] + + he_pm.register(Plugin()) + assert he_pm.hook.he_method1(arg=1) == [[1]] + + +def test_register_hookimpl_above_staticmethod(he_pm: PluginManager) -> None: + """@hookimpl applied above @staticmethod is discoverable via static lookup.""" + + class Plugin: + @hookimpl + @staticmethod + def he_method1(arg: object) -> list[object]: + return [arg] + + he_pm.register(Plugin()) + assert he_pm.hook.he_method1(arg=1) == [[1]] + + def test_register_mismatch_method(he_pm: PluginManager) -> None: class hello: @hookimpl From 679a27e317f61b0907ed55d6771abf2a0290c2cb Mon Sep 17 00:00:00 2001 From: Ronny Pfannschmidt Date: Tue, 21 Jul 2026 10:31:29 +0200 Subject: [PATCH 09/11] Add changelog fragment for getattr_static hook discovery Co-authored-by: Cursor AI Co-authored-by: Cursor Grok 4.5 --- changelog/701.bugfix.rst | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog/701.bugfix.rst diff --git a/changelog/701.bugfix.rst b/changelog/701.bugfix.rst new file mode 100644 index 00000000..d2adb05e --- /dev/null +++ b/changelog/701.bugfix.rst @@ -0,0 +1 @@ +Discover hookimpls/hookspecs via :func:`inspect.getattr_static` so properties and other descriptors are not executed during plugin registration. From 21186ba2b7146ef20b1c2f8fe95d4cd138235da3 Mon Sep 17 00:00:00 2001 From: Ronny Pfannschmidt Date: Sat, 12 Sep 2026 16:29:45 +0200 Subject: [PATCH 10/11] lint: adapt tests to newer ruff after rebase onto main ruff-check autofixes (unused List import, redundant return None) plus B018 silenced the same way as 095da32. Co-Authored-By: Claude Fable 5.1 Co-Authored-By: Claude Code --- testing/test_pluginmanager.py | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/testing/test_pluginmanager.py b/testing/test_pluginmanager.py index 652c665f..155d97af 100644 --- a/testing/test_pluginmanager.py +++ b/testing/test_pluginmanager.py @@ -4,7 +4,6 @@ import importlib.metadata from typing import Any -from typing import List from typing import cast import pytest @@ -133,7 +132,6 @@ class ClassWithProperties: @property def some_func(self): self.property_was_executed = True # pragma: no cover - return None # pragma: no cover # Registering the class is harmless (getattr returns the property object). he_pm.register(ClassWithProperties) @@ -152,7 +150,6 @@ class ClassWithCachedProperty: @cached_property def some_func(self) -> None: self.cached_was_executed = True # pragma: no cover - return None # pragma: no cover test_plugin = ClassWithCachedProperty() he_pm.register(test_plugin) @@ -177,7 +174,7 @@ def he_method1(self, arg: object) -> list[object]: plugin = PluginWithRaisingDescriptor() assert "weird_attr" in dir(plugin) with pytest.raises(AttributeError): - getattr(plugin, "weird_attr") + _ = plugin.weird_attr he_pm.register(plugin) assert he_pm.hook.he_method1(arg=1) == [[1]] From aa6e1f43a240b4aea292c576994d214d5aa1d4ff Mon Sep 17 00:00:00 2001 From: Ronny Pfannschmidt Date: Sat, 12 Sep 2026 20:40:44 +0200 Subject: [PATCH 11/11] Do not bind callables stored on the plugin instance getattr() hands out values found in an instance __dict__ or a __slots__ slot as they are, without running the descriptor protocol. The getattr_static rewrite bound them anyway, so a hookimpl assigned in __init__ was called with the plugin as its first argument, and a hookimpl in a slot was dropped entirely because getattr_static returns the member descriptor rather than the stored function. Both regressions were silent: the first produced a wrong result, the second no hook call at all. _get_hookable and _hook_marker_holder duplicated the same type dispatch and could disagree; _get_hookable returned None on a path that was unreachable behind an assert. They are now one _static_hook_attr() returning both the marker holder and the callable, so the two can no longer drift and every branch is reachable. Also exercise the hook bodies in test_get_hookcallers_no_duplicates, which were the only uncovered lines left in the test suite. Co-Authored-By: Claude Opus 5 Co-Authored-By: Claude Code --- changelog/701.bugfix.rst | 10 ++- src/pluggy/_manager.py | 102 ++++++++++++++++---------- testing/test_pluginmanager.py | 133 ++++++++++++++++++++++++++++++++++ 3 files changed, 207 insertions(+), 38 deletions(-) diff --git a/changelog/701.bugfix.rst b/changelog/701.bugfix.rst index d2adb05e..c612073d 100644 --- a/changelog/701.bugfix.rst +++ b/changelog/701.bugfix.rst @@ -1 +1,9 @@ -Discover hookimpls/hookspecs via :func:`inspect.getattr_static` so properties and other descriptors are not executed during plugin registration. +Hookimpls and hookspecs are now discovered via :func:`inspect.getattr_static`, so +properties, ``cached_property`` and other descriptors are no longer executed while +scanning a plugin. Hooks declared through such a descriptor are not supported. + +As a side effect, ``@hookimpl``/``@hookspec`` applied *above* ``@classmethod`` or +``@staticmethod`` is now picked up; previously the marker sat on the wrapper where +``getattr()`` never saw it. Callables stored directly on a plugin instance -- in its +``__dict__`` or in a ``__slots__`` slot -- keep being called exactly as ``getattr()`` +returned them, i.e. without an implicit ``self``. diff --git a/src/pluggy/_manager.py b/src/pluggy/_manager.py index 32c3ff3d..b265aecc 100644 --- a/src/pluggy/_manager.py +++ b/src/pluggy/_manager.py @@ -15,6 +15,7 @@ from . import _tracing from ._callers import _multicall +from ._hooks import _HookImplFunction from ._hooks import _Namespace from ._hooks import _Plugin from ._hooks import _SubsetHookCaller @@ -49,44 +50,66 @@ def _warn_for_function(warning: Warning, function: Callable[..., object]) -> Non ) -def _get_hookable(obj: object, name: str) -> Callable[..., object] | None: - """Return a hookable callable for ``name`` without triggering descriptors. +_ABSENT: Final = object() - Only plain functions, :class:`classmethod`, :class:`staticmethod`, and - already-bound :class:`~types.MethodType` objects are supported. Properties, - ``cached_property``, and other descriptors are intentionally skipped -- - hooks provided via such descriptors (or only via ``__getattr__``) are not - supported. + +def _instance_dict(obj: object) -> dict[str, Any]: + """Return ``obj``'s own ``__dict__``, without going through ``getattr``.""" + try: + instance_dict = object.__getattribute__(obj, "__dict__") + except AttributeError: + return {} + return instance_dict if isinstance(instance_dict, dict) else {} + + +def _static_hook_attr( + obj: object, name: str +) -> tuple[object, Callable[..., object]] | None: + """Return ``(marker_holder, function)`` for ``name`` on ``obj``, else None. + + Lookup avoids ``getattr()`` so that properties, ``cached_property`` and + other descriptors are skipped instead of being evaluated -- hooks provided + through such a descriptor (or only through ``__getattr__``) are not + supported. Only plain functions, :class:`classmethod`, :class:`staticmethod` + and already-bound :class:`~types.MethodType` objects are hookable. + + ``marker_holder`` is the object that may carry the hookimpl/hookspec + options: the ``classmethod``/``staticmethod`` wrapper when the marker was + applied above it, the underlying function otherwise. ``function`` is bound + exactly the way ``getattr()`` would have bound it. """ + if not (inspect.isclass(obj) or inspect.ismodule(obj)): + # Values stored on the instance itself -- in ``__dict__`` or in a + # ``__slots__`` slot -- are handed out by ``getattr()`` as they are, + # bypassing the descriptor protocol, so they must not be bound here. + value = _instance_dict(obj).get(name, _ABSENT) + if value is _ABSENT: + slot = inspect.getattr_static(obj, name, None) + if isinstance(slot, types.MemberDescriptorType): + # Reading a slot runs no user code. + try: + value = slot.__get__(obj, type(obj)) + except AttributeError: + value = _ABSENT + if value is not _ABSENT: + if isinstance(value, types.MethodType): + return value.__func__, value + if inspect.isfunction(value): + return value, value + return None + static: object = inspect.getattr_static(obj, name, None) if isinstance(static, staticmethod): - return static.__func__ + return static, static.__func__ if isinstance(static, classmethod): owner = obj if inspect.isclass(obj) else type(obj) - return cast(Callable[..., object], static.__get__(owner, owner)) + return static, cast(Callable[..., object], static.__get__(owner, owner)) if isinstance(static, types.MethodType): - return static + return static.__func__, static if inspect.isfunction(static): if inspect.isclass(obj) or inspect.ismodule(obj): - return static - return static.__get__(obj, type(obj)) - return None - - -def _hook_marker_holder(obj: object, name: str) -> object | None: - """Return the object that may carry hookimpl/hookspec marker options. - - Marker attributes may live on a ``classmethod``/``staticmethod`` wrapper - (``@hookimpl`` applied above them) or on the underlying function - (``@hookimpl`` applied below them). - """ - static: object = inspect.getattr_static(obj, name, None) - if isinstance(static, (classmethod, staticmethod)): - return static - if isinstance(static, types.MethodType): - return static.__func__ - if inspect.isfunction(static): - return static + return static, static + return static, static.__get__(obj, type(obj)) return None @@ -198,8 +221,13 @@ def register(self, plugin: _Plugin, name: str | None = None) -> str | None: hookimpl_opts = self.parse_hookimpl_opts(plugin, attr_name) if hookimpl_opts is not None: normalize_hookimpl_opts(hookimpl_opts) - method = _get_hookable(plugin, attr_name) - assert method is not None + found = _static_hook_attr(plugin, attr_name) + # Only reachable when a subclass overrode parse_hookimpl_opts + # to claim an attribute pluggy cannot bind. + assert found is not None, ( + f"{plugin!r}.{attr_name} is not a hookable attribute" + ) + method: _HookImplFunction[object] = found[1] hookimpl = HookImpl(plugin, plugin_name, method, hookimpl_opts) hook_name = hookimpl_opts.get("specname") or attr_name hook: HookCaller | None = getattr(self.hook, hook_name, None) @@ -227,12 +255,12 @@ def parse_hookimpl_opts(self, plugin: _Plugin, name: str) -> HookimplOpts | None descriptors are not executed. Only functions, classmethods, staticmethods, and bound methods are considered. """ - holder = _hook_marker_holder(plugin, name) - if holder is None: + found = _static_hook_attr(plugin, name) + if found is None: return None return cast( HookimplOpts | None, - _get_marker_opts(holder, self.project_name + "_impl"), + _get_marker_opts(found[0], self.project_name + "_impl"), ) def unregister( @@ -330,12 +358,12 @@ def parse_hookspec_opts( descriptors are not executed. Only functions, classmethods, staticmethods, and bound methods are considered. """ - holder = _hook_marker_holder(module_or_class, name) - if holder is None: + found = _static_hook_attr(module_or_class, name) + if found is None: return None return cast( HookspecOpts | None, - _get_marker_opts(holder, self.project_name + "_spec"), + _get_marker_opts(found[0], self.project_name + "_spec"), ) def get_plugins(self) -> set[Any]: diff --git a/testing/test_pluginmanager.py b/testing/test_pluginmanager.py index 155d97af..43c2f73a 100644 --- a/testing/test_pluginmanager.py +++ b/testing/test_pluginmanager.py @@ -3,6 +3,7 @@ """ import importlib.metadata +import types from typing import Any from typing import cast @@ -206,6 +207,135 @@ def he_method1(arg: object) -> list[object]: assert he_pm.hook.he_method1(arg=1) == [[1]] +def test_register_instance_attribute_function_is_not_bound( + he_pm: PluginManager, +) -> None: + """A function stored on the instance is called unbound, as ``getattr`` does.""" + + class Plugin: + def __init__(self) -> None: + @hookimpl + def he_method1(arg: object) -> list[object]: + return [arg] + + # Instance attributes bypass the descriptor protocol, so no ``self`` + # is injected and the hook must keep its declared signature. + self.he_method1 = he_method1 + + he_pm.register(Plugin()) + assert he_pm.hook.he_method1(arg=1) == [[1]] + + +def test_register_instance_attribute_bound_method(he_pm: PluginManager) -> None: + """A bound method stored on the instance stays bound to its own object.""" + + class Impl: + @hookimpl + def he_method1(self, arg: object) -> list[object]: + return [self, arg] + + impl = Impl() + + class Plugin: + pass + + plugin = Plugin() + plugin.he_method1 = impl.he_method1 # type: ignore[attr-defined] + + he_pm.register(plugin) + assert he_pm.hook.he_method1(arg=1) == [[impl, 1]] + + +def test_register_slot_attribute_function_is_not_bound(he_pm: PluginManager) -> None: + """A function stored in a ``__slots__`` slot is found and called unbound.""" + + class Plugin: + __slots__ = ("he_method1", "unset") + + def __init__(self) -> None: + @hookimpl + def he_method1(arg: object) -> list[object]: + return [arg] + + self.he_method1 = he_method1 + + plugin = Plugin() + # ``dir()`` lists ``unset`` too; an unassigned slot must be skipped quietly. + assert "unset" in dir(plugin) + he_pm.register(plugin) + assert he_pm.hook.he_method1(arg=1) == [[1]] + + +def test_register_module_level_bound_method(he_pm: PluginManager) -> None: + """A bound method assigned onto a module namespace is hookable.""" + + class Impl: + @hookimpl + def he_method1(self, arg: object) -> list[object]: + return [self, arg] + + impl = Impl() + module = types.ModuleType("module_plugin") + module.he_method1 = impl.he_method1 # type: ignore[attr-defined] + + he_pm.register(module) + assert he_pm.hook.he_method1(arg=1) == [[impl, 1]] + + +def test_register_ignores_unmarked_class_and_static_methods( + he_pm: PluginManager, +) -> None: + """Undecorated classmethods/staticmethods carry no marker on either side.""" + + class Plugin: + @classmethod + def not_a_hook_cm(cls) -> None: ... + + @staticmethod + def not_a_hook_sm() -> None: ... + + @hookimpl + def he_method1(self, arg: object) -> list[object]: + return [arg] + + plugin = Plugin() + assert he_pm.parse_hookimpl_opts(plugin, "not_a_hook_cm") is None + assert he_pm.parse_hookimpl_opts(plugin, "not_a_hook_sm") is None + + he_pm.register(plugin) + assert he_pm.hook.he_method1(arg=1) == [[1]] + + +def test_register_ignores_non_callable_instance_attributes(pm: PluginManager) -> None: + """Instance attributes that are not routines are never hook candidates.""" + + class Plugin: + def __init__(self) -> None: + self.some_value = 42 + + assert pm.parse_hookimpl_opts(Plugin(), "some_value") is None + + +def test_hookspec_lookup_ignores_properties(pm: PluginManager) -> None: + """Hookspec discovery must not evaluate descriptors either.""" + + class Spec: + was_executed = False + + @property + def not_a_spec(self) -> None: + type(self).was_executed = True # pragma: no cover + + @hookspec + def he_method1(self, arg: object) -> object: ... + + # Spec instances are supported at runtime, see the ``he_pm`` fixture. + spec = cast(Any, Spec()) + pm.add_hookspecs(spec) + assert not Spec.was_executed + assert pm.parse_hookspec_opts(spec, "not_a_spec") is None + + def test_register_mismatch_method(he_pm: PluginManager) -> None: class hello: @hookimpl @@ -974,3 +1104,6 @@ def goodbye(self, arg: object) -> int: assert len(hookcallers) == 2 caller_names = {hc.name for hc in hookcallers} assert caller_names == {"hello", "goodbye"} + # Both hello impls are still wired up, and each caller works. + assert pm.hook.hello(arg=1) == [101, 2] + assert pm.hook.goodbye(arg=1) == [201]