Prohibit list[int], etc (those fail at runtime) #2869

ilevkivskyi · 2017-02-15T13:16:03Z

All of the following are now allowed, but fail at runtime:

list[int]
dict[int, str]
set[int]
tuple[int]
frozenset[int]
enumerate[int]
collections.defaultdict[int, str]
collections.Counter[str]
collections.ChainMap[str, str]

I prohibit those by simply tracking whether a corresponding symbol table node was normalized or not.

@gvanrossum I make an exclusion for stubs, because a have found dozens of places where this is used in typeshed, if you think that it also makes sense to prohibit this in stubs, then I will make an additional PR to typeshed.

JelleZijlstra · 2017-02-15T15:18:32Z

mypy/nodes.py

@@ -2143,17 +2152,20 @@ class SymbolTableNode:
    # For deserialized MODULE_REF nodes, the referenced module name;
    # for other nodes, optionally the name of the referenced object.
    cross_ref = None  # type: Optional[str]
+    # Was this node created by normaloze_type_alias?


Thanks! Fixed.

JelleZijlstra · 2017-02-15T15:24:00Z

Would this approach also allow you to fix #999? Jukka said before that these two should be closely related. I tried a little while ago to fix #999 but couldn't find a good way to do it.

gvanrossum · 2017-02-15T16:10:44Z

Actually #999 was fixed yesterday by #2863.

JelleZijlstra · 2017-02-15T16:13:41Z

Oh, that's great! Sorry for missing that.

gvanrossum · 2017-02-15T17:51:53Z

Thanks! This found tons of bad annotations in our internal codebase.

FYI I am currently in a state where I can still write code but cannot focus long enough to review anything more complex than simple typeshed fixes. Hopefully this is not a permanent condition, but I expect it to persist for a few weeks...

ilevkivskyi · 2017-02-15T23:09:38Z

Thanks! This found tons of bad annotations in our internal codebase.
FYI I am currently in a state where I can still write code but cannot focus long enough to review anything more complex than simple typeshed fixes. Hopefully this is not a permanent condition, but I expect it to persist for a few weeks...

I am glad this is helpful. I hope you get better soon.

…to no-tuple-int

ilevkivskyi · 2017-03-05T22:10:14Z

@gvanrossum It turns out mypy itself uses collections.deque[State] in build.py.

ambv · 2017-03-10T10:03:35Z

Hold on.

What's the value added from making this change? The "failure at runtime" in this case is actually "failure at import" which is pretty obvious to the user.

In fact, I've been quietly hoping that once type checks get uncontroversial enough, we can revisit the ideas for Python 3.7 to either:

remove annotations at runtime altogether; or
make them all strings (listed as a possible future enhancement in PEP 484).

In both cases, we could start using generics on builtin collections without requiring runtime changes to the relevant C code. This makes for a big improvement in how obvious type hints look like to the reader. The proliferation of this problem in typeshed, internal codebases using type checks, proves that this is a problem. I'm getting support requests around this sometimes, and sometimes I myself make mistakes (either making a lowercase dict or an uppercase Str!).

So, I'll open a new issue on python/typing about the 3.7 enhancement. In the mean time, can we defer merging this?

ilevkivskyi · 2017-03-10T10:23:58Z

@ambv
There are still two places where defaultdict[int, str] etc. will fail at runtime even if annotations are strings - base class list and instantiation:

from collections import defaultdict, Counter, ChainMap
from typing import Any

class MyDDict(defaultdict[int, str]): # Fails at runtime, mypy is silent
    ...
class CustomList(list[int]): # Same here, fails at runtime, mypy is silent
    ...

c = Counter[int]() # Same situation here
m = ChainMap[str, Any]() # ... and here

In principle I am in favour of making annotations just strings (but rather via a flag or __future__ import), but still I would like mypy to warn about the above code. If such code appears inside functions it will be "failure late after import".

This PR will make all these errors, I think it is reasonable.

ambv · 2017-03-10T10:47:18Z

Suggestion reported as python/typing#400. Nice round number.

Your examples are valid concerns. Especially your generic base class examples are tricky. He we will require users to continue using the typing variants of collections even if python/typing#400 gets implemented.

Regarding your last two examples, mypy should warn here too but ultimately they are solved by using variable annotations or type comments. The suggestion in PEP 484 to instantiate like that needs revision, we should probably discourage further use of that syntax. It's confusing to users why DefaultDict[int, bytes]() is okay but List[int]() is invalid.

Does it make sense for your PR to only focus on raising errors for those cases?

ilevkivskyi · 2017-03-10T11:04:36Z

we should probably discourage further use of that syntax

DefaultDict[int, bytes]() is just a "syntactic sugar" to avoid duplicating the long name. I think PEP 484 should not prohibit this, the user could decide what to use, the shorthand form or the full form:

var: DefaultDict[int, str] = defaultdict()

The PEP already makes the full form preferable: "It is not recommended to use the subscripted class (e.g. Node[int]) directly in an expression".

Does it make sense for your PR to only focus on raising errors for those cases?

It is a bit tricky but possible. However, I think it could be surprising for a user why this is flagged in some places but not in others. Also Jukka have pointed few more places where this will fail: in cast, in TypeVar, and in NewType.

gvanrossum · 2017-03-10T20:09:17Z

I really want this to land. It's the right thing from the PEP's POV -- writing list[int] instead of List[int] is just bad form. This has long been on my list of mypy annoyances.

I'm going to review the code now and unless I see something terrible I will merge it.

Re: DefaultDict etc., I think we should really stick to our guns and disallow instantiation just like we disallow it for List etc., the reasons are the same -- these are generic "aliases" for concrete implementations (and that sets them apart from user-defined generic classes like Node as well as ABC's like Sequence). But that doesn't need to happen in the same PR.

gvanrossum

Just some nits, really.

gvanrossum · 2017-03-10T20:15:06Z

mypy/semanal.py

@@ -1283,13 +1286,19 @@ def process_import_over_existing_name(self,

    def normalize_type_alias(self, node: SymbolTableNode,
                             ctx: Context) -> SymbolTableNode:
+        normalized = False
        if node.fullname in type_aliases:


This is the only place I can find where frozenset is treated differently -- do you understand why?

gvanrossum · 2017-03-10T20:16:26Z

mypy/nodes.py

+                                  collections_type_aliases.items())  # type: Dict[str, str]
+
+nongen_builtins = {'builtins.tuple': 'typing.Tuple',
+                   'builtins.frozenset': 'typing.FrozenSet',


I'm curious why this isn't in type_aliases above. How is frozenset different from set?

Oh, it looks like an old oversight. I just checked stubs for builtins.frozenset and typing.FrozenSet, the former is much better (it has all actual methods). I can easily fix this, but this requires a tiny PR to typeshed to be merged first and synced python/typeshed#979

Could you please merge it?

gvanrossum · 2017-03-10T20:20:17Z

test-data/unit/check-generics.test

+[case testNoSubscriptionOfBuiltinAliases]
+from typing import List, TypeVar
+
+list[int]() # E: "list" is not subscriptable, use "typing.List" instead


This is somewhat sad, because if you follow the instructions in the message, you get List[int]() which elicits a different error ("Type List cannot be instantiated; use list() instead"), but at runtime.

gvanrossum · 2017-03-10T20:21:25Z

test-data/unit/check-generics.test

+reveal_type(fun())  # E: Revealed type is 'builtins.list[builtins.int]'
+
+BuiltinAlias = list
+BuiltinAlias[int]() # E: "list" is not subscriptable, use "typing.List" instead


gvanrossum · 2017-03-10T20:24:23Z

test-data/unit/pythoneval.test

+_program.py:5: error: "defaultdict" is not subscriptable, use "typing.DefaultDict" instead
+_program.py:6: error: "Counter" is not subscriptable, use "typing.Counter" instead
+_program.py:9: error: "defaultdict" is not subscriptable, use "typing.DefaultDict" instead
+_program.py:12: error: Invalid index type "int" for "dict"; expected type "str"


Hm, why does this refer to dict instead of to DefaultDict?

gvanrossum · 2017-03-13T22:58:04Z

As I am chugging through our internal codebase fixing the issues found here, I've noticed that the most common is defaultdict -- presumably because (a) that's a pretty useful data type, and (b) older versions of typing.py don't have it.

ilevkivskyi · 2017-03-13T23:39:05Z

I have made a PR to typeshed fixing some stubs python/typeshed#993

In the meantime I have found something more, something like this

from collections.abc import Iterable

it: Iterable[int]

is not flagged even with this PR. I understand the reason, but it is a bit more tedious to fix. I propose to make it in a separate PR.

gvanrossum · 2017-03-14T22:30:13Z

Merged! I am about halfway through changing our internal codebase and have not found additional complications.

ilevkivskyi · 2017-03-14T22:33:09Z

Thanks! I will open a separate issue about collections.abc.Iterable[int] (I don't think it is a priority, just not to forget about it).

gvanrossum · 2017-03-15T04:57:11Z

Hm. There's another issue, which is that this introduces the use of TYPE_CHECKING in mypy itself. This is unfortunately a problem with our internal installation that I can't easily fix, and we've in the past worked around this (using MYPY = False; if MYPY: ...).

ilevkivskyi · 2017-03-15T07:45:50Z

This is really a temporary measure until the new version of typing (with Deque) is on PyPI. Normally it was released around the same time as the next Python version.

Python 3.6.1 final is scheduled for Monday, but on the other hand we don't plan to make any changes to typing until that time (e.g. NoReturn will go into 3.6.2). So you could make the PyPI release sooner if necessary, and then I will make a PR removing if TYPE_CHECKING: ....

gvanrossum · 2017-03-15T15:06:42Z

No, the use of if TYPE_CHECKING itself inside mypy is the problem. We really need that to go back to if MYPY. I'll do it.

gvanrossum · 2017-03-15T16:21:54Z

Also the reason (which I suddenly recall) is that we're running mypy with Python 3.5.1 which forces one to use the stdlib typing.py which doesn't even have TYPE_CHECKING.

gvanrossum · 2017-03-15T16:24:29Z

Fix in #3008

Ivan Levkivskyi and others added 4 commits February 14, 2017 22:59

Inial ideas

46ad58b

Prohibit unnormalized except in stubs

0673935

Be careful with user defined aliases, refactor common code + tests

13d7067

Fix tests

66f1a1d

JelleZijlstra reviewed Feb 15, 2017

View reviewed changes

ilevkivskyi added 2 commits February 15, 2017 23:57

Fix typo; add two tests

4fe5399

One more typo in test

529a963

ilevkivskyi added 6 commits February 28, 2017 21:52

Merge branch 'master' into no-tuple-int

0746f87

Fix a 'typo'

18b0cd9

Merge branch 'no-tuple-int' of https://github.com/ilevkivskyi/mypy in…

504a324

…to no-tuple-int

Fix line numbers in test

8591a7c

Merge branch 'master' into no-tuple-int

4ac37f4

Simplify code following merging PR 2845, fix Deque

c227b11

Merge branch 'master' into no-tuple-int

f011425

JukkaL mentioned this pull request Mar 10, 2017

Type annotation erasure at compile time python/typing#400

Closed

gvanrossum reviewed Mar 10, 2017

View reviewed changes

ilevkivskyi added 3 commits March 10, 2017 23:08

Response to review

177173f

Merge remote-tracking branch 'upstream/master' into no-tuple-int

7d7e8ca

Simple logic for proposing alternatives

cc5fb0e

ilevkivskyi mentioned this pull request Mar 13, 2017

Fix non-subscriptable type aliases python/typeshed#993

Merged

gvanrossum merged commit 4ca67f9 into python:master Mar 14, 2017

gvanrossum mentioned this pull request May 23, 2017

Make deque subscriptable? #3413

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prohibit list[int], etc (those fail at runtime) #2869

Prohibit list[int], etc (those fail at runtime) #2869

ilevkivskyi commented Feb 15, 2017

JelleZijlstra Feb 15, 2017

ilevkivskyi Feb 15, 2017

JelleZijlstra commented Feb 15, 2017

gvanrossum commented Feb 15, 2017

JelleZijlstra commented Feb 15, 2017

gvanrossum commented Feb 15, 2017

ilevkivskyi commented Feb 15, 2017

ilevkivskyi commented Mar 5, 2017

ambv commented Mar 10, 2017

ilevkivskyi commented Mar 10, 2017 •

edited

Loading

ambv commented Mar 10, 2017

ilevkivskyi commented Mar 10, 2017

gvanrossum commented Mar 10, 2017

gvanrossum left a comment

gvanrossum Mar 10, 2017

gvanrossum Mar 10, 2017

ilevkivskyi Mar 10, 2017

gvanrossum Mar 10, 2017

gvanrossum Mar 10, 2017

gvanrossum Mar 10, 2017

gvanrossum commented Mar 13, 2017

ilevkivskyi commented Mar 13, 2017

gvanrossum commented Mar 14, 2017

ilevkivskyi commented Mar 14, 2017

gvanrossum commented Mar 15, 2017

ilevkivskyi commented Mar 15, 2017

gvanrossum commented Mar 15, 2017

gvanrossum commented Mar 15, 2017

gvanrossum commented Mar 15, 2017

Prohibit list[int], etc (those fail at runtime) #2869

Prohibit list[int], etc (those fail at runtime) #2869

Conversation

ilevkivskyi commented Feb 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JelleZijlstra commented Feb 15, 2017

gvanrossum commented Feb 15, 2017

JelleZijlstra commented Feb 15, 2017

gvanrossum commented Feb 15, 2017

ilevkivskyi commented Feb 15, 2017

ilevkivskyi commented Mar 5, 2017

ambv commented Mar 10, 2017

ilevkivskyi commented Mar 10, 2017 • edited Loading

ambv commented Mar 10, 2017

ilevkivskyi commented Mar 10, 2017

gvanrossum commented Mar 10, 2017

gvanrossum 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

gvanrossum commented Mar 13, 2017

ilevkivskyi commented Mar 13, 2017

gvanrossum commented Mar 14, 2017

ilevkivskyi commented Mar 14, 2017

gvanrossum commented Mar 15, 2017

ilevkivskyi commented Mar 15, 2017

gvanrossum commented Mar 15, 2017

gvanrossum commented Mar 15, 2017

gvanrossum commented Mar 15, 2017

ilevkivskyi commented Mar 10, 2017 •

edited

Loading