issue a warning when Item and Collector are used in diamond inheritance (#8447)

* issue a warning when Items and Collector form a diamond

addresses #8435

* Apply suggestions from code review

Co-authored-by: Ran Benita <[email protected]>

* Return support for the broken File/Item hybrids

* adds deprecation
* ads necessary support code in node construction

* fix incorrect mypy based assertions

* add docs for deprecation of Item/File inheritance

* warn when a non-cooperative ctor is encountered

* use getattr instead of cast to get the class __init__ for legacy ctors

* update documentation references for node inheritance

* clean up file+item inheritance test

enhance docs
move import upwards

Co-authored-by: Ran Benita <[email protected]>

Author: RonnyPfannschmidt

changelog/8447.deprecation.rst

@@ -0,0 +1,4 @@
+Defining a custom pytest node type which is both an item and a collector now issues a warning.
+It was never sanely supported and triggers hard to debug errors.
+
+Instead, a separate collector node should be used, which collects the item. See :ref:`non-python tests` for an example.

doc/en/deprecations.rst

@@ -42,6 +42,20 @@ As pytest tries to move off `py.path.local <https://py.readthedocs.io/en/latest/
 
 Pytest will provide compatibility for quite a while.
 
+Diamond inheritance between :class:`pytest.File` and :class:`pytest.Item`
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+.. deprecated:: 6.3
+
+Inheriting from both Item and file at once has never been supported officially,
+however some plugins providing linting/code analysis have been using this as a hack.
+
+This practice is now officially deprecated and a common way to fix this is `example pr fixing inheritance`_.
+
+
+
+.. _example pr fixing inheritance: https://github.com/asmeurer/pytest-flakes/pull/40/files
+
 
 Backward compatibilities in ``Parser.addoption``
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

src/_pytest/main.py

@@ -484,7 +484,7 @@ def __init__(self, config: Config) -> None:
 
     @classmethod
     def from_config(cls, config: Config) -> "Session":
-        session: Session = cls._create(config)
+        session: Session = cls._create(config=config)
         return session
 
     def __repr__(self) -> str:

src/_pytest/nodes.py

@@ -1,8 +1,10 @@
 import os
 import warnings
+from inspect import signature
 from pathlib import Path
 from typing import Any
 from typing import Callable
+from typing import cast
 from typing import Iterable
 from typing import Iterator
 from typing import List
@@ -34,6 +36,7 @@
 from _pytest.pathlib import absolutepath
 from _pytest.pathlib import commonpath
 from _pytest.store import Store
+from _pytest.warning_types import PytestWarning
 
 if TYPE_CHECKING:
     # Imported here due to circular import.
@@ -125,7 +128,20 @@ def __call__(self, *k, **kw):
         fail(msg, pytrace=False)
 
     def _create(self, *k, **kw):
-        return super().__call__(*k, **kw)
+        try:
+            return super().__call__(*k, **kw)
+        except TypeError:
+            sig = signature(getattr(self, "__init__"))
+            known_kw = {k: v for k, v in kw.items() if k in sig.parameters}
+            from .warning_types import PytestDeprecationWarning
+
+            warnings.warn(
+                PytestDeprecationWarning(
+                    f"{self} is not using a cooperative constructor and only takes {set(known_kw)}"
+                )
+            )
+
+            return super().__call__(*k, **known_kw)
 
 
 class Node(metaclass=NodeMeta):
@@ -539,26 +555,39 @@ def _check_initialpaths_for_relpath(session: "Session", path: Path) -> Optional[
 class FSCollector(Collector):
     def __init__(
         self,
-        fspath: Optional[LEGACY_PATH],
-        path: Optional[Path],
-        parent=None,
+        fspath: Optional[LEGACY_PATH] = None,
+        path_or_parent: Optional[Union[Path, Node]] = None,
+        path: Optional[Path] = None,
+        name: Optional[str] = None,
+        parent: Optional[Node] = None,
         config: Optional[Config] = None,
         session: Optional["Session"] = None,
         nodeid: Optional[str] = None,
     ) -> None:
+        if path_or_parent:
+            if isinstance(path_or_parent, Node):
+                assert parent is None
+                parent = cast(FSCollector, path_or_parent)
+            elif isinstance(path_or_parent, Path):
+                assert path is None
+                path = path_or_parent
+
         path, fspath = _imply_path(path, fspath=fspath)
-        name = path.name
-        if parent is not None and parent.path != path:
-            try:
-                rel = path.relative_to(parent.path)
-            except ValueError:
-                pass
-            else:
-                name = str(rel)
-            name = name.replace(os.sep, SEP)
+        if name is None:
+            name = path.name
+            if parent is not None and parent.path != path:
+                try:
+                    rel = path.relative_to(parent.path)
+                except ValueError:
+                    pass
+                else:
+                    name = str(rel)
+                name = name.replace(os.sep, SEP)
         self.path = path
 
-        session = session or parent.session
+        if session is None:
+            assert parent is not None
+            session = parent.session
 
         if nodeid is None:
             try:
@@ -570,7 +599,12 @@ def __init__(
                 nodeid = nodeid.replace(os.sep, SEP)
 
         super().__init__(
-            name, parent, config, session, nodeid=nodeid, fspath=fspath, path=path
+            name=name,
+            parent=parent,
+            config=config,
+            session=session,
+            nodeid=nodeid,
+            path=path,
         )
 
     @classmethod
@@ -610,15 +644,37 @@ class Item(Node):
 
     nextitem = None
 
+    def __init_subclass__(cls) -> None:
+        problems = ", ".join(
+            base.__name__ for base in cls.__bases__ if issubclass(base, Collector)
+        )
+        if problems:
+            warnings.warn(
+                f"{cls.__name__} is an Item subclass and should not be a collector, "
+                f"however its bases {problems} are collectors.\n"
+                "Please split the Collectors and the Item into separate node types.\n"
+                "Pytest Doc example: https://docs.pytest.org/en/latest/example/nonpython.html\n"
+                "example pull request on a plugin: https://github.com/asmeurer/pytest-flakes/pull/40/",
+                PytestWarning,
+            )
+
     def __init__(
         self,
         name,
         parent=None,
         config: Optional[Config] = None,
         session: Optional["Session"] = None,
         nodeid: Optional[str] = None,
+        **kw,
     ) -> None:
-        super().__init__(name, parent, config, session, nodeid=nodeid)
+        super().__init__(
+            name=name,
+            parent=parent,
+            config=config,
+            session=session,
+            nodeid=nodeid,
+            **kw,
+        )
         self._report_sections: List[Tuple[str, str, str]] = []
 
         #: A list of tuples (name, value) that holds user defined properties

testing/test_nodes.py

@@ -5,6 +5,7 @@
 
 import pytest
 from _pytest import nodes
+from _pytest.compat import legacy_path
 from _pytest.pytester import Pytester
 from _pytest.warning_types import PytestWarning
 
@@ -39,6 +40,36 @@ def test_node_from_parent_disallowed_arguments() -> None:
         nodes.Node.from_parent(None, config=None)  # type: ignore[arg-type]
 
 
+def test_subclassing_both_item_and_collector_deprecated(
+    request, tmp_path: Path
+) -> None:
+    """
+    Verifies we warn on diamond inheritance
+    as well as correctly managing legacy inheritance ctors with missing args
+    as found in plugins
+    """
+
+    with pytest.warns(
+        PytestWarning,
+        match=(
+            "(?m)SoWrong is an Item subclass and should not be a collector, however its bases File are collectors.\n"
+            "Please split the Collectors and the Item into separate node types.\n.*"
+        ),
+    ):
+
+        class SoWrong(nodes.File, nodes.Item):
+            def __init__(self, fspath, parent):
+                """Legacy ctor with legacy call # don't wana see"""
+                super().__init__(fspath, parent)
+
+    with pytest.warns(
+        PytestWarning, match=".*SoWrong.* not using a cooperative constructor.*"
+    ):
+        SoWrong.from_parent(
+            request.session, fspath=legacy_path(tmp_path / "broken.txt")
+        )
+
+
 @pytest.mark.parametrize(
     "warn_type, msg", [(DeprecationWarning, "deprecated"), (PytestWarning, "pytest")]
 )