fix fixture checker

Anis Da Silva Campos · Anis Da Silva Campos · commit 3987ae1fdda5 · 2023-12-23T22:42:39.000+01:00
the current implementation provokes recursion errors because of the
VariablesChecker.add_message patch not working properly.
This rework fix the issue by replacing the original variable checker by a subclass, without patch.
diff --git a/pylint_pytest/__init__.py b/pylint_pytest/__init__.py
@@ -1,30 +1,21 @@
-import glob
-import importlib
-import inspect
-import os
+from pylint.checkers.variables import VariablesChecker
+from pylint.lint import PyLinter
 
-from .checkers import BasePytestChecker
+from .checkers.class_attr_loader import ClassAttrLoader
+from .checkers.fixture import FixtureChecker
+from .checkers.variables import CustomVariablesChecker
 
 
-def register(linter):
-    """auto discover pylint checker classes"""
-    dirname = os.path.dirname(__file__)
-    for module in glob.glob(os.path.join(dirname, "checkers", "*.py")):
-        # trim file extension
-        module = os.path.splitext(module)[0]
+def register(linter: PyLinter) -> None:
+    """Register the checker classes"""
+    remove_original_variables_checker(linter)
+    linter.register_checker(CustomVariablesChecker(linter))
+    linter.register_checker(FixtureChecker(linter))
+    linter.register_checker(ClassAttrLoader(linter))
 
-        # use relative path only
-        module = module.replace(dirname, "", 1)
 
-        # translate file path into module import path
-        module = module.replace(os.sep, ".")
-
-        checker = importlib.import_module(module, package=os.path.basename(dirname))
-        for attr_name in dir(checker):
-            attr_val = getattr(checker, attr_name)
-            if (
-                attr_val != BasePytestChecker
-                and inspect.isclass(attr_val)
-                and issubclass(attr_val, BasePytestChecker)
-            ):
-                linter.register_checker(attr_val(linter))
+def remove_original_variables_checker(linter: PyLinter) -> None:
+    """We need to remove VariablesChecker before registering CustomVariablesChecker"""
+    variable_checkers = linter._checkers[VariablesChecker.name]  # pylint: disable=protected-access
+    for checker in [x for x in variable_checkers if isinstance(x, VariablesChecker)]:
+        variable_checkers.remove(checker)
diff --git a/pylint_pytest/checkers/fixture.py b/pylint_pytest/checkers/fixture.py
@@ -7,17 +7,15 @@
 
 import astroid
 import pytest
-from pylint.checkers.variables import VariablesChecker
 
 from ..utils import (
     _can_use_fixture,
     _is_pytest_fixture,
     _is_pytest_mark,
     _is_pytest_mark_usefixtures,
-    _is_same_module,
 )
 from . import BasePytestChecker
-from .types import FixtureDict, replacement_add_message
+from .types import FixtureDict
 
 # TODO: support pytest python_files configuration
 FILE_NAME_PATTERNS: tuple[str, ...] = ("test_*.py", "*_test.py")
@@ -75,38 +73,28 @@ class FixtureChecker(BasePytestChecker):
     }
 
     # Store all fixtures discovered by pytest session
-    _pytest_fixtures: FixtureDict = {}
+    pytest_fixtures: FixtureDict = {}
     # Stores all used function arguments
-    _invoked_with_func_args: set[str] = set()
+    invoked_with_func_args: set[str] = set()
     # Stores all invoked fixtures through @pytest.mark.usefixture(...)
-    _invoked_with_usefixtures: set[str] = set()
-    _original_add_message = replacement_add_message
-
-    def open(self):
-        # patch VariablesChecker.add_message
-        FixtureChecker._original_add_message = VariablesChecker.add_message
-        VariablesChecker.add_message = FixtureChecker.patch_add_message
+    invoked_with_usefixtures: set[str] = set()
 
     def close(self):
         """restore & reset class attr for testing"""
-        # restore add_message
-        VariablesChecker.add_message = FixtureChecker._original_add_message
-        FixtureChecker._original_add_message = replacement_add_message
-
         # reset fixture info storage
-        FixtureChecker._pytest_fixtures = {}
-        FixtureChecker._invoked_with_func_args = set()
-        FixtureChecker._invoked_with_usefixtures = set()
+        FixtureChecker.pytest_fixtures = {}
+        FixtureChecker.invoked_with_func_args = set()
+        FixtureChecker.invoked_with_usefixtures = set()
 
     def visit_module(self, node):
         """
         - only run once per module
         - invoke pytest session to collect available fixtures
         - create containers for the module to store args and fixtures
         """
-        FixtureChecker._pytest_fixtures = {}
-        FixtureChecker._invoked_with_func_args = set()
-        FixtureChecker._invoked_with_usefixtures = set()
+        FixtureChecker.pytest_fixtures = {}
+        FixtureChecker.invoked_with_func_args = set()
+        FixtureChecker.invoked_with_usefixtures = set()
 
         is_test_module = False
         for pattern in FILE_NAME_PATTERNS:
@@ -140,7 +128,7 @@ def visit_module(self, node):
                 # restore sys.path
                 sys.path = sys_path
 
-                FixtureChecker._pytest_fixtures = fixture_collector.fixtures
+                FixtureChecker.pytest_fixtures = fixture_collector.fixtures
 
                 legitimate_failure_paths = set(
                     collection_report.nodeid
@@ -224,92 +212,11 @@ def visit_functiondef(self, node):
                     if _is_pytest_mark_usefixtures(decorator):
                         # save all visited fixtures
                         for arg in decorator.args:
-                            self._invoked_with_usefixtures.add(arg.value)
+                            self.invoked_with_usefixtures.add(arg.value)
                     if int(pytest.__version__.split(".")[0]) >= 3 and _is_pytest_fixture(
                         decorator, fixture=False
                     ):
                         # raise deprecated warning for @pytest.yield_fixture
                         self.add_message("deprecated-pytest-yield-fixture", node=node)
             for arg in node.args.args:
-                self._invoked_with_func_args.add(arg.name)
-
-    # pylint: disable=bad-staticmethod-argument # The function itself is an if-return logic.
-    @staticmethod
-    def patch_add_message(
-        self, msgid, line=None, node=None, args=None, confidence=None, col_offset=None
-    ):
-        """
-        - intercept and discard unwanted warning messages
-        """
-        # check W0611 unused-import
-        if msgid == "unused-import":
-            # actual attribute name is not passed as arg so...dirty hack
-            # message is usually in the form of '%s imported from %s (as %)'
-            message_tokens = args.split()
-            fixture_name = message_tokens[0]
-
-            # ignoring 'import %s' message
-            if message_tokens[0] == "import" and len(message_tokens) == 2:
-                pass
-
-            # fixture is defined in other modules and being imported to
-            # conftest for pytest magic
-            elif (
-                isinstance(node.parent, astroid.Module)
-                and node.parent.name.split(".")[-1] == "conftest"
-                and fixture_name in FixtureChecker._pytest_fixtures
-            ):
-                return
-
-            # imported fixture is referenced in test/fixture func
-            elif (
-                fixture_name in FixtureChecker._invoked_with_func_args
-                and fixture_name in FixtureChecker._pytest_fixtures
-            ):
-                if _is_same_module(
-                    fixtures=FixtureChecker._pytest_fixtures,
-                    import_node=node,
-                    fixture_name=fixture_name,
-                ):
-                    return
-
-            # fixture is referenced in @pytest.mark.usefixtures
-            elif (
-                fixture_name in FixtureChecker._invoked_with_usefixtures
-                and fixture_name in FixtureChecker._pytest_fixtures
-            ):
-                if _is_same_module(
-                    fixtures=FixtureChecker._pytest_fixtures,
-                    import_node=node,
-                    fixture_name=fixture_name,
-                ):
-                    return
-
-        # check W0613 unused-argument
-        if (
-            msgid == "unused-argument"
-            and _can_use_fixture(node.parent.parent)
-            and isinstance(node.parent, astroid.Arguments)
-        ):
-            if node.name in FixtureChecker._pytest_fixtures:
-                # argument is used as a fixture
-                return
-
-            fixnames = (
-                arg.name for arg in node.parent.args if arg.name in FixtureChecker._pytest_fixtures
-            )
-            for fixname in fixnames:
-                if node.name in FixtureChecker._pytest_fixtures[fixname][0].argnames:
-                    # argument is used by a fixture
-                    return
-
-        # check W0621 redefined-outer-name
-        if (
-            msgid == "redefined-outer-name"
-            and _can_use_fixture(node.parent.parent)
-            and isinstance(node.parent, astroid.Arguments)
-            and node.name in FixtureChecker._pytest_fixtures
-        ):
-            return
-
-        FixtureChecker._original_add_message(self, msgid, line, node, args, confidence, col_offset)
+                self.invoked_with_func_args.add(arg.name)
diff --git a/pylint_pytest/checkers/variables.py b/pylint_pytest/checkers/variables.py
@@ -0,0 +1,104 @@
+from typing import Any, Optional
+
+from astroid import Arguments, Module
+from astroid.nodes.node_ng import NodeNG
+from pylint.checkers.variables import VariablesChecker
+from pylint.interfaces import Confidence
+
+from pylint_pytest.utils import _can_use_fixture, _is_same_module
+
+from .fixture import FixtureChecker
+
+
+class CustomVariablesChecker(VariablesChecker):
+    """Overrides the default VariablesChecker of pylint to discard unwanted warning messages"""
+
+    def add_message(
+        self,
+        msgid: str,
+        line: Optional[int] = None,
+        node: Optional[NodeNG] = None,
+        args: Any = None,
+        confidence: Confidence = None,
+        col_offset: Optional[int] = None,
+        end_lineno: Optional[int] = None,
+        end_col_offset: Optional[int] = None,
+    ) -> None:
+        """
+        - intercept and discard unwanted warning messages
+        """
+        # check W0611 unused-import
+        if msgid == "unused-import":
+            # actual attribute name is not passed as arg so...dirty hack
+            # message is usually in the form of '%s imported from %s (as %)'
+            message_tokens = args.split()
+            fixture_name = message_tokens[0]
+
+            # ignoring 'import %s' message
+            if message_tokens[0] == "import" and len(message_tokens) == 2:
+                pass
+
+            # fixture is defined in other modules and being imported to
+            # conftest for pytest magic
+            elif (
+                node
+                and isinstance(node.parent, Module)
+                and node.parent.name.split(".")[-1] == "conftest"
+                and fixture_name in FixtureChecker.pytest_fixtures
+            ):
+                return
+
+            # imported fixture is referenced in test/fixture func
+            elif (
+                fixture_name in FixtureChecker.invoked_with_func_args
+                and fixture_name in FixtureChecker.pytest_fixtures
+            ):
+                if _is_same_module(
+                    fixtures=FixtureChecker.pytest_fixtures,
+                    import_node=node,
+                    fixture_name=fixture_name,
+                ):
+                    return
+
+            # fixture is referenced in @pytest.mark.usefixtures
+            elif (
+                fixture_name in FixtureChecker.invoked_with_usefixtures
+                and fixture_name in FixtureChecker.pytest_fixtures
+            ):
+                if _is_same_module(
+                    fixtures=FixtureChecker.pytest_fixtures,
+                    import_node=node,
+                    fixture_name=fixture_name,
+                ):
+                    return
+
+        # check W0613 unused-argument
+        if (
+            msgid == "unused-argument"
+            and node
+            and _can_use_fixture(node.parent.parent)
+            and isinstance(node.parent, Arguments)
+        ):
+            if node.name in FixtureChecker.pytest_fixtures:
+                # argument is used as a fixture
+                return
+
+            fixnames = (
+                arg.name for arg in node.parent.args if arg.name in FixtureChecker.pytest_fixtures
+            )
+            for fixname in fixnames:
+                if node.name in FixtureChecker.pytest_fixtures[fixname][0].argnames:
+                    # argument is used by a fixture
+                    return
+
+        # check W0621 redefined-outer-name
+        if (
+            msgid == "redefined-outer-name"
+            and node
+            and _can_use_fixture(node.parent.parent)
+            and isinstance(node.parent, Arguments)
+            and node.name in FixtureChecker.pytest_fixtures
+        ):
+            return
+
+        super().add_message(msgid, line, node, args, confidence, col_offset)
diff --git a/tests/test_redefined_outer_name.py b/tests/test_redefined_outer_name.py
@@ -1,13 +1,13 @@
 import pytest
 from base_tester import BasePytestTester
-from pylint.checkers.variables import VariablesChecker
 
 from pylint_pytest.checkers.fixture import FixtureChecker
+from pylint_pytest.checkers.variables import CustomVariablesChecker
 
 
 class TestRedefinedOuterName(BasePytestTester):
     CHECKER_CLASS = FixtureChecker
-    IMPACTED_CHECKER_CLASSES = [VariablesChecker]
+    IMPACTED_CHECKER_CLASSES = [CustomVariablesChecker]
     MSG_ID = "redefined-outer-name"
 
     @pytest.mark.parametrize("enable_plugin", [True, False])
diff --git a/tests/test_regression.py b/tests/test_regression.py
@@ -1,15 +1,15 @@
 import pytest
 from base_tester import BasePytestTester
-from pylint.checkers.variables import VariablesChecker
 
 from pylint_pytest.checkers.fixture import FixtureChecker
+from pylint_pytest.checkers.variables import CustomVariablesChecker
 
 
 class TestRegression(BasePytestTester):
     """Covering some behaviors that shouldn't get impacted by the plugin"""
 
     CHECKER_CLASS = FixtureChecker
-    IMPACTED_CHECKER_CLASSES = [VariablesChecker]
+    IMPACTED_CHECKER_CLASSES = [CustomVariablesChecker]
     MSG_ID = "regression"
 
     @pytest.mark.parametrize("enable_plugin", [True, False])
diff --git a/tests/test_unused_argument.py b/tests/test_unused_argument.py
@@ -1,13 +1,13 @@
 import pytest
 from base_tester import BasePytestTester
-from pylint.checkers.variables import VariablesChecker
 
 from pylint_pytest.checkers.fixture import FixtureChecker
+from pylint_pytest.checkers.variables import CustomVariablesChecker
 
 
 class TestUnusedArgument(BasePytestTester):
     CHECKER_CLASS = FixtureChecker
-    IMPACTED_CHECKER_CLASSES = [VariablesChecker]
+    IMPACTED_CHECKER_CLASSES = [CustomVariablesChecker]
     MSG_ID = "unused-argument"
 
     @pytest.mark.parametrize("enable_plugin", [True, False])
diff --git a/tests/test_unused_import.py b/tests/test_unused_import.py
@@ -1,13 +1,13 @@
 import pytest
 from base_tester import BasePytestTester
-from pylint.checkers.variables import VariablesChecker
 
 from pylint_pytest.checkers.fixture import FixtureChecker
+from pylint_pytest.checkers.variables import CustomVariablesChecker
 
 
 class TestUnusedImport(BasePytestTester):
     CHECKER_CLASS = FixtureChecker
-    IMPACTED_CHECKER_CLASSES = [VariablesChecker]
+    IMPACTED_CHECKER_CLASSES = [CustomVariablesChecker]
     MSG_ID = "unused-import"
 
     @pytest.mark.parametrize("enable_plugin", [True, False])
