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

Give Pyright what it wants (alias attributes everywhere) #3114

Open
wants to merge 20 commits into
base: main
Choose a base branch
from
Open
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
1 change: 1 addition & 0 deletions newsfragments/3114.bugfix.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Ensure that Pyright recognizes our underscore prefixed attributes for attrs classes.
4 changes: 2 additions & 2 deletions src/trio/_core/_local.py
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,8 @@ class RunVar(Generic[T]):

"""

_name: str
_default: T | type[_NoValue] = _NoValue
_name: str = attrs.field(alias="name")
_default: T | type[_NoValue] = attrs.field(default=_NoValue, alias="default")

def get(self, default: T | type[_NoValue] = _NoValue) -> T:
"""Gets the value of this :class:`RunVar` for the current run call."""
Expand Down
10 changes: 7 additions & 3 deletions src/trio/_core/_run.py
Original file line number Diff line number Diff line change
Expand Up @@ -544,9 +544,13 @@ class CancelScope:
cancelled_caught: bool = attrs.field(default=False, init=False)

# Constructor arguments:
_relative_deadline: float = attrs.field(default=inf, kw_only=True)
_deadline: float = attrs.field(default=inf, kw_only=True)
_shield: bool = attrs.field(default=False, kw_only=True)
_relative_deadline: float = attrs.field(
default=inf,
kw_only=True,
alias="relative_deadline",
)
_deadline: float = attrs.field(default=inf, kw_only=True, alias="deadline")
_shield: bool = attrs.field(default=False, kw_only=True, alias="shield")

def __attrs_post_init__(self) -> None:
if isnan(self._deadline):
Expand Down
70 changes: 70 additions & 0 deletions src/trio/_tests/test_exports.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
import json
import socket as stdlib_socket
import sys
import tokenize
import types
from pathlib import Path, PurePath
from types import ModuleType
Expand Down Expand Up @@ -572,3 +573,72 @@
continue

assert class_is_final(class_)


def test_pyright_recognizes_init_attributes() -> None:
# Obviously, this isn't completely accurate.
# It should still be good enough. Hopefully.
# (attrs updates fields before we can access them)
files = []

for path in (Path(inspect.getfile(trio)) / "..").glob("**/*.py"):
with open(path, "rb") as f:
files.append(list(tokenize.tokenize(f.readline)))

Check warning on line 586 in src/trio/_tests/test_exports.py

View check run for this annotation

Codecov / codecov/patch

src/trio/_tests/test_exports.py#L586

Added line #L586 was not covered by tests

for module in PUBLIC_MODULES:
for name, class_ in module.__dict__.items():
if not attrs.has(class_):
continue
if isinstance(class_, _util.NoPublicConstructor):
continue

Check warning on line 593 in src/trio/_tests/test_exports.py

View check run for this annotation

Codecov / codecov/patch

src/trio/_tests/test_exports.py#L593

Added line #L593 was not covered by tests

file = None
start = None
for contents in files:
last_was_class = False

Check warning on line 598 in src/trio/_tests/test_exports.py

View check run for this annotation

Codecov / codecov/patch

src/trio/_tests/test_exports.py#L598

Added line #L598 was not covered by tests
for i, token in enumerate(contents):
if (
token.type == tokenize.NAME
and token.string == name
and last_was_class
):
assert file is None
file = contents
start = i - 1

Check warning on line 607 in src/trio/_tests/test_exports.py

View check run for this annotation

Codecov / codecov/patch

src/trio/_tests/test_exports.py#L605-L607

Added lines #L605 - L607 were not covered by tests

if token.type == tokenize.NAME and token.string == "class":
last_was_class = True

Check warning on line 610 in src/trio/_tests/test_exports.py

View check run for this annotation

Codecov / codecov/patch

src/trio/_tests/test_exports.py#L610

Added line #L610 was not covered by tests
else:
last_was_class = False

Check warning on line 612 in src/trio/_tests/test_exports.py

View check run for this annotation

Codecov / codecov/patch

src/trio/_tests/test_exports.py#L612

Added line #L612 was not covered by tests

assert file is not None, f"{name}: {class_!r}"
A5rocks marked this conversation as resolved.
Show resolved Hide resolved

count = -1

Check warning on line 616 in src/trio/_tests/test_exports.py

View check run for this annotation

Codecov / codecov/patch

src/trio/_tests/test_exports.py#L616

Added line #L616 was not covered by tests
# linters don't like my not using the index, go figure.
for end_offset, token in enumerate(file[start:]): # noqa: B007
Copy link
Member

Choose a reason for hiding this comment

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

if you declare end_offset outside the loop I think the warning goes away (and makes the code easier to read).

codecov wants a pragma: no branch on the loop to hit 100%

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately the warning doesn't go away; it does make the code more correct (previously it didn't handle start==len(file)-1, I think)

Copy link
Member

Choose a reason for hiding this comment

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

I think with the zero-initialization now that should be good

Copy link
Member

Choose a reason for hiding this comment

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

This is fairly gnarly to fix in an AST visitor, I don't see any way of doing it that's not overkill, but I think you could open an issue for ruff and see if they have a decent way of getting at it.

if token.type == tokenize.INDENT:
count += 1

Check warning on line 620 in src/trio/_tests/test_exports.py

View check run for this annotation

Codecov / codecov/patch

src/trio/_tests/test_exports.py#L620

Added line #L620 was not covered by tests
if token.type == tokenize.DEDENT and count:
count -= 1

Check warning on line 622 in src/trio/_tests/test_exports.py

View check run for this annotation

Codecov / codecov/patch

src/trio/_tests/test_exports.py#L622

Added line #L622 was not covered by tests
elif token.type == tokenize.DEDENT:
break

Check warning on line 624 in src/trio/_tests/test_exports.py

View check run for this annotation

Codecov / codecov/patch

src/trio/_tests/test_exports.py#L624

Added line #L624 was not covered by tests

assert token.type == tokenize.DEDENT
class_source = (

Check warning on line 627 in src/trio/_tests/test_exports.py

View check run for this annotation

Codecov / codecov/patch

src/trio/_tests/test_exports.py#L626-L627

Added lines #L626 - L627 were not covered by tests
tokenize.untokenize(file[start : start + end_offset])

Check failure on line 628 in src/trio/_tests/test_exports.py

View workflow job for this annotation

GitHub Actions / Ubuntu (3.12, check formatting)

Mypy-Linux+Mac+Windows

src/trio/_tests/test_exports.py:(628:50 - 628:50): Unsupported operand types for + ("None" and "int") [operator]

Check notice on line 628 in src/trio/_tests/test_exports.py

View workflow job for this annotation

GitHub Actions / Ubuntu (3.12, check formatting)

Mypy-Linux+Mac+Windows

src/trio/_tests/test_exports.py:(628:50 - 628:50): Left operand is of type "Optional[int]"
.replace("\\\n", "")
.strip()
)

attributes = list(attrs.fields(class_))

Check warning on line 633 in src/trio/_tests/test_exports.py

View check run for this annotation

Codecov / codecov/patch

src/trio/_tests/test_exports.py#L633

Added line #L633 was not covered by tests
attributes = [attr for attr in attributes if attr.name.startswith("_")]
attributes = [attr for attr in attributes if attr.init]

attributes = [
# could this be improved by parsing AST? yes. this is simpler though.
attr
for attr in attributes
if f'alias="{attr.alias}"' not in class_source
]

assert attributes == [], class_

Check warning on line 644 in src/trio/_tests/test_exports.py

View check run for this annotation

Codecov / codecov/patch

src/trio/_tests/test_exports.py#L644

Added line #L644 was not covered by tests
Loading