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

Lazily evaluate all PEP 695 type alias values #8033

Merged
merged 1 commit into from
Oct 18, 2023
Merged

Conversation

charliermarsh
Copy link
Member

Summary

In #7968, I introduced a regression whereby we started to treat imports used only in type annotation bounds (with __future__ annotations) as unused.

The root of the issue is that I started using visit_annotation for these bounds. So we'd queue up the bound in the list of deferred type parameters, then when visiting, we'd further queue it up in the list of deferred type annotations... Which we'd then never visit, since deferred type annotations are visited before deferred type parameters.

Anyway, the better solution here is to use a dedicated flag for these, since they have slightly different behavior than type annotations.

I've also fixed what I think is a bug whereby we previously failed to resolve Callable in:

type RecordCallback[R: Record] = Callable[[R], None]

from collections.abc import Callable

IIUC, the values in type aliases should be evaluated lazily, like type parameters.

Closes #8017.

Test Plan

cargo test

@charliermarsh charliermarsh requested a review from zanieb October 18, 2023 01:23
@charliermarsh charliermarsh added the bug Something isn't working label Oct 18, 2023
@@ -0,0 +1,5 @@
"""Test lazy evaluation of type alias values."""

type RecordCallback[R: Record] = Callable[[R], None]
Copy link
Member Author

Choose a reason for hiding this comment

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

Record should be undefined, but Callable should resolve.

from collections.abc import Callable

from .foo import Record as Record1
from .bar import Record as Record2
Copy link
Member Author

Choose a reason for hiding this comment

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

Both of these should be considered "used".

@github-actions
Copy link
Contributor

PR Check Results

Ecosystem

✅ ecosystem check detected no changes.

Copy link
Member

@zanieb zanieb left a comment

Choose a reason for hiding this comment

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

Thanks for fixing this! I was going to look into it but ya beat me to it (which is good because I would have been confused)

@charliermarsh charliermarsh merged commit a62c735 into main Oct 18, 2023
16 checks passed
@charliermarsh charliermarsh deleted the charlie/PEP branch October 18, 2023 01:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

F401 false positive with PEP-695 upper bound specification
2 participants