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. diff --git a/src/pluggy/_manager.py b/src/pluggy/_manager.py index c20105c8..9d188192 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 @@ -49,6 +48,60 @@ 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. + + 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): """Plugin failed validation. @@ -160,7 +213,8 @@ def register(self, plugin: _Plugin, name: str | None = None) -> str | None: hookimpl_opts = self.parse_hookimpl_opts(plugin, name) if hookimpl_opts is not None: normalize_hookimpl_opts(hookimpl_opts) - method: _HookImplFunction[object] = getattr(plugin, name) + method = _get_hookable(plugin, name) + assert method is not None hookimpl = HookImpl(plugin, plugin_name, method, hookimpl_opts) name = hookimpl_opts.get("specname") or name hook: HookCaller | None = getattr(self.hook, name, None) @@ -183,20 +237,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`. + + Discovery uses :func:`inspect.getattr_static` so properties and other + descriptors are not executed. Only functions, classmethods, + staticmethods, and bound methods are considered. """ - method: object = getattr(plugin, name) - if not inspect.isroutine(method): + 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 @@ -288,10 +340,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`. + + Discovery uses :func:`inspect.getattr_static` so properties and other + descriptors are not executed. Only functions, classmethods, + staticmethods, and bound methods are considered. """ - method = getattr(module_or_class, name) - opts: HookspecOpts | None = getattr(method, self.project_name + "_spec", None) - return opts + holder = _hook_marker_holder(module_or_class, name) + if holder is None: + return None + 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 219b17d5..3b488df7 100644 --- a/testing/test_pluginmanager.py +++ b/testing/test_pluginmanager.py @@ -124,6 +124,89 @@ class A: assert pm.register(A(), "somename") +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 # pragma: no cover + return None # pragma: no cover + + # Registering the class is harmless (getattr returns the property object). + he_pm.register(ClassWithProperties) + # 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_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: + """Descriptor attrs are skipped without accessing them via getattr.""" + + 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_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