Implement new NamedTuple syntax #2245

ilevkivskyi · 2016-10-13T14:44:00Z

Starting from Python 3.6b1, typing.NamedTuple supports the PEP 526 syntax:

from typing import NamedTuple

class Employee(NamedTuple):
    name: str
    salary: int

manager = Employee('John Doe', 9000)

Here is the support of this feature for mypy. It works only with --fast-parser, since it is required to parse PEP 526 syntax. Roughly half of the tests are simply adapted from tests for function NamedTuple syntax, plus another half are specific for the new syntax.

@gvanrossum Please take a look.

gvanrossum

Apart from the extensive test suite this was actually pretty easy! I have a bunch of suggestions here.

gvanrossum · 2016-10-15T02:05:08Z

mypy/semanal.py

    def visit_class_def(self, defn: ClassDef) -> None:
+        # special case for NamedTuple
+        for base_expr in defn.base_type_exprs:
+            if isinstance(base_expr, NameExpr):


Hm, this should probably also check for MemberExpr (similar to what's happening in check_namedtuple()).

gvanrossum · 2016-10-15T02:06:18Z

mypy/semanal.py

+        node.node = self.build_namedtuple_typeinfo(defn.name, items, types)
+
    def visit_class_def(self, defn: ClassDef) -> None:
+        # special case for NamedTuple


I'd refactor this into a helper function. Also it probably should be called after clean_up_bases_and_infer_type_variables().

gvanrossum · 2016-10-15T02:08:28Z

mypy/semanal.py

        elif len(sig.arg_types) > len(fdef.arguments):
            self.fail('Type signature has too many arguments', fdef, blocker=True)

+    def check_namedtuple_classdef(self, defn: ClassDef) -> Tuple[List[str], List[Type]]:


Believe it or not, mypy's (loose) convention for helper functions is that they follow the main (or only) call site.

Talking about placement, it would be nice to have it closer to other namedtuple-handling functions (~ 1600)

@elazarg 1600 is probably too far. I put it in the same place where other ClassDef helpers are (~ 750).

gvanrossum · 2016-10-15T02:11:55Z

mypy/semanal.py

+        return items, types
+
+    def analyze_namedtuple_classdef(self, defn: ClassDef) -> None:
+        node = self.lookup(defn.name, defn)


Careful, this may return None.

gvanrossum · 2016-10-15T02:16:42Z

mypy/semanal.py

            self.fail('Type signature has too many arguments', fdef, blocker=True)

+    def check_namedtuple_classdef(self, defn: ClassDef) -> Tuple[List[str], List[Type]]:
+        NAMEDTUP_CLASS_ERROR = 'Invalid statement in NamedTuple definition; ' \


Please use () for the line continuation instead of \.

gvanrossum · 2016-10-15T02:25:10Z

mypy/semanal.py

+        if len(defn.base_type_exprs) > 1:
+            self.fail('NamedTuple should be a single base', defn)
+        items = []  # type: List[str]
+        types = []  # type: List[Type]


It looks hard to prove (and preserve in future edits) that these have the same length. Maybe make a list of tuples and separate them at the end using list(zip(*x))?

I agree, and this should be uniform in other places (e.g. parse_namedtuple_fields_with_types). In my PR I did not refactor it, trying to stay somewhat closer to the original implementation, A namedtuple Field would add to readability: instead of returning Tuple[List[str], List[Type], bool] we can return Tuple[List[Field], bool]

list(zip(*x)) will require a special-casing for empty namedtuples, also mypy complains about its own code (probably because of not very precise stub for zip). I refactored this part, so that it is clear that the two lists have always equal length (there are only two appends now that stay next to each other) and added comments.

gvanrossum · 2016-10-15T02:25:47Z

mypy/semanal.py

+                if (not isinstance(stmt, PassStmt) and
+                    not (isinstance(stmt, ExpressionStmt) and
+                         isinstance(stmt.expr, EllipsisExpr))):
+                    self.fail(NAMEDTUP_CLASS_ERROR, stmt)


Why can't namedtuples have methods? Subclasses can.

This is because collections.namedtuple (and consequently typing.NamedTuple) does not support this. I was thinking about adding support for this in typing.py, but could not find a good solution (the main problem is support for super() in methods). Another argument is that named tuple is intended to be something simple (like record in some other languages).

gvanrossum · 2016-10-15T02:28:57Z

mypy/semanal.py

+                    # x: int assigns rvalue to TempNode(AnyType())
+                    self.fail('NamedTuple does not support initial values', stmt)
+                if stmt.type is None:
+                    self.fail('NamedTuple field type must be specified', stmt)


When you get this the previous fail() also triggers, right? I think that if someone writes x = 1 they deserve to get a single error that explains why that's not allowed -- they may not even realize you see this as an "initial value".

(Also, what happens when they write x = 1 # type: int ?)

OK. I changed this so that a single "generic" ... expected "field_name: field_type" error is displayed (also for x = 1 # type: int, since this will not work in typing.py). A more specific error Right hand side values are not supported in NamedTuple is displayed for x: int = 1 (the generic error could be not completely clear in this case).

ilevkivskyi · 2016-10-16T20:39:23Z

@gvanrossum I have pushed new commits with response to comments. Please take a look.

ilevkivskyi added 3 commits October 13, 2016 13:16

Add unit tests for new named tuple

98e5cab

Implement new NamedTuple syntax

409abb6

More checks and more tests

aa71fda

ilevkivskyi mentioned this pull request Oct 14, 2016

Alternative (simpler) repr for generics python/typing#296

Merged

gvanrossum requested changes Oct 15, 2016

View reviewed changes

ilevkivskyi added 2 commits October 16, 2016 21:26

First part of response to comments

a829987

Second part of responses to comments

3be0166

ilevkivskyi mentioned this pull request Oct 19, 2016

[needs tests] TypedDict: Recognize declaration of TypedDict('Point', {'x': int, 'y': int}). #2206

Merged

gvanrossum approved these changes Oct 25, 2016

View reviewed changes

gvanrossum merged commit ec5c476 into python:master Oct 25, 2016

Uh oh!

Implement new NamedTuple syntax #2245

Implement new NamedTuple syntax #2245

Uh oh!

Conversation

ilevkivskyi commented Oct 13, 2016

Uh oh!

gvanrossum left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elazarg Oct 15, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ilevkivskyi commented Oct 16, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

elazarg Oct 15, 2016 •

edited

Loading