From bc5190d16b99ac47e38815b0b547e2583bf9be18 Mon Sep 17 00:00:00 2001 From: daogilvie Date: Tue, 9 Aug 2022 17:13:14 +0100 Subject: [PATCH 01/17] 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. --- tests/lint/unittest_lint.py | 92 ++++++++++++++++++++++++++++++++++++- 1 file changed, 90 insertions(+), 2 deletions(-) diff --git a/tests/lint/unittest_lint.py b/tests/lint/unittest_lint.py index 04b275750e..67aa31ebb9 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 copy, copytree, rmtree import platformdirs import pytest @@ -65,7 +65,7 @@ def fake_home() -> Iterator[None]: 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,94 @@ def test_load_plugin_command_line() -> None: sys.path.remove(dummy_plugin_path) +@pytest.mark.usefixtures("pop_pylintrc") +@pytest.mark.xfail +def test_load_plugin_command_line_with_inithook_in_config_bad_value() -> None: + 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") as out: + out.writelines( + [ + "[MASTER]\n", + f"init-hook=\"import sys; sys.path.append('{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 ( + len( + [ch.name for ch in run.linter.get_checkers() if ch.name == "copy_dummy"] + ) + == 0 + ) + + # 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, + ), + ) + + + sys.path.remove(home_path) + + +def test_load_plugin_command_line_with_inithook_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 + ) + + 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) From 0ce83f0606b56f52fcec27722c3921744d633edf Mon Sep 17 00:00:00 2001 From: Drum Ogilvie Date: Mon, 29 Aug 2022 11:57:44 +0100 Subject: [PATCH 02/17] _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. --- pylint/lint/pylinter.py | 11 +++++++---- tests/lint/unittest_lint.py | 5 ++--- 2 files changed, 9 insertions(+), 7 deletions(-) diff --git a/pylint/lint/pylinter.py b/pylint/lint/pylinter.py index 8fa47ffe83..83e16a0593 100644 --- a/pylint/lint/pylinter.py +++ b/pylint/lint/pylinter.py @@ -18,7 +18,8 @@ from io import TextIOWrapper from pathlib import Path from re import Pattern -from typing import Any +from types import ModuleType +from typing import Any, Optional import astroid from astroid import nodes @@ -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 | None] = {} """Set of loaded plugin names.""" # Attributes related to registering messages and their handling @@ -363,12 +364,14 @@ def load_default_plugins(self) -> None: def load_plugin_modules(self, modnames: list[str]) -> None: """Check a list pylint plugins modules, load and register them.""" for modname in modnames: - if modname in self._dynamic_plugins: + if self._dynamic_plugins.get(modname, False): continue - self._dynamic_plugins.add(modname) + + self._dynamic_plugins[modname] = None try: module = astroid.modutils.load_module_from_name(modname) module.register(self) + self._dynamic_plugins[modname] = module except ModuleNotFoundError: pass diff --git a/tests/lint/unittest_lint.py b/tests/lint/unittest_lint.py index 67aa31ebb9..72a60dcd01 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 copy, copytree, rmtree +from shutil import copytree, rmtree import platformdirs import pytest @@ -540,7 +540,7 @@ def test_load_plugin_command_line_with_inithook_in_config_bad_value() -> None: ] ) - copytree(dummy_plugin_path, join(home_path , "copy_dummy")) + 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 @@ -588,7 +588,6 @@ def test_load_plugin_command_line_with_inithook_in_config_bad_value() -> None: ), ) - sys.path.remove(home_path) From 7855d5330f3ddcd57338378b348e299592afd8a6 Mon Sep 17 00:00:00 2001 From: Drum Ogilvie Date: Mon, 29 Aug 2022 12:29:57 +0100 Subject: [PATCH 03/17] 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. --- pylint/lint/pylinter.py | 24 ++++++++++-------------- 1 file changed, 10 insertions(+), 14 deletions(-) diff --git a/pylint/lint/pylinter.py b/pylint/lint/pylinter.py index 83e16a0593..eeadebf05f 100644 --- a/pylint/lint/pylinter.py +++ b/pylint/lint/pylinter.py @@ -19,7 +19,7 @@ from pathlib import Path from re import Pattern from types import ModuleType -from typing import Any, Optional +from typing import Any import astroid from astroid import nodes @@ -296,7 +296,7 @@ def __init__( str, list[checkers.BaseChecker] ] = collections.defaultdict(list) """Dictionary of registered and initialized checkers.""" - self._dynamic_plugins: dict[str, ModuleType | None] = {} + self._dynamic_plugins: dict[str, ModuleType | ModuleNotFoundError] = {} """Set of loaded plugin names.""" # Attributes related to registering messages and their handling @@ -364,16 +364,14 @@ def load_default_plugins(self) -> None: def load_plugin_modules(self, modnames: list[str]) -> None: """Check a list pylint plugins modules, load and register them.""" for modname in modnames: - if self._dynamic_plugins.get(modname, False): + if modname in self._dynamic_plugins: continue - - self._dynamic_plugins[modname] = None try: module = astroid.modutils.load_module_from_name(modname) module.register(self) self._dynamic_plugins[modname] = module - except ModuleNotFoundError: - pass + except ModuleNotFoundError as mnf_e: + self._dynamic_plugins[modname] = mnf_e def load_plugin_configuration(self) -> None: """Call the configuration hook for plugins. @@ -382,13 +380,11 @@ def load_plugin_configuration(self) -> None: hook, if exposed, and calls it to allow plugins to configure specific settings. """ - 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.""" From 3e35d38fdc41d7ecfef0dbe8b122c04d0a11ec7e Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Mon, 29 Aug 2022 11:54:53 +0000 Subject: [PATCH 04/17] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- pylint/lint/pylinter.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/pylint/lint/pylinter.py b/pylint/lint/pylinter.py index eeadebf05f..c365f634bf 100644 --- a/pylint/lint/pylinter.py +++ b/pylint/lint/pylinter.py @@ -382,7 +382,9 @@ def load_plugin_configuration(self) -> None: """ 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) + 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) From b425550fd2ca1b77e99e7e1b87288dc1b0f9243b Mon Sep 17 00:00:00 2001 From: daogilvie Date: Wed, 31 Aug 2022 09:21:56 +0100 Subject: [PATCH 05/17] 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. --- tests/lint/unittest_lint.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/tests/lint/unittest_lint.py b/tests/lint/unittest_lint.py index 72a60dcd01..8141e7b7b4 100644 --- a/tests/lint/unittest_lint.py +++ b/tests/lint/unittest_lint.py @@ -60,7 +60,7 @@ @contextmanager -def fake_home() -> Iterator[None]: +def fake_home() -> Iterator[str]: folder = tempfile.mkdtemp("fake-home") old_home = os.environ.get(HOME) try: @@ -526,13 +526,12 @@ def test_load_plugin_command_line() -> None: @pytest.mark.usefixtures("pop_pylintrc") -@pytest.mark.xfail def test_load_plugin_command_line_with_inithook_in_config_bad_value() -> None: 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") as out: + with open(pylintrc_file, "w", encoding="utf8") as out: out.writelines( [ "[MASTER]\n", From b1e02187af55a892487dd65300ce6fd9102bb0d1 Mon Sep 17 00:00:00 2001 From: Drum Ogilvie Date: Sat, 3 Sep 2022 11:58:16 +0100 Subject: [PATCH 06/17] 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. --- tests/lint/unittest_lint.py | 65 +++++++++++++++++++++++++++++++++++++ 1 file changed, 65 insertions(+) diff --git a/tests/lint/unittest_lint.py b/tests/lint/unittest_lint.py index 8141e7b7b4..1377c4f734 100644 --- a/tests/lint/unittest_lint.py +++ b/tests/lint/unittest_lint.py @@ -524,6 +524,71 @@ def test_load_plugin_command_line() -> None: sys.path.remove(dummy_plugin_path) +@pytest.mark.usefixtures("pop_pylintrc") +def test_load_plugin_command_line_and_config_with_inithook_in_config_bad_value() -> None: + 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('{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 ( + len( + [ch.name for ch in run.linter.get_checkers() if ch.name == "copy_dummy"] + ) + == 0 + ) + + # 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, + ), + ) + + sys.path.remove(home_path) @pytest.mark.usefixtures("pop_pylintrc") def test_load_plugin_command_line_with_inithook_in_config_bad_value() -> None: From e0139caf365124e50e4e3c798087838e8d94cca9 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Sat, 3 Sep 2022 11:00:25 +0000 Subject: [PATCH 07/17] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- tests/lint/unittest_lint.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/tests/lint/unittest_lint.py b/tests/lint/unittest_lint.py index 1377c4f734..5178ab7fc8 100644 --- a/tests/lint/unittest_lint.py +++ b/tests/lint/unittest_lint.py @@ -524,6 +524,7 @@ def test_load_plugin_command_line() -> None: sys.path.remove(dummy_plugin_path) + @pytest.mark.usefixtures("pop_pylintrc") def test_load_plugin_command_line_and_config_with_inithook_in_config_bad_value() -> None: dummy_plugin_path = abspath(join(REGRTEST_DATA_DIR, "dummy_plugin")) @@ -535,8 +536,7 @@ def test_load_plugin_command_line_and_config_with_inithook_in_config_bad_value() [ "[MASTER]\n", f"init-hook=\"import sys; sys.path.append('{home_path}')\"\n", - "load-plugins=copy_dummy\n" - + "load-plugins=copy_dummy\n", ] ) @@ -590,6 +590,7 @@ def test_load_plugin_command_line_and_config_with_inithook_in_config_bad_value() sys.path.remove(home_path) + @pytest.mark.usefixtures("pop_pylintrc") def test_load_plugin_command_line_with_inithook_in_config_bad_value() -> None: dummy_plugin_path = abspath(join(REGRTEST_DATA_DIR, "dummy_plugin")) From 0aa802ffb588bed104d55d03b895c2c5ae27b1c5 Mon Sep 17 00:00:00 2001 From: daogilvie Date: Mon, 5 Sep 2022 10:50:10 +0100 Subject: [PATCH 08/17] Add towncrier fragment --- doc/whatsnew/fragments/7264.bugfix | 8 ++++++++ 1 file changed, 8 insertions(+) create mode 100644 doc/whatsnew/fragments/7264.bugfix diff --git a/doc/whatsnew/fragments/7264.bugfix b/doc/whatsnew/fragments/7264.bugfix new file mode 100644 index 0000000000..75a3579f71 --- /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 init-hook changing the system 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. From 6812fe9b79134015f2177ff6110cfdc883519d1a Mon Sep 17 00:00:00 2001 From: daogilvie Date: Tue, 6 Sep 2022 09:33:51 +0100 Subject: [PATCH 09/17] Use raw string in the init hook syspath value This is because where we put the paths into the init hook files, the back slashes were causing bad escapes. This means we need to write the string as raw to avoid escaping in the eval. --- tests/lint/unittest_lint.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/lint/unittest_lint.py b/tests/lint/unittest_lint.py index 5178ab7fc8..cc7d318ec6 100644 --- a/tests/lint/unittest_lint.py +++ b/tests/lint/unittest_lint.py @@ -535,7 +535,7 @@ def test_load_plugin_command_line_and_config_with_inithook_in_config_bad_value() out.writelines( [ "[MASTER]\n", - f"init-hook=\"import sys; sys.path.append('{home_path}')\"\n", + f"init-hook=\"import sys; sys.path.append(r'{home_path}')\"\n", "load-plugins=copy_dummy\n", ] ) @@ -601,7 +601,7 @@ def test_load_plugin_command_line_with_inithook_in_config_bad_value() -> None: out.writelines( [ "[MASTER]\n", - f"init-hook=\"import sys; sys.path.append('{home_path}')\"\n", + f"init-hook=\"import sys; sys.path.append(r'{home_path}')\"\n", ] ) From cdf1ab842494ba788cd57368f74787cc1001d0aa Mon Sep 17 00:00:00 2001 From: Drummond Ogilvie Date: Tue, 6 Sep 2022 16:20:16 +0100 Subject: [PATCH 10/17] Apply towncrier fragment improvements MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Thanks @DanielNoord Co-authored-by: Daniël van Noord <13665637+DanielNoord@users.noreply.github.com> --- doc/whatsnew/fragments/7264.bugfix | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/doc/whatsnew/fragments/7264.bugfix b/doc/whatsnew/fragments/7264.bugfix index 75a3579f71..dc2aa5d401 100644 --- a/doc/whatsnew/fragments/7264.bugfix +++ b/doc/whatsnew/fragments/7264.bugfix @@ -1,7 +1,7 @@ -Fixed a case where custom plugins specified by command line could silently fail +Fixed a case where custom plugins specified by command line could silently fail. -Specifically, if a plugin relies on init-hook changing the system path before -it can be imported, this will now emit a bad-plugin-value message. Before this +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. From cc6b39195db256982e0ef04aec8f6d2a27b4878d Mon Sep 17 00:00:00 2001 From: daogilvie Date: Tue, 6 Sep 2022 17:43:18 +0100 Subject: [PATCH 11/17] Apply review feedback - Add an ordering test for CLI arguments - Add clarifying comments on affected functions - Tidy up test assertions to be more pythonic --- pylint/lint/pylinter.py | 20 +++++++++++++- tests/lint/unittest_lint.py | 53 +++++++++++++++++++++++++++---------- 2 files changed, 58 insertions(+), 15 deletions(-) diff --git a/pylint/lint/pylinter.py b/pylint/lint/pylinter.py index c365f634bf..d6a0def76f 100644 --- a/pylint/lint/pylinter.py +++ b/pylint/lint/pylinter.py @@ -362,7 +362,12 @@ 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 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 @@ -379,9 +384,22 @@ 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 dynamic plugins dictionary stores the result of attempting to load + the plugin of the given name 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 #7264. Making it use the stored result is more efficient, and + means that we avoid the ``init-hook`` problems from before. """ for modname, module_or_error in self._dynamic_plugins.items(): if isinstance(module_or_error, ModuleNotFoundError): + # If we tried to import the module again now, it might actually + # succeed if init-hook changes to the syspath made it possible. + # It would import fine, and load configuration, but would not + # have been registered. Safer and cleaner to emit this message. self.add_message( "bad-plugin-value", args=(modname, module_or_error), line=0 ) diff --git a/tests/lint/unittest_lint.py b/tests/lint/unittest_lint.py index cc7d318ec6..dda3ee1718 100644 --- a/tests/lint/unittest_lint.py +++ b/tests/lint/unittest_lint.py @@ -526,7 +526,14 @@ def test_load_plugin_command_line() -> None: @pytest.mark.usefixtures("pop_pylintrc") -def test_load_plugin_command_line_and_config_with_inithook_in_config_bad_value() -> None: +def test_load_plugin_path_manipulation_case_6() -> None: + """ + Case 6 refers to 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 @@ -559,12 +566,7 @@ def test_load_plugin_command_line_and_config_with_inithook_in_config_bad_value() assert run._rcfile == pylintrc_file assert home_path in sys.path # The module should not be loaded - assert ( - len( - [ch.name for ch in run.linter.get_checkers() if ch.name == "copy_dummy"] - ) - == 0 - ) + 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 @@ -592,7 +594,14 @@ def test_load_plugin_command_line_and_config_with_inithook_in_config_bad_value() @pytest.mark.usefixtures("pop_pylintrc") -def test_load_plugin_command_line_with_inithook_in_config_bad_value() -> None: +def test_load_plugin_path_manipulation_case_3() -> None: + """ + Case 3 refers to issue #7264. + This is where we supply a plugin we want to load on both 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 @@ -624,12 +633,7 @@ def test_load_plugin_command_line_with_inithook_in_config_bad_value() -> None: assert run._rcfile == pylintrc_file assert home_path in sys.path # The module should not be loaded - assert ( - len( - [ch.name for ch in run.linter.get_checkers() if ch.name == "copy_dummy"] - ) - == 0 - ) + 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 @@ -656,6 +660,27 @@ def test_load_plugin_command_line_with_inithook_in_config_bad_value() -> None: sys.path.remove(home_path) +def test_load_plugin_command_line_before_init_hook() -> None: + 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 + ) + + sys.path.remove(regrtest_data_dir_abs) + + def test_load_plugin_command_line_with_inithook_command_line() -> None: regrtest_data_dir_abs = abspath(REGRTEST_DATA_DIR) From dd9857b18abaf4bdbc985457e40be47dcd5218af Mon Sep 17 00:00:00 2001 From: Drummond Ogilvie Date: Wed, 7 Sep 2022 08:35:49 +0100 Subject: [PATCH 12/17] Add documentation improvements from review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Thanks again @DanielNoord, your patience is appreciated. Co-authored-by: Daniël van Noord <13665637+DanielNoord@users.noreply.github.com> --- pylint/lint/pylinter.py | 12 ++++-------- tests/lint/unittest_lint.py | 11 ++++++----- 2 files changed, 10 insertions(+), 13 deletions(-) diff --git a/pylint/lint/pylinter.py b/pylint/lint/pylinter.py index d6a0def76f..07fa79e152 100644 --- a/pylint/lint/pylinter.py +++ b/pylint/lint/pylinter.py @@ -362,7 +362,7 @@ 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`` @@ -385,21 +385,17 @@ def load_plugin_configuration(self) -> None: hook, if exposed, and calls it to allow plugins to configure specific settings. - The dynamic plugins dictionary stores the result of attempting to load - the plugin of the given name in ``load_plugin_modules`` above. + 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 #7264. Making it use the stored result is more efficient, and + 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, module_or_error in self._dynamic_plugins.items(): if isinstance(module_or_error, ModuleNotFoundError): - # If we tried to import the module again now, it might actually - # succeed if init-hook changes to the syspath made it possible. - # It would import fine, and load configuration, but would not - # have been registered. Safer and cleaner to emit this message. self.add_message( "bad-plugin-value", args=(modname, module_or_error), line=0 ) diff --git a/tests/lint/unittest_lint.py b/tests/lint/unittest_lint.py index dda3ee1718..b984d78c11 100644 --- a/tests/lint/unittest_lint.py +++ b/tests/lint/unittest_lint.py @@ -527,8 +527,8 @@ def test_load_plugin_command_line() -> None: @pytest.mark.usefixtures("pop_pylintrc") def test_load_plugin_path_manipulation_case_6() -> None: - """ - Case 6 refers to issue #7264. + """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 @@ -595,9 +595,9 @@ def test_load_plugin_path_manipulation_case_6() -> None: @pytest.mark.usefixtures("pop_pylintrc") def test_load_plugin_path_manipulation_case_3() -> None: - """ - Case 3 refers to issue #7264. - This is where we supply a plugin we want to load on both the CLI only, + """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. @@ -661,6 +661,7 @@ def test_load_plugin_path_manipulation_case_3() -> None: 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( From ef8bcf7c3d5ed0b79a619bb84ed87a55e2d46c77 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dani=C3=ABl=20van=20Noord?= <13665637+DanielNoord@users.noreply.github.com> Date: Wed, 7 Sep 2022 10:31:58 +0200 Subject: [PATCH 13/17] Final comments and changes --- tests/lint/unittest_lint.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/tests/lint/unittest_lint.py b/tests/lint/unittest_lint.py index b984d78c11..bac87670ae 100644 --- a/tests/lint/unittest_lint.py +++ b/tests/lint/unittest_lint.py @@ -590,6 +590,7 @@ def test_load_plugin_path_manipulation_case_6() -> None: ), ) + # Necessary as the executed init-hook modifies sys.path sys.path.remove(home_path) @@ -657,6 +658,7 @@ def test_load_plugin_path_manipulation_case_3() -> None: ), ) + # Necessary as the executed init-hook modifies sys.path sys.path.remove(home_path) @@ -678,11 +680,12 @@ def test_load_plugin_command_line_before_init_hook() -> None: 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_inithook_command_line() -> None: +def test_load_plugin_command_line_with_init_hook_command_line() -> None: regrtest_data_dir_abs = abspath(REGRTEST_DATA_DIR) run = Run( @@ -700,6 +703,7 @@ def test_load_plugin_command_line_with_inithook_command_line() -> None: == 2 ) + # Necessary as the executed init-hook modifies sys.path sys.path.remove(regrtest_data_dir_abs) From 5ad2e8b592c227b9df832febba3595736fe151d2 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Wed, 7 Sep 2022 08:33:08 +0000 Subject: [PATCH 14/17] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- tests/lint/unittest_lint.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/lint/unittest_lint.py b/tests/lint/unittest_lint.py index bac87670ae..c839da2bd4 100644 --- a/tests/lint/unittest_lint.py +++ b/tests/lint/unittest_lint.py @@ -680,7 +680,7 @@ def test_load_plugin_command_line_before_init_hook() -> None: 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) From c5d145fe5a79c36868018d96948b1d8ad98f50c4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dani=C3=ABl=20van=20Noord?= <13665637+DanielNoord@users.noreply.github.com> Date: Wed, 7 Sep 2022 10:35:11 +0200 Subject: [PATCH 15/17] Github > GitHub --- pylint/lint/pylinter.py | 2 +- tests/lint/unittest_lint.py | 12 ++++++------ 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/pylint/lint/pylinter.py b/pylint/lint/pylinter.py index 07fa79e152..28ad79e4ca 100644 --- a/pylint/lint/pylinter.py +++ b/pylint/lint/pylinter.py @@ -391,7 +391,7 @@ def load_plugin_configuration(self) -> None: ..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 + 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, module_or_error in self._dynamic_plugins.items(): diff --git a/tests/lint/unittest_lint.py b/tests/lint/unittest_lint.py index c839da2bd4..4c5f2de770 100644 --- a/tests/lint/unittest_lint.py +++ b/tests/lint/unittest_lint.py @@ -1,6 +1,6 @@ # Licensed under the GPL: https://www.gnu.org/licenses/old-licenses/gpl-2.0.html -# For details: https://github.com/PyCQA/pylint/blob/main/LICENSE -# Copyright (c) https://github.com/PyCQA/pylint/blob/main/CONTRIBUTORS.txt +# For details: https://GitHub.com/PyCQA/pylint/blob/main/LICENSE +# Copyright (c) https://GitHub.com/PyCQA/pylint/blob/main/CONTRIBUTORS.txt # pylint: disable=redefined-outer-name @@ -527,7 +527,7 @@ def test_load_plugin_command_line() -> None: @pytest.mark.usefixtures("pop_pylintrc") def test_load_plugin_path_manipulation_case_6() -> None: - """Case 6 refers to Github issue #7264. + """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 @@ -596,7 +596,7 @@ def test_load_plugin_path_manipulation_case_6() -> None: @pytest.mark.usefixtures("pop_pylintrc") def test_load_plugin_path_manipulation_case_3() -> None: - """Case 3 refers to Github issue #7264. + """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 @@ -1098,7 +1098,7 @@ def test_recursive_ignore(ignore_parameter: str, ignore_parameter_value: str) -> def test_relative_imports(initialized_linter: PyLinter) -> None: - """Regression test for https://github.com/PyCQA/pylint/issues/3651""" + """Regression test for https://GitHub.com/PyCQA/pylint/issues/3651""" linter = initialized_linter with tempdir() as tmpdir: create_files(["x/y/__init__.py", "x/y/one.py", "x/y/two.py"], tmpdir) @@ -1151,7 +1151,7 @@ def test_import_sibling_module_from_namespace(initialized_linter: PyLinter) -> N def test_lint_namespace_package_under_dir(initialized_linter: PyLinter) -> None: - """Regression test for https://github.com/PyCQA/pylint/issues/1667""" + """Regression test for https://GitHub.com/PyCQA/pylint/issues/1667""" linter = initialized_linter with tempdir(): create_files(["outer/namespace/__init__.py", "outer/namespace/module.py"]) From 05a956f4fb1a54f8feeab37d87706f000c36a23e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dani=C3=ABl=20van=20Noord?= <13665637+DanielNoord@users.noreply.github.com> Date: Wed, 7 Sep 2022 11:16:37 +0200 Subject: [PATCH 16/17] Spelling is hard --- .pyenchant_pylint_custom_dict.txt | 1 + tests/lint/unittest_lint.py | 4 ++-- 2 files changed, 3 insertions(+), 2 deletions(-) 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/tests/lint/unittest_lint.py b/tests/lint/unittest_lint.py index 4c5f2de770..f2dca7f37e 100644 --- a/tests/lint/unittest_lint.py +++ b/tests/lint/unittest_lint.py @@ -1,6 +1,6 @@ # Licensed under the GPL: https://www.gnu.org/licenses/old-licenses/gpl-2.0.html -# For details: https://GitHub.com/PyCQA/pylint/blob/main/LICENSE -# Copyright (c) https://GitHub.com/PyCQA/pylint/blob/main/CONTRIBUTORS.txt +# For details: https://github.com/PyCQA/pylint/blob/main/LICENSE +# Copyright (c) https://github.com/PyCQA/pylint/blob/main/CONTRIBUTORS.txt # pylint: disable=redefined-outer-name From 52f10ef960482869f0f9190f2a62220587cf8d15 Mon Sep 17 00:00:00 2001 From: Pierre Sassoulas Date: Wed, 7 Sep 2022 12:27:59 +0200 Subject: [PATCH 17/17] Apply suggestions from code review --- tests/lint/unittest_lint.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/lint/unittest_lint.py b/tests/lint/unittest_lint.py index f2dca7f37e..80532dea71 100644 --- a/tests/lint/unittest_lint.py +++ b/tests/lint/unittest_lint.py @@ -1098,7 +1098,7 @@ def test_recursive_ignore(ignore_parameter: str, ignore_parameter_value: str) -> def test_relative_imports(initialized_linter: PyLinter) -> None: - """Regression test for https://GitHub.com/PyCQA/pylint/issues/3651""" + """Regression test for https://github.com/PyCQA/pylint/issues/3651""" linter = initialized_linter with tempdir() as tmpdir: create_files(["x/y/__init__.py", "x/y/one.py", "x/y/two.py"], tmpdir) @@ -1151,7 +1151,7 @@ def test_import_sibling_module_from_namespace(initialized_linter: PyLinter) -> N def test_lint_namespace_package_under_dir(initialized_linter: PyLinter) -> None: - """Regression test for https://GitHub.com/PyCQA/pylint/issues/1667""" + """Regression test for https://github.com/PyCQA/pylint/issues/1667""" linter = initialized_linter with tempdir(): create_files(["outer/namespace/__init__.py", "outer/namespace/module.py"])