diff --git a/changelog/431.bugfix.rst b/changelog/431.bugfix.rst new file mode 100644 index 00000000..5c813c2a --- /dev/null +++ b/changelog/431.bugfix.rst @@ -0,0 +1 @@ +Fix ``HookCaller._remove_plugin()`` to remove all hook implementations for a plugin in a single call, instead of only the first one. Also fix ``PluginManager.get_hookcallers()`` to not return duplicate ``HookCaller`` entries when a plugin has multiple implementations on the same hook (e.g. via ``specname``). diff --git a/src/pluggy/_hooks.py b/src/pluggy/_hooks.py index 7fde78c9..cdd3085c 100644 --- a/src/pluggy/_hooks.py +++ b/src/pluggy/_hooks.py @@ -440,11 +440,11 @@ def is_historic(self) -> bool: return self._call_history is not None def _remove_plugin(self, plugin: _Plugin) -> None: - for i, method in enumerate(self._hookimpls): - if method.plugin == plugin: - del self._hookimpls[i] - return - raise ValueError(f"plugin {plugin!r} not found") + """Remove all hook implementations registered by the given plugin.""" + remaining = [impl for impl in self._hookimpls if impl.plugin != plugin] + if len(remaining) == len(self._hookimpls): + raise ValueError(f"plugin {plugin!r} not found") + self._hookimpls[:] = remaining def get_hookimpls(self) -> list[HookImpl]: """Get all registered hook implementations for this hook.""" diff --git a/src/pluggy/_manager.py b/src/pluggy/_manager.py index 1b994f25..c20105c8 100644 --- a/src/pluggy/_manager.py +++ b/src/pluggy/_manager.py @@ -442,9 +442,8 @@ def get_hookcallers(self, plugin: _Plugin) -> list[HookCaller] | None: return None hookcallers = [] for hookcaller in self.hook.__dict__.values(): - for hookimpl in hookcaller.get_hookimpls(): - if hookimpl.plugin is plugin: - hookcallers.append(hookcaller) + if any(impl.plugin is plugin for impl in hookcaller.get_hookimpls()): + hookcallers.append(hookcaller) return hookcallers def add_hookcall_monitoring( diff --git a/testing/test_pluginmanager.py b/testing/test_pluginmanager.py index f80b1b55..0c70c947 100644 --- a/testing/test_pluginmanager.py +++ b/testing/test_pluginmanager.py @@ -814,3 +814,76 @@ def a_hook(self, param): PluginValidationError, match="unknown hook 'a_hook' in plugin .*" ): pm.check_pending() + + +def test_unregister_plugin_with_multi_hookimpls(pm: PluginManager) -> None: + """Verify that unregistering a plugin with multiple hookimpls on the + same hook (via specname) removes all of them (#431).""" + + class Api: + @hookspec + def hello(self, arg: object) -> object: ... + + pm.add_hookspecs(Api) + + class Plugin: + @hookimpl + def hello(self, arg: object) -> int: + return arg + 1 # type: ignore[operator] + + @hookimpl(specname="hello") + def hello_again(self, arg: object) -> int: + return arg + 100 # type: ignore[operator] + + plugin = Plugin() + pm.register(plugin) + + # Both implementations should be registered. + impls = pm.hook.hello.get_hookimpls() + assert len(impls) == 2 + + # Both implementations should run. + out = pm.hook.hello(arg=3) + assert sorted(out) == [4, 103] + + # After unregister, no implementations should remain. + pm.unregister(plugin) + assert pm.hook.hello(arg=3) == [] + assert pm.hook.hello.get_hookimpls() == [] + + +def test_get_hookcallers_no_duplicates(pm: PluginManager) -> None: + """Verify that get_hookcallers does not return duplicate HookCaller + entries when a plugin has multiple hookimpls on the same hook (#431).""" + + class Api: + @hookspec + def hello(self, arg: object) -> object: ... + + @hookspec + def goodbye(self, arg: object) -> object: ... + + pm.add_hookspecs(Api) + + class Plugin: + @hookimpl + def hello(self, arg: object) -> int: + return arg + 1 # type: ignore[operator] + + @hookimpl(specname="hello") + def hello_again(self, arg: object) -> int: + return arg + 100 # type: ignore[operator] + + @hookimpl + def goodbye(self, arg: object) -> int: + return arg + 200 # type: ignore[operator] + + plugin = Plugin() + pm.register(plugin) + + hookcallers = pm.get_hookcallers(plugin) + assert hookcallers is not None + # Should return 2 unique callers (hello + goodbye), not 3. + assert len(hookcallers) == 2 + caller_names = {hc.name for hc in hookcallers} + assert caller_names == {"hello", "goodbye"}