From 82eb23b1cea017c06cec80b37f365ff6b086e671 Mon Sep 17 00:00:00 2001 From: jakkdl Date: Thu, 29 Dec 2022 14:46:49 +0100 Subject: [PATCH] use own flag, --enable-visitor-code-regex, instead of relying on flake8 being stable --- flake8_trio.py | 38 +++++-- tests/conftest.py | 8 +- tests/test_flake8_trio.py | 67 ++++++------- tests/trio103.py | 2 +- tests/trio103_no_104.py | 169 ++++++++++++++++++++++++++++++++ tests/trio104.py | 2 +- tests/trio107.py | 2 +- tests/trio108.py | 2 +- tox.ini | 11 +++ typings/flake8/main/options.pyi | 71 -------------- typings/flake8/style_guide.pyi | 83 ---------------- 11 files changed, 246 insertions(+), 209 deletions(-) create mode 100644 tests/trio103_no_104.py delete mode 100644 typings/flake8/main/options.pyi delete mode 100644 typings/flake8/style_guide.pyi diff --git a/flake8_trio.py b/flake8_trio.py index af7efc5..1601840 100644 --- a/flake8_trio.py +++ b/flake8_trio.py @@ -13,14 +13,16 @@ import ast import keyword +import re import tokenize from argparse import ArgumentTypeError, Namespace from collections.abc import Iterable from fnmatch import fnmatch -from typing import Any, NamedTuple, Union, cast +from typing import TYPE_CHECKING, Any, NamedTuple, Union, cast -from flake8.options.manager import OptionManager -from flake8.style_guide import Decision, DecisionEngine +# guard against internal flake8 changes +if TYPE_CHECKING: + from flake8.options.manager import OptionManager # CalVer: YY.month.patch, e.g. first release of July 2022 == "22.7.1" __version__ = "22.12.5" @@ -97,7 +99,7 @@ class Flake8TrioRunner(ast.NodeVisitor): def __init__(self, options: Namespace): super().__init__() self._problems: list[Error] = [] - self.decision_engine: DecisionEngine = DecisionEngine(options) + self.options = options self.visitors = { v(options, self._problems) @@ -105,13 +107,9 @@ def __init__(self, options: Namespace): if self.selected(v.error_codes) } - # Use flake8's internal decision engine to check if codes are included or not - # to ease debugging and speed up plugin time - # This isn't officially documented or supported afaik, but should be easily - # removed upon any breaking changes. def selected(self, error_codes: dict[str, str]) -> bool: return any( - self.decision_engine.decision_for(code) == Decision.Selected + re.match(self.options.enable_visitor_codes_regex, code) for code in error_codes ) @@ -123,6 +121,11 @@ def run(cls, tree: ast.AST, options: Namespace) -> Iterable[Error]: def visit(self, node: ast.AST): """Visit a node.""" + # don't bother visiting if no visitors are enabled, or all enabled visitors + # in parent nodes have marked novisit + if not self.visitors: + return + # tracks the subclasses that, from this node on, iterated through it's subfields # we need to remember it so we can restore it at the end of the function. novisit: set[Flake8TrioVisitor] = set() @@ -223,6 +226,9 @@ def error( if error_code is None: assert len(self.error_codes) == 1 error_code = next(iter(self.error_codes)) + # don't emit an error if this code is disabled in a multi-code visitor + elif not re.match(self.options.enable_visitor_codes_regex, error_code): + return if not self.suppress_errors: self._problems.append( @@ -1528,6 +1534,20 @@ def add_options(option_manager: OptionManager): "suggesting it be replaced with {value}" ), ) + option_manager.add_option( + "--enable-visitor-codes-regex", + type=re.compile, + default=".*", + parse_from_config=True, + required=False, + help=( + "Regex string of visitors to enable. Can be used to disable broken " + "visitors, or instead of --select/--disable to select error codes " + "in a way that is more performant. If a visitor raises multiple codes " + "it will not be disabled unless all codes are disabled, but it will " + "not report codes matching this regex." + ), + ) @staticmethod def parse_options(options: Namespace): diff --git a/tests/conftest.py b/tests/conftest.py index d0e81af..beeddb7 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -8,7 +8,9 @@ def pytest_addoption(parser: pytest.Parser): "--runfuzz", action="store_true", default=False, help="run fuzz tests" ) parser.addoption( - "--select", default="TRIO", help="select error codes whose visitors to run." + "--enable-visitor-codes-regex", + default=".*", + help="select error codes whose visitors to run.", ) @@ -29,5 +31,5 @@ def pytest_collection_modifyitems(config: pytest.Config, items: list[pytest.Item @pytest.fixture -def select(request: pytest.FixtureRequest): - return request.config.getoption("--select") +def enable_visitor_codes_regex(request: pytest.FixtureRequest): + return request.config.getoption("--enable-visitor-codes-regex") diff --git a/tests/test_flake8_trio.py b/tests/test_flake8_trio.py index 34562ee..91f7b8e 100644 --- a/tests/test_flake8_trio.py +++ b/tests/test_flake8_trio.py @@ -17,7 +17,6 @@ import pytest from flake8 import __version_info__ as flake8_version_info -from flake8.main.options import register_default_options from flake8.options.manager import OptionManager # import trio # type: ignore @@ -26,7 +25,7 @@ from flake8_trio import Error, Flake8TrioVisitor, Plugin, Statement -trio_test_files_regex = re.compile(r"trio\d\d\d(_py.*)?.py") +trio_test_files_regex = re.compile(r"trio\d\d\d(_.*)?.py") test_files: list[tuple[str, str]] = sorted( (os.path.splitext(f)[0].upper(), f) @@ -44,17 +43,11 @@ def _default_option_manager(): kwargs = {} if flake8_version_info[0] >= 6: kwargs["formatter_names"] = ["default"] - option_manager = OptionManager(version="", plugin_versions="", parents=[], **kwargs) - - # register `--select`,`--ignore`,etc needed for the DecisionManager - # and to be able to selectively enable/disable visitors - register_default_options(option_manager) - - return option_manager + return OptionManager(version="", plugin_versions="", parents=[], **kwargs) # check for presence of _pyXX, skip if version is later, and prune parameter -def check_version(test: str) -> str: +def check_version(test: str): python_version = re.search(r"(?<=_PY)\d*", test) if python_version: version_str = python_version.group() @@ -62,8 +55,6 @@ def check_version(test: str) -> str: v_i = sys.version_info if (v_i.major, v_i.minor) < (int(major), int(minor)): raise unittest.SkipTest("v_i, major, minor") - return test.split("_")[0] - return test ERROR_CODES = { @@ -74,18 +65,17 @@ def check_version(test: str) -> str: @pytest.mark.parametrize("test, path", test_files) -def test_eval(test: str, path: str, select: str): +def test_eval(test: str, path: str): # version check - test = check_version(test) + check_version(test) + test = test.split("_")[0] assert test in ERROR_CODES, "error code not defined in flake8_trio.py" - parsed_args = [] - - if select in test or test in select: - # `select` this error code to enable the visitor for it, if the test file - # specifies `# ARG --select=...` that will take precedence. - parsed_args = [f"--select={test}"] + # only enable the tested visitor to save performance and ease debugging + # if a test requires enabling multiple visitors they specify a + # `# ARG --enable-vis...` that comes later in the arg list, overriding this + parsed_args = [f"--enable-visitor-codes-regex={test}"] expected: list[Error] = [] @@ -152,8 +142,6 @@ def test_eval(test: str, path: str, select: str): raise ParseError(msg) from e assert expected, f"failed to parse any errors in file {path}" - if not any("--select" in arg for arg in parsed_args): - pytest.skip("no `--select`ed visitors") plugin = read_file(path) assert_expected_errors(plugin, *expected, args=parsed_args) @@ -199,19 +187,21 @@ def visit_AsyncFor(self, node: ast.AST): @pytest.mark.parametrize("test, path", test_files) -def test_noerror_on_sync_code(test: str, path: str, select: str): +def test_noerror_on_sync_code(test: str, path: str): if any(e in test for e in error_codes_ignored_when_checking_transformed_sync_code): return with tokenize.open(f"tests/{path}") as f: source = f.read() tree = SyncTransformer().visit(ast.parse(source)) - ignored_codes_string = ",".join( - error_codes_ignored_when_checking_transformed_sync_code + ignored_codes_regex = ( + "(?!(" + + "|".join(error_codes_ignored_when_checking_transformed_sync_code) + + "))" ) assert_expected_errors( Plugin(tree), - args=[f"--select={select}", f"--ignore={ignored_codes_string}"], + args=[f"--enable-visitor-codes-regex={ignored_codes_regex}"], ) @@ -225,11 +215,6 @@ def initialize_options(plugin: Plugin, args: list[str] | None = None): plugin.add_options(om) plugin.parse_options(om.parse_args(args=(args if args else []))) - # these are not registered like flake8's normal options so we need - # to manually initialize them for DecisionEngine to work. - plugin.options.extended_default_select = [] - plugin.options.extended_default_ignore = [] - def assert_expected_errors( plugin: Plugin, @@ -384,7 +369,7 @@ def test_107_permutations(): # the permutations in a single test is much faster than the permutations from using # pytest parametrization - and does not clutter up the output massively. plugin = Plugin(ast.AST()) - initialize_options(plugin, args=["--select=TRIO107"]) + initialize_options(plugin, args=["--enable-visitor-codes-regex=TRIO107"]) check = "await foo()" for try_, exc1, exc2, bare_exc, else_, finally_ in itertools.product( @@ -421,7 +406,7 @@ def test_107_permutations(): # not a type error per se, but it's pyright warning about assigning to a # protected class member - hence we silence it with a `type: ignore`. plugin._tree = tree # type: ignore - errors = [e for e in plugin.run() if e.code == "TRIO107"] + errors = list(plugin.run()) if ( # return in exception @@ -528,16 +513,18 @@ class TestFuzz(unittest.TestCase): @given((from_grammar() | from_node()).map(ast.parse)) def test_does_not_crash_on_any_valid_code(self, syntax_tree: ast.AST): # TODO: figure out how to get unittest to play along with pytest options - # so `--select` can be passed through - # though I barely notice a difference manually changing this value, or even + # so `--enable-visitor-codes-regex` can be passed through. + # Though I barely notice a difference manually changing this value, or even # not running the plugin at all, so overhead looks to be vast majority of runtime - select = "TRIO" + enable_visitor_codes_regex = ".*" # Given any syntatically-valid source code, the checker should # not crash. This tests doesn't check that we do the *right* thing, # just that we don't crash on valid-if-poorly-styled code! plugin = Plugin(syntax_tree) - initialize_options(plugin, [f"--select={select}"]) + initialize_options( + plugin, [f"--enable-visitor-codes-regex={enable_visitor_codes_regex}"] + ) consume(plugin.run()) @@ -554,11 +541,13 @@ def _iter_python_files(): @pytest.mark.fuzz -def test_does_not_crash_on_site_code(select: str): +def test_does_not_crash_on_site_code(enable_visitor_codes_regex: str): for path in _iter_python_files(): try: plugin = Plugin.from_filename(str(path)) - initialize_options(plugin, [f"--select={select}"]) + initialize_options( + plugin, [f"--enable-visitor-codes-regex={enable_visitor_codes_regex}"] + ) consume(plugin.run()) except Exception as err: raise AssertionError(f"Failed on {path}") from err diff --git a/tests/trio103.py b/tests/trio103.py index 13844e8..4204b32 100644 --- a/tests/trio103.py +++ b/tests/trio103.py @@ -1,4 +1,4 @@ -# ARG --select=TRIO103,TRIO104 +# ARG --enable-visitor-codes-regex=(TRIO103)|(TRIO104) from typing import Any diff --git a/tests/trio103_no_104.py b/tests/trio103_no_104.py new file mode 100644 index 0000000..28688af --- /dev/null +++ b/tests/trio103_no_104.py @@ -0,0 +1,169 @@ +# check that partly disabling a visitor works +from typing import Any + +import trio + + +def foo() -> Any: + ... + + +try: + pass +except (SyntaxError, ValueError, BaseException): + raise +except (SyntaxError, ValueError, trio.Cancelled) as p: # error: 33, "trio.Cancelled" + pass +except (SyntaxError, ValueError): + raise +except trio.Cancelled as e: + raise e +except trio.Cancelled as e: + raise # acceptable - see https://peps.python.org/pep-0678/#example-usage +except trio.Cancelled: # error: 7, "trio.Cancelled" + pass + +# if +except BaseException as e: # error: 7, "BaseException" + if True: + raise e + elif True: + pass + else: + raise e +except BaseException: # error: 7, "BaseException" + if True: + raise +except BaseException: # safe + if True: + raise + elif True: + raise + else: + raise + +# loops +# raises inside the body are never guaranteed to run and are ignored +except trio.Cancelled: # error: 7, "trio.Cancelled" + while foo(): + raise + +# raise inside else are guaranteed to run, unless there's a break +except trio.Cancelled: + while ...: + ... + else: + raise +except trio.Cancelled: + for _ in "": + ... + else: + raise +except BaseException: # error: 7, "BaseException" + while ...: + if ...: + break + raise + else: + raise +except BaseException: # error: 7, "BaseException" + for _ in "": + if ...: + break + raise + else: + raise +# ensure we don't ignore previous guaranteed raise (although that's unreachable code) +except BaseException: + raise + for _ in "": + if ...: + break + raise + else: + raise + +# nested try +# in theory safe if the try, and all excepts raises - and there's a bare except. +# But is a very weird pattern that we don't handle. +except BaseException as e: # error: 7, "BaseException" + try: + raise e + except ValueError: + raise e + except: + raise e # disabled TRIO104 error +except BaseException: # safe + try: + pass + finally: + raise +# check that nested non-critical exceptions are ignored +except BaseException: + try: + pass + except ValueError: + pass # safe + raise +# check that name isn't lost +except trio.Cancelled as e: + try: + pass + except BaseException as f: + raise f + raise e +# don't bypass raise by raising from nested except +except trio.Cancelled as e: + try: + pass + except ValueError as g: + raise g # disabled TRIO104 error + except BaseException as h: + raise h # error? currently treated as safe + raise e +# bare except, equivalent to `except baseException` +except: # error: 0, "bare except" + pass +try: + pass +except: + raise + +# point to correct exception in multi-line handlers +my_super_mega_long_exception_so_it_gets_split = SyntaxError +try: + pass +except ( + my_super_mega_long_exception_so_it_gets_split, + SyntaxError, + BaseException, # error: 4, "BaseException" + ValueError, + trio.Cancelled, # no complaint on this line +): + pass + +# loop over non-empty static collection +except BaseException as e: + for i in [1, 2, 3]: + raise +except BaseException: # error: 7, "BaseException" + for i in [1, 2, 3]: + ... +except BaseException: # error: 7, "BaseException" + for i in [1, 2, 3]: + if ...: + continue + raise +except BaseException: + while True: + raise +except BaseException: # error: 7, "BaseException" + while True: + if ...: + break + raise +except BaseException: + while True: + if ...: + continue + raise diff --git a/tests/trio104.py b/tests/trio104.py index 7f48df3..8c60147 100644 --- a/tests/trio104.py +++ b/tests/trio104.py @@ -1,4 +1,4 @@ -# ARG --select=TRIO103,TRIO104 +# ARG --enable-visitor-codes-regex=(TRIO103)|(TRIO104) import trio diff --git a/tests/trio107.py b/tests/trio107.py index 898f941..0abb4ed 100644 --- a/tests/trio107.py +++ b/tests/trio107.py @@ -10,7 +10,7 @@ def __() -> Any: ... -# INCLUDE TRIO108 +# ARG --enable-visitor-codes-regex=(TRIO107)|(TRIO108) # function whose body solely consists of pass, ellipsis, or string constants is safe async def foo_empty_1(): diff --git a/tests/trio108.py b/tests/trio108.py index 8d978d4..b3caa7a 100644 --- a/tests/trio108.py +++ b/tests/trio108.py @@ -4,7 +4,7 @@ _: Any = "" -# INCLUDE TRIO107 +# ARG --enable-visitor-codes-regex=(TRIO107)|(TRIO108) async def foo() -> Any: diff --git a/tox.ini b/tox.ini index 9437750..91c6566 100644 --- a/tox.ini +++ b/tox.ini @@ -63,3 +63,14 @@ filterwarnings = [flake8] max-line-length = 90 + +[coverage:report] +exclude_lines = + # Have to re-enable the standard pragma + pragma: no cover + + # Don't complain about abstract methods, they aren't run: + @(abc\.)?abstractmethod + + # Don't check guarded type imports + if (typing.)?TYPE_CHECKING: diff --git a/typings/flake8/main/options.pyi b/typings/flake8/main/options.pyi deleted file mode 100644 index 33fdcd1..0000000 --- a/typings/flake8/main/options.pyi +++ /dev/null @@ -1,71 +0,0 @@ -""" -This type stub file was generated by pyright. -""" - -import argparse - -from flake8.options.manager import OptionManager - -"""Contains the logic for all of the default options for Flake8.""" - -def stage1_arg_parser() -> argparse.ArgumentParser: - """Register the preliminary options on our OptionManager. - - The preliminary options include: - - - ``-v``/``--verbose`` - - ``--output-file`` - - ``--append-config`` - - ``--config`` - - ``--isolated`` - - ``--enable-extensions`` - """ - ... - -class JobsArgument: - """Type callback for the --jobs argument.""" - - def __init__(self, arg: str) -> None: - """Parse and validate the --jobs argument. - - :param arg: The argument passed by argparse for validation - """ - ... - def __repr__(self) -> str: - """Representation for debugging.""" - ... - def __str__(self) -> str: - """Format our JobsArgument class.""" - ... - -def register_default_options(option_manager: OptionManager) -> None: - """Register the default options on our OptionManager. - - The default options include: - - - ``-q``/``--quiet`` - - ``--color`` - - ``--count`` - - ``--exclude`` - - ``--extend-exclude`` - - ``--filename`` - - ``--format`` - - ``--hang-closing`` - - ``--ignore`` - - ``--extend-ignore`` - - ``--per-file-ignores`` - - ``--max-line-length`` - - ``--max-doc-length`` - - ``--indent-size`` - - ``--select`` - - ``--extend-select`` - - ``--disable-noqa`` - - ``--show-source`` - - ``--statistics`` - - ``--exit-zero`` - - ``-j``/``--jobs`` - - ``--tee`` - - ``--benchmark`` - - ``--bug-report`` - """ - ... diff --git a/typings/flake8/style_guide.pyi b/typings/flake8/style_guide.pyi deleted file mode 100644 index d48487c..0000000 --- a/typings/flake8/style_guide.pyi +++ /dev/null @@ -1,83 +0,0 @@ -""" -This type stub file was generated by pyright. -Removed, __all__, StyleGuide and StyleGuideManager to not need to import -(and therefore generate stub for) flake8.statistics -""" - -import argparse -import enum - -"""Implementation of the StyleGuide used by Flake8.""" - -LOG = ... - -class Selected(enum.Enum): - """Enum representing an explicitly or implicitly selected code.""" - - Explicitly = ... - Implicitly = ... - -class Ignored(enum.Enum): - """Enum representing an explicitly or implicitly ignored code.""" - - Explicitly = ... - Implicitly = ... - -class Decision(enum.Enum): - """Enum representing whether a code should be ignored or selected.""" - - Ignored = ... - Selected = ... - -class DecisionEngine: - """A class for managing the decision process around violations. - - This contains the logic for whether a violation should be reported or - ignored. - """ - - def __init__(self, options: argparse.Namespace) -> None: - """Initialize the engine.""" - ... - def was_selected(self, code: str) -> Selected | Ignored: - """Determine if the code has been selected by the user. - - :param code: The code for the check that has been run. - :returns: - Selected.Implicitly if the selected list is empty, - Selected.Explicitly if the selected list is not empty and a match - was found, - Ignored.Implicitly if the selected list is not empty but no match - was found. - """ - ... - def was_ignored(self, code: str) -> Selected | Ignored: - """Determine if the code has been ignored by the user. - - :param code: - The code for the check that has been run. - :returns: - Selected.Implicitly if the ignored list is empty, - Ignored.Explicitly if the ignored list is not empty and a match was - found, - Selected.Implicitly if the ignored list is not empty but no match - was found. - """ - ... - def make_decision(self, code: str) -> Decision: - """Decide if code should be ignored or selected.""" - ... - def decision_for(self, code: str) -> Decision: - """Return the decision for a specific code. - - This method caches the decisions for codes to avoid retracing the same - logic over and over again. We only care about the select and ignore - rules as specified by the user in their configuration files and - command-line flags. - - This method does not look at whether the specific line is being - ignored in the file itself. - - :param code: The code for the check that has been run. - """ - ...