Improve consistency of NoReturn

[kreathon]

Motivation

Follow-up pull request for #11996, implementing proposals from @JukkaL.

The mentioned PR used make_simplified_union to enable the following behavior:

from typing import Union, NoReturn
def f() -> Union[int, NoReturn]: ...
reveal_type(f()) # N: Revealed type is "builtins.int"

However, there are two problems with it:

1. This does not solve the problem in general:

from typing import Union, NoReturn
x: Union[str, NoReturn]
x + 's'  # Unsupported left operand type for + ("NoReturn")

2. The outcome of the type checker should not change depending on whether a Union was simplified or not.

Implementation

1. Undo the changes made in Fix handling of NoReturn in Union #11996. This will lead to a regression because of a bug in the format checking, that is "hidden" by the call of make_simplified_union (I am planning to work on this bug afterwards):

spark (https://github.com/apache/spark)
+ python/pyspark/pandas/series.py:4246: error: On Python 3 formatting "b'abc'" with "{}" produces "b'abc'", not "abc"; use "{!r}" if this is desired behavior  [str-bytes-safe]

We could also keep it, to avoid this minor regression (the fix was never released so actually there is no regression).

2. Remove NoReturn types during the construction (after running flatten_nested_unions). For code readability reasons I decided to introduce another method reduce_proper_noreturn_types to perform this job (instead of doing the operation in flatten_nested_unions).

3. In order to fulfill the invariant for non changing semantic for make_simplified_union, the method was changed in order to ignore NoReturn types (except for reducing Union[NoReturn] to NoReturn. In fact, this check is kind of useless, at the moment, because NoReturns were already removed during the Union construction. However, it might help with consistency (for future code changes).

Limitations

The implementation cannot handle type aliases:

Never = NoReturn
v: Union[Never, int]
reveal_type(v) # N: Revealed type is "Union[<nothing>, int]"

The problem is that Unions are created before type aliases are fully expanded.

However, we can get the expected behavior with the following horribly "hacky" code. Are there any idea how to solve this elegantly?

def is_noreturn_type(t: Type) -> bool:
    try:
        t = get_proper_type(t)
    except:
        pass
    return isinstance(t, UninhabitedType) and t.is_noreturn

Test Plan

1. Extend [case testUnionWithNoReturn] to include more checks
2. Add NoReturn checks into test_simplified_union.

mypy_primer

Reverse of #11996.

[github-actions]

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

steam.py (https://github.com/Gobot1234/steam.py)
- steam/state.py:842: error: Incompatible types in assignment (expression has type "User", variable has type "CMsgClientFriendsListFriend")  [assignment]
+ steam/state.py:842: error: Incompatible types in assignment (expression has type "Optional[User]", variable has type "CMsgClientFriendsListFriend")  [assignment]

spark (https://github.com/apache/spark)
- python/pyspark/pandas/series.py:4247: error: On Python 3 formatting "b'abc'" with "{}" produces "b'abc'", not "abc"; use "{!r}" if this is desired behavior  [str-bytes-safe]

pip (https://github.com/pypa/pip)
- src/pip/_internal/cli/progress_bars.py:99: error: Incompatible types in assignment (expression has type "Callable[[int, FrameType], None]", variable has type "Union[Callable[[int, Optional[FrameType]], Any], int, None]")
+ src/pip/_internal/cli/progress_bars.py:99: error: Incompatible types in assignment (expression has type "Callable[[int, FrameType], None]", variable has type "Union[Callable[[int, Optional[FrameType]], Any], int, Handlers, None]")

[sobolevn]

Thanks! Please, see my comments 🙂

[sobolevn]

Or we can have an empty types[] iterable.

[kreathon]

I assumed this cannot happen, because in the call context of this method (constructor of Union) there must be at least one item in the Union.

So are you fine if I just add an assert len(types) >= 1 and chaning the type of types to List (instead of Iterable)

[sobolevn]

It can be Sequence, the same as __init__ has

[sobolevn]

t is always ProperType if it is UninhabitedType.

I guess what we should do here is:

t = get_proper_type(t)
return isinstance(t, UninhabitedType) and t.is_noreturn

[kreathon]

This is related to the problem of the "Limitation" section.

t = get_proper_type(t)
return isinstance(t, UninhabitedType) and t.is_noreturn

here get_proper_type(t) fails in _expand_once() on the following assertion assert self.alias is not None

Therefore, I limited the implementation to not handle type aliases and added the isinstance(t, ProperType) check instead.

[sobolevn]

This should not happen, Union of a single item should be simplified to just this item.

[kreathon]

The longer I think about it, the less useful I see these changes. The main problem I have right now is:

make_simplified_union should not change the outcome of the type checker (thus Union[NoReturn, int] should not be modified). However, make_simplified_union create a new Union, thus (if NoReturn is removed during its construction) in fact it changes Union[NoReturn, int] to int. Hence changing the outcome.

I will close this. In case I have a new idea, I would open a new PR. Sorry, for wasting time and thank you for your feedback :)