Add more cases that emit bad-plugin-value (#7284)

* Add x-failing test for issue 7264 case 3

This is the case where a plugin can be imported only after the init-hook
is run, and the init hook is specified in a pylintrc file
We would expect the module to not load in any case, and cause the
emission of a bad-plugin-value message.

* _dynamic_plugins is a dict not a set

This is in preparation for caching the loaded modules, and for storing
other information that will help in identifying times loading is
dependent on init-hook magic.

* Use cached module objects for load_configuration

This fixes case 3 in #7264, and is technically a breaking change, in
that it now emits a message for what would have previously been a silent
failure.

* Interim review comment fixes

1. Remove the xfail that snuck back in after the rebases.
2. Now that fake_home yields a str, update the type hint.

* Add test for bad plugin with duplicated load

This is case 6 in issue #7264, and represents the other silent failure
that is being made non-silent by this change.

* Apply review feedback

- Add an ordering test for CLI arguments
- Add clarifying comments on affected functions
- Tidy up test assertions to be more pythonic

Co-authored-by: Daniël van Noord <[email protected]>
Co-authored-by: Pierre Sassoulas <[email protected]>

Author: 3 people

.pyenchant_pylint_custom_dict.txt

@@ -49,6 +49,7 @@ classmethod
 classmethod's
 classname
 classobj
+CLI
 cls
 cmp
 codebase

doc/whatsnew/fragments/7264.bugfix

@@ -0,0 +1,8 @@
+Fixed a case where custom plugins specified by command line could silently fail.
+
+Specifically, if a plugin relies on the ``init-hook`` option changing ``sys.path`` before
+it can be imported, this will now emit a ``bad-plugin-value`` message. Before this
+change, it would silently fail to register the plugin for use, but would load
+any configuration, which could have unintended effects.
+
+Fixes part of #7264.

pylint/lint/pylinter.py

@@ -18,6 +18,7 @@
 from io import TextIOWrapper
 from pathlib import Path
 from re import Pattern
+from types import ModuleType
 from typing import Any
 
 import astroid
@@ -295,7 +296,7 @@ def __init__(
             str, list[checkers.BaseChecker]
         ] = collections.defaultdict(list)
         """Dictionary of registered and initialized checkers."""
-        self._dynamic_plugins: set[str] = set()
+        self._dynamic_plugins: dict[str, ModuleType | ModuleNotFoundError] = {}
         """Set of loaded plugin names."""
 
         # Attributes related to registering messages and their handling
@@ -361,31 +362,45 @@ def load_default_plugins(self) -> None:
         reporters.initialize(self)
 
     def load_plugin_modules(self, modnames: list[str]) -> None:
-        """Check a list pylint plugins modules, load and register them."""
+        """Check a list of pylint plugins modules, load and register them.
+
+        If a module cannot be loaded, never try to load it again and instead
+        store the error message for later use in ``load_plugin_configuration``
+        below.
+        """
         for modname in modnames:
             if modname in self._dynamic_plugins:
                 continue
-            self._dynamic_plugins.add(modname)
             try:
                 module = astroid.modutils.load_module_from_name(modname)
                 module.register(self)
-            except ModuleNotFoundError:
-                pass
+                self._dynamic_plugins[modname] = module
+            except ModuleNotFoundError as mnf_e:
+                self._dynamic_plugins[modname] = mnf_e
 
     def load_plugin_configuration(self) -> None:
         """Call the configuration hook for plugins.
 
         This walks through the list of plugins, grabs the "load_configuration"
         hook, if exposed, and calls it to allow plugins to configure specific
         settings.
+
+        The result of attempting to load the plugin of the given name
+        is stored in the dynamic plugins dictionary in ``load_plugin_modules`` above.
+
+        ..note::
+            This function previously always tried to load modules again, which
+            led to some confusion and silent failure conditions as described
+            in GitHub issue #7264. Making it use the stored result is more efficient, and
+            means that we avoid the ``init-hook`` problems from before.
         """
-        for modname in self._dynamic_plugins:
-            try:
-                module = astroid.modutils.load_module_from_name(modname)
-                if hasattr(module, "load_configuration"):
-                    module.load_configuration(self)
-            except ModuleNotFoundError as e:
-                self.add_message("bad-plugin-value", args=(modname, e), line=0)
+        for modname, module_or_error in self._dynamic_plugins.items():
+            if isinstance(module_or_error, ModuleNotFoundError):
+                self.add_message(
+                    "bad-plugin-value", args=(modname, module_or_error), line=0
+                )
+            elif hasattr(module_or_error, "load_configuration"):
+                module_or_error.load_configuration(self)
 
     def _load_reporters(self, reporter_names: str) -> None:
         """Load the reporters if they are available on _reporters."""

tests/lint/unittest_lint.py

@@ -19,7 +19,7 @@
 from os import chdir, getcwd
 from os.path import abspath, dirname, join, sep
 from pathlib import Path
-from shutil import rmtree
+from shutil import copytree, rmtree
 
 import platformdirs
 import pytest
@@ -59,12 +59,12 @@
 
 
 @contextmanager
-def fake_home() -> Iterator:
+def fake_home() -> Iterator[str]:
     folder = tempfile.mkdtemp("fake-home")
     old_home = os.environ.get(HOME)
     try:
         os.environ[HOME] = folder
-        yield
+        yield folder
     finally:
         os.environ.pop("PYLINTRC", "")
         if old_home is None:
@@ -524,6 +524,188 @@ def test_load_plugin_command_line() -> None:
     sys.path.remove(dummy_plugin_path)
 
 
+@pytest.mark.usefixtures("pop_pylintrc")
+def test_load_plugin_path_manipulation_case_6() -> None:
+    """Case 6 refers to GitHub issue #7264.
+
+    This is where we supply a plugin we want to load on both the CLI and
+    config file, but that plugin is only loadable after the ``init-hook`` in
+    the config file has run. This is not supported, and was previously a silent
+    failure. This test ensures a ``bad-plugin-value`` message is emitted.
+    """
+    dummy_plugin_path = abspath(join(REGRTEST_DATA_DIR, "dummy_plugin"))
+    with fake_home() as home_path:
+        # construct a basic rc file that just modifies the path
+        pylintrc_file = join(home_path, "pylintrc")
+        with open(pylintrc_file, "w", encoding="utf8") as out:
+            out.writelines(
+                [
+                    "[MASTER]\n",
+                    f"init-hook=\"import sys; sys.path.append(r'{home_path}')\"\n",
+                    "load-plugins=copy_dummy\n",
+                ]
+            )
+
+        copytree(dummy_plugin_path, join(home_path, "copy_dummy"))
+
+        # To confirm we won't load this module _without_ the init hook running.
+        assert home_path not in sys.path
+
+        run = Run(
+            [
+                "--rcfile",
+                pylintrc_file,
+                "--load-plugins",
+                "copy_dummy",
+                join(REGRTEST_DATA_DIR, "empty.py"),
+            ],
+            reporter=testutils.GenericTestReporter(),
+            exit=False,
+        )
+        assert run._rcfile == pylintrc_file
+        assert home_path in sys.path
+        # The module should not be loaded
+        assert not any(ch.name == "copy_dummy" for ch in run.linter.get_checkers())
+
+        # There should be a bad-plugin-message for this module
+        assert len(run.linter.reporter.messages) == 1
+        assert run.linter.reporter.messages[0] == Message(
+            msg_id="E0013",
+            symbol="bad-plugin-value",
+            msg="Plugin 'copy_dummy' is impossible to load, is it installed ? ('No module named 'copy_dummy'')",
+            confidence=interfaces.Confidence(
+                name="UNDEFINED",
+                description="Warning without any associated confidence level.",
+            ),
+            location=MessageLocationTuple(
+                abspath="Command line or configuration file",
+                path="Command line or configuration file",
+                module="Command line or configuration file",
+                obj="",
+                line=1,
+                column=0,
+                end_line=None,
+                end_column=None,
+            ),
+        )
+
+        # Necessary as the executed init-hook modifies sys.path
+        sys.path.remove(home_path)
+
+
+@pytest.mark.usefixtures("pop_pylintrc")
+def test_load_plugin_path_manipulation_case_3() -> None:
+    """Case 3 refers to GitHub issue #7264.
+
+    This is where we supply a plugin we want to load on the CLI only,
+    but that plugin is only loadable after the ``init-hook`` in
+    the config file has run. This is not supported, and was previously a silent
+    failure. This test ensures a ``bad-plugin-value`` message is emitted.
+    """
+    dummy_plugin_path = abspath(join(REGRTEST_DATA_DIR, "dummy_plugin"))
+    with fake_home() as home_path:
+        # construct a basic rc file that just modifies the path
+        pylintrc_file = join(home_path, "pylintrc")
+        with open(pylintrc_file, "w", encoding="utf8") as out:
+            out.writelines(
+                [
+                    "[MASTER]\n",
+                    f"init-hook=\"import sys; sys.path.append(r'{home_path}')\"\n",
+                ]
+            )
+
+        copytree(dummy_plugin_path, join(home_path, "copy_dummy"))
+
+        # To confirm we won't load this module _without_ the init hook running.
+        assert home_path not in sys.path
+
+        run = Run(
+            [
+                "--rcfile",
+                pylintrc_file,
+                "--load-plugins",
+                "copy_dummy",
+                join(REGRTEST_DATA_DIR, "empty.py"),
+            ],
+            reporter=testutils.GenericTestReporter(),
+            exit=False,
+        )
+        assert run._rcfile == pylintrc_file
+        assert home_path in sys.path
+        # The module should not be loaded
+        assert not any(ch.name == "copy_dummy" for ch in run.linter.get_checkers())
+
+        # There should be a bad-plugin-message for this module
+        assert len(run.linter.reporter.messages) == 1
+        assert run.linter.reporter.messages[0] == Message(
+            msg_id="E0013",
+            symbol="bad-plugin-value",
+            msg="Plugin 'copy_dummy' is impossible to load, is it installed ? ('No module named 'copy_dummy'')",
+            confidence=interfaces.Confidence(
+                name="UNDEFINED",
+                description="Warning without any associated confidence level.",
+            ),
+            location=MessageLocationTuple(
+                abspath="Command line or configuration file",
+                path="Command line or configuration file",
+                module="Command line or configuration file",
+                obj="",
+                line=1,
+                column=0,
+                end_line=None,
+                end_column=None,
+            ),
+        )
+
+        # Necessary as the executed init-hook modifies sys.path
+        sys.path.remove(home_path)
+
+
+def test_load_plugin_command_line_before_init_hook() -> None:
+    """Check that the order of 'load-plugins' and 'init-hook' doesn't affect execution."""
+    regrtest_data_dir_abs = abspath(REGRTEST_DATA_DIR)
+
+    run = Run(
+        [
+            "--load-plugins",
+            "dummy_plugin",
+            "--init-hook",
+            f'import sys; sys.path.append("{regrtest_data_dir_abs}")',
+            join(REGRTEST_DATA_DIR, "empty.py"),
+        ],
+        exit=False,
+    )
+    assert (
+        len([ch.name for ch in run.linter.get_checkers() if ch.name == "dummy_plugin"])
+        == 2
+    )
+
+    # Necessary as the executed init-hook modifies sys.path
+    sys.path.remove(regrtest_data_dir_abs)
+
+
+def test_load_plugin_command_line_with_init_hook_command_line() -> None:
+    regrtest_data_dir_abs = abspath(REGRTEST_DATA_DIR)
+
+    run = Run(
+        [
+            "--init-hook",
+            f'import sys; sys.path.append("{regrtest_data_dir_abs}")',
+            "--load-plugins",
+            "dummy_plugin",
+            join(REGRTEST_DATA_DIR, "empty.py"),
+        ],
+        exit=False,
+    )
+    assert (
+        len([ch.name for ch in run.linter.get_checkers() if ch.name == "dummy_plugin"])
+        == 2
+    )
+
+    # Necessary as the executed init-hook modifies sys.path
+    sys.path.remove(regrtest_data_dir_abs)
+
+
 def test_load_plugin_config_file() -> None:
     dummy_plugin_path = join(REGRTEST_DATA_DIR, "dummy_plugin")
     sys.path.append(dummy_plugin_path)