Validate all hookimpls before registering a plugin

changelog/733.bugfix.rst

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

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:

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)