diff --git a/changelog/747.bugfix.rst b/changelog/747.bugfix.rst new file mode 100644 index 00000000..52fbacdb --- /dev/null +++ b/changelog/747.bugfix.rst @@ -0,0 +1,3 @@ +:meth:`PluginManager.unregister() ` now raises a :exc:`ValueError` when both ``plugin`` and ``name`` are passed and they refer to different registrations, which its documentation already required. + +Previously the call removed the given plugin's hook implementations while dropping an unrelated plugin's registry entry, so that other plugin's implementations remained callable even though it was no longer registered. diff --git a/src/pluggy/_manager.py b/src/pluggy/_manager.py index 1d7984a9..e5487fb9 100644 --- a/src/pluggy/_manager.py +++ b/src/pluggy/_manager.py @@ -290,6 +290,16 @@ def unregister( assert plugin is not None, "one of name or plugin needs to be specified" name = self.get_name(plugin) assert name is not None, "plugin is not registered" + elif plugin is not None: + # Both given: the hookcallers are looked up by plugin but the + # registry entry is deleted by name, so a mismatch would strip + # one plugin's impls while dropping an unrelated registry entry. + registered_name = self.get_name(plugin) + if registered_name is not None and registered_name != name: + raise ValueError( + f"Plugin {plugin!r} is registered under name " + f"{registered_name!r}, not {name!r}" + ) if plugin is None: plugin = self.get_plugin(name) diff --git a/testing/test_pluginmanager.py b/testing/test_pluginmanager.py index 7e5058b2..af5b1055 100644 --- a/testing/test_pluginmanager.py +++ b/testing/test_pluginmanager.py @@ -490,6 +490,39 @@ class Plugin: pm.unregister(p, "error") +def test_unregister_plugin_and_name_must_agree(pm: PluginManager) -> None: + """Passing both ``plugin`` and ``name`` which do not refer to the same + registration is rejected instead of unregistering a mixture of the two.""" + + class Hooks: + @hookspec + def he_method1(self, arg): ... + + class Plugin: + @hookimpl + def he_method1(self, arg): + return arg + 1 + + pm.add_hookspecs(Hooks) + first, second = Plugin(), Plugin() + pm.register(first, name="first") + pm.register(second, name="second") + + with pytest.raises(ValueError, match="registered under name 'first'"): + pm.unregister(plugin=first, name="second") + + # The rejected call must not have changed any state: both plugins stay + # registered and both implementations stay on the hook caller. + assert pm.get_name(first) == "first" + assert pm.get_name(second) == "second" + assert pm.hook.he_method1(arg=1) == [2, 2] + + # Agreeing arguments still unregister, whichever form is used. + assert pm.unregister(plugin=first, name="first") is first + assert pm.get_name(first) is None + assert pm.hook.he_method1(arg=1) == [2] + + def test_register_unknown_hooks(pm: PluginManager) -> None: class Plugin1: @hookimpl