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

TC003/TC002/TC001 False positive on manual TYPE_CHECKING with __future__.annotations #15681

Open
cibere opened this issue Jan 23, 2025 · 13 comments · May be fixed by #15719
Open

TC003/TC002/TC001 False positive on manual TYPE_CHECKING with __future__.annotations #15681

cibere opened this issue Jan 23, 2025 · 13 comments · May be fixed by #15719
Labels
rule Implementing or modifying a lint rule

Comments

@cibere
Copy link

cibere commented Jan 23, 2025

Description

If you manually set TYPE_CHECKING to False and use it with __future__.annotations, ruff will raise TC003/TC002/TC001 falsepositives on the imports in your TYPE_CHECKING block as it seems to only check for typing.TYPE_CHECKING.

Command: ruff check test.py --isolated --fix --select TC
Version: 0.9.2
Keywords: "TYPE_CHECKING", "TC003", "future.annotations", "TC002", "TC001"

Sample Code

test.py

from __future__ import annotations

TYPE_CHECKING = False
if TYPE_CHECKING:
    from types import TracebackType

def foo(tb: TracebackType):...
$ ruff check test.py --isolated --fix --select TC
test.py:5:23: TC003 Move standard library import `types.TracebackType` into a type-checking block
  |
3 | TYPE_CHECKING = False
4 | if TYPE_CHECKING:
5 |     from types import TracebackType
  |                       ^^^^^^^^^^^^^ TC003
6 |
7 | def foo(tb: TracebackType):...
  |
  = help: Move into type-checking block

Found 1 error.
@cibere cibere changed the title TC003 Falsepositive on manual TYPE_CHECKING with __future__.annotations TC003/TC002/TC001 Falsepositive on manual TYPE_CHECKING with __future__.annotations Jan 23, 2025
@Daverball
Copy link
Contributor

Daverball commented Jan 23, 2025

In the original flake8 plugin I treat all ast.Name loads with the id TYPE_CHECKING as implicitly typing.TYPE_CHECKING. This would allow this to work, but it can lead to inaccurate behavior the other way (although I don't think it's likely to ever cause any issues in real code, that doesn't explicitly try to trick us).

We could be more narrow and just detect this specific case, where in addition to the above condition, if the qualified name doesn't match a typing module, resolve_assignment on the ast.Name must result in builtins.False. It is a slightly more expensive check (although not that much more expensive either, considering there's usually only one of these in a file, unless you make heavy use of mixins).

@Daverball
Copy link
Contributor

The only other thing I'll say, is that the fixes for the TC range currently don't support this "avoid importing typing at runtime entirely" pattern currently. So this is something we may want to give people as an option (maybe we just switch to this style as soon as typing isn't part of exempt_modules)?

I.e. when the typing module isn't exempt and no typing imports other than TYPE_CHECKING are used at runtime, it will add TYPE_CHECKING = False above the first block in global scope and move the typing import inside. But there's a lot of potential complexities here, that could make this difficult, so maybe we'll just let people handle these cases on their own.

The other caveat is, that the fix for TC005 would need to remove TYPE_CHECKING = False in addition to the empty type checking block, if there are no other references to TYPE_CHECKING in that file.


It's also worth noting that ruff supports if False and if 0, but newer versions of type checkers no longer support this, since it was taken out of the spec. There have been recent discussions about putting something back into the spec that allows getting rid of the runtime typing import. While this pattern works with mypy and pyright, it's not part of the typing spec and I'm not sure if other type checkers recognize it.

@MichaReiser MichaReiser added the rule Implementing or modifying a lint rule label Jan 23, 2025
@MichaReiser
Copy link
Member

@carljm @AlexWaygood what are our plans for Red Knot around supporting non-typing TYPE_CHECKING guards? Is this something we plan on supporting and if not, why?

@Daverball
Copy link
Contributor

The recent discussion I talked about can be found here: https://discuss.python.org/t/specify-type-checking-false-without-typing-import/76766

@Daverball
Copy link
Contributor

FWIW, both mypy and Pyright seems to support non-typing TYPE_CHECKING guards:

Yup, although if you look at the linked discussion, Eric opposes the addition of this form to the typing spec, so I'm guessing the only reason pyright currently supports it, is so he can offer better feature parity with mypy. The discussion didn't really reach a consensus about how to support this use-case.

Although given, that mypy and pyright have the biggest install bases currently, I don't see a strong reason for ruff to hold out on support for this, even if it ends up being phased out again, like the previous if False support in type checkers, which was used for supporting versions of Python, that did not yet have a typing module.

I personally think if False: paired with a pragma style comment would offer the best UX, beyond adding TYPE_CHECKING to builtins (which would only really help with new Python versions going forward), but that's besides the point, until there's actually something in the typing spec, we can rely on.

@carljm
Copy link
Contributor

carljm commented Jan 23, 2025

what are our plans for Red Knot around supporting non-typing TYPE_CHECKING guards?

I'm not sure. My vague memory is that I had commented somewhere that we should support this, and there was some pushback (I don't recall from who) so we deferred it. I can't seem to find that discussion now.

I don't have strong feelings in either direction, but my inclination is that we should support it in order to lower migration barriers from mypy/pyright, because I don't think there is much cost in supporting it.

@dylwil3 dylwil3 changed the title TC003/TC002/TC001 Falsepositive on manual TYPE_CHECKING with __future__.annotations TC003/TC002/TC001 False positive on manual TYPE_CHECKING with __future__.annotations Jan 23, 2025
@sharkdp
Copy link
Contributor

sharkdp commented Jan 24, 2025

what are our plans for Red Knot around supporting non-typing TYPE_CHECKING guards?

I'm not sure. My vague memory is that I had commented somewhere that we should support this, and there was some pushback (I don't recall from who) so we deferred it. I can't seem to find that discussion now.

This one? #14759 (comment)

@AlexWaygood
Copy link
Member

what are our plans for Red Knot around supporting non-typing TYPE_CHECKING guards?

I'm not sure. My vague memory is that I had commented somewhere that we should support this, and there was some pushback (I don't recall from who) so we deferred it. I can't seem to find that discussion now.

This one? #14759 (comment)

I tried to find that comment thread yesterday but failed — thanks @sharkdp!

I still have the same opinion as I did then, that treating all variables named TYPE_CHECKING as always truthy doesn't feel very principled, and that it feels like it might be redundant once we have a generalised --always-truthy setting for specifying constants that red-knot should consider to be always truthy or always falsey. That said, I don't have a strong opinion on this. I'm okay with doing the same as mypy and pyright if it turns out to be very painful or unintuitive for users that we have different behaviour to the other type checkers here.

@Daverball
Copy link
Contributor

Daverball commented Jan 24, 2025

[...] once we have a generalised --always-truthy setting for specifying constants that red-knot should consider to be always truthy or always falsey.

Unfortunately that's not really a viable solution for library authors. You can't really ask that all your downstream users adjust their type checker configuration for your special TYPE_CHECKING constant. It's only really a viable solution, if your code has no downstream consumers, or you are the only downstream consumer.

The use-case is pretty clear. People want there to be a portable way for not making every downstream user pay the cost of the typing import. Remember that not every user of a typed library will actually make use of static type checking and even when they do, they might operate under tight startup time constraints, so every import they can shave off matters, especially expensive ones like typing.

While shipping separate stub files achieves that goal, it can be a pain to maintain. So I have sympathy for people that want there to be a standard way, to do the if TYPE_CHECKING check without having to import typing.

I do however agree with you, that this solution feels icky (especially since it messes with runtime type checking) and I think we can provide a better solution eventually, where we don't have to rely on TYPE_CHECKING blocks, like e.g. providing a new type import statement, that defers the import until the symbol is explicitly accessed.

But until we have a better solution, it would be nice to give people a way that's at least supported by the most popular type checkers right now, even if it's not part of the typing spec.

@carljm
Copy link
Contributor

carljm commented Jan 24, 2025

I agree with @Daverball here. I agree that in principle it's icky to special case a variable name, but I think the practical harm is very small, the use case is real, and the version that requires explicit configuration is not usable for libraries. So I would support exploring standardizing nicer ways to do this, but pending that I think we should match mypy and pyright.

@AlexWaygood
Copy link
Member

AlexWaygood commented Jan 24, 2025

Yeah, that all seems reasonable to me; I'm happy to go with that. I agree that @Daverball makes a really good point about libraries.

@AlexWaygood
Copy link
Member

AlexWaygood commented Jan 24, 2025

We've opened an issue to track changing red-knot's behaviour to match other type checkers: #15722 (thanks @carljm!)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rule Implementing or modifying a lint rule
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants