Skip to content
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.

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

Runtime implementation of TypedDict extension #2552

Merged
merged 7 commits into from
Dec 14, 2016
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
1 change: 1 addition & 0 deletions extensions/__init__.py
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
# This page intentioanlly left blank.
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: intentioanlly -> intentionally

64 changes: 57 additions & 7 deletions extensions/mypy_extensions.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,15 +8,65 @@
# NOTE: This module must support Python 2.7 in addition to Python 3.x


def TypedDict(typename, fields):
"""TypedDict creates a dictionary type that expects all of its
import sys
from typing import _type_check # type: ignore
Copy link
Member Author

Choose a reason for hiding this comment

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

I am using a non-public API here, but probably it is better than just copy the whole function here.

Copy link
Member

Choose a reason for hiding this comment

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

Are you sure you want to depend on this internal API? It means that if in the future we change some version of typing.py it will break with this version of mypy. This may be acceptable given our plans here, but it's still a red flag...

Copy link
Member Author

Choose a reason for hiding this comment

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

I understand this is a bad idea to use the private API, this is why I added this comment #2552 (comment) But the function is really helpful, it takes care about forward references, callables, invalid types, etc. And it is not easy to reimplement this function here, since the implementation depends on other private typing API like _ForwardRef and _TypingBase. I think it is OK to keep this. What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

OK, it's fine to keep, but please add a comment to the code for posterity. (I also worry that before we know it this will become a de-facto public API -- I've seen others ask for the same functionality, e.g. in the issue asking to restore get_type_hint() for Python 2.)



def _check_fails(cls, other):
if sys._getframe(1).f_globals['__name__'] not in ['abc', 'functools']:
Copy link
Contributor

Choose a reason for hiding this comment

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

(1) Is there a reason that these particular modules are special-cased so that they do allow isinstance checks?

(2) Perhaps the call to sys._getframe() should be guarded when using a non-CPython implementation that doesn't support sys._getframe()? I see this is done in _TypedDictMeta.__new__ below.

Copy link
Member Author

Choose a reason for hiding this comment

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

(1) Yes, this is because there are several functions in these two modules that do "reckless" scan with isinstance and issubclass over .__subclasses__.
(2) Good catch. Fixed.

raise TypeError('TypedDict does not support instance and class checks')

class _TypedDictMeta(type):
def __new__(cls, name, bases, ns):
tp_dict = super(_TypedDictMeta, cls).__new__(cls, name, (dict,), ns)
try:
tp_dict.__module__ = sys._getframe(2).f_globals.get('__name__', '__main__')
except (AttributeError, ValueError):
pass
anns = ns.get('__annotations__', {})
msg = "TypedDict('Name', {f0: t0, f1: t1, ...}); each t must be a type"
anns = {n: _type_check(tp, msg) for n, tp in anns.items()}
for base in bases:
anns.update(base.__dict__.get('__annotations__', {}))
tp_dict.__annotations__ = anns
return tp_dict

__instancecheck__ = __subclasscheck__ = _check_fails


class _TypedDict(object):
"""A simple typed name space. At runtime it is equivalent to a plain dict.

TypedDict creates a dictionary type that expects all of its
instances to have a certain set of keys, with each key
associated with a value of a consistent type. This expectation
is not checked at runtime but is only enforced by typecheckers.
Usage::

Point2D = TypedDict('Point2D', {'x': int, 'y': int, 'label': str})
a: Point2D = {'x': 1, 'y': 2, 'label': 'good'} # OK
b: Point2D = {'z': 3, 'label': 'bad'} # Fails type check
assert Point2D(x=1, y=2, label='first') == dict(x=1, y=2, label='first')

The type info could be accessed via Point2D.__annotations__. TypedDict
supports two additional equivalent forms::

Point2D = TypedDict('Point2D', x=int, y=int, label=str)

class Point2D(TypedDict):
x: int
y: int
label: str

The latter syntax is only supported in Python 3.6+
Copy link
Member

Choose a reason for hiding this comment

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

Maybe add explicitly (as a reminder) that first two syntax forms works for Python 2(.7) and 3?

"""
def new_dict(*args, **kwargs):
return dict(*args, **kwargs)
def __new__(cls, _typename, fields=None, **kwargs):
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd suggest _fields as the parameter name (rather than fields) so that it doesn't clash with an attempt to use a dictionary item called "fields". This fulfills the desire that TypedDict should accept any item names that do not begin with an underscore.

Class = TypedDict('Class', name=str, fields=List[str], methods=List[str])

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

if fields is None:
fields = kwargs
elif kwargs:
raise TypeError("Either list of fields or keywords"
" can be provided to TypedDict, not both")
Copy link
Member

Choose a reason for hiding this comment

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

It's not a list but a dict, right? Anyway, I'd rewrite this error in the active tense, and without mentioning "fields" (which is questionable terminology here): "TypedDict takes either a dict or keyword arguments, but not both".

return cls.__class__(_typename, (), {'__annotations__': dict(fields)})


new_dict.__name__ = typename
new_dict.__supertype__ = dict
return new_dict
TypedDict = _TypedDictMeta('TypedDict', _TypedDict.__bases__, dict(_TypedDict.__dict__))
Copy link
Member

Choose a reason for hiding this comment

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

I'm guessing this awkward way of defining the class is to get around the different metaclass syntaxes for Python 2 and 3. But it makes my head explode trying to understand what's happening. I think that one of the problems is actually that TypedDict is itself a metaclass. (We see this when we realize that instances of TypedDict are really classes whose instances are dicts.) So maybe _TypedDict needs to be combined into TypedDictMeta? What tipped me off was that _TypedDict.__new__ ends up not calling object() or super().__new__() but cls.__class__(), which is really a fancy way of spelling TypedDictMeta().

103 changes: 103 additions & 0 deletions mypy/test/testextensions.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,103 @@
import sys
import pickle
from unittest import TestCase, main, skipUnless, SkipTest
from extensions.mypy_extensions import TypedDict
Copy link
Member

Choose a reason for hiding this comment

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

This line should somehow be from mypy_extensions import TypedDict, see my comment on __init__.py.



class BaseTestCase(TestCase):

def assertIsSubclass(self, cls, class_or_tuple, msg=None):
if not issubclass(cls, class_or_tuple):
message = '%r is not a subclass of %r' % (cls, class_or_tuple)
if msg is not None:
message += ' : %s' % msg
raise self.failureException(message)

def assertNotIsSubclass(self, cls, class_or_tuple, msg=None):
if issubclass(cls, class_or_tuple):
message = '%r is a subclass of %r' % (cls, class_or_tuple)
if msg is not None:
message += ' : %s' % msg
raise self.failureException(message)


PY36 = sys.version_info[:2] >= (3, 6)

PY36_TESTS = """
Label = TypedDict('Label', [('label', str)])

class Point2D(TypedDict):
x: int
y: int

class LabelPoint2D(Point2D, Label): ...
"""

if PY36:
exec(PY36_TESTS)


class TypedDictTests(BaseTestCase):

def test_basics_iterable_syntax(self):
# Check that two iterables allowed
Emp = TypedDict('Emp', [('name', str), ('id', int)])
Copy link
Contributor

Choose a reason for hiding this comment

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

I was under the impression that we weren't going to support the list of tuples syntax, based on python/typing#28 (comment) and python/typing#28 (comment) .

Copy link
Member Author

Choose a reason for hiding this comment

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

This is rather by accident. The implementation just takes an arbitrary iterable as an argument, e.g., a dictionary TypedDict('Employee', {'name': str, 'id': int}). List of tuples etc. also work. Basically everything that could be given to a normal dict works. Of course we could prohibit everything apart from a real dict, but I think that would be redundant. Anyway, I just removed this test, since I agree that it is unnecessary.

Emp = TypedDict('Emp', {'name': str, 'id': int})
self.assertIsSubclass(Emp, dict)
jim = Emp(name='Jim', id=1)
self.assertIsInstance(jim, Emp)
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this check fail? I thought we were going to prevent isinstance() checks with TypedDicts since they would true for any dict type. Indeed in test_typeddict_errors() below that seems to be the case.

Actually in rereading your implementation it looks like the runtime type of Emp(...) is actually an Emp rather than a plain dict, as the TypedDict doc comment says. I would advocate that even if Emp itself has __attributes__ and other nice introspection properties that instances still be plain dict objects. This is similar to how NewType works.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I remember I tried this, but it didn't give any visible speed-up. Current implementation is already slower than plain dict only by 10%, so that it is difficult to improve this. Anyway, probably you are right that it is better to make instances just dicts for consistency with other types. I did this in a new commit (this unfortunately required a bit more "magic" to be compatible with both Python 2 and 3, note some other magic already present is only because of different class machinery in Python 2 and 3).

self.assertIsInstance(jim, dict)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggest strengthen this check to:

self.assertTrue(type(jim) is dict)

Ditto for other occurrences below.

self.assertEqual(jim['name'], 'Jim')
self.assertEqual(jim['id'], 1)
self.assertEqual(Emp.__name__, 'Emp')
self.assertEqual(Emp.__bases__, (dict,))
self.assertEqual(Emp.__annotations__, {'name': str, 'id': int})

def test_basics_keywords_syntax(self):
Emp = TypedDict('Emp', name=str, id=int)
self.assertIsSubclass(Emp, dict)
jim = Emp(name='Jim', id=1)
self.assertIsInstance(jim, Emp)
self.assertIsInstance(jim, dict)
self.assertEqual(jim['name'], 'Jim')
self.assertEqual(jim['id'], 1)
self.assertEqual(Emp.__name__, 'Emp')
self.assertEqual(Emp.__bases__, (dict,))
self.assertEqual(Emp.__annotations__, {'name': str, 'id': int})

def test_typeddict_errors(self):
Emp = TypedDict('Emp', {'name': str, 'id': int})
with self.assertRaises(TypeError):
isinstance({}, Emp)
with self.assertRaises(TypeError):
issubclass(dict, Emp)
with self.assertRaises(TypeError):
TypedDict('Hi', x=1)
with self.assertRaises(TypeError):
TypedDict('Hi', [('x', int), ('y', 1)])
with self.assertRaises(TypeError):
TypedDict('Hi', [('x', int)], y=int)

@skipUnless(PY36, 'Python 3.6 required')
def test_class_syntax_usage(self):
Copy link
Member

Choose a reason for hiding this comment

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

Can you add "py36" or something similar to the test case name?

self.assertEqual(LabelPoint2D.__annotations__, {'x': int, 'y': int, 'label': str}) # noqa
Copy link
Member Author

Choose a reason for hiding this comment

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

I added # noqa because flake8 complains about undefined name.

self.assertEqual(LabelPoint2D.__bases__, (dict,)) # noqa
not_origin = Point2D(x=0, y=1) # noqa
self.assertEqual(not_origin['x'], 0)
self.assertEqual(not_origin['y'], 1)
other = LabelPoint2D(x=0, y=1, label='hi') # noqa
self.assertEqual(other['label'], 'hi')

def test_pickle(self):
global EmpD # pickle wants to reference the class by name
EmpD = TypedDict('EmpD', name=str, id=int)
jane = EmpD({'name': 'jane', 'id': 37})
for proto in range(pickle.HIGHEST_PROTOCOL + 1):
z = pickle.dumps(jane, proto)
jane2 = pickle.loads(z)
self.assertEqual(jane2, jane)
self.assertEqual(jane2, {'name': 'jane', 'id': 37})


if __name__ == '__main__':
main()
2 changes: 1 addition & 1 deletion runtests.py
Original file line number Diff line number Diff line change
Expand Up @@ -207,7 +207,7 @@ def add_imports(driver: Driver) -> None:


PYTEST_FILES = ['mypy/test/{}.py'.format(name) for name in [
'testcheck',
'testcheck', 'testextensions',
]]


Expand Down