Recognize all symbols named TYPE_CHECKING for in_type_checking_block
#15719
Conversation
This matches the current behavior of mypy and pyright as well as `flake8-type-checking`.
Daverball
force-pushed
the
feat/TYPE_CHECKING_without_typing_import
branch
from
cdea018 to
f9f6540
Compare
|
I will add more test cases, since I'm worried some of the TC fixes will not be able to deal with these new kinds of type checking blocks without additional changes. |
|
| code | total | + violation | - violation | + fix | - fix |
|---|---|---|---|---|---|
| TC004 | 1 | 1 | 0 | 0 | 0 |
|
That was quick, thanks. I'm leaning towards making this a preview change because it's technically breaking. The counterargument is that there's barely any ecosystem change. |
|
I'm a little confused by the scikit ecosystem change. If anything I would've expected a previous TC003 error to disappear, not a new TC004 to appear, especially since there is no runtime use of So it seems like there's a different bug at play here, that was previously obscured by not recognizing the |
|
@MichaReiser If we're going to treat it as a breaking change anyways and put it behind the preview flag, I would suggest to also stop treating |
Looks like references to bindings don't really try to find their matching import beyond the first level, so since there are multiple So it looks like |
|
@MichaReiser I wonder if it would be alright to copy the preview flag to the I will also need to make some structural changes to |
| Some(Edit::range_replacement( | ||
| self.locator.slice(type_checking.range()).to_string(), | ||
| type_checking.range(), | ||
| )) |
There was a problem hiding this comment.
By restructuring the logic, this no-op edit would now only happen if we don't have an existing preceding type checking block. I'm not sure whether this is problematic or why we're adding a no-op edit to begin with (maybe to ensure no other edit is allowed to remove the import?).
If this no-op edit indeed accomplishes something useful, we may need a more sophisticated change, which allows find_type_checking to return a name based on an assignment target in addition to an import. This would allow us to write separate fix logic for when it's imported and when it's defined in the same file.
There was a problem hiding this comment.
Oh yeah, the no-op edit is important. It's a "hack" to prevent any other fix to remove the import. At least, I remember something along those lines from @charliermarsh
There was a problem hiding this comment.
I assumed that was the case, but then was surprised that none of the tests were failing. Seems kind of important to have at least one regression test for this case. In the absence of a failing test the other possibility is that this since has been fixed in a different way and is now essentially doing nothing.
There was a problem hiding this comment.
The other thing that bothered me, is that this hack does not seem very reliable, since the type checking block we're moving the imports to might not correspond to the typing/TYPE_CHECKING import we find in the first step...
Maybe we revert it back to my earlier refactored version, but when we have an existing type checking block we lookup the relevant statement from the binding that's referenced by the condition in the type checking block and emit a no-op edit for that specific statement if it doesn't overlap with one of the import edits. That seems a lot more robust to me.
There was a problem hiding this comment.
I had another little think about this. I don't think we have to worry about the no-op edit when we find a preceding type checking block. If another edit would remove the binding we're referencing, it would necessarily also have to remove the type checking block itself (otherwise there would still be a reference to the binding), but then the edit that removes the type checking block would overlap with our edit to add a new import to it, so we don't need to artificially create another overlap.
In the case where there isn't a type checking block, but there is a unused TYPE_CHECKING import, it makes sense, since we don't want to let F401 remove the import when our edit adds a new reference to it.
There was a problem hiding this comment.
Yeah, I believe this is to prevent things like:
from typing import TYPE_CHECKING
from foo import Bar
def func() -> Bar: ...If we don't add the no-op edit, TYPE_CHECKING might be removed, leaving us with:
if TYPE_CHECKING:
from foo import Bar
def func() -> "Bar": ...
MichaReiser
requested review from
carljm,
MichaReiser,
AlexWaygood and
sharkdp
as code owners
| // TODO: Should we provide an option to avoid this import? | ||
| // E.g. either through an explicit setting, or implicitly | ||
| // when `typing` isn't part of the exempt modules and there | ||
| // are no other existing runtime imports of `typing`. |
There was a problem hiding this comment.
Hmm, yeah this is interesting. Not having an option seems fine to me for now, we can introduce one later if it is being asked for.
The only problematic part of that is adding a preview flag check will add quite the rat tail to the diff, since the function is used in so many places. One way to get around this would be to add the preview flag to the semantic model. Which I would've found useful in the past anyways. (Either as a |
|
Ah right, sorry. I forgot about our previous conversation while I was on PTO. Let me have a quick look |
|
Adding a preview option to |
|
@MichaReiser Since we're now gating the change I also removed special casing for |
Sounds reasonable. Would you mind updating your PR summary to reflect this change? |
This tries to still emit a no-op edit whenever possible
| let type_checking_edit = | ||
| if let Some(statement) = Self::type_checking_binding_statement(semantic, block) { | ||
| if statement == import.statement { | ||
| // Special-case: if the `TYPE_CHECKING` symbol is imported as part of the same | ||
| // statement that we're modifying, avoid adding a no-op edit. For example, here, | ||
| // the `TYPE_CHECKING` no-op edit would overlap with the edit to remove `Final` | ||
| // from the import: | ||
| // ```python | ||
| // from __future__ import annotations | ||
| // | ||
| // from typing import Final, TYPE_CHECKING | ||
| // | ||
| // Const: Final[dict] = {} | ||
| // ``` | ||
| None | ||
| } else { | ||
| Some(Edit::range_replacement( | ||
| self.locator.slice(statement.range()).to_string(), | ||
| statement.range(), | ||
| )) | ||
| } | ||
| } else { | ||
| None | ||
| }; |
There was a problem hiding this comment.
This should retain the no-op edit in the most common case (the top-most TYPE_CHECKING block is defined in global scope). Although this brought up a question about the implementation of Importer:
Why are we adding both the if statement and the body AST node to type_checking_blocks in global scope but only the body AST node elsewhere? I don't really see an obvious reason. Shouldn't we just always pass the body or always the entire if statement? Or at least either one or the other, but never both. Either way it is pretty confusing and it would seem more consistent to pass in something like a struct that contains the condition, the body and the scope. That would also make it flexible enough to support a non-idiomatic if not TYPE_CHECKING with an else clause.
Closes #15681
Summary
This changes
analyze::typing::is_type_checking_blockto recognize all symbols named "TYPE_CHECKING".This matches the current behavior of mypy and pyright as well as
flake8-type-checking.It also drops support for detecting
if False:andif 0:as type checking blocks. This used to be an option forproviding backwards compatibility with Python versions that did not have a
typingmodule, but has sincebeen removed from the typing spec and is no longer supported by any of the mainstream type checkers.
Test Plan
cargo nextest run