python / mypy

Optional static typing for Python
https://www.mypy-lang.org/
Other
18.5k stars 2.83k forks source link

MyPy should report error if variable imported in TYPE_CHECKING block? #6104

Open sid-kap opened 5 years ago

sid-kap commented 5 years ago

Currently, MyPy doesn't report an error when a variable is imported in the if TYPE_CHECKING block and then used elsewhere. For example, MyPy accepts this code:

from typing import TYPE_CHECKING

if TYPE_CHECKING:
    from typing import NamedTuple

class C(NamedTuple):
    x: int
    y: int

even though, at runtime, it fails:

Traceback (most recent call last):
  File "type_checking_test.py", line 6, in <module>
    class C(NamedTuple):
NameError: name 'NamedTuple' is not defined

(I'm using MyPy version 0.650.)

Should this be fixed? I think this could be fixed by adding an is_type_checking_only variable to SymbolTableNode, which is True if the variable was imported or assigned within a TYPE_CHECKING block, and ensuring that this flag is false for any variables used in places other than type annotations in the actual code.

ilevkivskyi commented 5 years ago

Should this be fixed?

This is highly questionable, TBH. Although implementation is not hard, it still will require some work, while the value is relatively small. Also this fails immediately at runtime with a (relatively) clear error.

sid-kap commented 5 years ago

Thanks for your response! It doesn't always fail immediately—I have had situations where the imports are used somewhere other than the top-level, so the failure sometimes happens an hour into a long-running job.

I'd be interested to hear thoughts from other maintainers, but if you're sure this is not worth doing, then feel free to close the issue.

gvanrossum commented 5 years ago

I think it’s worth fixing.

JukkaL commented 5 years ago

I agree that this would be worth fixing.

The implementation would have to be clever enough to catch cast(A, x) if A is not defined at runtime but to allow cast('A', x).

If A is also defined in an else: block, it should not be treated as undefined at runtime:

if TYPE_CHECKING:
    from m import A
else:
    class A: pass

class B(A): pass  # Should be OK

It would also be nice to require quotes to be used in type annotations (if not using from __future__ import annotations).

ilevkivskyi commented 5 years ago

@JukkaL

It would also be nice to require quotes to be used in type annotations:

We already have https://github.com/python/mypy/issues/948 to track this.

NiklasRosenstein commented 2 years ago

I would like to raise this point again.

For a command-line application I am optimising global imports to increase its performance; that often means I need to move imports into a if TYPE_CHECKING block and repeat the import at the place in the code where the type is referenced by value.

Since Mypy doesn't catch that the value is undefined at runtime, if I make the mistake of not adding the local import, it results in a NameError at runtime that should be really easy for Mypy to catch.

With some pointers, I'm happy to look into a contribution for this fix. (I am very unfamiliar with the Mypy codebase).

Viicos commented 1 year ago

Related use case:

module1 has the following:

if TYPE_CHECKING:
    MyType = int

module2:

# No if TYPE_CHECKING block
from module1 import MyType
bluenote10 commented 5 months ago

@JelleZijlstra To quote from the linked duplicate issue https://github.com/python/mypy/issues/16587#issuecomment-1832410680

I don't think we should warn here. if TYPE_CHECKING means that the type checker should treat this condition as true, and therefore assume that FunctionType is imported. It's a structured way of lying to the type checker. And if you lie to the type checker, you're responsible for the consequences.

I would argue that this is a special case, because it is a required technique for breaking import cycles as documented. Without this feature, breaking import cycles cannot be done cleanly, or even the contrary: It is likely to cause bugs!

First a general observation: import cycles are in fact more prominent when using strict typing compared to untyped Python, because even techniques like dependency injection (which typically is intended to break runtime cyclic dependencies) now require imports, but just for the type annotations (i.e., untyped Python would not have these imports). Since import cycles are a more common problem now, it is sensible to provide a clean way to support breaking them.

Now without having a check that a symbol got only imported for type annotation usage, we are actually opening the door for a lot of runtime bugs! In other words, without this feature, the recommendation given in the documentation is actually very likely cause bugs, i.e., "type checks fine, but crashes at runtime" -- which is exactly what we want to prevent with type checking. To demonstrate:

from dataclasses import dataclass, field
from typing import TYPE_CHECKING

# Pure "type import" to break runtime import cycle e.g. for dependency injection.
if TYPE_CHECKING:
    from other_module import Foo

# The following usages are valid, because they only use the symbol in type
# annotation contexts. Note that such usages are common for dependency
# injection.

Alias = list[Foo]

def f1(x: Foo):
    ...

@dataclass
class ClassUsage1:
    foo: Foo

# On the other hand, all the following usages are invalid, because they make
# runtime use of the symbol. Unfortunately the type checker happily accepts
# all these usages in the same module, even though they are all bugs leading
# to crashes at runtime. Ideally the type checker should be aware of the fact
# that the symbol does not exist at runtime, and report all these usages as type
# check errors.

foo = Foo()

def f2(x: Foo = Foo()):
    ...

@dataclass
class ClassUsage2:
    foo: Foo = field(default_factory=lambda: Foo())

We've been running into such bugs rather frequently now.