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..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,6 +55,7 @@ "`trio.[fail/move_on]_[after/at]` instead" ), "TRIO110": "`while : await trio.sleep()` should be replaced by a `trio.Event`.", + "TRIO112": "Redundant nursery {}, consider replacing with a regular function call", } @@ -63,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: @@ -157,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)) @@ -177,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 ( @@ -211,6 +200,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") @@ -219,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 @@ -234,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]): @@ -258,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 ---- @@ -269,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 @@ -290,18 +280,48 @@ 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]): + # 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 ( + 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(*body_call.args, *body_call.keywords) + ) + ): + self.error("TRIO112", item.context_expr, var_name) + def critical_except(node: ast.ExceptHandler) -> Optional[Statement]: def has_exception(node: Optional[ast.expr]) -> str: @@ -333,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 @@ -364,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 new file mode 100644 index 00000000..41bf5d6d --- /dev/null +++ b/tests/trio112.py @@ -0,0 +1,88 @@ +import trio +import trio as noterror + +# error +with trio.open_nursery() as n: # error: 5, "n" + n.start(...) + +with trio.open_nursery(...) as nurse: # error: 5, "nurse" + nurse.start_soon(...) + +with trio.open_nursery() as n: # error: 5, "n" + n.start_soon(n=7) + + +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 if passing variable as parameter +with trio.open_nursery() as n: + n.start(..., n, ...) + +with trio.open_nursery() as n: + 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(...) + +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