From 08340c29a939a203868d3abd5cfd997720228f32 Mon Sep 17 00:00:00 2001 From: Stanislav Terliakov <50529348+sterliakov@users.noreply.github.com> Date: Thu, 21 Nov 2024 15:46:50 +0100 Subject: [PATCH] Do not consider bare TypeVar not overlapping with None for reachability analysis (#18138) Fixes #18126. Simply allowing such intersection was insufficient: existing binder logic widened the type to `T | None` after the `is None` check. This PR extends the binder logic to prevent constructing a union type when all conditional branches are reachable and contain no assignments: checking `if isinstance(something, Something)` does not change the type of `something` after the end of the `if` block. --- mypy/binder.py | 55 +++++++++++++++++------- mypy/checker.py | 27 ++++++------ test-data/unit/check-enum.test | 4 +- test-data/unit/check-isinstance.test | 17 ++++---- test-data/unit/check-narrowing.test | 19 ++++++++ test-data/unit/check-python310.test | 30 +++++++++++++ test-data/unit/check-type-promotion.test | 6 +-- 7 files changed, 116 insertions(+), 42 deletions(-) diff --git a/mypy/binder.py b/mypy/binder.py index 9d0a33b54bc2..52ae9774e6d4 100644 --- a/mypy/binder.py +++ b/mypy/binder.py @@ -2,7 +2,7 @@ from collections import defaultdict from contextlib import contextmanager -from typing import DefaultDict, Iterator, List, Optional, Tuple, Union, cast +from typing import DefaultDict, Iterator, List, NamedTuple, Optional, Tuple, Union from typing_extensions import TypeAlias as _TypeAlias from mypy.erasetype import remove_instance_last_known_values @@ -30,6 +30,11 @@ BindableExpression: _TypeAlias = Union[IndexExpr, MemberExpr, NameExpr] +class CurrentType(NamedTuple): + type: Type + from_assignment: bool + + class Frame: """A Frame represents a specific point in the execution of a program. It carries information about the current types of expressions at @@ -44,7 +49,7 @@ class Frame: def __init__(self, id: int, conditional_frame: bool = False) -> None: self.id = id - self.types: dict[Key, Type] = {} + self.types: dict[Key, CurrentType] = {} self.unreachable = False self.conditional_frame = conditional_frame self.suppress_unreachable_warnings = False @@ -132,10 +137,10 @@ def push_frame(self, conditional_frame: bool = False) -> Frame: self.options_on_return.append([]) return f - def _put(self, key: Key, type: Type, index: int = -1) -> None: - self.frames[index].types[key] = type + def _put(self, key: Key, type: Type, from_assignment: bool, index: int = -1) -> None: + self.frames[index].types[key] = CurrentType(type, from_assignment) - def _get(self, key: Key, index: int = -1) -> Type | None: + def _get(self, key: Key, index: int = -1) -> CurrentType | None: if index < 0: index += len(self.frames) for i in range(index, -1, -1): @@ -143,7 +148,7 @@ def _get(self, key: Key, index: int = -1) -> Type | None: return self.frames[i].types[key] return None - def put(self, expr: Expression, typ: Type) -> None: + def put(self, expr: Expression, typ: Type, *, from_assignment: bool = True) -> None: if not isinstance(expr, (IndexExpr, MemberExpr, NameExpr)): return if not literal(expr): @@ -153,7 +158,7 @@ def put(self, expr: Expression, typ: Type) -> None: if key not in self.declarations: self.declarations[key] = get_declaration(expr) self._add_dependencies(key) - self._put(key, typ) + self._put(key, typ, from_assignment) def unreachable(self) -> None: self.frames[-1].unreachable = True @@ -164,7 +169,10 @@ def suppress_unreachable_warnings(self) -> None: def get(self, expr: Expression) -> Type | None: key = literal_hash(expr) assert key is not None, "Internal error: binder tried to get non-literal" - return self._get(key) + found = self._get(key) + if found is None: + return None + return found.type def is_unreachable(self) -> bool: # TODO: Copy the value of unreachable into new frames to avoid @@ -193,7 +201,7 @@ def update_from_options(self, frames: list[Frame]) -> bool: If a key is declared as AnyType, only update it if all the options are the same. """ - + all_reachable = all(not f.unreachable for f in frames) frames = [f for f in frames if not f.unreachable] changed = False keys = {key for f in frames for key in f.types} @@ -207,17 +215,30 @@ def update_from_options(self, frames: list[Frame]) -> bool: # know anything about key in at least one possible frame. continue - type = resulting_values[0] - assert type is not None + if all_reachable and all( + x is not None and not x.from_assignment for x in resulting_values + ): + # Do not synthesize a new type if we encountered a conditional block + # (if, while or match-case) without assignments. + # See check-isinstance.test::testNoneCheckDoesNotMakeTypeVarOptional + # This is a safe assumption: the fact that we checked something with `is` + # or `isinstance` does not change the type of the value. + continue + + current_type = resulting_values[0] + assert current_type is not None + type = current_type.type declaration_type = get_proper_type(self.declarations.get(key)) if isinstance(declaration_type, AnyType): # At this point resulting values can't contain None, see continue above - if not all(is_same_type(type, cast(Type, t)) for t in resulting_values[1:]): + if not all( + t is not None and is_same_type(type, t.type) for t in resulting_values[1:] + ): type = AnyType(TypeOfAny.from_another_any, source_any=declaration_type) else: for other in resulting_values[1:]: assert other is not None - type = join_simple(self.declarations[key], type, other) + type = join_simple(self.declarations[key], type, other.type) # Try simplifying resulting type for unions involving variadic tuples. # Technically, everything is still valid without this step, but if we do # not do this, this may create long unions after exiting an if check like: @@ -236,8 +257,8 @@ def update_from_options(self, frames: list[Frame]) -> bool: ) if simplified == self.declarations[key]: type = simplified - if current_value is None or not is_same_type(type, current_value): - self._put(key, type) + if current_value is None or not is_same_type(type, current_value[0]): + self._put(key, type, from_assignment=True) changed = True self.frames[-1].unreachable = not frames @@ -374,7 +395,9 @@ def most_recent_enclosing_type(self, expr: BindableExpression, type: Type) -> Ty key = literal_hash(expr) assert key is not None enclosers = [get_declaration(expr)] + [ - f.types[key] for f in self.frames if key in f.types and is_subtype(type, f.types[key]) + f.types[key].type + for f in self.frames + if key in f.types and is_subtype(type, f.types[key][0]) ] return enclosers[-1] diff --git a/mypy/checker.py b/mypy/checker.py index ef3f7502d7ce..1b11caf01f18 100644 --- a/mypy/checker.py +++ b/mypy/checker.py @@ -4725,11 +4725,11 @@ def visit_if_stmt(self, s: IfStmt) -> None: # XXX Issue a warning if condition is always False? with self.binder.frame_context(can_skip=True, fall_through=2): - self.push_type_map(if_map) + self.push_type_map(if_map, from_assignment=False) self.accept(b) # XXX Issue a warning if condition is always True? - self.push_type_map(else_map) + self.push_type_map(else_map, from_assignment=False) with self.binder.frame_context(can_skip=False, fall_through=2): if s.else_body: @@ -5310,18 +5310,21 @@ def visit_match_stmt(self, s: MatchStmt) -> None: if b.is_unreachable or isinstance( get_proper_type(pattern_type.type), UninhabitedType ): - self.push_type_map(None) + self.push_type_map(None, from_assignment=False) else_map: TypeMap = {} else: pattern_map, else_map = conditional_types_to_typemaps( named_subject, pattern_type.type, pattern_type.rest_type ) self.remove_capture_conflicts(pattern_type.captures, inferred_types) - self.push_type_map(pattern_map) + self.push_type_map(pattern_map, from_assignment=False) if pattern_map: for expr, typ in pattern_map.items(): - self.push_type_map(self._get_recursive_sub_patterns_map(expr, typ)) - self.push_type_map(pattern_type.captures) + self.push_type_map( + self._get_recursive_sub_patterns_map(expr, typ), + from_assignment=False, + ) + self.push_type_map(pattern_type.captures, from_assignment=False) if g is not None: with self.binder.frame_context(can_skip=False, fall_through=3): gt = get_proper_type(self.expr_checker.accept(g)) @@ -5347,11 +5350,11 @@ def visit_match_stmt(self, s: MatchStmt) -> None: continue type_map[named_subject] = type_map[expr] - self.push_type_map(guard_map) + self.push_type_map(guard_map, from_assignment=False) self.accept(b) else: self.accept(b) - self.push_type_map(else_map) + self.push_type_map(else_map, from_assignment=False) # This is needed due to a quirk in frame_context. Without it types will stay narrowed # after the match. @@ -7372,12 +7375,12 @@ def iterable_item_type( def function_type(self, func: FuncBase) -> FunctionLike: return function_type(func, self.named_type("builtins.function")) - def push_type_map(self, type_map: TypeMap) -> None: + def push_type_map(self, type_map: TypeMap, *, from_assignment: bool = True) -> None: if type_map is None: self.binder.unreachable() else: for expr, type in type_map.items(): - self.binder.put(expr, type) + self.binder.put(expr, type, from_assignment=from_assignment) def infer_issubclass_maps(self, node: CallExpr, expr: Expression) -> tuple[TypeMap, TypeMap]: """Infer type restrictions for an expression in issubclass call.""" @@ -7750,9 +7753,7 @@ def conditional_types( ) and is_proper_subtype(current_type, proposed_type, ignore_promotions=True): # Expression is always of one of the types in proposed_type_ranges return default, UninhabitedType() - elif not is_overlapping_types( - current_type, proposed_type, prohibit_none_typevar_overlap=True, ignore_promotions=True - ): + elif not is_overlapping_types(current_type, proposed_type, ignore_promotions=True): # Expression is never of any type in proposed_type_ranges return UninhabitedType(), default else: diff --git a/test-data/unit/check-enum.test b/test-data/unit/check-enum.test index 422beb5c82f0..e6e42d805052 100644 --- a/test-data/unit/check-enum.test +++ b/test-data/unit/check-enum.test @@ -815,7 +815,7 @@ elif x is Foo.C: reveal_type(x) # N: Revealed type is "Literal[__main__.Foo.C]" else: reveal_type(x) # No output here: this branch is unreachable -reveal_type(x) # N: Revealed type is "__main__.Foo" +reveal_type(x) # N: Revealed type is "Union[Literal[__main__.Foo.A], Literal[__main__.Foo.B], Literal[__main__.Foo.C]]" if Foo.A is x: reveal_type(x) # N: Revealed type is "Literal[__main__.Foo.A]" @@ -825,7 +825,7 @@ elif Foo.C is x: reveal_type(x) # N: Revealed type is "Literal[__main__.Foo.C]" else: reveal_type(x) # No output here: this branch is unreachable -reveal_type(x) # N: Revealed type is "__main__.Foo" +reveal_type(x) # N: Revealed type is "Union[Literal[__main__.Foo.A], Literal[__main__.Foo.B], Literal[__main__.Foo.C]]" y: Foo if y is Foo.A: diff --git a/test-data/unit/check-isinstance.test b/test-data/unit/check-isinstance.test index 8fa1bc1ca1ac..99bd62765b11 100644 --- a/test-data/unit/check-isinstance.test +++ b/test-data/unit/check-isinstance.test @@ -2207,23 +2207,24 @@ def foo2(x: Optional[str]) -> None: reveal_type(x) # N: Revealed type is "builtins.str" [builtins fixtures/isinstance.pyi] -[case testNoneCheckDoesNotNarrowWhenUsingTypeVars] - -# Note: this test (and the following one) are testing checker.conditional_type_map: -# if you set the 'prohibit_none_typevar_overlap' keyword argument to False when calling -# 'is_overlapping_types', the binder will incorrectly infer that 'out' has a type of -# Union[T, None] after the if statement. - +[case testNoneCheckDoesNotMakeTypeVarOptional] from typing import TypeVar T = TypeVar('T') -def foo(x: T) -> T: +def foo_if(x: T) -> T: out = None out = x if out is None: pass return out + +def foo_while(x: T) -> T: + out = None + out = x + while out is None: + pass + return out [builtins fixtures/isinstance.pyi] [case testNoneCheckDoesNotNarrowWhenUsingTypeVarsNoStrictOptional] diff --git a/test-data/unit/check-narrowing.test b/test-data/unit/check-narrowing.test index bc763095477e..285d56ff7e50 100644 --- a/test-data/unit/check-narrowing.test +++ b/test-data/unit/check-narrowing.test @@ -2333,3 +2333,22 @@ def f(x: C) -> None: f(C(5)) [builtins fixtures/primitives.pyi] + +[case testNarrowingTypeVarNone] +# flags: --warn-unreachable + +# https://github.com/python/mypy/issues/18126 +from typing import TypeVar + +T = TypeVar("T") + +def fn_if(arg: T) -> None: + if arg is None: + return None + return None + +def fn_while(arg: T) -> None: + while arg is None: + return None + return None +[builtins fixtures/primitives.pyi] diff --git a/test-data/unit/check-python310.test b/test-data/unit/check-python310.test index 435fbde3e2ae..0231b47cf4a0 100644 --- a/test-data/unit/check-python310.test +++ b/test-data/unit/check-python310.test @@ -2409,3 +2409,33 @@ def f(x: T) -> None: case _: accept_seq_int(x) # E: Argument 1 to "accept_seq_int" has incompatible type "T"; expected "Sequence[int]" [builtins fixtures/tuple.pyi] + +[case testNarrowingTypeVarMatch] +# flags: --warn-unreachable + +# https://github.com/python/mypy/issues/18126 +from typing import TypeVar + +T = TypeVar("T") + +def fn_case(arg: T) -> None: + match arg: + case None: + return None + return None +[builtins fixtures/primitives.pyi] + +[case testNoneCheckDoesNotMakeTypeVarOptionalMatch] +from typing import TypeVar + +T = TypeVar('T') + +def foo(x: T) -> T: + out = None + out = x + match out: + case None: + pass + return out + +[builtins fixtures/isinstance.pyi] diff --git a/test-data/unit/check-type-promotion.test b/test-data/unit/check-type-promotion.test index e66153726e7d..d98d0c60e164 100644 --- a/test-data/unit/check-type-promotion.test +++ b/test-data/unit/check-type-promotion.test @@ -91,7 +91,7 @@ else: reveal_type(x) # N: Revealed type is "builtins.complex" # Note we make type precise, since type promotions are involved -reveal_type(x) # N: Revealed type is "Union[builtins.complex, builtins.int, builtins.float]" +reveal_type(x) # N: Revealed type is "builtins.complex" [builtins fixtures/primitives.pyi] [case testIntersectionUsingPromotion3] @@ -127,7 +127,7 @@ if isinstance(x, int): reveal_type(x) # N: Revealed type is "builtins.int" else: reveal_type(x) # N: Revealed type is "Union[builtins.float, builtins.complex]" -reveal_type(x) # N: Revealed type is "Union[builtins.int, builtins.float, builtins.complex]" +reveal_type(x) # N: Revealed type is "Union[builtins.float, builtins.complex]" [builtins fixtures/primitives.pyi] [case testIntersectionUsingPromotion6] @@ -139,7 +139,7 @@ if isinstance(x, int): reveal_type(x) # N: Revealed type is "builtins.int" else: reveal_type(x) # N: Revealed type is "Union[builtins.str, builtins.complex]" -reveal_type(x) # N: Revealed type is "Union[builtins.str, builtins.int, builtins.complex]" +reveal_type(x) # N: Revealed type is "Union[builtins.str, builtins.complex]" [builtins fixtures/primitives.pyi] [case testIntersectionUsingPromotion7]