From d753e10dccd773d05458fd70f24b17dd476e777e Mon Sep 17 00:00:00 2001 From: jakkdl Date: Wed, 21 Dec 2022 10:50:49 +0100 Subject: [PATCH 1/2] runner now skips unselected visitors, added --select pytest option, fixed code coverage & tests, renamed ARGS to ARG, generated required type stubs --- CONTRIBUTING.md | 7 +- flake8_trio.py | 15 +++- tests/conftest.py | 8 ++ tests/test_decorator.py | 19 +---- tests/test_flake8_trio.py | 144 +++++++++++++++++++------------- tests/trio103.py | 6 +- tests/trio104.py | 8 +- tests/trio106.py | 1 + tests/trio107.py | 19 +++++ tests/trio112.py | 4 + tests/trio113.py | 2 +- tests/trio114.py | 2 +- tests/trio200.py | 2 +- typings/flake8/main/options.pyi | 71 ++++++++++++++++ typings/flake8/style_guide.pyi | 83 ++++++++++++++++++ 15 files changed, 303 insertions(+), 88 deletions(-) create mode 100644 typings/flake8/main/options.pyi create mode 100644 typings/flake8/style_guide.pyi diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index e3bc3f49..0b85567c 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -36,10 +36,9 @@ Lines containing `error:` are parsed as expecting an error of the code matching #### `TRIOxxx:` You can instead of `error` specify the error code. -### `# INCLUDE` -Test files by default filter out all errors not matching the file name, but if there's a line `# INCLUDE TRIO\d\d\d TRIO\d\d\d` those additional error codes are not filtered out and will be an error if encountered. -### `# ARGS` -With a line `# ARGS` you can also specify command-line arguments that should be passed to the plugin when parsing a file. Can be specified multiple times for several different arguments. +### `# ARG` +With `# ARG` lines you can also specify command-line arguments that should be passed to the plugin when parsing a file. Can be specified multiple times for several different arguments. +Generated tests will by default `--select` the error code of the file, which will enable any visitors that can generate that code (and if those visitors can raise other codes they might be raised too). This can be overriden by adding an `# ARG --select=...` line. ## Style Guide diff --git a/flake8_trio.py b/flake8_trio.py index 6f6c7fe1..af7efc5a 100644 --- a/flake8_trio.py +++ b/flake8_trio.py @@ -20,6 +20,7 @@ from typing import Any, NamedTuple, Union, cast from flake8.options.manager import OptionManager +from flake8.style_guide import Decision, DecisionEngine # CalVer: YY.month.patch, e.g. first release of July 2022 == "22.7.1" __version__ = "22.12.5" @@ -96,12 +97,24 @@ class Flake8TrioRunner(ast.NodeVisitor): def __init__(self, options: Namespace): super().__init__() self._problems: list[Error] = [] + self.decision_engine: DecisionEngine = DecisionEngine(options) + self.visitors = { v(options, self._problems) for v in Flake8TrioVisitor.__subclasses__() - # TODO: could here refrain from including subclasses for disabled checks + 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 + for code in error_codes + ) + @classmethod def run(cls, tree: ast.AST, options: Namespace) -> Iterable[Error]: runner = cls(options) diff --git a/tests/conftest.py b/tests/conftest.py index 0bc2c0da..d0e81afa 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -7,6 +7,9 @@ def pytest_addoption(parser: pytest.Parser): parser.addoption( "--runfuzz", action="store_true", default=False, help="run fuzz tests" ) + parser.addoption( + "--select", default="TRIO", help="select error codes whose visitors to run." + ) def pytest_configure(config: pytest.Config): @@ -23,3 +26,8 @@ def pytest_collection_modifyitems(config: pytest.Config, items: list[pytest.Item for item in items: if "fuzz" in item.keywords: item.add_marker(skip_fuzz) + + +@pytest.fixture +def select(request: pytest.FixtureRequest): + return request.config.getoption("--select") diff --git a/tests/test_decorator.py b/tests/test_decorator.py index c03a0e19..aeeab7f8 100644 --- a/tests/test_decorator.py +++ b/tests/test_decorator.py @@ -3,9 +3,8 @@ import ast from flake8.main.application import Application -from test_flake8_trio import _default_option_manager -from flake8_trio import Plugin, Statement, Visitor107_108, fnmatch_qualified_name +from flake8_trio import Statement, Visitor107_108, fnmatch_qualified_name def dec_list(*decorators: str) -> ast.Module: @@ -82,22 +81,6 @@ def test_pep614(): assert not wrap(("(any, expression, we, like)",), "no match here") -def test_plugin(): - tree = dec_list("app.route") - plugin = Plugin(tree) - - om = _default_option_manager() - plugin.add_options(om) - - plugin.parse_options(om.parse_args(args=[])) - assert tuple(plugin.run()) - - arg = "--no-checkpoint-warning-decorators=app.route" - plugin.parse_options(om.parse_args(args=[arg])) - - assert not tuple(plugin.run()) - - common_flags = ["--select=TRIO", "tests/trio_options.py"] diff --git a/tests/test_flake8_trio.py b/tests/test_flake8_trio.py index 73627504..34562ee8 100644 --- a/tests/test_flake8_trio.py +++ b/tests/test_flake8_trio.py @@ -10,12 +10,14 @@ import sys import tokenize import unittest +from collections import deque from collections.abc import Iterable, Sequence from pathlib import Path -from typing import DefaultDict +from typing import Any, DefaultDict 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 @@ -42,7 +44,13 @@ def _default_option_manager(): kwargs = {} if flake8_version_info[0] >= 6: kwargs["formatter_names"] = ["default"] - return OptionManager(version="", plugin_versions="", parents=[], **kwargs) + 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 # check for presence of _pyXX, skip if version is later, and prune parameter @@ -66,14 +74,19 @@ def check_version(test: str) -> str: @pytest.mark.parametrize("test, path", test_files) -def test_eval(test: str, path: str): +def test_eval(test: str, path: str, select: str): # version check test = check_version(test) assert test in ERROR_CODES, "error code not defined in flake8_trio.py" - include = [test] - parsed_args = [""] + 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}"] + expected: list[Error] = [] with open(os.path.join("tests", path), encoding="utf-8") as file: @@ -85,14 +98,8 @@ def test_eval(test: str, path: str): line = line.strip() - # add other error codes to check if #INCLUDE is specified - if reg_match := re.search(r"(?<=INCLUDE).*", line): - for other_code in reg_match.group().split(" "): - if other_code.strip(): - include.append(other_code.strip()) - - # add command-line args if specified with #ARGS - elif reg_match := re.search(r"(?<=ARGS).*", line): + # add command-line args if specified with #ARG + if reg_match := re.search(r"(?<=ARG ).*", line): parsed_args.append(reg_match.group().strip()) # skip commented out lines @@ -145,9 +152,11 @@ def test_eval(test: str, path: 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, include, *expected, args=parsed_args) + assert_expected_errors(plugin, *expected, args=parsed_args) # Codes that are supposed to also raise errors when run on sync code, and should @@ -190,17 +199,19 @@ def visit_AsyncFor(self, node: ast.AST): @pytest.mark.parametrize("test, path", test_files) -def test_noerror_on_sync_code(test: str, path: str): +def test_noerror_on_sync_code(test: str, path: str, select: 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 + ) assert_expected_errors( Plugin(tree), - include=error_codes_ignored_when_checking_transformed_sync_code, - invert_include=True, + args=[f"--select={select}", f"--ignore={ignored_codes_string}"], ) @@ -209,23 +220,26 @@ def read_file(test_file: str): return Plugin.from_filename(str(filename)) +def initialize_options(plugin: Plugin, args: list[str] | None = None): + om = _default_option_manager() + 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, - include: Iterable[str], *expected: Error, args: list[str] | None = None, - invert_include: bool = False, ): # initialize default option values - om = _default_option_manager() - plugin.add_options(om) - plugin.parse_options(om.parse_args(args=(args if args else [""]))) + initialize_options(plugin, args) - errors = sorted( - e - for e in plugin.run() - if (e.code in include if not invert_include else e.code not in include) - ) + errors = sorted(e for e in plugin.run()) expected_ = sorted(expected) print_first_diff(errors, expected_) @@ -369,6 +383,9 @@ def test_107_permutations(): # since each test is so fast, and there's so many permutations, manually doing # 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"]) + check = "await foo()" for try_, exc1, exc2, bare_exc, else_, finally_ in itertools.product( (check, "..."), @@ -401,7 +418,10 @@ def test_107_permutations(): ) return - errors = [e for e in Plugin(tree).run() if e.code == "TRIO107"] + # 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"] if ( # return in exception @@ -422,14 +442,10 @@ def test_107_permutations(): def test_114_raises_on_invalid_parameter(capsys: pytest.CaptureFixture[str]): plugin = Plugin(ast.AST()) - om = _default_option_manager() - plugin.add_options(om) # flake8 will reraise ArgumentError as SystemExit for arg in "blah.foo", "foo*", "*": with pytest.raises(SystemExit): - plugin.parse_options( - om.parse_args(args=[f"--startable-in-context-manager={arg}"]) - ) + initialize_options(plugin, args=[f"--startable-in-context-manager={arg}"]) out, err = capsys.readouterr() assert not out assert f"{arg!r} is not a valid method identifier" in err @@ -437,13 +453,9 @@ def test_114_raises_on_invalid_parameter(capsys: pytest.CaptureFixture[str]): def test_200_options(capsys: pytest.CaptureFixture[str]): plugin = Plugin(ast.AST()) - om = _default_option_manager() - plugin.add_options(om) for i, arg in (0, "foo"), (2, "foo->->bar"), (2, "foo->bar->fee"): with pytest.raises(SystemExit): - plugin.parse_options( - om.parse_args(args=[f"--trio200-blocking-calls={arg}"]) - ) + initialize_options(plugin, args=[f"--trio200-blocking-calls={arg}"]) out, err = capsys.readouterr() assert not out, out assert all(word in err for word in (str(i), arg, "->")) @@ -505,30 +517,48 @@ def test_200_from_config_subprocess(tmp_path: Path): assert res.stdout == err_msg.encode("ascii") +# from https://docs.python.org/3/library/itertools.html#itertools-recipes +def consume(iterator: Iterable[Any]): + deque(iterator, maxlen=0) + + @pytest.mark.fuzz class TestFuzz(unittest.TestCase): @settings(max_examples=1_000, suppress_health_check=[HealthCheck.too_slow]) @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 + # not running the plugin at all, so overhead looks to be vast majority of runtime + select = "TRIO" + # 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(syntax_tree).run() - - @staticmethod - def _iter_python_files(): - # Because the generator isn't perfect, we'll also test on all the code - # we can easily find in our current Python environment - this includes - # the standard library, and all installed packages. - for base in sorted(set(site.PREFIXES)): - for dirname, _, files in os.walk(base): - for f in files: - if f.endswith(".py"): - yield Path(dirname) / f - - def test_does_not_crash_on_site_code(self): - for path in self._iter_python_files(): - try: - Plugin.from_filename(str(path)).run() - except Exception as err: - raise AssertionError(f"Failed on {path}") from err + plugin = Plugin(syntax_tree) + initialize_options(plugin, [f"--select={select}"]) + + consume(plugin.run()) + + +def _iter_python_files(): + # Because the generator isn't perfect, we'll also test on all the code + # we can easily find in our current Python environment - this includes + # the standard library, and all installed packages. + for base in sorted(set(site.PREFIXES)): + for dirname, _, files in os.walk(base): + for f in files: + if f.endswith(".py"): + yield Path(dirname) / f + + +@pytest.mark.fuzz +def test_does_not_crash_on_site_code(select: str): + for path in _iter_python_files(): + try: + plugin = Plugin.from_filename(str(path)) + initialize_options(plugin, [f"--select={select}"]) + 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 a1ad29fd..13844e85 100644 --- a/tests/trio103.py +++ b/tests/trio103.py @@ -1,3 +1,5 @@ +# ARG --select=TRIO103,TRIO104 + from typing import Any import trio @@ -91,7 +93,7 @@ def foo() -> Any: except ValueError: raise e except: - raise e # though sometimes okay, TRIO104 + raise e # TRIO104: 8 except BaseException: # safe try: pass @@ -116,7 +118,7 @@ def foo() -> Any: try: pass except ValueError as g: - raise g # TRIO104 + raise g # TRIO104: 8 except BaseException as h: raise h # error? currently treated as safe raise e diff --git a/tests/trio104.py b/tests/trio104.py index 2512f797..7f48df3e 100644 --- a/tests/trio104.py +++ b/tests/trio104.py @@ -1,3 +1,5 @@ +# ARG --select=TRIO103,TRIO104 + import trio try: @@ -15,7 +17,7 @@ # 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: # TRIO103 +except BaseException as e: # TRIO103: 7, "BaseException" try: raise e except ValueError: @@ -67,11 +69,11 @@ def foo(): try: pass - except BaseException: # TRIO103 + except BaseException: # TRIO103: 11, "BaseException" return # error: 8 # check that we properly iterate over all nodes in try - except BaseException: # TRIO103 + except BaseException: # TRIO103: 11, "BaseException" try: return # error: 12 except ValueError: diff --git a/tests/trio106.py b/tests/trio106.py index 4e1cf06f..efd60c1e 100644 --- a/tests/trio106.py +++ b/tests/trio106.py @@ -2,6 +2,7 @@ import trio import trio as foo # error: 0 +from foo import blah from trio import * # type: ignore # noqa # error: 0 from trio import blah, open_file as foo # noqa # error: 0 diff --git a/tests/trio107.py b/tests/trio107.py index bfe35c25..898f9414 100644 --- a/tests/trio107.py +++ b/tests/trio107.py @@ -476,3 +476,22 @@ async def f2(): while True: await trio.sleep(0) return + + +# code coverage +def foo_sync(): + # try in sync function + try: + pass + except: + pass + + # continue/break in sync function + while True: + if ...: + continue + if ...: + break + + # boolop in sync function + True and True diff --git a/tests/trio112.py b/tests/trio112.py index 59e1f951..50b4fd0b 100644 --- a/tests/trio112.py +++ b/tests/trio112.py @@ -102,3 +102,7 @@ async def foo_1(): # redundant nursery, not handled with trio.open_nursery(): pass + +# code coverage: no variable name and body is an expression +with trio.open_nursery(): + print() diff --git a/tests/trio113.py b/tests/trio113.py index fd818ae6..5f16812e 100644 --- a/tests/trio113.py +++ b/tests/trio113.py @@ -7,7 +7,7 @@ import trio -# ARGS --startable-in-context-manager=custom_startable_function +# ARG --startable-in-context-manager=custom_startable_function @asynccontextmanager diff --git a/tests/trio114.py b/tests/trio114.py index e4e8b4af..7c245c83 100644 --- a/tests/trio114.py +++ b/tests/trio114.py @@ -1,6 +1,6 @@ import trio -# ARGS --startable-in-context-manager=foo +# ARG --startable-in-context-manager=foo async def foo(task_status): diff --git a/tests/trio200.py b/tests/trio200.py index 1462662b..50ac2357 100644 --- a/tests/trio200.py +++ b/tests/trio200.py @@ -1,7 +1,7 @@ # specify command-line arguments to be used when testing this file. # Test spaces in options, and trailing comma # Cannot test newlines, since argparse splits on those if passed on the CLI -# ARGS --trio200-blocking-calls=bar -> BAR, bee-> SHOULD_NOT_BE_PRINTED,bonnet ->SHOULD_NOT_BE_PRINTED,bee.bonnet->BEEBONNET,*.postwild->POSTWILD,prewild.*->PREWILD,*.*.*->TRIPLEDOT, +# ARG --trio200-blocking-calls=bar -> BAR, bee-> SHOULD_NOT_BE_PRINTED,bonnet ->SHOULD_NOT_BE_PRINTED,bee.bonnet->BEEBONNET,*.postwild->POSTWILD,prewild.*->PREWILD,*.*.*->TRIPLEDOT, # don't error in sync function def foo(): diff --git a/typings/flake8/main/options.pyi b/typings/flake8/main/options.pyi new file mode 100644 index 00000000..33fdcd1d --- /dev/null +++ b/typings/flake8/main/options.pyi @@ -0,0 +1,71 @@ +""" +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 new file mode 100644 index 00000000..d48487c4 --- /dev/null +++ b/typings/flake8/style_guide.pyi @@ -0,0 +1,83 @@ +""" +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. + """ + ... From 82eb23b1cea017c06cec80b37f365ff6b086e671 Mon Sep 17 00:00:00 2001 From: jakkdl Date: Thu, 29 Dec 2022 14:46:49 +0100 Subject: [PATCH 2/2] 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 af7efc5a..16018405 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 d0e81afa..beeddb7c 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 34562ee8..91f7b8ef 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 13844e85..4204b329 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 00000000..28688af2 --- /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 7f48df3e..8c601479 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 898f9414..0abb4ede 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 8d978d43..b3caa7a2 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 94377503..91c6566c 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 33fdcd1d..00000000 --- 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 d48487c4..00000000 --- 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. - """ - ...