Infer types from issubclass() calls #3005

pkch · 2017-03-15T05:14:18Z

This PR includes a few commits from other PRs that haven't been merged in yet; I'll squash and fix any merge conflicts after the other PRs are merged in.

The only commits that are unique to this PR are the last 2: a4db99e and fe27fda, and the first of them is just a failing unit test.

The commit is somewhat messy, so I just wanted to get feedback on whether it's going in the right direction. Basically, I had to deal with the interplay of Union and issubclass. Everything seems to work, but it's not too elegant.

ilevkivskyi

Thank you for working on this! It will be a useful feature. Here are some comments.

ilevkivskyi · 2017-03-18T11:27:35Z

mypy/checker.py

@@ -2588,6 +2588,20 @@ def or_conditional_maps(m1: TypeMap, m2: TypeMap) -> TypeMap:
    return result


+def convert_to_types(m: MutableMapping[Expression, Type]) -> None:


I think the name of this function does not reflect its behaviour. Something like "convert_to_typetype" would be better.

ilevkivskyi · 2017-03-18T11:29:27Z

mypy/checker.py

@@ -2588,6 +2588,20 @@ def or_conditional_maps(m1: TypeMap, m2: TypeMap) -> TypeMap:
    return result


+def convert_to_types(m: MutableMapping[Expression, Type]) -> None:


Why don't you just reuse the TypeMap alias defined above instead of MutableMapping[Expression, Type]?
Or maybe just use Dict?

ilevkivskyi · 2017-03-18T11:31:15Z

mypy/checker.py

@@ -2588,6 +2588,20 @@ def or_conditional_maps(m1: TypeMap, m2: TypeMap) -> TypeMap:
    return result


+def convert_to_types(m: MutableMapping[Expression, Type]) -> None:
+    for k in m:


Please use longer names for variables:

m -> type_map

k -> expr

x -> typ

done for this and other CR changes

ilevkivskyi · 2017-03-18T11:42:10Z

mypy/checker.py

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


We should warn about this. isinstance(obj, Iterable[int]) etc. will unconditionally fail at runtime with a TypeError.

I should probably do it in a separate PR though? (Since I deleted my function, your comment now applies only to get_isinstance_type, which I'm not modifying.) Or is it ok to incorporate it here as well?

I would prefer a separate PR for this.

I added #3037 ; not sure where to actually catch this problem.

ilevkivskyi · 2017-03-18T11:46:28Z

test-data/unit/fixtures/isinstancelist.pyi

@@ -42,3 +43,9 @@ class dict(Iterable[KT], Mapping[KT, VT], Generic[KT, VT]):
    def __setitem__(self, k: KT, v: VT) -> None: pass
    def __iter__(self) -> Iterator[KT]: pass
    def update(self, a: Mapping[KT, VT]) -> None: pass
+
+class set(Iterable[T], Generic[T]):


Generic[T] is now not needed.

ilevkivskyi · 2017-03-18T12:05:30Z

mypy/semanal.py

-                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


Why did you add three # type: ignores in this file? Are you trying to self check mypy with --strct-optional?
If yes, then I think you should not put them here in this PR.

No, it's because of #2999. A bug fix in this commit triggered that issue. @JukkaL suggested to avoid # type: ignore by replacing isinstance with cast to make it safer, so I'll do that.

This is still not done.

ilevkivskyi · 2017-03-18T12:09:43Z

test-data/unit/check-isinstance.test

+        x + 1  # E: Unsupported operand types for + (likely involving Union)
+[builtins fixtures/isinstancelist.pyi]
+
+[case testIssubclass]


Is it possible to split this test into few smaller unit tests?

ilevkivskyi · 2017-03-18T12:12:52Z

test-data/unit/check-isinstance.test

+from typing import Union, List, Tuple
+def f(x: Union[int, str, List, Tuple]) -> None:
+    if isinstance(x, (str, (int, tuple))):
+        x[1]  # E: Value of type "Union[int, str, tuple]" is not indexable


I would also add a similar test with a successful check (i.e. not causing an error).

ilevkivskyi · 2017-03-18T12:14:11Z

test-data/unit/check-isinstance.test

@@ -1344,3 +1351,131 @@ def f(x: object) -> None:
    reveal_type(b)  # E: Revealed type is '__main__.A'
 [builtins fixtures/isinstance.pyi]
 [out]
+
+[case testIsInstanceWithTypeVariable]


The name of this test is misleading. You don't use type variables here.

ilevkivskyi · 2017-03-18T12:16:30Z

test-data/unit/check-isinstance.test

+        g.job  # E: "Goblin" has no attribute "job"
+
+
+[builtins fixtures/isinstancelist.pyi]


I would add also some tests with built-in types, e.g. something involving

class MyList(List[int]): ...

and then

if issuclass(MyList, List): ...

pkch · 2017-03-19T08:49:43Z

@ilevkivskyi I addressed all your comments, except #3005 (comment) (I replied). Some changes appeared in #2995 and #2997, and you can see them. The rest are in my local repo and you'll see them in this branch when I push them. I'll push only after the other 2 PRs are merged in, to avoid more confusion.

I had a huge number of merge mistakes (not just conflicts, but actual mistakes...) 😞 I'll never again create a PR that depends on 2 unmerged PRs...

ilevkivskyi · 2017-03-19T23:02:01Z

@pkch

I had a huge number of merge mistakes (not just conflicts, but actual mistakes...) 😞 I'll never again create a PR that depends on 2 unmerged PRs...

This is not a big deal. But from organizational point of view, yes, independent PRs are better.

It looks like both PRs you mentioned are merged now. I am waiting to review your new commits (also a merge conflict needs to be fixed).

…ance

ilevkivskyi

Thanks! This is almost ready, I have few more comments on this PR

ilevkivskyi · 2017-03-21T12:07:04Z

test-data/unit/check-isinstance.test

+class GoblinAmbusher(Goblin):
+    job: ClassVar[str] = 'Ranger'
+
+


One blanc line is enough here

ilevkivskyi · 2017-03-21T12:07:24Z

mypy/semanal.py

-                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


This is still not done.

ilevkivskyi · 2017-03-21T12:09:37Z

mypy/checker.py

+                            union_list.append(t.item)
+                        else:
+                            #  this an error; should be caught earlier, so we should never be here
+                            return {}, {}


If you are sure this is an unreachable branch, then you could put assert False here. If this could happen as a result of an error that is reported elsewhere, then adjust the comment correspondingly.

ilevkivskyi · 2017-03-21T12:10:06Z

mypy/checker.py

+                        if isinstance(t, TypeType):
+                            union_list.append(t.item)
+                        else:
+                            #  this an error; should be caught earlier, so we should never be here


typo: "this an error;" -> "this is an error;"

ilevkivskyi · 2017-03-21T12:15:51Z

test-data/unit/check-isinstance.test

+    if issubclass(cls, MyList):
+        cls()[0]
+    else:
+        cls()[0]  # E:


What is the error message here? Why didn't you write it here?

Oh, no error message I just accidentally left E:

But shouldn't this actually be an error? In the else branch, cls is not List and therefore not subscriptable. Or am I missing something?

No, it's my bad. I confused this test with another where I left E:; in this case I did mean for it to return an error, and it doesn't, and I forgot about it.

The reason it doesn't complain is that cls() is revealed as Any, since cls is defined as Type, which is translated to Type[Any]. So the entire test was useless.

Btw, I would prefer Type to be expanded to Type[object] rather than Type[Any], but I suppose it would be too inconsistent.

ilevkivskyi · 2017-03-21T12:19:24Z

test-data/unit/check-isinstance.test

+
+class Goblin:
+    aggressive: bool = True
+    level: int


Do you need both attributes here? Some test cases could be simplified if you have only one instance variable.
Also you probably don't need to check both access and assignment to instance variables in all tests, just checking assignment is enough.

ilevkivskyi · 2017-03-21T20:00:34Z

mypy/semanal.py

@@ -2696,7 +2696,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
+                                       else None, expr.name)


I think you don't need this formatting change, it only creates noise in the PR. Otherwise now it looks good to me

* Manually resolve test merge conflict * Also update one test that now has a better error message

ilevkivskyi · 2017-04-13T10:11:04Z

@pkch I know that I already approved this PR but there are two more things that could be fixed before I merge this:

most tests should use reveal_type in if issubclas ... else: ... branches (this will probably also allow further simplifying some tests).
there are two TODO items that should be fixed.

ilevkivskyi · 2017-04-13T23:08:35Z

@pkch Sorry, I cannot merge this yet. This PR crashes with:

x: Type[str]
if issubclass(x, int):
    reveal_type(x)

The reason is that conditional_type_map sometimes returns None (for example for non-overlapping case like in the example above) and this case should be carefully treated.

pkch · 2017-04-14T00:20:16Z

Ah yes. BTW, this is the type of bug that mypy --strict-optional mypy would have caught.

gvanrossum · 2017-04-14T00:34:21Z

Speaking of which, we should really convert all of mypy to strict optional checking. (It's not so easy, or we would have done it already.)

ilevkivskyi · 2017-04-14T23:07:12Z

test-data/unit/check-isinstance.test

-        cls.job  # E: Type[Goblin] has no attribute "job"
-        g = cls()
-        g.level = 15
-        g.job  # E: "Goblin" has no attribute "job"


I think few integration tests would be useful, so I was happy with these tests. Why are you removing them in this commit?

Ah I thought the reveal_type makes for a much more focused unit test. I was afraid the test was needlessly long and hard to read. After all, this PR is only for issubclass functionality, once we know that the revealed type is correct, do we really need to check that the attribute access is done correctly? But I'm adding back a couple integration tests.

Sometimes mypy does copy_modified, so same representation of type is not a 100% guarantee. Also, there were some tests with destructuring unions in the initial version, I could not find them now, something like:

x: Type[Union[int, str, X]] if isinstance(x, (int, str)): reveal_type(x) if isinstance(x, str): reval_type(x) else: reval_type(x) else: reval_type(x) # Revealed type is '__main__.X'

etc.

same representation of type is not a 100%

TBH, I am only a bit worried about ClassVar, but still few other integration tests will be useful.

Hmm are you sure by "destructuring unions" you didn't mean this:

[case testIsinstanceNestedTuple] from typing import Union, List, Tuple, Dict def f(x: Union[int, str, List]) -> None: if isinstance(x, (str, (int,))): reveal_type(x) # E: Revealed type is 'Union[builtins.int, builtins.str]' x[1] # E: Value of type "Union[int, str]" is not indexable else: reveal_type(x) # E: Revealed type is 'builtins.list[Any]' x[1] reveal_type(x) # E: Revealed type is 'Union[builtins.int, builtins.str, builtins.list[Any]]' if isinstance(x, (str, (list,))): reveal_type(x) # E: Revealed type is 'Union[builtins.str, builtins.list[Any]]' x[1] reveal_type(x) # E: Revealed type is 'Union[builtins.int, builtins.str, builtins.list[Any]]' [builtins fixtures/isinstancelist.pyi]

I don't recall adding it to issubclass, and it's still there from my earlier nested tuples / isinstance PR.

Ah yes, this was for isinstance(), could you add a similar test for issubclass() (as I have proposed above)? Sorry, you are probably already tired, but I would like to be safe here.

@pkch Just in case you didn't get notification.

Heh, np, this is a useful test. A bunch of weird stuff but I think it's tangential to the main purpose of this PR:

Type[List] is revealed differently in different locations: sometimes "Type[builtins.list]", sometimes "Type[builtins.list[Any]]"

Instantiation of that is revealed as builtins.list[<uninhabited>]

Other than revealed type strings, everything looks good. Should I open an issue for those two, or should we pretend we didn't notice?

(the second one is probably going to be fixed with #3032)

@pkch
I think this is a separate issue related to what we discussed in tuple fallback issue #3117. I think we should weed out all generic types with incorrect type argument count, there should be no such thing as builtins.list, only builtins.list[AnyType()], etc. internally. Otherwise such inconsistencies lead to weird bugs like strange <uninhabited> or the problem with tuple

Sadly, my #3129 doesn't achieve that unification of tuple fallbacks in this case (I was hoping it might, but when I merged that PR into here, nothing changed). Should this be done in this PR or in a separate one?

@pkch
This could be done in #3129 or in an additional PR, but not here.

ilevkivskyi · 2017-04-19T17:40:47Z

@gvanrossum could you please try this PR with your internal codebase, just to be sure that everything is OK?

I think it is a useful feature and the PR is almost ready to be merged (after @pkch will restore the tests that were inadvertently removed).

gvanrossum · 2017-04-19T19:06:58Z

This is clean on our internal codebase (FWIW).

ilevkivskyi · 2017-04-20T07:56:51Z

test-data/unit/check-isinstance.test

@@ -1443,6 +1443,28 @@ else:
 [builtins fixtures/isinstancelist.pyi]


+[case testIssubclasDestructuringUnions]
+from typing import Union, List, Tuple, Dict, Type
+def f(x: Union[Type[int], Type[str], Type[List]]) -> None:


@pkch Is it possible to use Type[Union[int, str, List]] here?

Wow, it is, but I thought it's illegal? I thought Type requires only simple types inside, not constructs like Union etc.?

But it 50% works... Here's the output. The first couple lines int and str get swapped; then a couple lines nothing changed; and then it completely breaks down and becomes Any:

def f(x: Type[Union[int, str, List]]) -> None: if issubclass(x, (str, (int,))): reveal_type(x) # E: Revealed type is 'Union[Type[builtins.str], Type[builtins.int]]' reveal_type(x()) # E: Revealed type is 'Union[builtins.str, builtins.int]' x()[1] # E: Value of type "Union[str, int]" is not indexable else: reveal_type(x) # E: Revealed type is 'Type[builtins.list]' reveal_type(x()) # E: Revealed type is 'builtins.list[<uninhabited>]' x()[1] reveal_type(x) # E: Revealed type is 'Any' reveal_type(x()) # E: Revealed type is 'Any' if issubclass(x, (str, (list,))): reveal_type(x) # E: Revealed type is 'Any' reveal_type(x()) # E: Revealed type is 'Any' x()[1] reveal_type(x) # E: Revealed type is 'Any' reveal_type(x()) # E: Revealed type is 'Any'

@pkch

I thought Type requires only simple types inside, not constructs like Union

Union is legal as a special case, see for example typing docs (emphasis my):

The only legal parameters for Type are classes, unions of classes, and Any.

But it 50% works...

Hmm, these two Type[Union[int, str]] and Union[Type[int], Type[str]] should be equivalent I think. Is it easy to fix?

Wow reveal_type changes from early to late in the same scope:

from typing import Union, List, Tuple, Dict, Type def f(x: Type[Union[int, str, List]]) -> None: reveal_type(x) # E: Revealed type is 'Type[Union[builtins.int, builtins.str, builtins.list]]' reveal_type(x()) # E: Revealed type is 'Union[builtins.int, builtins.str, builtins.list[<uninhabited>]]' if issubclass(x, (str, (int,))): 1 else: 1 reveal_type(x) # E: Revealed type is 'Any' reveal_type(x()) # E: Revealed type is 'Any'

@pkch
I think this should be fixed. Without this PR I get:

def f(x: Type[Union[int, str, List]]) -> None: reveal_type(x) # E: Revealed type is 'Type[Union[builtins.int, builtins.str, builtins.list]]' if issubclass(x, (str, (int,))): 1 else: 1 reveal_type(x) # E: Revealed type is 'Type[Union[builtins.int, builtins.str, builtins.list]]'

A type should not be "destroyed" after an issubclass() check.

@pkch

Then I will merge this PR

This time for sure :-)

Done!

Interesting, when I git reset --hard @~ and git push -f, I effectively remove the last commit from this PR. Then AppVeyor decided to rebuild, while Travis decided not to. At first I thought that AppVeyor was smarter because it noticed that master branch changed so it figured the new merge with master will be different, and it's worth rebuilding.

It turns out that AppVeyor rebuilds regardless of whether master changed since the last time it built the commit; in fact, it even rebuilt on my own branch in the forked repo.

So, when they already built for the commit that just became the head of the PR or branch:

AppVeyor always rebuilds, even if there's absolutely no need

Travis does not rebuild, even if it's arguably worth rebuilding due to the change in master

Edit: Travis is perfectly reasonable, actually. Its green mark means that the branch merged into master without errors at some point in the past - not that it would do the same right this moment. And it maintains the same meaning in the situation I just described.

@pkch

Something strange happened with typeshed. Could you please fix this?
(And don't forget to open an issue about Type[Union[...]] vs Union[Type[...]].)

@ilevkivskyi what do you see happening? everything seems fine in my forked repo, when I merged this branch into master.

@pkch
Hm, this is strange, GitHub shows me that your PR makes changes to typeshed, but I checked the commit hash, and it is the correct one. I think it is safe to merge.

ilevkivskyi reviewed Mar 18, 2017

View reviewed changes

ilevkivskyi mentioned this pull request Mar 18, 2017

Fix inference logic for isinstance #2997

Merged

pkch added 4 commits March 20, 2017 00:59

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

158c889

…ance

Add failing unit tests

c5525bd

Infer type from issubclass

1776e90

Apply CR comments

3334b8e

pkch force-pushed the issubclass branch from fe27fda to 3334b8e Compare March 20, 2017 08:25

pkch mentioned this pull request Mar 20, 2017

Prohibit parameterized generics in isinstance #3037

Closed

Fix 3.3 syntax, updated typeshed

3587549

ilevkivskyi reviewed Mar 21, 2017

View reviewed changes

pkch added 2 commits March 21, 2017 12:20

More CR fixes

81e1923

Fix testr

e4ffdc5

ilevkivskyi reviewed Mar 21, 2017

View reviewed changes

ilevkivskyi approved these changes Mar 21, 2017

View reviewed changes

pkch added 2 commits March 21, 2017 13:04

Revert accidental format change

9440a52

Merge branch 'master' into issubclass

c831401

* Manually resolve test merge conflict * Also update one test that now has a better error message

pkch added 4 commits April 13, 2017 07:01

Merge branch 'master' into issubclass

a4a7214

Address CR

f94ca45

Rename type into typ

a260b79

Merge branch 'master' into issubclass

fbd0b91

pkch force-pushed the issubclass branch from 75bee74 to fbd0b91 Compare April 13, 2017 14:54

Fix unreachable block crash

e48d983

pkch force-pushed the issubclass branch from 62d0d0a to e48d983 Compare April 14, 2017 00:22

ilevkivskyi reviewed Apr 14, 2017

View reviewed changes

pkch added 2 commits April 19, 2017 12:38

Add back integration tests

b0ce683

Add union destructuring test

aca05c7

ilevkivskyi reviewed Apr 20, 2017

View reviewed changes

pkch force-pushed the issubclass branch from 408f19e to aca05c7 Compare April 21, 2017 10:17

ilevkivskyi merged commit c38a836 into python:master Apr 21, 2017

pkch mentioned this pull request Apr 21, 2017

Standardize Type[Union[...]] -> Union[Type[...]] #3209

Merged

		@@ -2588,6 +2588,20 @@ def or_conditional_maps(m1: TypeMap, m2: TypeMap) -> TypeMap:
		return result


		def convert_to_types(m: MutableMapping[Expression, Type]) -> None:

		g.job # E: "Goblin" has no attribute "job"


		[builtins fixtures/isinstancelist.pyi]

Infer types from issubclass() calls #3005

Infer types from issubclass() calls #3005

Conversation

pkch commented Mar 15, 2017

ilevkivskyi left a 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

pkch Mar 20, 2017 • edited Loading

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

ilevkivskyi Mar 18, 2017 • edited Loading

Choose a reason for hiding this comment

pkch Mar 18, 2017 • edited Loading

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

pkch commented Mar 19, 2017 • edited Loading

ilevkivskyi commented Mar 19, 2017

ilevkivskyi left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilevkivskyi commented Apr 13, 2017

ilevkivskyi commented Apr 13, 2017

pkch commented Apr 14, 2017

gvanrossum commented Apr 14, 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

pkch Apr 19, 2017 • edited Loading

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

ilevkivskyi Apr 20, 2017 • edited Loading

Choose a reason for hiding this comment

ilevkivskyi commented Apr 19, 2017

gvanrossum commented Apr 19, 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

ilevkivskyi Apr 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pkch Apr 21, 2017 • edited Loading

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

pkch Mar 20, 2017 •

edited

Loading

ilevkivskyi Mar 18, 2017 •

edited

Loading

pkch Mar 18, 2017 •

edited

Loading

pkch commented Mar 19, 2017 •

edited

Loading

pkch Apr 19, 2017 •

edited

Loading

ilevkivskyi Apr 20, 2017 •

edited

Loading

ilevkivskyi Apr 20, 2017 •

edited

Loading

pkch Apr 21, 2017 •

edited

Loading