Fix #733: roll back PluginManager.register on validation failure

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.

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:

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