diff --git a/changelog/733.bugfix.rst b/changelog/733.bugfix.rst new file mode 100644 index 00000000..ba4902c3 --- /dev/null +++ b/changelog/733.bugfix.rst @@ -0,0 +1 @@ +:meth:`PluginManager.register() ` now validates all hook implementations of a plugin before registering any of them, so a plugin that fails validation no longer stays partially registered. diff --git a/src/pluggy/_manager.py b/src/pluggy/_manager.py index 080ef333..8a0959f7 100644 --- a/src/pluggy/_manager.py +++ b/src/pluggy/_manager.py @@ -215,11 +215,9 @@ 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 + # Validate every hookimpl before changing any state, so a plugin that + # fails validation leaves the manager untouched. + hookimpls: list[tuple[str, HookImpl, bool]] = [] for attr_name in dir(plugin): hookimpl_opts = self.parse_hookimpl_opts(plugin, attr_name) if hookimpl_opts is not None: @@ -234,13 +232,25 @@ def register(self, plugin: _Plugin, name: str | None = None) -> str | None: hookimpl = HookImpl(plugin, plugin_name, method, hookimpl_opts) hook_name = hookimpl_opts.get("specname") or attr_name hook: HookCaller | None = getattr(self.hook, hook_name, None) - if hook is None: - hook = HookCaller(hook_name, self._hookexec) - setattr(self.hook, hook_name, hook) - elif hook.has_spec(): + verified = False + if hook is not None and hook.has_spec(): self._verify_hook(hook, hookimpl) - hook._maybe_apply_history(hookimpl) - hook._add_hookimpl(hookimpl) + verified = True + hookimpls.append((hook_name, hookimpl, verified)) + + self._name2plugin[plugin_name] = plugin + for hook_name, hookimpl, verified in hookimpls: + hook = getattr(self.hook, hook_name, None) + if hook is None: + hook = HookCaller(hook_name, self._hookexec) + setattr(self.hook, hook_name, hook) + elif not verified and hook.has_spec(): + # An earlier impl's historic replay added this spec after the + # first pass. Failing here leaves the plugin partially + # registered, but only user code during replay can get here. + self._verify_hook(hook, hookimpl) + hook._maybe_apply_history(hookimpl) + hook._add_hookimpl(hookimpl) 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..07d11b4e 100644 --- a/testing/test_pluginmanager.py +++ b/testing/test_pluginmanager.py @@ -371,6 +371,63 @@ def he_method1(self, qlwkje): assert excinfo.value.plugin is plugin +def test_register_validation_failure_leaves_no_state(he_pm: PluginManager) -> None: + """The invalid impl sorts last in dir(), after a valid impl and an impl + for an unknown hook; a failed register must install none of them (#733).""" + + class hello: + @hookimpl + def a_new_hook(self): + pass # pragma: no cover + + @hookimpl + def he_method1(self, arg): + pass # pragma: no cover + + @hookimpl(specname="he_method1") + def he_method1_invalid(self, qlwkje): + pass # pragma: no cover + + plugin = hello() + + with pytest.raises(PluginValidationError) as excinfo: + he_pm.register(plugin) + assert excinfo.value.plugin is plugin + assert not he_pm.is_registered(plugin) + assert he_pm.hook.he_method1.get_hookimpls() == [] + assert not hasattr(he_pm.hook, "a_new_hook") + + +def test_register_verifies_spec_added_by_historic_replay(pm: PluginManager) -> None: + """A spec added while replaying one impl's historic call must still be + checked for the impls registered after it.""" + + class Specs: + @hookspec(historic=True) + def configure(self): + pass # pragma: no cover + + class LateSpecs: + @hookspec + def late(self, arg): + pass # pragma: no cover + + pm.add_hookspecs(Specs) + pm.hook.configure.call_historic() + + class hello: + @hookimpl + def configure(self): + pm.add_hookspecs(LateSpecs) + + @hookimpl + def late(self, qlwkje): + pass # pragma: no cover + + with pytest.raises(PluginValidationError, match="qlwkje"): + pm.register(hello()) + + def test_register_hookwrapper_not_a_generator_function(he_pm: PluginManager) -> None: class hello: @hookimpl(hookwrapper=True)