diff --git a/.pyenchant_pylint_custom_dict.txt b/.pyenchant_pylint_custom_dict.txt index a17cb040fc..f223005f65 100644 --- a/.pyenchant_pylint_custom_dict.txt +++ b/.pyenchant_pylint_custom_dict.txt @@ -49,6 +49,7 @@ classmethod classmethod's classname classobj +CLI cls cmp codebase diff --git a/doc/whatsnew/fragments/7264.bugfix b/doc/whatsnew/fragments/7264.bugfix new file mode 100644 index 0000000000..dc2aa5d401 --- /dev/null +++ b/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. diff --git a/pylint/lint/pylinter.py b/pylint/lint/pylinter.py index 8fa47ffe83..28ad79e4ca 100644 --- a/pylint/lint/pylinter.py +++ b/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,16 +362,21 @@ 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. @@ -378,14 +384,23 @@ def load_plugin_configuration(self) -> None: 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.""" diff --git a/tests/lint/unittest_lint.py b/tests/lint/unittest_lint.py index 04b275750e..80532dea71 100644 --- a/tests/lint/unittest_lint.py +++ b/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 @@ -60,12 +60,12 @@ @contextmanager -def fake_home() -> Iterator[None]: +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: @@ -525,6 +525,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)