diff --git a/changelog/759.bugfix.rst b/changelog/759.bugfix.rst new file mode 100644 index 00000000..1e754a55 --- /dev/null +++ b/changelog/759.bugfix.rst @@ -0,0 +1,2 @@ +:meth:`PluginManager.unregister() ` now raises :exc:`ValueError` when both ``plugin`` and ``name`` are given but refer to different plugins, instead of removing one plugin's hook implementations and another plugin's registry entry. +It also fully unregisters plugin objects that are falsy (e.g. define ``__len__`` returning ``0``); previously their name stayed in the registry. diff --git a/src/pluggy/_manager.py b/src/pluggy/_manager.py index 33009344..4994c867 100644 --- a/src/pluggy/_manager.py +++ b/src/pluggy/_manager.py @@ -294,6 +294,19 @@ def unregister( plugin = self.get_plugin(name) if plugin is None: return None + else: + # Both given: hook callers are looked up by plugin but the registry + # entry is removed by name, so they must refer to the same plugin. + registered_plugin = self._name2plugin.get(name) + registered_name = self.get_name(plugin) + if (registered_plugin is not None and registered_plugin is not plugin) or ( + registered_name is not None and registered_name != name + ): + raise ValueError( + f"Plugin name {name!r} and plugin {plugin!r} do not agree" + f" (name is registered to {registered_plugin!r}," + f" plugin is registered as {registered_name!r})" + ) hookcallers = self.get_hookcallers(plugin) if hookcallers: @@ -301,8 +314,7 @@ def unregister( hookcaller._remove_plugin(plugin) # if self._name2plugin[name] == None registration was blocked: ignore - if self._name2plugin.get(name): - assert name is not None + if self._name2plugin.get(name) is not None: del self._name2plugin[name] return plugin @@ -499,7 +511,7 @@ def load_setuptools_entrypoints(self, group: str, name: str | None = None) -> in ep.group != group or (name is not None and ep.name != name) # already registered - or self.get_plugin(ep.name) + or self.has_plugin(ep.name) or self.is_blocked(ep.name) ): continue diff --git a/testing/test_pluginmanager.py b/testing/test_pluginmanager.py index 7e5058b2..c07c5e81 100644 --- a/testing/test_pluginmanager.py +++ b/testing/test_pluginmanager.py @@ -1281,3 +1281,51 @@ def goodbye(self, arg: object) -> int: # 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] + + +def test_unregister_plugin_and_name_must_agree(pm: PluginManager) -> None: + class Plugin: + @hookimpl + def he_method1(self, arg): + return arg + + a, b, c = Plugin(), Plugin(), Plugin() + pm.register(a, "a") + pm.register(b, "b") + # Both registered, but under different names. + with pytest.raises(ValueError, match="do not agree"): + pm.unregister(a, "b") + # Plugin not registered, name belongs to another plugin. + with pytest.raises(ValueError, match="do not agree"): + pm.unregister(c, "b") + # Plugin registered, name unknown. + with pytest.raises(ValueError, match="do not agree"): + pm.unregister(a, "unknown") + # Rejected calls leave everything untouched. + assert pm.get_plugin("a") is a + assert pm.get_plugin("b") is b + assert pm.hook.he_method1(arg=1) == [1, 1] + # Agreeing arguments still work. + assert pm.unregister(a, "a") is a + assert not pm.is_registered(a) + + +def test_unregister_falsy_plugin(pm: PluginManager) -> None: + class FalsyPlugin: + def __len__(self) -> int: + return 0 + + @hookimpl + def he_method1(self, arg): + return arg + + plugin = FalsyPlugin() + assert not plugin + pm.register(plugin, "falsy") + assert pm.hook.he_method1(arg=1) == [1] + assert pm.unregister(plugin) is plugin + assert not pm.is_registered(plugin) + assert pm.get_plugin("falsy") is None + # The name is free again. + pm.register(plugin, "falsy") + assert pm.is_registered(plugin)