diff --git a/changelog/760.bugfix.rst b/changelog/760.bugfix.rst new file mode 100644 index 00000000..7e108858 --- /dev/null +++ b/changelog/760.bugfix.rst @@ -0,0 +1,3 @@ +Identify plugins by object identity when registering, looking up, unregistering, +and excluding hook implementations. Distinct plugins which compare equal no +longer interfere with each other's registrations or hook calls. diff --git a/src/pluggy/_caller.py b/src/pluggy/_caller.py index 52b02316..255323e6 100644 --- a/src/pluggy/_caller.py +++ b/src/pluggy/_caller.py @@ -7,7 +7,6 @@ from collections.abc import Callable from collections.abc import Mapping from collections.abc import Sequence -from collections.abc import Set as AbstractSet from typing import Any from typing import Final from typing import final @@ -115,7 +114,7 @@ def is_historic(self) -> bool: def _remove_plugin(self, plugin: _Plugin) -> None: """Remove all hook implementations registered by the given plugin.""" - remaining = [impl for impl in self._hookimpls if impl.plugin != plugin] + remaining = [impl for impl in self._hookimpls if impl.plugin is not plugin] if len(remaining) == len(self._hookimpls): raise ValueError(f"plugin {plugin!r} not found") self._hookimpls[:] = remaining @@ -296,7 +295,7 @@ class _SubsetHookCaller(HookCaller): "_remove_plugins", ) - def __init__(self, orig: HookCaller, remove_plugins: AbstractSet[_Plugin]) -> None: + def __init__(self, orig: HookCaller, remove_plugins: Mapping[int, _Plugin]) -> None: self._orig = orig self._remove_plugins = remove_plugins self.name = orig.name # type: ignore[misc] @@ -307,7 +306,7 @@ def _hookimpls(self) -> list[HookImpl]: return [ impl for impl in self._orig._hookimpls - if impl.plugin not in self._remove_plugins + if id(impl.plugin) not in self._remove_plugins ] @property diff --git a/src/pluggy/_manager.py b/src/pluggy/_manager.py index 4994c867..e07f6258 100644 --- a/src/pluggy/_manager.py +++ b/src/pluggy/_manager.py @@ -208,7 +208,7 @@ def register(self, plugin: _Plugin, name: str | None = None) -> str | None: f"{plugin_name}={plugin}\n{self._name2plugin}" ) - if plugin in self._name2plugin.values(): + if self.is_registered(plugin): raise ValueError( "Plugin already registered under a different name: " f"{plugin_name}={plugin}\n{self._name2plugin}" @@ -396,7 +396,9 @@ def get_plugins(self) -> set[Any]: def is_registered(self, plugin: _Plugin) -> bool: """Return whether the plugin is already registered.""" - return any(plugin == val for val in self._name2plugin.values()) + return plugin is not None and any( + plugin is val for val in self._name2plugin.values() + ) def get_canonical_name(self, plugin: _Plugin) -> str: """Return a canonical name for a plugin object. @@ -421,7 +423,7 @@ def get_name(self, plugin: _Plugin) -> str | None: """Return the name the plugin is registered under, or ``None`` if is isn't.""" for name, val in self._name2plugin.items(): - if plugin == val: + if val is not None and plugin is val: return name return None @@ -632,12 +634,16 @@ def subset_hook_caller( """Return a proxy :class:`~pluggy.HookCaller` for the named hook which calls all registered plugins except the ones from remove_plugins.""" orig: HookCaller = getattr(self.hook, name) - plugins_to_remove = set(remove_plugins) + # Retain the objects so removed plugin ids cannot be reused by new plugins. + plugins_to_remove = {id(plugin): plugin for plugin in remove_plugins} # Optimization: discard plugins to remove which don't actually implement # the hook, to make checks faster. - plugins_to_remove.intersection_update( - hookimpl.plugin for hookimpl in orig._hookimpls - ) + hook_plugin_ids = {id(hookimpl.plugin) for hookimpl in orig._hookimpls} + plugins_to_remove = { + key: plugin + for key, plugin in plugins_to_remove.items() + if key in hook_plugin_ids + } # Optimization: if none of the plugins to remove actually implement the # hook, avoid the subset hook caller overhead. if not plugins_to_remove: diff --git a/testing/test_pluginmanager.py b/testing/test_pluginmanager.py index 06d645d7..3481d05b 100644 --- a/testing/test_pluginmanager.py +++ b/testing/test_pluginmanager.py @@ -37,6 +37,68 @@ def test_register_rejects_none(pm: PluginManager) -> None: assert not pm.is_registered(None) +def test_register_distinct_equal_plugins(pm: PluginManager) -> None: + class Spec: + @hookspec + def myhook(self): + pass + + class Plugin: + def __init__(self, value: str) -> None: + self.value = value + + def __eq__(self, other): + return isinstance(other, Plugin) + + @hookimpl + def myhook(self): + return self.value + + pm.add_hookspecs(Spec) + first, second, unknown = Plugin("one"), Plugin("two"), Plugin("unknown") + assert first == second + pm.register(first, "one") + assert not pm.is_registered(second) + assert pm.get_name(second) is None + assert pm.get_hookcallers(second) is None + pm.register(second, "two") + assert pm.get_name(first) == "one" + assert pm.get_name(second) == "two" + assert pm.hook.myhook() == ["two", "one"] + assert not pm.is_registered(unknown) + assert pm.subset_hook_caller("myhook", [unknown]) is pm.hook.myhook + assert pm.subset_hook_caller("myhook", [second])() == ["one"] + assert pm.subset_hook_caller("myhook", [first])() == ["two"] + assert pm.subset_hook_caller("myhook", [first, second])() == [] + assert pm.unregister(second) is second + assert pm.is_registered(first) + assert not pm.is_registered(second) + assert pm.hook.myhook() == ["one"] + with pytest.raises(ValueError, match="already registered"): + pm.register(first, "other") + + +def test_plugin_registry_does_not_call_equality(pm: PluginManager) -> None: + class Plugin: + def __eq__(self, other): + raise AssertionError("plugin equality must not be called") + + first, second = Plugin(), Plugin() + with pytest.raises(AssertionError, match="plugin equality must not be called"): + assert first == second + pm.register(first, "one") + pm.register(second, "two") + assert pm.is_registered(first) + assert pm.get_name(first) == "one" + assert pm.unregister(second) is second + + +def test_blocked_name_is_not_a_registered_plugin(pm: PluginManager) -> None: + pm.set_blocked("blocked") + assert not pm.is_registered(None) + assert pm.get_name(None) is None + + def test_pm(pm: PluginManager) -> None: """Basic registration with objects"""