From 8e35d6344f682d525ea5eada91853541feeb7669 Mon Sep 17 00:00:00 2001 From: jakkdl Date: Tue, 9 Aug 2022 22:28:19 +0200 Subject: [PATCH 1/2] ugly implementation of trio112 --- CHANGELOG.md | 3 +++ README.md | 1 + flake8_trio.py | 29 +++++++++++++++++++++++++++++ tests/trio112.py | 35 +++++++++++++++++++++++++++++++++++ 4 files changed, 68 insertions(+) create mode 100644 tests/trio112.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 1e5ad2de..853a4443 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,9 @@ # Changelog *[CalVer, YY.month.patch](https://calver.org/)* +## Future +- add TRIO112, nursery body with only a call to `nursery.start[_soon]` and not passing itself as a parameter can be replaced with a regular function call. + ## 22.8.4 - Fix TRIO108 raising errors on yields in some sync code. - TRIO109 now skips all decorated functions to avoid false alarms diff --git a/README.md b/README.md index 17c8c986..25c1e3d7 100644 --- a/README.md +++ b/README.md @@ -33,3 +33,4 @@ pip install flake8-trio Checkpoints are `await`, `async for`, and `async with` (on one of enter/exit). - **TRIO109**: Async function definition with a `timeout` parameter - use `trio.[fail/move_on]_[after/at]` instead - **TRIO110**: `while : await trio.sleep()` should be replaced by a `trio.Event`. +- **TRIO112**: nursery body with only a call to `nursery.start[_soon]` and not passing itself as a parameter can be replaced with a regular function call. diff --git a/flake8_trio.py b/flake8_trio.py index 12c50184..ca7bf67a 100644 --- a/flake8_trio.py +++ b/flake8_trio.py @@ -44,6 +44,7 @@ "`trio.[fail/move_on]_[after/at]` instead" ), "TRIO110": "`while : await trio.sleep()` should be replaced by a `trio.Event`.", + "TRIO112": "consider using a regular function call instead", } @@ -211,6 +212,7 @@ def __init__(self): def visit_With(self, node: Union[ast.With, ast.AsyncWith]): # 100 self.check_for_trio100(node) + self.check_for_trio112(node) # 101 for rest of function outer = self.get_state("_yield_is_error") @@ -302,6 +304,33 @@ def check_for_110(self, node: ast.While): ): self.error("TRIO110", node) + def check_for_trio112(self, node: Union[ast.With, ast.AsyncWith]): + if ( + len(node.items) == 1 + and len(node.body) == 1 + and isinstance(node.items[0].optional_vars, ast.Name) + ): + var_name = node.items[0].optional_vars.id + scope = get_trio_scope(node.items[0].context_expr, "open_nursery") + if ( + isinstance(node.body[0], ast.Expr) + and isinstance(node.body[0].value, ast.Call) + and isinstance(node.body[0].value.func, ast.Attribute) + and isinstance(node.body[0].value.func.value, ast.Name) + and node.body[0].value.func.value.id == var_name + and node.body[0].value.func.attr in ("start", "start_soon") + and scope + and not any( + (isinstance(n, ast.Name) and n.id == var_name) + for n in self.walk( + *node.body[0].value.args, *node.body[0].value.keywords + ) + ) + ): + + self.error("TRIO112", node) + self.generic_visit(node) + def critical_except(node: ast.ExceptHandler) -> Optional[Statement]: def has_exception(node: Optional[ast.expr]) -> str: diff --git a/tests/trio112.py b/tests/trio112.py new file mode 100644 index 00000000..afc4bd53 --- /dev/null +++ b/tests/trio112.py @@ -0,0 +1,35 @@ +import trio + +with trio.open_nursery() as n: # error: 0 + n.start(...) + +with trio.open_nursery() as n: # error: 0 + n.start_soon(...) + +with trio.open_nursery() as n: # error: 0 + n.start_soon(n=7) + +with trio.open_nursery() as n: # error in theory + n.start_soon(lambda n: n + 1) + +# safe? +with trio.open_nursery() as n, open("") as o: + n.start(...) + +# safe +with trio.open_nursery() as n: + n.start(..., n, ...) + +with trio.open_nursery() as n: + n.start(..., foo=n, bar=...) + +with trio.open_nursery() as n: + n.start(foo=tuple(tuple(tuple(tuple(n))))) + +with trio.open_nursery() as n: + ... + n.start_soon(...) + +with trio.open_nursery() as n: + n.start_soon(...) + ... From c57790b70f210793eaa0c5e64a31e8de2714b83e Mon Sep 17 00:00:00 2001 From: jakkdl Date: Wed, 10 Aug 2022 12:36:44 +0200 Subject: [PATCH 2/2] clean up code, support multiple withitems --- flake8_trio.py | 159 +++++++++++++++++++++++++---------------------- tests/trio112.py | 73 +++++++++++++++++++--- 2 files changed, 149 insertions(+), 83 deletions(-) diff --git a/flake8_trio.py b/flake8_trio.py index ca7bf67a..ef5fe351 100644 --- a/flake8_trio.py +++ b/flake8_trio.py @@ -11,7 +11,18 @@ import ast import tokenize -from typing import Any, Dict, Iterable, List, NamedTuple, Optional, Set, Union +from typing import ( + Any, + Dict, + Iterable, + List, + NamedTuple, + Optional, + Set, + Tuple, + Union, + cast, +) # CalVer: YY.month.patch, e.g. first release of July 2022 == "22.7.1" __version__ = "22.8.4" @@ -44,7 +55,7 @@ "`trio.[fail/move_on]_[after/at]` instead" ), "TRIO110": "`while : await trio.sleep()` should be replaced by a `trio.Event`.", - "TRIO112": "consider using a regular function call instead", + "TRIO112": "Redundant nursery {}, consider replacing with a regular function call", } @@ -64,32 +75,21 @@ def __eq__(self, other: Any) -> bool: ) -HasLineInfo = Union[ast.expr, ast.stmt, ast.arg, ast.excepthandler, Statement] - - -class TrioScope: - def __init__(self, node: ast.Call, funcname: str): - self.node = node - self.funcname = funcname - self.variable_name: Optional[str] = None - self.shielded: bool = False - self.has_timeout: bool = True +HasLineCol = Union[ast.expr, ast.stmt, ast.arg, ast.excepthandler, Statement] - # scope.shield is assigned to in visit_Assign - if self.funcname == "CancelScope": - self.has_timeout = False - for kw in node.keywords: - # Only accepts constant values - if kw.arg == "shield" and isinstance(kw.value, ast.Constant): - self.shielded = kw.value.value - # sets to True even if timeout is explicitly set to inf - if kw.arg == "deadline": - self.has_timeout = True - - def __str__(self): - # Not supporting other ways of importing trio, per TRIO106 - return f"trio.{self.funcname}" +def get_matching_call( + node: ast.AST, *names: str, base: str = "trio" +) -> Optional[Tuple[ast.Call, str]]: + if ( + isinstance(node, ast.Call) + and isinstance(node.func, ast.Attribute) + and isinstance(node.func.value, ast.Name) + and node.func.value.id == base + and node.func.attr in names + ): + return node, node.func.attr + return None class Error: @@ -158,7 +158,7 @@ def visit_nodes( for node in arg: visit(node) - def error(self, error: str, node: HasLineInfo, *args: object): + def error(self, error: str, node: HasLineCol, *args: object): if not self.suppress_errors: self._problems.append(Error(error, node.lineno, node.col_offset, *args)) @@ -178,18 +178,6 @@ def walk(self, *body: ast.AST) -> Iterable[ast.AST]: yield from ast.walk(b) -def get_trio_scope(node: ast.AST, *names: str) -> Optional[TrioScope]: - if ( - isinstance(node, ast.Call) - and isinstance(node.func, ast.Attribute) - and isinstance(node.func.value, ast.Name) - and node.func.value.id == "trio" - and node.func.attr in names - ): - return TrioScope(node, node.func.attr) - return None - - def has_decorator(decorator_list: List[ast.expr], *names: str): for dec in decorator_list: if (isinstance(dec, ast.Name) and dec.id in names) or ( @@ -221,7 +209,7 @@ def visit_With(self, node: Union[ast.With, ast.AsyncWith]): if not self._safe_decorator: for item in (i.context_expr for i in node.items): if ( - get_trio_scope(item, "open_nursery", *cancel_scope_names) + get_matching_call(item, "open_nursery", *cancel_scope_names) is not None ): self._yield_is_error = True @@ -236,14 +224,14 @@ def visit_With(self, node: Union[ast.With, ast.AsyncWith]): # ---- 100 ---- def check_for_trio100(self, node: Union[ast.With, ast.AsyncWith]): - # Context manager with no `await` call within + # Context manager with no `await trio.X` call within for item in (i.context_expr for i in node.items): - call = get_trio_scope(item, *cancel_scope_names) + call = get_matching_call(item, *cancel_scope_names) if call and not any( isinstance(x, checkpoint_node_types) and x != node for x in ast.walk(node) ): - self.error("TRIO100", item, str(call)) + self.error("TRIO100", item, f"trio.{call[1]}") # ---- 101 ---- def visit_FunctionDef(self, node: Union[ast.FunctionDef, ast.AsyncFunctionDef]): @@ -260,7 +248,7 @@ def visit_FunctionDef(self, node: Union[ast.FunctionDef, ast.AsyncFunctionDef]): # ---- 101, 109 ---- def visit_AsyncFunctionDef(self, node: ast.AsyncFunctionDef): - self.check_109(node) + self.check_for_trio109(node) self.visit_FunctionDef(node) # ---- 101 ---- @@ -271,7 +259,7 @@ def visit_Yield(self, node: ast.Yield): self.generic_visit(node) # ---- 109 ---- - def check_109(self, node: ast.AsyncFunctionDef): + def check_for_trio109(self, node: ast.AsyncFunctionDef): if node.decorator_list: return args = node.args @@ -292,44 +280,47 @@ def visit_Import(self, node: ast.Import): # ---- 110 ---- def visit_While(self, node: ast.While): - self.check_for_110(node) + self.check_for_trio110(node) self.generic_visit(node) - def check_for_110(self, node: ast.While): + def check_for_trio110(self, node: ast.While): if ( len(node.body) == 1 and isinstance(node.body[0], ast.Expr) and isinstance(node.body[0].value, ast.Await) - and get_trio_scope(node.body[0].value.value, "sleep", "sleep_until") + and get_matching_call(node.body[0].value.value, "sleep", "sleep_until") ): self.error("TRIO110", node) + # if with has a withitem `trio.open_nursery() as `, + # and the body is only a single expression .start[_soon](), + # and does not pass as a parameter to the expression def check_for_trio112(self, node: Union[ast.With, ast.AsyncWith]): - if ( - len(node.items) == 1 - and len(node.body) == 1 - and isinstance(node.items[0].optional_vars, ast.Name) - ): - var_name = node.items[0].optional_vars.id - scope = get_trio_scope(node.items[0].context_expr, "open_nursery") + # body is single expression + if len(node.body) != 1 or not isinstance(node.body[0], ast.Expr): + return + for item in node.items: + # get variable name + if not isinstance(item.optional_vars, ast.Name): + continue + var_name = item.optional_vars.id + + # check for trio.open_nursery + nursery = get_matching_call(item.context_expr, "open_nursery") + + # isinstance(..., ast.Call) is done in get_matching_call + body_call = cast(ast.Call, node.body[0].value) + if ( - isinstance(node.body[0], ast.Expr) - and isinstance(node.body[0].value, ast.Call) - and isinstance(node.body[0].value.func, ast.Attribute) - and isinstance(node.body[0].value.func.value, ast.Name) - and node.body[0].value.func.value.id == var_name - and node.body[0].value.func.attr in ("start", "start_soon") - and scope + nursery is not None + and get_matching_call(body_call, "start", "start_soon", base=var_name) + # check for presence of as parameter and not any( (isinstance(n, ast.Name) and n.id == var_name) - for n in self.walk( - *node.body[0].value.args, *node.body[0].value.keywords - ) + for n in self.walk(*body_call.args, *body_call.keywords) ) ): - - self.error("TRIO112", node) - self.generic_visit(node) + self.error("TRIO112", item.context_expr, var_name) def critical_except(node: ast.ExceptHandler) -> Optional[Statement]: @@ -362,10 +353,30 @@ def has_exception(node: Optional[ast.expr]) -> str: class Visitor102(Flake8TrioVisitor): + class TrioScope: + def __init__(self, node: ast.Call, funcname: str): + self.node = node + self.funcname = funcname + self.variable_name: Optional[str] = None + self.shielded: bool = False + self.has_timeout: bool = True + + # scope.shielded is assigned to in visit_Assign + + if self.funcname == "CancelScope": + self.has_timeout = False + for kw in node.keywords: + # Only accepts constant values + if kw.arg == "shield" and isinstance(kw.value, ast.Constant): + self.shielded = kw.value.value + # sets to True even if timeout is explicitly set to inf + if kw.arg == "deadline": + self.has_timeout = True + def __init__(self): super().__init__() self._critical_scope: Optional[Statement] = None - self._trio_context_managers: List[TrioScope] = [] + self._trio_context_managers: List[Visitor102.TrioScope] = [] self._safe_decorator = False # if we're inside a finally, and not inside a context_manager, and we're not @@ -393,17 +404,19 @@ def visit_With(self, node: Union[ast.With, ast.AsyncWith]): # Check for a `with trio.` for item in node.items: - trio_scope = get_trio_scope( + call = get_matching_call( item.context_expr, "open_nursery", *cancel_scope_names ) - if trio_scope is None: + if call is None: continue - self._trio_context_managers.append(trio_scope) - has_context_manager = True + trio_scope = self.TrioScope(*call) # check if it's saved in a variable if isinstance(item.optional_vars, ast.Name): trio_scope.variable_name = item.optional_vars.id + + self._trio_context_managers.append(trio_scope) + has_context_manager = True break self.generic_visit(node) diff --git a/tests/trio112.py b/tests/trio112.py index afc4bd53..41bf5d6d 100644 --- a/tests/trio112.py +++ b/tests/trio112.py @@ -1,31 +1,48 @@ import trio +import trio as noterror -with trio.open_nursery() as n: # error: 0 +# error +with trio.open_nursery() as n: # error: 5, "n" n.start(...) -with trio.open_nursery() as n: # error: 0 - n.start_soon(...) +with trio.open_nursery(...) as nurse: # error: 5, "nurse" + nurse.start_soon(...) -with trio.open_nursery() as n: # error: 0 +with trio.open_nursery() as n: # error: 5, "n" n.start_soon(n=7) -with trio.open_nursery() as n: # error in theory - n.start_soon(lambda n: n + 1) -# safe? -with trio.open_nursery() as n, open("") as o: +async def foo(): + async with trio.open_nursery() as n: # error: 15, "n" + n.start(...) + + +# weird ones with multiple `withitem`s +# but if split among several `with` they'd all be treated as error (or TRIO111), so +# treating as error for now. +with trio.open_nursery() as n, trio.open("") as n: # error: 5, "n" + n.start(...) + +with open("") as o, trio.open_nursery() as n: # error: 20, "n" + n.start(o) + +with trio.open_nursery() as n, trio.open_nursery() as nurse: # error: 31, "nurse" + nurse.start(n.start(...)) + +with trio.open_nursery() as n, trio.open_nursery() as n: # error: 5, "n" # error: 31, "n" n.start(...) -# safe +# safe if passing variable as parameter with trio.open_nursery() as n: n.start(..., n, ...) with trio.open_nursery() as n: - n.start(..., foo=n, bar=...) + n.start(..., foo=n + 7, bar=...) with trio.open_nursery() as n: n.start(foo=tuple(tuple(tuple(tuple(n))))) +# safe if multiple lines with trio.open_nursery() as n: ... n.start_soon(...) @@ -33,3 +50,39 @@ with trio.open_nursery() as n: n.start_soon(...) ... + +# fmt: off +with trio.open_nursery() as n: + n.start_soon(...) ; ... +# fmt: on + +# n as a parameter to lambda is in fact not using it, but we don't parse +with trio.open_nursery() as n: + n.start_soon(lambda n: n + 1) + +# body isn't a call to n.start +async def foo_1(): + with trio.open_nursery(...) as n: + await n.start(...) + + +# not *trio*.open_nursery +with noterror.open_nursery(...) as n: + n.start(...) + +# not trio.*open_nursery* +with trio.not_error(...) as n: + n.start(...) + +p = trio.not_error +# not *n*.start[_soon] +with trio.open_nursery() as n: + p.start(...) + +# not n.*start[_soon]* +with trio.open_nursery() as n: + n.start_never(...) + +# redundant nursery, not handled +with trio.open_nursery(): + pass