Do not blindly undefer on leaving fuction

[ilevkivskyi]

Fixes #16496 for real.

The fix is trivial, just save and restore the previous value.

[ilevkivskyi]

Hm, some of the new errors are truly bizarre, I will need to dig deeper in this.

[ilevkivskyi]

OK, so here is the analysis for mypy_primer:

homeassistant: An existing bug, really simple one so going to push a fix to this PR.

porcupine: a "legitimate" error, because we only allow 2 passes in type checker. IMO we should allow at least 3 (for comparison semantic analyzer does 20 passes). @JukkaL what do you think?

spark and spack: existing bug, type narrowing is not propagated to nested function if it is deferred. Test case:

[case testNarrowInFunctionDefer]
from typing import Optional, Callable, TypeVar

def top() -> None:
    x: Optional[int]
    assert x is not None

    def foo() -> None:
        defer()
        reveal_type(x)  # N: Revealed type is "builtins.int"

T = TypeVar("T")
def deco(fn: Callable[[], T]) -> Callable[[], T]: ...

@deco
def defer() -> int: ...

cc @JukkaL who initially implemented this feature. IIUC possible fix is to save/restore full binder stack when checking deferred functions. Alternatively we may consider only deferring top-level functions, instead of the nested ones. IMO performance impact from this will be small.

discord.py: An existing bug, and a problematic one. Right now we have a rule that if a subclass assigns to an attribute on self then we only create a new Var if either there is an explicit annotation, or no superclasses define this attribute. However methods during semantic analysis are processed in a random order, so we don't know if the attribute was defined in superclass. Test case:

[case testForwardBaseDeferAttr]
from typing import Optional, Callable, TypeVar

class C(B):
    def a(self) -> None:
        reveal_type(self._foo)  # Revealed type is "Union[builtins.int, None]"
        self._foo = defer()

class B:
    def __init__(self) -> None:
        self._foo: Optional[int] = None

T = TypeVar("T")
def deco(fn: Callable[[], T]) -> Callable[[], T]: ...

@deco
def defer() -> int: ...

I remember fixing something similar for the daemon was a huge pain (and I am not even sure we fixed it completely, see e.g. saved_class_attrs). It looks like we will need to do something for non-daemon mode as well. One possible way would be order methods by their .info MRO inclusion.

[JukkaL]

Alternatively we may consider only deferring top-level functions, instead of the nested ones. IMO performance impact from this will be small.

If we defer nested functions, that sounds like a problem. I think we should only defer top-level functions.

[JukkaL]

a "legitimate" error, because we only allow 2 passes in type checker. IMO we should allow at least 3 (for comparison semantic analyzer does 20 passes)

Yeah, seems reasonable. More than two passes should only be needed in very rare circumstances.

[ilevkivskyi]

@JukkaL OK, I will create two small PRs for these two, and probably merge them first. IIUC it should be safer that other way around.

[github-actions]

Diff from mypy_primer, showing the effect of this PR on open source code:

django-stubs (https://github.com/typeddjango/django-stubs)

discord.py (https://github.com/Rapptz/discord.py)
+ discord/shard.py:540: error: Cannot determine type of "_closing_task"  [has-type]
+ discord/shard.py:541: error: Cannot determine type of "_closing_task"  [has-type]

[ilevkivskyi]

I decide also to put up #18723 (for discord issue) that I am also going to merge before this one.