Mark code after for loops that never end as unreachable

[A5rocks]

Fixes #18891
Fixes #7374

[sterliakov]

Looks good! However, I'm afraid that NoReturn in third position is not actually enforced by any means for user-defined generator subclasses.

from typing import *

class G(Generator[int, None, Never]):
    def __iter__(self) -> 'G':
        return self
    
    def __next__(self) -> int:
        raise StopIteration
    
    def send(self, value: None, /) -> int:
        assert False, "unused"
        
    def throw(
        self, typ: object, val: object = None, tb: object | None = None, /
    ) -> int:
        assert False, "unused"
    

def check() -> str:
    for _ in G():
        return ""
    print("Reached")

check()

This (untested, sorry for any typos) should print "Reached", and this PR should mark that line unreachable. It's a separate problem, of course, but I'm not sure this can be merged until we can somehow enforce Never vs None distinction there.

[A5rocks]

I'm not sure it's possible to enforce any distinction there. We could enforce that raise StopIteration only happens in __iter__ but that seems overly limiting. Otherwise we would need some sort of checked exceptions... Any ideas? I guess we could do some best effort thing (check StopIteration in __iter__, throw up our arms and give up. Or we could require GeneratorType? That's final so there's no possible subtype with weird behavior... We could even enforce some back-compat by mutating generator function return types to replace Generator with GeneratorType? Last part seems a bit much.)

[sterliakov]

I'll think about this tomorrow. __next__ is the right place for raise StopIteration, restricting it to __iter__ would be weird.

The problem stems from the fact that _ReturnT is only referenced in close method that is not abstract and __iter__ method that can return self, so the default implementation cannot be safe for arbitrary type _ReturnT. I can suggest to go as far as prohibiting non-None third parameter when subclassing Generator without overriding close, that would be sufficient to enforce consistency. My example above could have been class G(Generator[int, None, str]) just as well, and type checkers won't mark that as error.

[A5rocks]

Sorry yes that's what I meant re: __next__. OK that actually makes sense -- I agree. I guess close can't be abstract because it isn't at runtime (only send and throw are).

I'm not convinced that subclassing a subclass will work correctly for close detection... Would be nice if there were a conditional way to define abstract methods!

[sterliakov]

Hm, okay, I revisited this once again, maybe it's not really a blocker and we should file a separate feature request to check third type argument of Generator more strictly.

I don't think we need to support initial definition in subclasses. This should be an error even if some subclasses define close with correct signature, as G itself violates its contract:

class G(Generator[int, None, str]):
    def __iter__(self) -> 'G':
        return self
    
    def __next__(self) -> int:
        raise StopIteration
    
    def send(self, value: None, /) -> int:
        assert False, "unused"
        
    def throw(
        self, typ: object, val: object = None, tb: object | None = None, /
    ) -> int:
        assert False, "unused"

On the other hand, if the direct subclass of Generator was confirmed to be correct, we'll get checking close in future subclasses for free - that's just plain override compatibility. If I'm not missing anything, we only need one special case - immediate subclasses of Generator without close definition can only inherit from Generator[..., ..., None]). For the third argument to be non-None, explicit close must be provided (only provided, the rest should be checked by existing infrastructure).

I only envision this as a sanity check specifically for Generator. Since it can't be safely implemented (close must return some unknown _ReturnT, not its default, so must be abstract), no general support is necessary - only a patch to handle this specific typeshed definition.

[A5rocks]

@sterliakov sorry I haven't actually thought about this more until now. I think pushing off doing this properly is fine. I think doing this properly could be what you talk about, or it could be something like:

• update close to have a self-type of Generator[Any, Any, None] in typeshed?
• fix self annotation not checked in __index__? #19341

EDIT: nevermind, I thought a bit more and the second approach wouldn't work (because we need to say there's a close that returns the last type always, it's just not defined by default for anything other than None...). Unless we allow overloading with abstractmethod, but that's sounds complex.

[sterliakov]

Fixes #7374?

[A5rocks]

Good catch, I'll separate out the part of that issue that this doesn't fix into another issue.

[github-actions]

According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅