diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index e3bc3f4..0b85567 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 6f6c7fe..1601840 100644 --- a/flake8_trio.py +++ b/flake8_trio.py @@ -13,13 +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 +# 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" @@ -96,12 +99,20 @@ class Flake8TrioRunner(ast.NodeVisitor): def __init__(self, options: Namespace): super().__init__() self._problems: list[Error] = [] + self.options = 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) } + def selected(self, error_codes: dict[str, str]) -> bool: + return any( + re.match(self.options.enable_visitor_codes_regex, code) + for code in error_codes + ) + @classmethod def run(cls, tree: ast.AST, options: Namespace) -> Iterable[Error]: runner = cls(options) @@ -110,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() @@ -210,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( @@ -1515,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 0bc2c0d..beeddb7 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -7,6 +7,11 @@ def pytest_addoption(parser: pytest.Parser): parser.addoption( "--runfuzz", action="store_true", default=False, help="run fuzz tests" ) + parser.addoption( + "--enable-visitor-codes-regex", + default=".*", + help="select error codes whose visitors to run.", + ) def pytest_configure(config: pytest.Config): @@ -23,3 +28,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 enable_visitor_codes_regex(request: pytest.FixtureRequest): + return request.config.getoption("--enable-visitor-codes-regex") diff --git a/tests/test_decorator.py b/tests/test_decorator.py index c03a0e1..aeeab7f 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 7362750..91f7b8e 100644 --- a/tests/test_flake8_trio.py +++ b/tests/test_flake8_trio.py @@ -10,9 +10,10 @@ 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 @@ -24,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) @@ -46,7 +47,7 @@ def _default_option_manager(): # 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() @@ -54,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 = { @@ -68,12 +67,16 @@ def check_version(test: str) -> str: @pytest.mark.parametrize("test, path", test_files) 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" - include = [test] - parsed_args = [""] + # 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] = [] with open(os.path.join("tests", path), encoding="utf-8") as file: @@ -85,14 +88,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 @@ -147,7 +144,7 @@ def test_eval(test: str, path: str): assert expected, f"failed to parse any errors in file {path}" 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 @@ -197,10 +194,14 @@ def test_noerror_on_sync_code(test: str, path: str): source = f.read() tree = SyncTransformer().visit(ast.parse(source)) + ignored_codes_regex = ( + "(?!(" + + "|".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"--enable-visitor-codes-regex={ignored_codes_regex}"], ) @@ -209,23 +210,21 @@ 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 []))) + + 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 +368,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=["--enable-visitor-codes-regex=TRIO107"]) + check = "await foo()" for try_, exc1, exc2, bare_exc, else_, finally_ in itertools.product( (check, "..."), @@ -401,7 +403,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 = list(plugin.run()) if ( # return in exception @@ -422,14 +427,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 +438,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 +502,52 @@ 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 `--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 + 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(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"--enable-visitor-codes-regex={enable_visitor_codes_regex}"] + ) + + 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(enable_visitor_codes_regex: str): + for path in _iter_python_files(): + try: + plugin = Plugin.from_filename(str(path)) + 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 a1ad29f..4204b32 100644 --- a/tests/trio103.py +++ b/tests/trio103.py @@ -1,3 +1,5 @@ +# ARG --enable-visitor-codes-regex=(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/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 2512f79..8c60147 100644 --- a/tests/trio104.py +++ b/tests/trio104.py @@ -1,3 +1,5 @@ +# ARG --enable-visitor-codes-regex=(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 4e1cf06..efd60c1 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 bfe35c2..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(): @@ -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/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/tests/trio112.py b/tests/trio112.py index 59e1f95..50b4fd0 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 fd818ae..5f16812 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 e4e8b4a..7c245c8 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 1462662..50ac235 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/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: