diff --git a/changelog/733.bugfix.rst b/changelog/733.bugfix.rst new file mode 100644 index 00000000..5b0a6e3f --- /dev/null +++ b/changelog/733.bugfix.rst @@ -0,0 +1 @@ +``PluginManager.register()`` no longer leaves a plugin or its hook implementations installed when registration fails validation, so the call can be retried after the plugin is fixed. diff --git a/src/pluggy/_manager.py b/src/pluggy/_manager.py index 080ef333..c4f67050 100644 --- a/src/pluggy/_manager.py +++ b/src/pluggy/_manager.py @@ -215,14 +215,23 @@ def register(self, plugin: _Plugin, name: str | None = None) -> str | None: f"{plugin_name}={plugin}\n{self._name2plugin}" ) - # XXX if an error happens we should make sure no state has been - # changed at point of return - self._name2plugin[plugin_name] = plugin - - # register matching hook implementations of the plugin - for attr_name in dir(plugin): - hookimpl_opts = self.parse_hookimpl_opts(plugin, attr_name) - if hookimpl_opts is not None: + # Validate every implementation before the plugin is visible. + # A later failure must not leave this plugin registered or any of + # its hook implementations installed (#733). + prepared: list[tuple[HookCaller, HookImpl]] = [] + new_hook_names: list[str] = [] + + def discard_empty_new_hooks() -> None: + for created_name in new_hook_names: + created = getattr(self.hook, created_name, None) + if created is not None and not created.get_hookimpls(): + delattr(self.hook, created_name) + + try: + for attr_name in dir(plugin): + hookimpl_opts = self.parse_hookimpl_opts(plugin, attr_name) + if hookimpl_opts is None: + continue normalize_hookimpl_opts(hookimpl_opts) found = _static_hook_attr(plugin, attr_name) # Only reachable when a subclass overrode parse_hookimpl_opts @@ -237,10 +246,24 @@ def register(self, plugin: _Plugin, name: str | None = None) -> str | None: if hook is None: hook = HookCaller(hook_name, self._hookexec) setattr(self.hook, hook_name, hook) + new_hook_names.append(hook_name) elif hook.has_spec(): self._verify_hook(hook, hookimpl) + prepared.append((hook, hookimpl)) + except BaseException: + discard_empty_new_hooks() + raise + + self._name2plugin[plugin_name] = plugin + try: + for hook, hookimpl in prepared: + if hook.has_spec(): hook._maybe_apply_history(hookimpl) hook._add_hookimpl(hookimpl) + except BaseException: + self.unregister(plugin=plugin, name=plugin_name) + discard_empty_new_hooks() + raise return plugin_name def parse_hookimpl_opts(self, plugin: _Plugin, name: str) -> HookimplOpts | None: diff --git a/testing/test_pluginmanager.py b/testing/test_pluginmanager.py index 65b322d4..62da3f44 100644 --- a/testing/test_pluginmanager.py +++ b/testing/test_pluginmanager.py @@ -358,6 +358,42 @@ def he_method_notexists(self): assert excinfo.value.plugin is plugin +def test_register_validation_failure_leaves_manager_unchanged( + he_pm: PluginManager, +) -> None: + """A later invalid hook must not keep earlier impls or the plugin (#733).""" + + class Plugin: + @hookimpl + def he_method1(self, arg): + return arg + + @hookimpl(specname="he_method1") + def he_method1_bad(self, arg, extra): + return arg # pragma: no cover + + @hookimpl + def brand_new_hook(self, arg): + return arg # pragma: no cover + + plugin = Plugin() + with pytest.raises(PluginValidationError): + he_pm.register(plugin) + + assert not he_pm.is_registered(plugin) + assert he_pm.get_plugin("Plugin") is None + assert he_pm.hook.he_method1.get_hookimpls() == [] + assert not hasattr(he_pm.hook, "brand_new_hook") + + class Fixed: + @hookimpl + def he_method1(self, arg): + return arg + 1 + + he_pm.register(Fixed()) + assert he_pm.hook.he_method1(arg=1) == [2] + + def test_register_mismatch_arg(he_pm: PluginManager) -> None: class hello: @hookimpl