Mypy: "Missing return statement" caused by context manager?

[jonapich]

""" Unit tests. """

from types import TracebackType
from typing import Optional, Type


class TestManager:
    def __enter__(self) -> None:
        ...

    def __exit__(self, exc_type: Optional[Type[BaseException]], 
                 exc_val: Optional[BaseException],
                 exc_tb: Optional[TracebackType]) -> bool:
        ...


def func() -> int:
    with TestManager():
        return 1

Mypy will complain about func(), saying it doesn't have a return statement.

tests/__init__.py:16:1: error: Missing return statement

Is this intended?

_Note: I also tried class TestManager(ContextManager[None]): just to see if that would help mypy but it didn't change anything._

[ilevkivskyi]

If the TestManager can "swallow" exceptions, then this is a valid error. Otherwise you should annotate the __exit__() as returning Literal[False] (see 0.730 release notes).

[gvanrossum]

I wonder if we should just roll this back. This was reported several times
since the 0.730 release and the answer is not easy to find in the docs.
There are a lot of context managers in the world, and I would guess that
few of them ever swallow exceptions.

Worse, there may be situations where a subclass could override __exit__
to swallow some exceptions for a base class that doesn't -- if the base
class' __exit__ has type Literal[False] then wouldn't the subclass be
in error if it had __exit__ returning type bool?

Or maybe the error message could have a note pointing to this solution, if
there's a context manager involved?

[ilevkivskyi]

I wonder if we should just roll this back.

It is not as easy as that, since reverting will cause false positives (like falsely complaining about unreachable code).

Or maybe the error message could have a note pointing to this solution, if there's a context manager involved?

Unfortunately this may be not easy due to how we track the control flow (binder). We can move the docs to a more prominent location.

[JukkaL]

I agree that we should do something about this. I think that reverting is fine, at least temporarily, since I generally prefer preserving existing "questionable" behavior to introducing new, different "questionable" behavior (to avoid friction) -- unless we can expect that the new behavior occurs much less frequently. It would be better to fix this or to improve the generated error message, however.

Note that a None return also works, and this may be preferable to using Literal[False]. This requires a semantic change to code, though.

Here are some ideas about how we could make this better without reverting:

1. If an__exit__ method is annotated to return bool, but all the return statement are return False, generate an error and require annotating the return type as Literal[False] (or None).
2. If we get a missing return statement error, look for all 'with' statements in the function, and if one of these uses a context manager with a bool return in __exit__, explain how this might be causing the issue, and suggest changing the return type of __exit__ to Literal[False] or None.
3. If an __exit__ method is annotated to return bool, but it always returns False, flag this and somehow treat the return type implicitly as Literal[False] when type checking 'with' statements.
4. Document this in the mypy docs. This is probably necessary but likely not sufficient, since the current behavior is pretty unintuitive.

Only 2 and 4 help with stubs. My current preference would be to implement 1, 4 and 2 (in this order). 3 seems too magical and ad hoc.