Repository: pytest-dev/pluggy (python)

Issue:
Issue with ordering of hooks
I recently observed a strange issue in `pytest-cases` in conjunction with a fake hook mimicking `pytest-repeat`.

Basically, the problem is that a hook with `tryfirst=True`, `wrapper=True` gets called *after* a plugin with `trylast=True`.
See issue description here : https://github.com/smarie/python-pytest-cases/issues/302

Could it be related with these lines ?
 
https://github.com/pytest-dev/pluggy/blob/136bbe8c98b66cbcefdc2438bf3c59705678249b/src/pluggy/_hooks.py#L542C1-L548C15

```python
            # Find last non-tryfirst nonwrapper method.
            i = len(hookimpls) - 1
            while (
                i >= 0
                and hookimpls[i].tryfirst
                and not (hookimpls[i].hookwrapper or hookimpls[i].wrapper)
            ):
```

It seems to me that the correct statement in the while is rather

```python
            # Find last non-tryfirst nonwrapper method.
            i = len(hookimpls) - 1
            while (
                i >= 0
                and (hookimpls[i].tryfirst
                or hookimpls[i].hookwrapper or hookimpls[i].wrapper)
            ):
```

Am I correct ?


Pull request:
Fix `call_extra` call ordering regression in pluggy 1.1.0
Fix `call_extra` extra methods getting ordered before everything else when there is at least one wrapper impl.

Fix #441.
Regressed in https://github.com/pytest-dev/pluggy/commit/63b7e908b4b22c30d86cd2cff240b3b7aa6da596 - pluggy 1.1.0.

This is adapted from #461 by @vishutupili, separate PR because the existing one can't be updated. Closes #461. Reviewed by me.

Patch:
diff --git a/CHANGELOG.rst b/CHANGELOG.rst
index b7ae00f..2ce5732 100644
--- a/CHANGELOG.rst
+++ b/CHANGELOG.rst
@@ -148,7 +148,7 @@ Features
 --------
 
 - `#282 <https://github.com/pytest-dev/pluggy/issues/282>`_: When registering a hookimpl which is declared as ``hookwrapper=True`` but whose
-  function is not a generator function, a :class:`~pluggy.PluginValidationError` exception is
+  function is not a generator function, a ``PluggyValidationError`` exception is
   now raised.
 
   Previously this problem would cause an error only later, when calling the hook.
@@ -158,7 +158,7 @@ Features
 
   .. code-block:: python
 
-      def my_hook_implementation(arg):
+      def my_hook_real_implementation(arg):
           print("before")
           yield
           print("after")
diff --git a/docs/index.rst b/docs/index.rst
index 42fb8f4..5bc90b7 100644
--- a/docs/index.rst
+++ b/docs/index.rst
@@ -416,11 +416,9 @@ of the hook thus far, or, if the previous calls raised an exception, it is
 :py:meth:`thrown <python:generator.throw>` the exception.
 
 The function should do one of two things:
-
-- Return a value, which can be the same value as received from the ``yield``, or something else entirely.
-
+- Return a value, which can be the same value as received from the ``yield``, or
+something else entirely.
 - Raise an exception.
-
 The return value or exception propagate to further hook wrappers, and finally
 to the hook caller.
 
diff --git a/src/pluggy/_hooks.py b/src/pluggy/_hooks.py
index 916ca70..6623c14 100644
--- a/src/pluggy/_hooks.py
+++ b/src/pluggy/_hooks.py
@@ -389,6 +389,13 @@ class HookCaller:
         #: Name of the hook getting called.
         self.name: Final = name
         self._hookexec: Final = hook_execute
+        # The hookimpls list. The caller iterates it *in reverse*. Format:
+        # 1. trylast nonwrappers
+        # 2. nonwrappers
+        # 3. tryfirst nonwrappers
+        # 4. trylast wrappers
+        # 5. wrappers
+        # 6. tryfirst wrappers
         self._hookimpls: Final[list[HookImpl]] = []
         self._call_history: _CallHistory | None = None
         # TODO: Document, or make private.
@@ -541,10 +548,11 @@ class HookCaller:
             hookimpl = HookImpl(None, "<temp>", method, opts)
             # Find last non-tryfirst nonwrapper method.
             i = len(hookimpls) - 1
-            while (
-                i >= 0
-                and hookimpls[i].tryfirst
-                and not (hookimpls[i].hookwrapper or hookimpls[i].wrapper)
+            while i >= 0 and (
+                # Skip wrappers.
+                (hookimpls[i].hookwrapper or hookimpls[i].wrapper)
+                # Skip tryfirst nonwrappers.
+                or hookimpls[i].tryfirst
             ):
                 i -= 1
             hookimpls.insert(i + 1, hookimpl)


Test patch:
diff --git a/testing/test_hookcaller.py b/testing/test_hookcaller.py
index 88ed131..9eb6f97 100644
--- a/testing/test_hookcaller.py
+++ b/testing/test_hookcaller.py
@@ -1,4 +1,5 @@
 from typing import Callable
+from typing import Generator
 from typing import List
 from typing import Sequence
 from typing import TypeVar
@@ -448,3 +449,74 @@ def test_hook_conflict(pm: PluginManager) -> None:
         "Hook 'conflict' is already registered within namespace "
         "<class 'test_hookcaller.test_hook_conflict.<locals>.Api1'>"
     )
+
+
+def test_call_extra_hook_order(hc: HookCaller, addmeth: AddMeth) -> None:
+    """Ensure that call_extra is calling hooks in the right order."""
+    order = []
+
+    @addmeth(tryfirst=True)
+    def method1() -> str:
+        order.append("1")
+        return "1"
+
+    @addmeth()
+    def method2() -> str:
+        order.append("2")
+        return "2"
+
+    @addmeth(trylast=True)
+    def method3() -> str:
+        order.append("3")
+        return "3"
+
+    @addmeth(wrapper=True, tryfirst=True)
+    def method4() -> Generator[None, str, str]:
+        order.append("4pre")
+        result = yield
+        order.append("4post")
+        return result
+
+    @addmeth(wrapper=True)
+    def method5() -> Generator[None, str, str]:
+        order.append("5pre")
+        result = yield
+        order.append("5post")
+        return result
+
+    @addmeth(wrapper=True, trylast=True)
+    def method6() -> Generator[None, str, str]:
+        order.append("6pre")
+        result = yield
+        order.append("6post")
+        return result
+
+    def extra1() -> str:
+        order.append("extra1")
+        return "extra1"
+
+    def extra2() -> str:
+        order.append("extra2")
+        return "extra2"
+
+    result = hc.call_extra([extra1, extra2], {"arg": "test"})
+    assert order == [
+        "4pre",
+        "5pre",
+        "6pre",
+        "1",
+        "extra2",
+        "extra1",
+        "2",
+        "3",
+        "6post",
+        "5post",
+        "4post",
+    ]
+    assert result == [
+        "1",
+        "extra2",
+        "extra1",
+        "2",
+        "3",
+    ]