Fix partial state after hookspec validation and historic replay failures(#761)

changelog/761.bugfix.rst

@@ -0,0 +1,9 @@
+:meth:`PluginManager.add_hookspecs() <pluggy.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()
+<pluggy.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.

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:

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]

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: