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

Improve detection of TYPE_CHECKING blocks imported from typing_extensions or _typeshed #8429

Merged
merged 6 commits into from
Nov 9, 2023

Conversation

zanieb
Copy link
Member

@zanieb zanieb commented Nov 2, 2023

Improves detection of types imported from typing_extensions. Removes the hard-coded list of supported types in typing_extensions; instead assuming all types could be imported from typing, _typeshed, or typing_extensions.

The typing extensions package appears to re-export types even if they do not need modification.

Adds detection of if typing_extensions.TYPE_CHECKING blocks. Avoids inserting a new if TYPE_CHECKING block and from typing import TYPE_CHECKING if typing_extensions.TYPE_CHECKING is used (closes #8427)

@zanieb

This comment was marked as outdated.

@zanieb
Copy link
Member Author

zanieb commented Nov 2, 2023

It looks like at

let (type_checking_edit, type_checking) = self.get_or_import_symbol(
&ImportRequest::import_from("typing", "TYPE_CHECKING"),
at,
semantic,
)?;
we need to check if a typing_extensions.TYPE_CHECKING block is used before inserting the typing.TYPE_CHECKING block to actually solve the issue.

Copy link
Contributor

github-actions bot commented Nov 9, 2023

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

return true;
}

if is_typing_extension(target) {
Copy link
Member

Choose a reason for hiding this comment

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

I decided to remove this behavior in which we check if the member is part of typing_extensions. We could retain it... but that list may vary over time, so hard-coding it makes Ruff less future-proof.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm what's the effect of this then? Is it a breaking change?

Copy link
Member

Choose a reason for hiding this comment

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

It's not breaking, it's more lax (the net effect is that we consider all imports from typing_extensions to be treated equivalently to typing, rather than the allow-list that we encoded of specific members). But let's back it out.

@charliermarsh charliermarsh marked this pull request as ready for review November 9, 2023 04:40
@charliermarsh
Copy link
Member

@zanieb - I decided to finish this one up -- feel free to review and merge.

Comment on lines 177 to 180
if matches!(
call_path.as_slice(),
["typing" | "_typeshed" | "typing_extensions", target]
["typing" | "_typeshed" | "typing_extensions", _target]
) {
Copy link
Member Author

Choose a reason for hiding this comment

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

fyi using matches! really broke things because target here isn't what you wanted — it's capturing second item in the slice not using the provided target.

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed in 545bd30 — not sure how else to write it

@astral-sh astral-sh deleted a comment from github-actions bot Nov 9, 2023
@zanieb zanieb changed the title Add typing_extensions.TYPE_CHECKING to detection of type checking blocks Improves detection of types imported from typing_extensions Nov 9, 2023
@zanieb zanieb added the bug Something isn't working label Nov 9, 2023
@zanieb zanieb changed the title Improves detection of types imported from typing_extensions Improve detection of TYPE_CHECKING blocks imported from typing_extensions or _typeshed Nov 9, 2023
@zanieb zanieb merged commit 565ddeb into main Nov 9, 2023
17 checks passed
@zanieb zanieb deleted the zanie/type-checking branch November 9, 2023 18:21
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account?