diff --git a/changelog/761.bugfix.rst b/changelog/761.bugfix.rst new file mode 100644 index 00000000..1c4e6e8f --- /dev/null +++ b/changelog/761.bugfix.rst @@ -0,0 +1,9 @@ +:meth:`PluginManager.add_hookspecs() ` now +validates all specifications before installing any of them, so failed validation +no longer leaves partially installed specifications that prevent a retry. +Existing hook caller references and registered implementations are preserved. + +Failed historic replay during :meth:`PluginManager.register() +` now removes the failed registration and its +implementations, including unused hook callers created by that registration, +while preserving existing state and successful changes made by replay callbacks. diff --git a/src/pluggy/_manager.py b/src/pluggy/_manager.py index 4994c867..a9449b62 100644 --- a/src/pluggy/_manager.py +++ b/src/pluggy/_manager.py @@ -159,6 +159,7 @@ def __init__(self, project_name: str) -> None: #: The project name. self.project_name: Final = project_name self._name2plugin: Final[dict[str, _Plugin]] = {} + self._pending_registrations: Final[dict[str, object]] = {} self._plugin_distinfo: Final[ list[tuple[_Plugin, importlib.metadata.Distribution]] ] = [] @@ -237,19 +238,50 @@ def register(self, plugin: _Plugin, name: str | None = None) -> str | None: verified = True hookimpls.append((hook_name, hookimpl, verified)) + registration = object() + self._pending_registrations[plugin_name] = registration 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) + added: list[tuple[HookCaller, HookImpl]] = [] + created: dict[str, HookCaller] = {} + try: + for hook_name, hookimpl, verified in hookimpls: + hook = getattr(self.hook, hook_name, None) + if hook is None: + hook = HookCaller(hook_name, self._hookexec) + created[hook_name] = hook + 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, so validate against it now. + self._verify_hook(hook, hookimpl) + hook._maybe_apply_history(hookimpl) + added.append((hook, hookimpl)) + hook._add_hookimpl(hookimpl) + except BaseException: + # Replay runs user code. Remove only this registration's impls, + # without calling plugin equality or undoing nested registrations. + for hook, hookimpl in added: + hook._hookimpls[:] = [ + impl for impl in hook._hookimpls if impl is not hookimpl + ] + for hook_name, created_hook in created.items(): + if ( + getattr(self.hook, hook_name, None) is created_hook + and not created_hook.has_spec() + and not created_hook.get_hookimpls() + ): + delattr(self.hook, hook_name) + # A callback may have unregistered and re-registered even the same + # object under the same name. Only remove our own registry entry. + if ( + self._pending_registrations.get(plugin_name) is registration + and self._name2plugin.get(plugin_name) is plugin + ): + del self._name2plugin[plugin_name] + raise + finally: + if self._pending_registrations.get(plugin_name) is registration: + del self._pending_registrations[plugin_name] return plugin_name def parse_hookimpl_opts(self, plugin: _Plugin, name: str) -> HookimplOpts | None: @@ -316,6 +348,7 @@ def unregister( # if self._name2plugin[name] == None registration was blocked: ignore if self._name2plugin.get(name) is not None: del self._name2plugin[name] + self._pending_registrations.pop(name, None) return plugin @@ -343,27 +376,39 @@ def add_hookspecs(self, module_or_class: _Namespace) -> None: Functions are recognized as hook specifications if they have been decorated with a matching :class:`HookspecMarker`. + + All specifications are validated before any are installed. """ - names = [] + pending: list[tuple[str, HookCaller | None, HookCaller]] = [] for name in dir(module_or_class): spec_opts = self.parse_hookspec_opts(module_or_class, name) if spec_opts is not None: hc: HookCaller | None = getattr(self.hook, name, None) - if hc is None: - hc = HookCaller(name, self._hookexec, module_or_class, spec_opts) - setattr(self.hook, name, hc) - else: - # Plugins registered this hook without knowing the spec. - hc.set_specification(module_or_class, spec_opts) + if hc is not None and hc.spec is not None: + raise ValueError( + f"Hook {hc.spec.name!r} is already registered " + f"within namespace {hc.spec.namespace}" + ) + candidate = HookCaller(name, self._hookexec, module_or_class, spec_opts) + if hc is not None: + # Validate without installing the spec on the live caller. for hookfunction in hc.get_hookimpls(): - self._verify_hook(hc, hookfunction) - names.append(name) + self._verify_hook(candidate, hookfunction) + pending.append((name, hc, candidate)) - if not names: + if not pending: raise ValueError( f"did not find any {self.project_name!r} hooks in {module_or_class!r}" ) + for name, hc, candidate in pending: + if hc is None: + setattr(self.hook, name, candidate) + else: + # Preserve references held by plugins and subset hook callers. + hc.spec = candidate.spec + hc._call_history = candidate._call_history + def parse_hookspec_opts( self, module_or_class: _Namespace, name: str ) -> HookspecOpts | None: diff --git a/testing/test_hookspec_validation.py b/testing/test_hookspec_validation.py new file mode 100644 index 00000000..8daf3550 --- /dev/null +++ b/testing/test_hookspec_validation.py @@ -0,0 +1,183 @@ +"""Failed specification validation must leave registered hook callers usable.""" + +import warnings + +import pytest + +from pluggy import HookimplMarker +from pluggy import HookspecMarker +from pluggy import PluginManager +from pluggy import PluginValidationError + + +hookspec = HookspecMarker("example") +hookimpl = HookimplMarker("example") + + +@pytest.mark.parametrize("historic", [False, True]) +def test_failed_hookspec_can_be_retried(pm: PluginManager, historic: bool) -> None: + class Hooks: + @hookspec(historic=historic) + def process(self, value: int) -> int: + raise NotImplementedError # pragma: no cover + + class ValidPlugin: + @hookimpl + def process(self, value: int) -> int: + return value + 1 + + class InvalidPlugin: + @hookimpl + def process(self, unexpected: int) -> int: + return unexpected + + valid, invalid = ValidPlugin(), InvalidPlugin() + pm.register(valid) + pm.register(invalid) + caller = pm.hook.process + impls = caller.get_hookimpls() + subset = pm.subset_hook_caller("process", remove_plugins=[invalid]) + + with pytest.raises(PluginValidationError, match="unexpected") as excinfo: + pm.add_hookspecs(Hooks) + + assert excinfo.value.plugin is invalid + assert pm.hook.process is caller + assert not caller.has_spec() + assert not caller.is_historic() + assert caller.get_hookimpls() == impls + assert pm.get_plugins() == {valid, invalid} + + pm.unregister(invalid) + pm.add_hookspecs(Hooks) + assert pm.hook.process is caller + assert caller.spec is not None + assert caller.spec.namespace is Hooks + assert caller.is_historic() is historic + if historic: + results: list[int] = [] + subset.call_historic(results.append, {"value": 3}) + pm.register(ValidPlugin()) + assert results == [4, 4] + else: + assert caller(value=3) == [4] + assert subset(value=3) == [4] + + +def test_failed_hookspec_namespace_installs_nothing(pm: PluginManager) -> None: + class Hooks: + @hookspec + def a_new(self) -> None: ... + + @hookspec + def b_valid(self, value: int) -> int: + raise NotImplementedError # pragma: no cover + + @hookspec + def z_invalid(self, value: int) -> int: + raise NotImplementedError # pragma: no cover + + class ValidPlugin: + @hookimpl + def b_valid(self, value: int) -> int: + return value + + class InvalidPlugin: + @hookimpl + def z_invalid(self, unexpected: int) -> int: + return unexpected + + valid, invalid = ValidPlugin(), InvalidPlugin() + pm.register(valid) + pm.register(invalid) + valid_caller, invalid_caller = pm.hook.b_valid, pm.hook.z_invalid + + with pytest.raises(PluginValidationError, match="unexpected"): + pm.add_hookspecs(Hooks) + + assert not hasattr(pm.hook, "a_new") + assert not valid_caller.has_spec() + assert not invalid_caller.has_spec() + pm.unregister(invalid) + pm.add_hookspecs(Hooks) + assert pm.hook.b_valid is valid_caller + assert pm.hook.z_invalid is invalid_caller + assert valid_caller(value=7) == [7] + assert pm.hook.a_new() == [] + + +@pytest.mark.parametrize("historic", [False, True]) +def test_duplicate_hookspec_preserves_existing_state( + pm: PluginManager, historic: bool +) -> None: + class ExistingHooks: + @hookspec(historic=historic) + def z_existing(self, value: int) -> int: + raise NotImplementedError # pragma: no cover + + class MoreHooks: + @hookspec + def a_new(self) -> None: ... + + @hookspec + def z_existing(self, value: int) -> int: + raise NotImplementedError # pragma: no cover + + pm.add_hookspecs(ExistingHooks) + caller = pm.hook.z_existing + original_spec = caller.spec + results: list[int] = [] + if historic: + caller.call_historic(results.append, {"value": 8}) + + with pytest.raises(ValueError, match="Hook 'z_existing' is already registered"): + pm.add_hookspecs(MoreHooks) + + assert pm.hook.z_existing is caller + assert caller.spec is original_spec + assert not hasattr(pm.hook, "a_new") + + class Plugin: + @hookimpl + def z_existing(self, value: int) -> int: + return value + + pm.register(Plugin()) + if historic: + assert results == [8] + else: + assert caller(value=8) == [8] + + +@pytest.mark.parametrize("warn_on_argument", [False, True]) +def test_hookspec_warning_as_error_allows_retry( + pm: PluginManager, warn_on_argument: bool +) -> None: + warning = UserWarning("deprecated implementation") + + class Hooks: + @hookspec( + warn_on_impl=None if warn_on_argument else warning, + warn_on_impl_args={"value": warning} if warn_on_argument else None, + ) + def process(self, value: int) -> int: + raise NotImplementedError # pragma: no cover + + class Plugin: + @hookimpl + def process(self, value: int) -> int: + return value + + pm.register(Plugin()) + caller = pm.hook.process + with warnings.catch_warnings(): + warnings.simplefilter("error") + with pytest.raises(UserWarning, match="deprecated implementation"): + pm.add_hookspecs(Hooks) + + assert not caller.has_spec() + with pytest.warns(UserWarning, match="deprecated implementation") as recorded: + pm.add_hookspecs(Hooks) + assert len(recorded) == 1 + assert pm.hook.process is caller + assert caller(value=5) == [5] diff --git a/testing/test_pluginmanager.py b/testing/test_pluginmanager.py index 06d645d7..10d0aa56 100644 --- a/testing/test_pluginmanager.py +++ b/testing/test_pluginmanager.py @@ -424,8 +424,30 @@ def configure(self): def late(self, qlwkje): pass # pragma: no cover - with pytest.raises(PluginValidationError, match="qlwkje"): - pm.register(hello()) + plugin = hello() + caller = pm.hook.configure + spec = caller.spec + history = caller._call_history + with pytest.raises(PluginValidationError, match="qlwkje") as excinfo: + pm.register(plugin) + + assert excinfo.value.plugin is plugin + assert pm.list_name_plugin() == [] + assert pm.hook.configure is caller + assert caller.spec is spec + assert caller._call_history is history + assert caller.get_hookimpls() == [] + assert pm.hook.late.spec is not None + assert pm.hook.late.spec.namespace is LateSpecs + assert pm.hook.late.get_hookimpls() == [] + + class Valid: + @hookimpl + def late(self, arg): + return arg + + pm.register(Valid()) + assert pm.hook.late(arg=7) == [7] def test_register_hookwrapper_not_a_generator_function(he_pm: PluginManager) -> None: diff --git a/testing/test_registration_rollback.py b/testing/test_registration_rollback.py new file mode 100644 index 00000000..93a980d2 --- /dev/null +++ b/testing/test_registration_rollback.py @@ -0,0 +1,277 @@ +"""Historic replay failures must only undo the failed registration.""" + +import pytest + +from pluggy import HookimplMarker +from pluggy import HookspecMarker +from pluggy import PluginManager + + +hookspec = HookspecMarker("example") +hookimpl = HookimplMarker("example") + + +@pytest.mark.parametrize("exception_type", [RuntimeError, KeyboardInterrupt]) +@pytest.mark.parametrize("fail_in_callback", [False, True]) +def test_failed_replay_cleans_up_and_allows_retry( + pm: PluginManager, exception_type: type[BaseException], fail_in_callback: bool +) -> None: + class Hooks: + @hookspec + def a_existing(self) -> None: ... + + @hookspec(historic=True) + def c_history(self, value: int) -> None: ... + + class Existing: + @hookimpl + def a_existing(self) -> str: + return "existing" + + @hookimpl + def c_history(self, value: int) -> tuple[str, int]: + return "existing", value + + class Empty: + @hookimpl + def d_empty(self) -> None: ... + + error = exception_type("replay failed") + fail = True + calls: list[int] = [] + results: list[tuple[str, int]] = [] + + def callback(result: tuple[str, int]) -> None: + results.append(result) + if fail and fail_in_callback and result == ("new", 20): + raise error + + class Plugin: + @hookimpl + def a_existing(self) -> str: + return "new" + + @hookimpl + def b_new(self) -> str: + return "new hook" + + @hookimpl + def c_history(self, value: int) -> tuple[str, int]: + calls.append(value) + if fail and not fail_in_callback and value == 20: + raise error + return "new", value + + @hookimpl + def z_unprocessed(self) -> None: ... + + pm.add_hookspecs(Hooks) + existing = Existing() + pm.register(existing, "existing") + empty = Empty() + pm.register(empty, "empty") + pm.unregister(empty) + callers = dict(pm.hook.__dict__) + impls = {name: caller.get_hookimpls() for name, caller in callers.items()} + specs = {name: caller.spec for name, caller in callers.items()} + history = pm.hook.c_history._call_history + for value in (10, 20): + pm.hook.c_history.call_historic(callback, {"value": value}) + plugin = Plugin() + + with pytest.raises(exception_type, match="replay failed") as excinfo: + pm.register(plugin, "new") + + assert excinfo.value is error + frame_names = [entry.name for entry in excinfo.traceback] + assert frame_names[-1] == ("callback" if fail_in_callback else "c_history") + assert frame_names.count("register") == 1 + assert pm.list_name_plugin() == [("existing", existing)] + assert pm.get_hookcallers(plugin) is None + assert pm.hook.__dict__ == callers + for name, caller in callers.items(): + assert getattr(pm.hook, name) is caller + assert caller.spec is specs[name] + assert caller.get_hookimpls() == impls[name] + assert pm.hook.c_history._call_history is history + assert history is not None and len(history) == 2 + assert calls == [10, 20] # External effects of replay are retained. + assert results == [("existing", 10), ("existing", 20), ("new", 10)] + ( + [("new", 20)] if fail_in_callback else [] + ) + assert pm.hook.a_existing() == ["existing"] + + fail = False + assert pm.register(plugin, "new") == "new" + assert calls == [10, 20, 10, 20] + assert results[-2:] == [("new", 10), ("new", 20)] + assert pm.hook.a_existing() == ["new", "existing"] + assert pm.hook.b_new() == ["new hook"] + assert pm.hook.z_unprocessed() == [] + pm.unregister(plugin) + assert pm.list_name_plugin() == [("existing", existing)] + for name, caller in callers.items(): + assert caller.get_hookimpls() == impls[name] + + +@pytest.mark.parametrize("adopted_by", ["spec", "plugin", "both"]) +def test_failed_replay_preserves_adopted_new_caller( + pm: PluginManager, adopted_by: str +) -> None: + class Hooks: + @hookspec(historic=True) + def configure(self) -> None: ... + + class LateHooks: + @hookspec + def a_new(self) -> None: ... + + class Nested: + @hookimpl + def a_new(self) -> str: + return "nested" + + nested = Nested() + error = RuntimeError("outer replay failed") + saved = [] + + class Plugin: + @hookimpl + def a_new(self) -> str: + return "outer" # pragma: no cover + + @hookimpl + def configure(self) -> None: + caller = pm.hook.a_new + if adopted_by in ("spec", "both"): + pm.add_hookspecs(LateHooks) + if adopted_by in ("plugin", "both"): + pm.register(nested, "nested") + saved.append((caller, caller.spec, caller.get_hookimpls()[-1:])) + raise error + + pm.add_hookspecs(Hooks) + pm.hook.configure.call_historic() + with pytest.raises(RuntimeError) as excinfo: + pm.register(Plugin(), "outer") + + assert excinfo.value is error + caller, spec, nested_impls = saved[0] + assert pm.hook.a_new is caller + assert caller.spec is spec + assert caller.has_spec() is (adopted_by != "plugin") + assert pm.hook.configure.get_hookimpls() == [] + if adopted_by == "spec": + assert pm.list_name_plugin() == [] + assert caller.get_hookimpls() == [] + assert caller() == [] + else: + assert pm.list_name_plugin() == [("nested", nested)] + assert caller.get_hookimpls() == nested_impls + assert caller() == ["nested"] + + +@pytest.mark.parametrize( + "replacement", ["other", "same", "same_without_hooks", "blocked", "removed"] +) +def test_failed_replay_preserves_reentrant_registry_changes( + pm: PluginManager, replacement: str +) -> None: + class Hooks: + @hookspec(historic=True) + def configure(self) -> None: ... + + class Other: + @hookimpl + def a_new(self) -> str: + return "other" + + other = Other() + error = RuntimeError("replay replaced registration") + + class Plugin: + replace = True + + @hookimpl + def a_new(self) -> str: + return "same" + + @hookimpl + def configure(self) -> None: + if self.replace: + self.replace = False + pm.unregister(name="plugin") + if replacement == "other": + pm.register(other, "plugin") + elif replacement == "same": + pm.register(self, "plugin") + elif replacement == "same_without_hooks": + self.a_new = None # type: ignore[assignment] + self.configure = None # type: ignore[assignment] + pm.register(self, "plugin") + elif replacement == "blocked": + pm.set_blocked("plugin") + raise error + + pm.add_hookspecs(Hooks) + pm.hook.configure.call_historic() + plugin = Plugin() + with pytest.raises(RuntimeError) as excinfo: + pm.register(plugin, "plugin") + + assert excinfo.value is error + if replacement == "other": + assert pm.list_name_plugin() == [("plugin", other)] + assert pm.hook.a_new() == ["other"] + assert pm.hook.configure.get_hookimpls() == [] + elif replacement == "same": + assert pm.list_name_plugin() == [("plugin", plugin)] + assert pm.hook.a_new() == ["same"] + assert len(pm.hook.configure.get_hookimpls()) == 1 + pm.unregister(plugin) + assert pm.hook.a_new.get_hookimpls() == [] + assert pm.hook.configure.get_hookimpls() == [] + elif replacement == "same_without_hooks": + assert pm.list_name_plugin() == [("plugin", plugin)] + assert pm.get_hookcallers(plugin) == [] + assert not hasattr(pm.hook, "a_new") + assert pm.hook.configure.get_hookimpls() == [] + else: + assert pm.get_plugin("plugin") is None + assert pm.is_blocked("plugin") is (replacement == "blocked") + assert not hasattr(pm.hook, "a_new") + assert pm.hook.configure.get_hookimpls() == [] + + +def test_rollback_does_not_call_plugin_equality(pm: PluginManager) -> None: + class Hooks: + @hookspec(historic=True) + def configure(self) -> None: ... + + error = RuntimeError("original replay failure") + + class Plugin: + fail = False + + def __eq__(self, other: object) -> bool: + if self.fail: + raise AssertionError("equality must not mask the replay error") + return self is other + + @hookimpl + def a_new(self) -> None: ... + + @hookimpl + def configure(self) -> None: + self.fail = True + raise error + + pm.add_hookspecs(Hooks) + pm.hook.configure.call_historic() + with pytest.raises(RuntimeError) as excinfo: + pm.register(Plugin(), "plugin") + + assert excinfo.value is error + assert pm.list_name_plugin() == [] + assert not hasattr(pm.hook, "a_new") + assert pm.hook.configure.get_hookimpls() == []