From acced5f62aa4f928f0b3522592a7ab013ec3b899 Mon Sep 17 00:00:00 2001 From: Peter Law Date: Fri, 8 May 2020 13:10:26 +0100 Subject: [PATCH 1/2] Emit an error when entry points are duplicated This avoids flake8 silently ignoring one of the plugins and fixes https://gitlab.com/pycqa/flake8/-/issues/634. --- src/flake8/exceptions.py | 31 +++++++++++++++++++ src/flake8/plugins/manager.py | 6 ++++ .../local-plugin-duplicate-entry-point.ini | 4 +++ tests/integration/test_plugins.py | 11 +++++++ 4 files changed, 52 insertions(+) create mode 100644 tests/fixtures/config_files/local-plugin-duplicate-entry-point.ini diff --git a/src/flake8/exceptions.py b/src/flake8/exceptions.py index bef6f4b..92bb276 100644 --- a/src/flake8/exceptions.py +++ b/src/flake8/exceptions.py @@ -1,6 +1,10 @@ """Exception classes for all of Flake8.""" from typing import Dict +if False: # `typing.TYPE_CHECKING` was introduced in 3.5.2 + from flake8.plugins.manager import Plugin + from flake8._compat import importlib_metadata + class Flake8Exception(Exception): """Plain Flake8 exception.""" @@ -14,6 +18,33 @@ class ExecutionError(Flake8Exception): """Exception raised during execution of Flake8.""" +class DuplicatePluginEntryPoint(Flake8Exception): + """Exception raised when a plugin entry point is already taken.""" + + FORMAT = ( + 'Plugin entry point "%(entry_point)s" for "%(new)s" already taken by ' + '"%(existing)s"' + ) + + def __init__(self, entry_point, existing_plugin): + # type: (importlib_metadata.EntryPoint, Plugin) -> None + """Initialize our DuplicatePluginEntryPoint exception.""" + self.entry_point = entry_point + self.existing_plugin = existing_plugin + super(DuplicatePluginEntryPoint, self).__init__( + entry_point, + existing_plugin, + ) + + def __str__(self): # type: () -> str + """Format our exception message.""" + return self.FORMAT % { + "entry_point": self.entry_point.name, + "new": self.entry_point.value, + "existing": self.existing_plugin.entry_point.value, + } + + class FailedToLoadPlugin(Flake8Exception): """Exception raised when a plugin fails to load.""" diff --git a/src/flake8/plugins/manager.py b/src/flake8/plugins/manager.py index 3d4371d..8bdd94a 100644 --- a/src/flake8/plugins/manager.py +++ b/src/flake8/plugins/manager.py @@ -273,6 +273,12 @@ class PluginManager(object): # pylint: disable=too-few-public-methods Is this a repo-local plugin? """ name = entry_point.name + + if name in self.plugins: + raise exceptions.DuplicatePluginEntryPoint( + entry_point, self.plugins[name], + ) + self.plugins[name] = Plugin(name, entry_point, local=local) self.names.append(name) LOG.debug('Loaded %r for plugin "%s".', self.plugins[name], name) diff --git a/tests/fixtures/config_files/local-plugin-duplicate-entry-point.ini b/tests/fixtures/config_files/local-plugin-duplicate-entry-point.ini new file mode 100644 index 0000000..8a0e940 --- /dev/null +++ b/tests/fixtures/config_files/local-plugin-duplicate-entry-point.ini @@ -0,0 +1,4 @@ +[flake8:local-plugins] +extension = + XE = aplugin:ExtensionTestPluginA + XE = aplugin:ExtensionTestPluginB diff --git a/tests/integration/test_plugins.py b/tests/integration/test_plugins.py index 859fb69..57cc9e3 100644 --- a/tests/integration/test_plugins.py +++ b/tests/integration/test_plugins.py @@ -1,9 +1,13 @@ """Integration tests for plugin loading.""" +import pytest + from flake8.main import application +from flake8 import exceptions LOCAL_PLUGIN_CONFIG = 'tests/fixtures/config_files/local-plugin.ini' LOCAL_PLUGIN_PATH_CONFIG = 'tests/fixtures/config_files/local-plugin-path.ini' +LOCAL_PLUGIN_DUPLICATE_CONFIG = 'tests/fixtures/config_files/local-plugin-duplicate-entry-point.ini' class ExtensionTestPlugin(object): @@ -61,3 +65,10 @@ def test_enable_local_plugin_at_non_installed_path(): app.initialize(['flake8', '--config', LOCAL_PLUGIN_PATH_CONFIG]) assert app.check_plugins['XE'].plugin.name == 'ExtensionTestPlugin2' + + +def test_reject_local_plugins_with_duplicate_entry_point(): + """Reject duplicate entry points in local-plugins config section.""" + with pytest.raises(exceptions.DuplicatePluginEntryPoint): + app = application.Application() + app.initialize(['flake8', '--config', LOCAL_PLUGIN_DUPLICATE_CONFIG]) From 6167df4992ecbdc4e9b99bf9269eff02a26f30b6 Mon Sep 17 00:00:00 2001 From: Peter Law Date: Fri, 15 May 2020 16:58:34 +0100 Subject: [PATCH 2/2] Show a nicer message when duplicate entry points are found This removes the stack trace from the message printed, which is somewhat more friendly, though still treats this misconfiguration as a catastrophic error. --- src/flake8/exceptions.py | 2 +- src/flake8/main/application.py | 11 +++++++---- tests/unit/test_application.py | 12 ++++++++++++ 3 files changed, 20 insertions(+), 5 deletions(-) diff --git a/src/flake8/exceptions.py b/src/flake8/exceptions.py index 92bb276..3d5af70 100644 --- a/src/flake8/exceptions.py +++ b/src/flake8/exceptions.py @@ -18,7 +18,7 @@ class ExecutionError(Flake8Exception): """Exception raised during execution of Flake8.""" -class DuplicatePluginEntryPoint(Flake8Exception): +class DuplicatePluginEntryPoint(ExecutionError): """Exception raised when a plugin entry point is already taken.""" FORMAT = ( diff --git a/src/flake8/main/application.py b/src/flake8/main/application.py index 001ad6c..8740bfa 100644 --- a/src/flake8/main/application.py +++ b/src/flake8/main/application.py @@ -121,15 +121,18 @@ class Application(object): This should be the last thing called on the application instance. It will check certain options and exit appropriately. """ + if self.catastrophic_failure: + # Don't rely on any attributes being set if things failued + # catastrophically + raise SystemExit(True) + if self.options.count: print(self.result_count) if self.options.exit_zero: - raise SystemExit(self.catastrophic_failure) + raise SystemExit(False) else: - raise SystemExit( - (self.result_count > 0) or self.catastrophic_failure - ) + raise SystemExit(self.result_count > 0) def find_plugins(self, config_finder): # type: (config.ConfigFileFinder) -> None diff --git a/tests/unit/test_application.py b/tests/unit/test_application.py index 51adefb..0683603 100644 --- a/tests/unit/test_application.py +++ b/tests/unit/test_application.py @@ -48,6 +48,18 @@ def test_exit_does_raise(result_count, catastrophic, exit_zero, value, assert excinfo.value.args[0] is value +def test_exit_raises(application): + """Verify Application.exit raises SystemExit under configuration failure.""" + application.catastrophic_failure = True + # Note: no application.options set -- configuration issues can lead to + # errors before it's assigned. + + with pytest.raises(SystemExit) as excinfo: + application.exit() + + assert excinfo.value.args[0] is True + + def test_warns_on_unknown_formatter_plugin_name(application): """Verify we log a warning with an unfound plugin.""" default = mock.Mock()