Fix inference logic for isinstance #2997

pkch · 2017-03-14T12:41:09Z

gvanrossum

Can you add unit tests?

gvanrossum · 2017-03-14T17:51:29Z

mypy/checker.py

@@ -2711,9 +2711,14 @@ def get_isinstance_type(expr: Expression, type_map: Dict[Expression, Type]) -> T

            types.append(type)



I think there are too many blank lines here. IMO the only blank line should be outside the for-loop, before the assert on line 2720 below.

ilevkivskyi · 2017-03-18T16:44:49Z

It looks like all commits from this PR and from #2995 are also included in #3005. I have made a review on the latter that also concern this PR and #2995.

For the future, I would propose to have non-overlapping PRs, it will be much easier to understand them.

JukkaL · 2017-03-18T20:08:23Z

mypy/semanal.py

@@ -2640,8 +2640,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 = base.node
-            assert isinstance(file, (MypyFile, type(None)))
+            file = cast(Optional[MypyFile], base.node)


Add comment describing why we don't use assert isinstance here.

JukkaL · 2017-03-18T20:09:10Z

mypy/semanal.py

@@ -2658,7 +2657,8 @@ 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


This change seems unnecessary now?

JukkaL · 2017-03-18T20:10:12Z

test-data/unit/check-isinstance.test

+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)


Add also an else: block and do a reveal_type(x) in that block. Add another reveal_type(x) just after the if statement.

JukkaL · 2017-03-18T20:14:50Z

test-data/unit/check-isinstance.test

+    if isinstance(x, (typ, int)):
+        x + 1  # E: Unsupported operand types for + (likely involving Union)
+[builtins fixtures/isinstancelist.pyi]
+


Add test for the type object that is Type[A] or similar. Example:

class A: pass def f(x: Union[int, A], a: Type[A]) -> None: if isinstance(x, a): reveal_type(x) elif isinstance(x, int): reveal_type(x) else: reveal_type(x) reveal_type(x)

…ance

JukkaL · 2017-03-19T17:51:41Z

mypy/checker.py

-        return UnionType(types)
+            types.append(TypeRange(type, is_upper_bound=False))
+        elif isinstance(type, TypeType):
+            types.append(TypeRange(type.item, is_upper_bound=True))


Add comment here describing why is_upper_bound=True.

JukkaL · 2017-03-19T17:54:20Z

mypy/checker.py

+            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


I wonder if we should return None if the length if 0? For example, what happens if we have isinstance(x, ())?

JukkaL · 2017-03-19T17:55:29Z

test-data/unit/check-isinstance.test

+        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]'


Here we could actually infer the type to be builtins.str, since we know that it can't be builtins.int due to the isinstance check.

JukkaL · 2017-03-19T17:56:05Z

test-data/unit/check-isinstance.test

+        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]


Not directly related, but can you add a test case that does something like isinstance(x, ())?

JukkaL · 2017-03-19T17:56:34Z

mypy/checker.py

        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) \


Use if (...)" to avoid the use of \.

gvanrossum mentioned this pull request Mar 14, 2017

Incorrect inference with isinstance() #2993

Closed

gvanrossum reviewed Mar 14, 2017

View reviewed changes

pkch force-pushed the fix-isinstance-inference branch from a61bf32 to 79a8085 Compare March 14, 2017 20:30

JukkaL reviewed Mar 18, 2017

View reviewed changes

pkch force-pushed the fix-isinstance-inference branch from bca997d to 6ea4f4d Compare March 18, 2017 21:30

pkch added 7 commits March 18, 2017 18:25

Make no inference when variables present in second argument to isinst…

a29069e

…ance

Fix formatting

8d7811a

Add unit test

14efa16

Replace isinstance with cast to deal with python#2999; rename test

d9f261f

Apply CR comments

b110b11

Add failing unit test for ininstance with type objects

fc1aa62

Fix type inference with type objects in isinstance

be30faa

pkch force-pushed the fix-isinstance-inference branch from 2fa26d1 to be30faa Compare March 19, 2017 01:58

pkch mentioned this pull request Mar 19, 2017

Infer types from issubclass() calls #3005

Merged

JukkaL reviewed Mar 19, 2017

View reviewed changes

Apply CR comments

f0bfcb6

JukkaL merged commit 0b28da6 into python:master Mar 19, 2017

JukkaL mentioned this pull request Mar 27, 2017

isinstance with self.__class__ infers 'object' as the type #3061

Closed

pkch mentioned this pull request Apr 15, 2017

Type inference engine simplifies too aggressively #3178

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix inference logic for isinstance #2997

Fix inference logic for isinstance #2997

pkch commented Mar 14, 2017

gvanrossum left a comment

gvanrossum Mar 14, 2017

ilevkivskyi commented Mar 18, 2017

JukkaL Mar 18, 2017

JukkaL Mar 18, 2017

JukkaL Mar 18, 2017

JukkaL Mar 18, 2017

JukkaL Mar 19, 2017

JukkaL Mar 19, 2017

JukkaL Mar 19, 2017

JukkaL Mar 19, 2017

JukkaL Mar 19, 2017

		@@ -2711,9 +2711,14 @@ def get_isinstance_type(expr: Expression, type_map: Dict[Expression, Type]) -> T

		types.append(type)

Fix inference logic for isinstance #2997

Fix inference logic for isinstance #2997

Conversation

pkch commented Mar 14, 2017

gvanrossum left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilevkivskyi commented Mar 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment