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

Followup changes to fix ruff & pyright warnings #203

Merged
merged 6 commits into from
Feb 22, 2024
Merged
Show file tree
Hide file tree
Changes from 2 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
1 change: 1 addition & 0 deletions README.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
[![pre-commit.ci status](https://results.pre-commit.ci/badge/github/python-trio/flake8-trio/main.svg)](https://results.pre-commit.ci/latest/github/python-trio/flake8-trio/main)
# flake8-trio

A highly opinionated flake8 plugin for [Trio](https://github.com/python-trio/trio)-related problems.
Expand Down
12 changes: 0 additions & 12 deletions flake8_trio/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -29,18 +29,6 @@ class Statement(NamedTuple):
lineno: int
col_offset: int = -1

# pyright is unhappy about defining __eq__ but not __hash__ .. which it should
# but it works :tm: and needs changing in a couple places to avoid it.
def __eq__(self, other: object) -> bool:
return (
isinstance(other, Statement)
and self[:2] == other[:2]
and (
self.col_offset == other.col_offset
or -1 in (self.col_offset, other.col_offset)
)
)


class Error:
def __init__(
Expand Down
4 changes: 2 additions & 2 deletions flake8_trio/runner.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@
from .visitors.visitor_utility import NoqaHandler

if TYPE_CHECKING:
from collections.abc import Iterable
from collections.abc import Iterable, Mapping

from libcst import Module

Expand All @@ -46,7 +46,7 @@ def __init__(self, options: Options):
super().__init__()
self.state = SharedState(options)

def selected(self, error_codes: dict[str, str]) -> bool:
def selected(self, error_codes: Mapping[str, str]) -> bool:
enabled_or_autofix = (
self.state.options.enabled_codes | self.state.options.autofix_codes
)
Expand Down
8 changes: 4 additions & 4 deletions flake8_trio/visitors/flake8triovisitor.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,15 +4,15 @@

import ast
from abc import ABC
from typing import TYPE_CHECKING, Any, ClassVar, Union
from typing import TYPE_CHECKING, Any, Union

import libcst as cst
from libcst.metadata import PositionProvider

from ..base import Error, Statement

if TYPE_CHECKING:
from collections.abc import Iterable
from collections.abc import Iterable, Mapping

from ..runner import SharedState

Expand All @@ -23,7 +23,7 @@

class Flake8TrioVisitor(ast.NodeVisitor, ABC):
# abstract attribute by not providing a value
error_codes: ClassVar[dict[str, str]]
error_codes: Mapping[str, str]

def __init__(self, shared_state: SharedState):
super().__init__()
Expand Down Expand Up @@ -158,7 +158,7 @@ def add_library(self, name: str) -> None:

class Flake8TrioVisitor_cst(cst.CSTTransformer, ABC):
# abstract attribute by not providing a value
error_codes: dict[str, str]
error_codes: Mapping[str, str]
METADATA_DEPENDENCIES = (PositionProvider,)

def __init__(self, shared_state: SharedState):
Expand Down
10 changes: 4 additions & 6 deletions flake8_trio/visitors/visitor91x.py
Original file line number Diff line number Diff line change
Expand Up @@ -26,9 +26,10 @@
)

if TYPE_CHECKING:
from collections.abc import Sequence
from collections.abc import Mapping, Sequence


# Statement injected at the start of loops to track missed checkpoints.
ARTIFICIAL_STATEMENT = Statement("artificial", -1)


Expand Down Expand Up @@ -226,7 +227,7 @@ def leave_Yield(
@error_class_cst
@disabled_by_default
class Visitor91X(Flake8TrioVisitor_cst, CommonVisitors):
error_codes = {
error_codes: Mapping[str, str] = {
"TRIO910": (
"{0} from async function with no guaranteed checkpoint or exception "
"since function definition on line {1.lineno}."
Expand Down Expand Up @@ -591,10 +592,7 @@ def visit_While_body(self, node: cst.For | cst.While):
if getattr(node, "asynchronous", None):
self.uncheckpointed_statements = set()
else:
# pyright correctly dislikes Statement defining __eq__ but not __hash__
# but it works:tm:, and changing it touches on various bits of code, so
# leaving it for another time.
self.uncheckpointed_statements = {ARTIFICIAL_STATEMENT} # pyright: ignore
self.uncheckpointed_statements = {ARTIFICIAL_STATEMENT}

self.loop_state.uncheckpointed_before_continue = set()
self.loop_state.uncheckpointed_before_break = set()
Expand Down
4 changes: 1 addition & 3 deletions pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -95,10 +95,8 @@ ignore = [
'TRY003', # Avoid specifying long messages outside the exception class
'B904', # Use `raise from` to specify exception cause
'TRY201', # Use `raise` without specifying exception name
'FIX002', # line contains #TODO
'RUF012' # Mutable class attribute should be annotated with `typing.ClassVar`
'FIX002' # line contains #TODO
]
# RUF012 occurs in 25 places ... I'm not going to fix that unless type checkers also complain
select = ["ALL"]

[tool.ruff.lint.per-file-ignores]
Expand Down
20 changes: 18 additions & 2 deletions tests/test_flake8_trio.py
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,22 @@
class ParseError(Exception): ...


class ColumnAgnosticStatement(Statement):
def __eq__(self, other: object) -> bool:

Choose a reason for hiding this comment

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

Probably worth a comment explaining what this class is, and why we override __eq__ and __hash__ (and what the semantics of this class are).

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 reverting stuff and implementing a simple __hash__ :)

assert isinstance(other, Statement)
return (
self.name == other.name
and self.lineno == other.lineno
and (
self.col_offset == other.col_offset
or -1 in (self.col_offset, other.col_offset)
)
)

def __hash__(self) -> int:
raise NotImplementedError

Choose a reason for hiding this comment

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

Why not super().__hash__()?

Copy link
Member Author

@jakkdl jakkdl Feb 21, 2024

Choose a reason for hiding this comment

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

That could cause objects that are equal to have different hashes (because -1 is a wildcard value), which is Not Allowed ™️
https://docs.python.org/3/reference/datamodel.html#object.__hash__

Rereading it now, it turns out I misread that originally and I didn't have to complicate things as much as I did. I originally read it as __eq__ objects had to have the same hash, and objects with same hash value had to be equal to eachother...

So I could implement __hash__() with return hash(tuple(self.name, self.lineno))



# check for presence of _pyXX, skip if version is later, and prune parameter
def check_version(test: str):
python_version = re.search(r"(?<=_PY)\d*", test)
Expand Down Expand Up @@ -332,8 +348,8 @@ def _parse_eval_file(test: str, content: str) -> tuple[list[Error], list[str], s
{
"lineno": lineno,
"line": lineno,
"Statement": Statement,
"Stmt": Statement,
"Statement": ColumnAgnosticStatement,
"Stmt": ColumnAgnosticStatement,
},
)
except NameError:
Expand Down
Loading