From eaff6747399a75d313542480b3dfe5203cbac776 Mon Sep 17 00:00:00 2001 From: opencode Date: Wed, 30 Sep 2026 15:07:01 +0800 Subject: [PATCH 1/3] Raise when unregister's plugin and name disagree MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit unregister() looks hookcallers up by plugin but deletes the registry entry by name, and never checked that the two agree. Passing a plugin registered as "first" together with name="second" therefore stripped the plugin's own implementations while dropping an unrelated plugin's registry entry — leaving that plugin reported as unregistered with its implementations still on the hook caller. The docstring already says "If both are specified, they must agree", so raise before mutating anything. --- src/pluggy/_manager.py | 10 ++++++++++ testing/test_pluginmanager.py | 34 ++++++++++++++++++++++++++++++++++ 2 files changed, 44 insertions(+) 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..7b91843e 100644 --- a/testing/test_pluginmanager.py +++ b/testing/test_pluginmanager.py @@ -490,6 +490,40 @@ 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): + return arg + 1 + + 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 From a037a4206ccae99368db9eff69c5985f963d05ca Mon Sep 17 00:00:00 2001 From: opencode Date: Wed, 30 Sep 2026 15:13:56 +0800 Subject: [PATCH 2/3] Add changelog entry for #747 --- changelog/747.bugfix.rst | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 changelog/747.bugfix.rst 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. From 3cbddf334e9e9a274c38adcea34b7357bdc8980d Mon Sep 17 00:00:00 2001 From: opencode Date: Wed, 30 Sep 2026 17:48:20 +0800 Subject: [PATCH 3/3] Give the new test's hookspec an ellipsis body The spec body is never executed, so it was the one uncovered line this diff added, and it dragged Codecov's patch coverage to 96%. The .coveragerc already excludes '`- `...`' spec bodies and three other tests in this file use one. --- testing/test_pluginmanager.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/testing/test_pluginmanager.py b/testing/test_pluginmanager.py index 7b91843e..af5b1055 100644 --- a/testing/test_pluginmanager.py +++ b/testing/test_pluginmanager.py @@ -496,8 +496,7 @@ def test_unregister_plugin_and_name_must_agree(pm: PluginManager) -> None: class Hooks: @hookspec - def he_method1(self, arg): - return arg + 1 + def he_method1(self, arg): ... class Plugin: @hookimpl