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 5 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
39 changes: 35 additions & 4 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 @@ -320,13 +326,20 @@ class AttributeCall(NamedTuple):
def with_has_call(
node: cst.With, *names: str, base: Iterable[str] = ("trio", "anyio")
) -> list[AttributeCall]:
if isinstance(base, str):
base = (base,) # pragma: no cover

for b in base:
if b.count(".") > 1: # pragma: no cover
raise NotImplementedError("Does not support 3-module bases atm.")

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"),
value=m.SaveMatchedNode(m.Name() | m.Attribute(), name="library"),
attr=m.SaveMatchedNode(
oneof_names(*names),
name="function",
Expand All @@ -335,12 +348,30 @@ def with_has_call(
),
):
assert isinstance(item.item, cst.Call)
assert isinstance(res["library"], cst.Name)
assert isinstance(res["library"], (cst.Name, cst.Attribute))
assert isinstance(res["function"], cst.Name)
if res["library"].value not in base:
library_node = res["library"]
for library_str in base:
if (
isinstance(library_node, cst.Name)
and library_str == library_node.value
):
break
Copy link
Member

Choose a reason for hiding this comment

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

These checks suggest to me that using a libcst matcher might be more efficient.

Could we have a helper function which parses a string ("name", or "x.y", or "x.y.z", or...) into the nodes, and then walks the nodes to return a corresponding matcher?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, though sounds complicated to engineer. I'll give it a try and see if I get anywhere easily, otherwise throw in a TODO and maybe deal with it at a later point.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ended up being straightforward to write, but got sidetracked by Instagram/LibCST#1143

if (
isinstance(library_node, cst.Attribute)
and isinstance(library_node.value, cst.Name)
and "." in library_str
):
base_1, base_2 = library_str.split(".")
if (
library_node.attr.value == base_2
and library_node.value.value == base_1
):
break
else:
continue
res_list.append(
AttributeCall(item.item, res["library"].value, res["function"].value)
AttributeCall(item.item, library_str, 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