python / mypy

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

mypy reports `used-before-def` with `TypeAlias`es despite `from __future__ import annotations` #14539

Open bersbersbers opened 1 year ago

bersbersbers commented 1 year ago

Bug Report

from __future__ import annotations helps avoid NameErrors with later-defined entities in python, but seems to ineffective with use(d)-before-def in mypy.

To Reproduce (edited example code, see edit history for previous version if I mis-edited anything):

"""Bug."""
from __future__ import annotations
from typing import TYPE_CHECKING

# This works (only) with `from __future__ import annotations`:
list_direct: list[Class] = []

# By contrast, mypy reports `used-before-def` either way:
if TYPE_CHECKING:
    ListOfClass = list[Class]
list_via_alias: ListOfClass = []

class Class:
    pass

Expected Behavior

No error

Actual Behavior

bug.py:8: error: Name "MyClass" is used before definition [used-before-def]

Your Environment

Related: https://github.com/python/mypy/pull/14163, https://github.com/python/mypy/pull/14166

Interestingly, no error on playground on master: https://mypy-play.net/?mypy=master&python=3.11&gist=f323516535c0d04cf4e0a2f2c985f4ff

erictraut commented 1 year ago

I think mypy's behavior is correct here. The from __future__ import annotations affects deferred evaluation of type annotations only. It does not affect evaluation of other expressions such as the RHS of the assignment statement you are using to define FunctionOfMyClass.

AlexWaygood commented 1 year ago

I think mypy's behavior is correct here. The from __future__ import annotations affects deferred evaluation of type annotations only. It does not affect evaluation of other expressions such as the RHS of the assignment statement you are using to define FunctionOfMyClass.

I think that's arguable — in an ideal world, perhaps mypy would never have allowed forward references in runtime contexts in .py files. Given that it has allowed constructs like this for a long time, however, there may be many users who have forward references like this in if TYPE_CHECKING blocks, implicitly depending on mypy's previous behaviour. Disallowing constructs like this now is potentially a serious backwards-compatibility issue, and (since if TYPE_CHECKING blocks are never executed at runtime), has dubious benefits in this case.

bersbersbers commented 1 year ago

In my (outsider's) opinion, @erictraut is not wrong, technically. But I'd raise the question what mypy should do here, not what it currently does.

Ultimately, both my uses cases above are for type annotations only, even though one of the two cases passes via definition of a helper TypeAlias. (Edit: I edited the original code to highlight this only difference.) Semantically, I feel these two use cases should work the same.

bersbersbers commented 1 year ago

Another point to take into account: pylama/pyflakes/flake8 as well as Pylance report this (as E0602, F821, undefinedVariable, respectively), so it is somewhat less likely that mypys behavior will be hindering many projects. I guess ListOfClass = list["Class"] may be the way to go for now whenever any of the aforementioned linters are involved.

tabatkins commented 1 year ago

Just updated mypy from 0.960 to 1.3.0 and hit this, and wow this is not correct behavior.

Mypy handles unresolved references just fine; it's the Python runtime that chokes on them. If the reference is in a TYPE_CHECKING conditional, thus specifically hidden from the Python runtime and only meant for mypy, there is thus absolutely no problem with referencing something before it's defined. Mypy knows what you're doing, and Python has no idea it exists; this is manifestly not an error and flagging it as such (and requiring the annoying "wrap it in a string" forward-reference pattern) is user-hostile. And mypy knows about TYPE_CHECKING conditionals, so it can absolutely condition its checks on this.

(Unresolved references outside of a TYPE_CHECKING conditional are clearly an error and mypy would be correct to flag them, as they would cause Python to choke as well.)

In the meantime I'm just going to disable the used-before-def error entirely, which is unfortunate, because this is a useful error to catch. Luckily pylint is also catching this particular error (and I've already had to mark the offending type alias lines to not trigger a pylint error) so I'm not losing any actual coverage.

erictraut commented 1 year ago

The TYPE_CHECKING conditional is meant as a way to "lie" to the type checker about what is seen at runtime. Because of this, it should be used with great care — and avoided if possible.

Mypy is behaving correctly in this case. This is not a bug. Recommend closing.

bersbersbers commented 1 year ago

TYPE_CHECKING [...] should be [...] avoided if possible.

Wow, that is news to me. I thought it would make sense to put as much as possible into type checking blocks, in line with the following linter rules that I use:

TC001 | Move application import into a type-checking block TC002 | Move third-party import into a type-checking block TC003 | Move built-in import into a type-checking block

https://github.com/snok/flake8-type-checking#error-codes

erictraut commented 1 year ago

Yikes, those look to me like very bad linting rules. Every time you use TYPE_CHECKING in your code, you're "blinding" the type checker to potential runtime errors because you're telling it to analyze code that is different from what is executed at runtime. In certain rare cases, the use of TYPE_CHECKING is unavoidable, but I recommend avoiding it wherever possible.

AlexWaygood commented 1 year ago

There are a few reasons why you might want to move as much code as possible inside an if TYPE_CHECKING block:

  1. To minimise the risk of import cycles
  2. To reduce the performance cost associated with importing expensive modules that are only needed for type annotations
  3. To ensure that as little time as possible is spent evaluating annotations at runtime

I agree that it leads to worse type-checking, and I also personally try to use if TYPE_CHECKING blocks as little as possible. However, I can see the motivation behind these rules -- some libraries have very high import costs associated with them; in some code bases it's very difficult to avoid import cycles without guarding imports behind if TYPE_CHECKING blocks; etc.

I think we can be pragmatic and recognise that different people are making different trade-offs in the way they choose to write their code.

tabatkins commented 1 year ago

The TYPE_CHECKING conditional is meant as a way to "lie" to the type checker about what is seen at runtime. Because of this, it should be used with great care — and avoided if possible.

This is false, weakly in theory but strongly in practice. The Python runtime has a number of behaviors that are actively hostile to type-checking, and which aren't issues in languages that have robust type systems built in - the most important one is circular imports which is impossible to resolve without it, but @AlexWaygood lists several additional important ones. Without TYPE_CHECKING blocks I'd have abandoned mypy fairly quickly, because at best it would have required drastically restructuring my code base for circular-import reasons, and likely would have simply been fundamentally incompatible.

Keeping their use to a minimum is definitely correct practice (using them solely for the purpose of establishing names that the type system needs but the runtime doesn't), but they're not an escape hatch, they're a required feature of the type system due to the fundamental disconnect between Python's module system and a good type system's requirements for a module system.


It wouldn't be unreasonable to, say, flag uses of a TypeAlias variable in runtime code as errors. If said variable is defined in a TYPE_CHECKING block it would actively be helpful, since that is guaranteed to be a ReferenceError at runtime. But saying that one use of a forward-referenced name in code not executed by the runtime (in an annotation) is fine, while another use in code not executed by the runtime (in TYPE_CHECKING) isn't, is just wrong. It's just a beta reduction.