From a29069e1c487468d7b50a538890e11a6aeb40893 Mon Sep 17 00:00:00 2001 From: Max Moroz Date: Tue, 14 Mar 2017 04:25:51 -0700 Subject: [PATCH 1/8] Make no inference when variables present in second argument to isinstance --- mypy/checker.py | 11 ++++++++--- mypy/semanal.py | 7 ++++--- 2 files changed, 12 insertions(+), 6 deletions(-) diff --git a/mypy/checker.py b/mypy/checker.py index 09c58c2b2dc8..421bcbac0cb5 100644 --- a/mypy/checker.py +++ b/mypy/checker.py @@ -2739,9 +2739,14 @@ def get_isinstance_type(expr: Expression, type_map: Dict[Expression, Type]) -> T types.append(type) - if len(types) == 0: - return None - elif len(types) == 1: + elif isinstance(type, TypeType): + types.append(type.item) + + else: # we didn't see an actual type, but rather a variable whose value is unknown to us + return None + + assert len(types) != 0 + if len(types) == 1: return types[0] else: return UnionType(types) diff --git a/mypy/semanal.py b/mypy/semanal.py index 664d74800c07..34fa34163dbc 100644 --- a/mypy/semanal.py +++ b/mypy/semanal.py @@ -2681,7 +2681,7 @@ def visit_member_expr(self, expr: MemberExpr) -> None: # bar in its namespace. This must be done for all types of bar. file = base.node assert isinstance(file, (MypyFile, type(None))) - n = file.names.get(expr.name, None) if file is not None else None + n = file.names.get(expr.name, None) if file is not None else None # type: ignore if n: n = self.normalize_type_alias(n, expr) if not n: @@ -2697,8 +2697,9 @@ def visit_member_expr(self, expr: MemberExpr) -> None: # one type checker run. If we reported errors here, # the build would terminate after semantic analysis # and we wouldn't be able to report any type errors. - full_name = '%s.%s' % (file.fullname() if file is not None else None, expr.name) - mod_name = " '%s'" % file.fullname() if file is not None else '' + full_name = '%s.%s' % (file.fullname() if file is not None # type: ignore + else None, expr.name) + mod_name = " '%s'" % file.fullname() if file is not None else '' # type: ignore if full_name in obsolete_name_mapping: self.fail("Module%s has no attribute %r (it's now called %r)" % ( mod_name, expr.name, obsolete_name_mapping[full_name]), expr) From 8d7811ac0bc414b028c3a751c69bb0813923c39f Mon Sep 17 00:00:00 2001 From: Max Moroz Date: Tue, 14 Mar 2017 13:24:14 -0700 Subject: [PATCH 2/8] Fix formatting --- mypy/checker.py | 3 --- 1 file changed, 3 deletions(-) diff --git a/mypy/checker.py b/mypy/checker.py index 421bcbac0cb5..20709422ea35 100644 --- a/mypy/checker.py +++ b/mypy/checker.py @@ -2736,12 +2736,9 @@ def get_isinstance_type(expr: Expression, type_map: Dict[Expression, Type]) -> T # Type variables may be present -- erase them, which is the best # we can do (outside disallowing them here). type = erase_typevars(type.items()[0].ret_type) - types.append(type) - elif isinstance(type, TypeType): types.append(type.item) - else: # we didn't see an actual type, but rather a variable whose value is unknown to us return None From 14efa1639b50bce05052b36c5abdd3e6a8189aad Mon Sep 17 00:00:00 2001 From: Max Moroz Date: Tue, 14 Mar 2017 13:24:38 -0700 Subject: [PATCH 3/8] Add unit test --- test-data/unit/check-isinstance.test | 8 ++++++++ typeshed | 2 +- 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/test-data/unit/check-isinstance.test b/test-data/unit/check-isinstance.test index 43dbfd8321d1..3f9b8d1040c7 100644 --- a/test-data/unit/check-isinstance.test +++ b/test-data/unit/check-isinstance.test @@ -1360,3 +1360,11 @@ def f(x: object) -> None: reveal_type(b) # E: Revealed type is '__main__.A' [builtins fixtures/isinstance.pyi] [out] + +[case testIsInstanceWithTypeVariable] +from typing import * +def f(x: Union[int, str], typ: type) -> None: + if isinstance(x, (typ, int)): + x + 1 # E: Unsupported operand types for + (likely involving Union) +[builtins fixtures/isinstancelist.pyi] + diff --git a/typeshed b/typeshed index db0c106d2fe7..bd5b33f3b18f 160000 --- a/typeshed +++ b/typeshed @@ -1 +1 @@ -Subproject commit db0c106d2fe76204f6506f459edeff9cfde4d607 +Subproject commit bd5b33f3b18fc8811ad2403d35f21ad6aae94b62 From d9f261f3726a7a10f040e2ffd314ce5ef0e9ab5a Mon Sep 17 00:00:00 2001 From: Max Moroz Date: Sat, 18 Mar 2017 12:26:39 -0700 Subject: [PATCH 4/8] Replace isinstance with cast to deal with #2999; rename test --- mypy/semanal.py | 9 ++++----- test-data/unit/check-isinstance.test | 2 +- 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/mypy/semanal.py b/mypy/semanal.py index 34fa34163dbc..06fb69a05497 100644 --- a/mypy/semanal.py +++ b/mypy/semanal.py @@ -2679,9 +2679,8 @@ def visit_member_expr(self, expr: MemberExpr) -> None: # This branch handles the case foo.bar where foo is a module. # In this case base.node is the module's MypyFile and we look up # bar in its namespace. This must be done for all types of bar. - file = base.node - assert isinstance(file, (MypyFile, type(None))) - n = file.names.get(expr.name, None) if file is not None else None # type: ignore + file = cast(Optional[MypyFile], base.node) + n = file.names.get(expr.name, None) if file is not None else None if n: n = self.normalize_type_alias(n, expr) if not n: @@ -2697,9 +2696,9 @@ def visit_member_expr(self, expr: MemberExpr) -> None: # one type checker run. If we reported errors here, # the build would terminate after semantic analysis # and we wouldn't be able to report any type errors. - full_name = '%s.%s' % (file.fullname() if file is not None # type: ignore + full_name = '%s.%s' % (file.fullname() if file is not None else None, expr.name) - mod_name = " '%s'" % file.fullname() if file is not None else '' # type: ignore + mod_name = " '%s'" % file.fullname() if file is not None else '' if full_name in obsolete_name_mapping: self.fail("Module%s has no attribute %r (it's now called %r)" % ( mod_name, expr.name, obsolete_name_mapping[full_name]), expr) diff --git a/test-data/unit/check-isinstance.test b/test-data/unit/check-isinstance.test index 3f9b8d1040c7..bbf5ed478f82 100644 --- a/test-data/unit/check-isinstance.test +++ b/test-data/unit/check-isinstance.test @@ -1361,7 +1361,7 @@ def f(x: object) -> None: [builtins fixtures/isinstance.pyi] [out] -[case testIsInstanceWithTypeVariable] +[case testIsInstanceWithUnknownType] from typing import * def f(x: Union[int, str], typ: type) -> None: if isinstance(x, (typ, int)): From b110b1143c6a97c0445b441e65a90f75980f281d Mon Sep 17 00:00:00 2001 From: Max Moroz Date: Sat, 18 Mar 2017 14:24:09 -0700 Subject: [PATCH 5/8] Apply CR comments --- mypy/semanal.py | 5 ++--- test-data/unit/check-isinstance.test | 6 ++++-- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/mypy/semanal.py b/mypy/semanal.py index 06fb69a05497..f3b1eaec8568 100644 --- a/mypy/semanal.py +++ b/mypy/semanal.py @@ -2679,7 +2679,7 @@ def visit_member_expr(self, expr: MemberExpr) -> None: # This branch handles the case foo.bar where foo is a module. # In this case base.node is the module's MypyFile and we look up # bar in its namespace. This must be done for all types of bar. - file = cast(Optional[MypyFile], base.node) + file = cast(Optional[MypyFile], base.node) # can't use isinstance due to issue #2999 n = file.names.get(expr.name, None) if file is not None else None if n: n = self.normalize_type_alias(n, expr) @@ -2696,8 +2696,7 @@ def visit_member_expr(self, expr: MemberExpr) -> None: # one type checker run. If we reported errors here, # the build would terminate after semantic analysis # and we wouldn't be able to report any type errors. - full_name = '%s.%s' % (file.fullname() if file is not None - else None, expr.name) + full_name = '%s.%s' % (file.fullname() if file is not None else None, expr.name) mod_name = " '%s'" % file.fullname() if file is not None else '' if full_name in obsolete_name_mapping: self.fail("Module%s has no attribute %r (it's now called %r)" % ( diff --git a/test-data/unit/check-isinstance.test b/test-data/unit/check-isinstance.test index bbf5ed478f82..733338dc3946 100644 --- a/test-data/unit/check-isinstance.test +++ b/test-data/unit/check-isinstance.test @@ -1362,9 +1362,11 @@ def f(x: object) -> None: [out] [case testIsInstanceWithUnknownType] -from typing import * +from typing import Union def f(x: Union[int, str], typ: type) -> None: if isinstance(x, (typ, int)): x + 1 # E: Unsupported operand types for + (likely involving Union) + reveal_type(x) # E: Revealed type is 'Union[builtins.int, builtins.str]' + else: + reveal_type(x) # E: Revealed type is 'Union[builtins.int, builtins.str]' [builtins fixtures/isinstancelist.pyi] - From fc1aa62332e185066688340e4fb378283bb65428 Mon Sep 17 00:00:00 2001 From: Max Moroz Date: Sat, 18 Mar 2017 14:31:30 -0700 Subject: [PATCH 6/8] Add failing unit test for ininstance with type objects --- test-data/unit/check-isinstance.test | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/test-data/unit/check-isinstance.test b/test-data/unit/check-isinstance.test index 733338dc3946..8638e305064a 100644 --- a/test-data/unit/check-isinstance.test +++ b/test-data/unit/check-isinstance.test @@ -1370,3 +1370,21 @@ def f(x: Union[int, str], typ: type) -> None: else: reveal_type(x) # E: Revealed type is 'Union[builtins.int, builtins.str]' [builtins fixtures/isinstancelist.pyi] + + +[case testIsInstanceWithTypeObject] +from typing import Union, Type + +class A: pass + +def f(x: Union[int, A], a: Type[A]) -> None: + if isinstance(x, a): + reveal_type(x) # E: Revealed type is '__main__.A' + elif isinstance(x, int): + reveal_type(x) # E: Revealed type is 'builtins.int' + else: + reveal_type(x) # E: Revealed type is '__main__.A' + reveal_type(x) # E: Revealed type is 'Union[builtins.int, __main__.A]' + +[builtins fixtures/isinstancelist.pyi] + From be30faa7f9c230c966a5f9555b3713086f935bcf Mon Sep 17 00:00:00 2001 From: Max Moroz Date: Sat, 18 Mar 2017 18:18:14 -0700 Subject: [PATCH 7/8] Fix type inference with type objects in isinstance --- mypy/checker.py | 55 ++++++++++++++++++++++++++++++------------------- 1 file changed, 34 insertions(+), 21 deletions(-) diff --git a/mypy/checker.py b/mypy/checker.py index 20709422ea35..4e017abaab6d 100644 --- a/mypy/checker.py +++ b/mypy/checker.py @@ -2452,10 +2452,19 @@ def push_type_map(self, type_map: Optional[Dict[Expression, Type]]) -> None: TypeMap = Optional[Dict[Expression, Type]] +# An object that represents either a precise type or a type with an upper bound; +# it is important for correct type inference with isinstance. +TypeRange = NamedTuple( + 'TypeRange', + [ + ('item', Type), + ('is_upper_bound', bool), # False => precise type + ]) + def conditional_type_map(expr: Expression, current_type: Optional[Type], - proposed_type: Optional[Type], + proposed_type_ranges: Optional[List[TypeRange]], ) -> Tuple[TypeMap, TypeMap]: """Takes in an expression, the current type of the expression, and a proposed type of that expression. @@ -2463,17 +2472,26 @@ def conditional_type_map(expr: Expression, Returns a 2-tuple: The first element is a map from the expression to the proposed type, if the expression can be the proposed type. The second element is a map from the expression to the type it would hold - if it was not the proposed type, if any.""" - if proposed_type: + if it was not the proposed type, if any. None means bot, {} means top""" + if proposed_type_ranges: + if len(proposed_type_ranges) == 1: + proposed_type = proposed_type_ranges[0].item # Union with a single type breaks tests + else: + proposed_type = UnionType([type_range.item for type_range in proposed_type_ranges]) if current_type: - if is_proper_subtype(current_type, proposed_type): - # Expression is always of type proposed_type + if not any(type_range.is_upper_bound for type_range in proposed_type_ranges) \ + and is_proper_subtype(current_type, proposed_type): + # Expression is always of one of the types in proposed_type_ranges return {}, None elif not is_overlapping_types(current_type, proposed_type): - # Expression is never of type proposed_type + # Expression is never of any type in proposed_type_ranges return None, {} else: - remaining_type = restrict_subtype_away(current_type, proposed_type) + # we can only restrict when the type is precise, not bounded + proposed_precise_type = UnionType([type_range.item + for type_range in proposed_type_ranges + if not type_range.is_upper_bound]) + remaining_type = restrict_subtype_away(current_type, proposed_precise_type) return {expr: proposed_type}, {expr: remaining_type} else: return {expr: proposed_type}, {} @@ -2644,8 +2662,8 @@ def find_isinstance_check(node: Expression, expr = node.args[0] if expr.literal == LITERAL_TYPE: vartype = type_map[expr] - type = get_isinstance_type(node.args[1], type_map) - return conditional_type_map(expr, vartype, type) + types = get_isinstance_type(node.args[1], type_map) + return conditional_type_map(expr, vartype, types) elif refers_to_fullname(node.callee, 'builtins.callable'): expr = node.args[0] if expr.literal == LITERAL_TYPE: @@ -2663,7 +2681,8 @@ def find_isinstance_check(node: Expression, # two elements in node.operands, and at least one of them # should represent a None. vartype = type_map[expr] - if_vars, else_vars = conditional_type_map(expr, vartype, NoneTyp()) + none_typ = [TypeRange(NoneTyp(), is_upper_bound=False)] + if_vars, else_vars = conditional_type_map(expr, vartype, none_typ) break if is_not: @@ -2725,28 +2744,22 @@ def flatten(t: Expression) -> List[Expression]: return [t] -def get_isinstance_type(expr: Expression, type_map: Dict[Expression, Type]) -> Type: +def get_isinstance_type(expr: Expression, type_map: Dict[Expression, Type]) -> List[TypeRange]: all_types = [type_map[e] for e in flatten(expr)] - - types = [] # type: List[Type] - + types = [] # type: List[TypeRange] for type in all_types: if isinstance(type, FunctionLike): if type.is_type_obj(): # Type variables may be present -- erase them, which is the best # we can do (outside disallowing them here). type = erase_typevars(type.items()[0].ret_type) - types.append(type) + types.append(TypeRange(type, is_upper_bound=False)) elif isinstance(type, TypeType): - types.append(type.item) + types.append(TypeRange(type.item, is_upper_bound=True)) else: # we didn't see an actual type, but rather a variable whose value is unknown to us return None - assert len(types) != 0 - if len(types) == 1: - return types[0] - else: - return UnionType(types) + return types def expand_func(defn: FuncItem, map: Dict[TypeVarId, Type]) -> FuncItem: From f0bfcb683e65ed032e193e0480bed1b0cf0b8b50 Mon Sep 17 00:00:00 2001 From: Max Moroz Date: Sun, 19 Mar 2017 13:00:34 -0700 Subject: [PATCH 8/8] Apply CR comments --- mypy/checker.py | 24 ++++++++++++++++-------- test-data/unit/check-isinstance.test | 24 ++++++++++++++++++++++++ 2 files changed, 40 insertions(+), 8 deletions(-) diff --git a/mypy/checker.py b/mypy/checker.py index 4e017abaab6d..cb55cfc73dc2 100644 --- a/mypy/checker.py +++ b/mypy/checker.py @@ -2479,8 +2479,8 @@ def conditional_type_map(expr: Expression, else: proposed_type = UnionType([type_range.item for type_range in proposed_type_ranges]) if current_type: - if not any(type_range.is_upper_bound for type_range in proposed_type_ranges) \ - and is_proper_subtype(current_type, proposed_type): + if (not any(type_range.is_upper_bound for type_range in proposed_type_ranges) + and is_proper_subtype(current_type, proposed_type)): # Expression is always of one of the types in proposed_type_ranges return {}, None elif not is_overlapping_types(current_type, proposed_type): @@ -2748,17 +2748,25 @@ def get_isinstance_type(expr: Expression, type_map: Dict[Expression, Type]) -> L all_types = [type_map[e] for e in flatten(expr)] types = [] # type: List[TypeRange] for type in all_types: - if isinstance(type, FunctionLike): - if type.is_type_obj(): - # Type variables may be present -- erase them, which is the best - # we can do (outside disallowing them here). - type = erase_typevars(type.items()[0].ret_type) + if isinstance(type, FunctionLike) and type.is_type_obj(): + # Type variables may be present -- erase them, which is the best + # we can do (outside disallowing them here). + type = erase_typevars(type.items()[0].ret_type) types.append(TypeRange(type, is_upper_bound=False)) elif isinstance(type, TypeType): + # Type[A] means "any type that is a subtype of A" rather than "precisely type A" + # we indicate this by setting is_upper_bound flag types.append(TypeRange(type.item, is_upper_bound=True)) + elif isinstance(type, Instance) and type.type.fullname() == 'builtins.type': + object_type = Instance(type.type.mro[-1], []) + types.append(TypeRange(object_type, is_upper_bound=True)) else: # we didn't see an actual type, but rather a variable whose value is unknown to us return None - assert len(types) != 0 + if not types: + # this can happen if someone has empty tuple as 2nd argument to isinstance + # strictly speaking, we should return UninhabitedType but for simplicity we will simply + # refuse to do any type inference for now + return None return types diff --git a/test-data/unit/check-isinstance.test b/test-data/unit/check-isinstance.test index 8638e305064a..973d463d6085 100644 --- a/test-data/unit/check-isinstance.test +++ b/test-data/unit/check-isinstance.test @@ -1361,12 +1361,36 @@ def f(x: object) -> None: [builtins fixtures/isinstance.pyi] [out] + [case testIsInstanceWithUnknownType] from typing import Union def f(x: Union[int, str], typ: type) -> None: if isinstance(x, (typ, int)): x + 1 # E: Unsupported operand types for + (likely involving Union) reveal_type(x) # E: Revealed type is 'Union[builtins.int, builtins.str]' + else: + reveal_type(x) # E: Revealed type is 'builtins.str' +[builtins fixtures/isinstancelist.pyi] + + +[case testIsInstanceWithBoundedType] +from typing import Union, Type + +class A: pass +def f(x: Union[int, A], a: Type[A]) -> None: + if isinstance(x, (a, int)): + reveal_type(x) # E: Revealed type is 'Union[builtins.int, __main__.A]' + else: + reveal_type(x) # E: Revealed type is '__main__.A' + +[builtins fixtures/isinstancelist.pyi] + + +[case testIsInstanceWithEmtpy2ndArg] +from typing import Union +def f(x: Union[int, str]) -> None: + if isinstance(x, ()): + reveal_type(x) # E: Revealed type is 'Union[builtins.int, builtins.str]' else: reveal_type(x) # E: Revealed type is 'Union[builtins.int, builtins.str]' [builtins fixtures/isinstancelist.pyi]