8000 Infer types from issubclass() calls by pkch · Pull Request #3005 · python/mypy · GitHub
[go: up one dir, main page]
Skip to content

Infer types from issubclass() calls #3005

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 16 commits into from
Apr 21, 2017
Merged

Infer types from issubclass() calls #3005

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
158c889
Make no inference when variables present in second argument to isinst…
pkch Mar 14, 2017
c5525bd
Add failing unit tests
pkch Mar 15, 2017
1776e90
Infer type from issubclass
pkch Mar 15, 2017
3334b8e
Apply CR comments
pkch Mar 18, 2017
3587549
Fix 3.3 syntax, updated typeshed
pkch Mar 21, 2017
81e1923
More CR fixes
pkch Mar 21, 2017
e4ffdc5
Fix testr
pkch Mar 21, 2017
9440a52
Revert accidental format change
pkch Mar 21, 2017
c831401
Merge branch 'master' into issubclass
pkch Apr 11, 2017
a4a7214
Merge branch 'master' into issubclass
pkch Apr 13, 2017
f94ca45
Address CR
pkch Apr 13, 2017
a260b79
Rename type into typ
pkch Apr 13, 2017
fbd0b91
Merge branch 'master' into issubclass
pkch Apr 13, 2017
e48d983
Fix unreachable block crash
pkch Apr 14, 2017
b0ce683
Add back integration tests
pkch Apr 19, 2017
aca05c7
Add union destructuring test
pkch Apr 20, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Add union destructuring test
  • Loading branch information
@pkch
pkch committed Apr 20, 2017
commit aca05c7d4e271686f11335a9157a8db18e525587
22 changes: 22 additions & 0 deletions test-data/unit/check-isinstance.test
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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'

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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'

Copy link
Member
@ilevkivskyi ilevkivskyi Apr 20, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pkch

Then I will merge this PR

This time for sure :-)

Copy link
Contributor Author
@pkch pkch Apr 21, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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[...]].)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

if issubclass(x, (str, (int,))):
reveal_type(x) # E: Revealed type is 'Union[Type[builtins.int], Type[builtins.str]]'
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 'Type[builtins.list]'
reveal_type(x()) # E: Revealed type is 'builtins.list[<uninhabited>]'
x()[1]
reveal_type(x) # E: Revealed type is 'Union[Type[builtins.int], Type[builtins.str], Type[builtins.list]]'
reveal_type(x()) # E: Revealed type is 'Union[builtins.int, builtins.str, builtins.list[<uninhabited>]]'
if issubclass(x, (str, (list,))):
reveal_type(x) # E: Revealed type is 'Union[Type[builtins.str], Type[builtins.list[Any]]]'
reveal_type(x()) # E: Revealed type is 'Union[builtins.str, builtins.list[<uninhabited>]]'
x()[1]
reveal_type(x) # E: Revealed type is 'Union[Type[builtins.int], Type[builtins.str], Type[builtins.list[Any]]]'
reveal_type(x()) # E: Revealed type is 'Union[builtins.int, builtins.str, builtins.list[<uninhabited>]]'
[builtins fixtures/isinstancelist.pyi]


[case testIssubc 10000 lass]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

from typing import Type, ClassVar

Expand Down
2 changes: 1 addition & 1 deletion typeshed
Open in desktop
Submodule typeshed updated 53 files
+2 −0 .travis.yml
+9 −1 README.md
+0 −5 stdlib/2/__builtin__.pyi
+16 −14 stdlib/2/_io.pyi
+3 −3 stdlib/2/_warnings.pyi
+1 −8 stdlib/2/abc.pyi
+3 −4 stdlib/2/io.pyi
+1 −0 stdlib/2/macpath.pyi
+1 −0 stdlib/2/ntpath.pyi
+1 −0 stdlib/2/os2emxpath.pyi
+0 −4 stdlib/2/tokenize.pyi
+5 −3 stdlib/2/types.pyi
+2 −1 stdlib/2/unittest.pyi
+1 −1 stdlib/2/urllib2.pyi
+6 −1 stdlib/2and3/contextlib.pyi
+14 −1 stdlib/2and3/distutils/core.pyi
+1 −0 stdlib/2and3/lib2to3/__init__.pyi
+10 −0 stdlib/2and3/lib2to3/pgen2/__init__.pyi
+24 −0 stdlib/2and3/lib2to3/pgen2/driver.pyi
+29 −0 stdlib/2and3/lib2to3/pgen2/grammar.pyi
+9 −0 stdlib/2and3/lib2to3/pgen2/literals.pyi
+29 −0 stdlib/2and3/lib2to3/pgen2/parse.pyi
+49 −0 stdlib/2and3/lib2to3/pgen2/pgen.pyi
+73 −0 stdlib/2and3/lib2to3/pgen2/token.pyi
+30 −0 stdlib/2and3/lib2to3/pgen2/tokenize.pyi
+116 −0 stdlib/2and3/lib2to3/pygram.pyi
+86 −0 stdlib/2and3/lib2to3/pytree.pyi
+4 −2 stdlib/2and3/tarfile.pyi
+21 −21 stdlib/2and3/webbrowser.pyi
+7 −0 stdlib/3.4/asyncio/subprocess.pyi
+2 −1 stdlib/3/_importlib_modulespec.pyi
+3 −3 stdlib/3/_warnings.pyi
+0 −6 stdlib/3/builtins.pyi
+46 −0 stdlib/3/macpath.pyi
+46 −0 stdlib/3/ntpath.pyi
+27 −0 tests/mypy_selftest.py
+0 −1 tests/pytype_blacklist.txt
+0 −0 third_party/2and3/yaml/__init__.pyi
+0 −0 third_party/2and3/yaml/composer.pyi
+0 −0 third_party/2and3/yaml/constructor.pyi
+0 −0 third_party/2and3/yaml/dumper.pyi
+0 −0 third_party/2and3/yaml/emitter.pyi
+0 −0 third_party/2and3/yaml/error.pyi
+0 −0 third_party/2and3/yaml/events.pyi
+0 −0 third_party/2and3/yaml/loader.pyi
+0 −0 third_party/2and3/yaml/nodes.pyi
+0 −0 third_party/2and3/yaml/parser.pyi
+0 −0 third_party/2and3/yaml/reader.pyi
+0 −0 third_party/2and3/yaml/representer.pyi
+0 −0 third_party/2and3/yaml/resolver.pyi
+0 −0 third_party/2and3/yaml/scanner.pyi
+0 −0 third_party/2and3/yaml/serializer.pyi
+0 −0 third_party/2and3/yaml/tokens.pyi
0