Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ASYNC912: timeout/cancelscope with only conditional checkpoints #242

Merged
merged 6 commits into from
May 13, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,6 +1,11 @@
# Changelog
*[CalVer, YY.month.patch](https://calver.org/)*

## 24.5.1
- Add ASYNC912: no checkpoints in with statement are guaranteed to run.
- ASYNC100 now properly treats async for comprehensions as checkpoints.
- ASYNC100 now supports autofixing on asyncio.

## 24.4.2
- Add ASYNC119: yield in contextmanager in async generator.

Expand Down
1 change: 1 addition & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ Note: 22X, 23X and 24X has not had asyncio-specific suggestions written.
- **ASYNC910**: Exit or `return` from async function with no guaranteed checkpoint or exception since function definition. You might want to enable this on a codebase to make it easier to reason about checkpoints, and make the logic of ASYNC911 correct.
- **ASYNC911**: Exit, `yield` or `return` from async iterable with no guaranteed checkpoint since possible function entry (yield or function definition)
Checkpoints are `await`, `async for`, and `async with` (on one of enter/exit).
- **ASYNC912**: Timeout/Cancelscope has no awaits that are guaranteed to run. If the scope has no checkpoints at all, then `ASYNC100` will be raised instead.

### Removed Warnings
- **TRIOxxx**: All error codes are now renamed ASYNCxxx
Expand Down
1 change: 1 addition & 0 deletions docs/rules.rst
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ Optional rules disabled by default
- **ASYNC910**: Exit or ``return`` from async function with no guaranteed checkpoint or exception since function definition. You might want to enable this on a codebase to make it easier to reason about checkpoints, and make the logic of ASYNC911 correct.
- **ASYNC911**: Exit, ``yield`` or ``return`` from async iterable with no guaranteed checkpoint since possible function entry (yield or function definition)
Checkpoints are ``await``, ``async for``, and ``async with`` (on one of enter/exit).
-- **ASYNC912**: A timeout/cancelscope has checkpoints, but they're not guaranteed to run. Similar to ASYNC100, but it does not warn on trivial cases where there is no checkpoint at all. It instead shares logic with ASYNC910 and ASYNC911 for parsing conditionals and branches.

Removed rules
================
Expand Down
2 changes: 1 addition & 1 deletion flake8_async/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@


# CalVer: YY.month.patch, e.g. first release of July 2022 == "22.7.1"
__version__ = "24.4.2"
__version__ = "24.5.1"


# taken from https://github.com/Zac-HD/shed
Expand Down
1 change: 0 additions & 1 deletion flake8_async/visitors/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,6 @@
from . import (
visitor2xx,
visitor91x,
visitor100,
visitor101,
visitor102,
visitor103_104,
Expand Down
14 changes: 11 additions & 3 deletions flake8_async/visitors/flake8asyncvisitor.py
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,11 @@ def error(
), "No error code defined, but class has multiple codes"
error_code = next(iter(self.error_codes))
# don't emit an error if this code is disabled in a multi-code visitor
elif strip_error_subidentifier(error_code) not in self.options.enabled_codes:
elif (
(ec_no_sub := strip_error_subidentifier(error_code))
not in self.options.enabled_codes
and ec_no_sub not in self.options.autofix_codes
):
Comment on lines -101 to +105
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's not great that there's duplication of logic between the visitors (it confused me for a bit while developing). I originally expected all visitors to be rewritten to use libcst, but given that's not going to happen anytime soon (or at all), I should probably refactor these two and put common code in a parent class.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense to me 👍

return

self.__state.problems.append(
Expand Down Expand Up @@ -217,7 +221,11 @@ def error(
error_code = next(iter(self.error_codes))
# don't emit an error if this code is disabled in a multi-code visitor
# TODO: write test for only one of 910/911 enabled/autofixed
elif strip_error_subidentifier(error_code) not in self.options.enabled_codes:
elif (
(ec_no_sub := strip_error_subidentifier(error_code))
not in self.options.enabled_codes
and ec_no_sub not in self.options.autofix_codes
):
return False # pragma: no cover

if self.is_noqa(node, error_code):
Expand All @@ -237,7 +245,7 @@ def error(
return True

def should_autofix(self, node: cst.CSTNode, code: str | None = None) -> bool:
if code is None:
if code is None: # pragma: no cover
assert len(self.error_codes) == 1
code = next(iter(self.error_codes))
# this does not currently need to check for `noqa`s, as error() does that
Expand Down
76 changes: 60 additions & 16 deletions flake8_async/visitors/helpers.py
Original file line number Diff line number Diff line change
Expand Up @@ -51,13 +51,19 @@ def error_class_cst(error_class: type[T_CST]) -> type[T_CST]:


def disabled_by_default(error_class: type[T_EITHER]) -> type[T_EITHER]:
"""Default-disables all error codes in a class."""
assert error_class.error_codes # type: ignore[attr-defined]
default_disabled_error_codes.extend(
error_class.error_codes # type: ignore[attr-defined]
)
return error_class


def disable_codes_by_default(*codes: str) -> None:
"""Default-disables only specified codes."""
default_disabled_error_codes.extend(codes)


def utility_visitor(c: type[T]) -> type[T]:
assert not hasattr(c, "error_codes")
c.error_codes = {}
Expand Down Expand Up @@ -317,30 +323,68 @@ class AttributeCall(NamedTuple):
function: str


# the custom __or__ in libcst breaks pyright type checking. It's possible to use
# `Union` as a workaround ... except pyupgrade will automatically replace that.
# So we have to resort to specifying one of the base classes.
# See https://github.com/Instagram/LibCST/issues/1143
def build_cst_matcher(attr: str) -> m.BaseExpression:
"""Build a cst matcher structure with attributes&names matching a string `a.b.c`."""
if "." not in attr:
return m.Name(value=attr)
body, tail = attr.rsplit(".")
return m.Attribute(value=build_cst_matcher(body), attr=m.Name(value=tail))


def identifier_to_string(attr: cst.Name | cst.Attribute) -> str:
if isinstance(attr, cst.Name):
return attr.value
assert isinstance(attr.value, (cst.Attribute, cst.Name))
return identifier_to_string(attr.value) + "." + attr.attr.value


def with_has_call(
node: cst.With, *names: str, base: Iterable[str] = ("trio", "anyio")
) -> list[AttributeCall]:
"""Check if a with statement has a matching call, returning a list with matches.

`names` specify the names of functions to match, `base` specifies the
library/module(s) the function must be in.
The list elements in the return value are named tuples with the matched node,
base and function.

Examples_

`with_has_call(node, "bar", base="foo")` matches foo.bar.
`with_has_call(node, "bar", "bee", base=("foo", "a.b.c")` matches
`foo.bar`, `foo.bee`, `a.b.c.bar`, and `a.b.c.bee`.

"""
if isinstance(base, str):
base = (base,) # pragma: no cover

# build matcher, using SaveMatchedNode to save the base and the function name.
matcher = m.Call(
func=m.Attribute(
value=m.SaveMatchedNode(
m.OneOf(*(build_cst_matcher(b) for b in base)), name="base"
),
attr=m.SaveMatchedNode(
oneof_names(*names),
name="function",
),
)
)

res_list: list[AttributeCall] = []
for item in node.items:
if res := m.extract(
item.item,
m.Call(
func=m.Attribute(
value=m.SaveMatchedNode(m.Name(), name="library"),
attr=m.SaveMatchedNode(
oneof_names(*names),
name="function",
),
)
),
):
if res := m.extract(item.item, matcher):
assert isinstance(item.item, cst.Call)
assert isinstance(res["library"], cst.Name)
assert isinstance(res["base"], (cst.Name, cst.Attribute))
assert isinstance(res["function"], cst.Name)
if res["library"].value not in base:
continue
res_list.append(
AttributeCall(item.item, res["library"].value, res["function"].value)
AttributeCall(
item.item, identifier_to_string(res["base"]), res["function"].value
)
)
return res_list

Expand Down
90 changes: 0 additions & 90 deletions flake8_async/visitors/visitor100.py

This file was deleted.

Loading
Loading