Skip to content

Implement new NamedTuple syntax #2245

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

Merged
merged 5 commits into from
Oct 25, 2016
Merged
Show file tree
Hide file tree
Changes from 3 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
54 changes: 52 additions & 2 deletions mypy/semanal.py
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@
ImportFrom, ImportAll, Block, LDEF, NameExpr, MemberExpr,
IndexExpr, TupleExpr, ListExpr, ExpressionStmt, ReturnStmt,
RaiseStmt, AssertStmt, OperatorAssignmentStmt, WhileStmt,
ForStmt, BreakStmt, ContinueStmt, IfStmt, TryStmt, WithStmt, DelStmt,
ForStmt, BreakStmt, ContinueStmt, IfStmt, TryStmt, WithStmt, DelStmt, PassStmt,
GlobalDecl, SuperExpr, DictExpr, CallExpr, RefExpr, OpExpr, UnaryExpr,
SliceExpr, CastExpr, RevealTypeExpr, TypeApplication, Context, SymbolTable,
SymbolTableNode, BOUND_TVAR, UNBOUND_TVAR, ListComprehension, GeneratorExpr,
Expand All @@ -63,7 +63,7 @@
YieldFromExpr, NamedTupleExpr, NonlocalDecl, SymbolNode,
SetComprehension, DictionaryComprehension, TYPE_ALIAS, TypeAliasExpr,
YieldExpr, ExecStmt, Argument, BackquoteExpr, ImportBase, AwaitExpr,
IntExpr, FloatExpr, UnicodeExpr, EllipsisExpr,
IntExpr, FloatExpr, UnicodeExpr, EllipsisExpr, TempNode,
COVARIANT, CONTRAVARIANT, INVARIANT, UNBOUND_IMPORTED, LITERAL_YES,
)
from mypy.visitor import NodeVisitor
Expand Down Expand Up @@ -545,7 +545,57 @@ def check_function_signature(self, fdef: FuncItem) -> None:
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]]:
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

NAMEDTUP_CLASS_ERROR = 'Invalid statement in NamedTuple definition; ' \
Copy link
Member

Choose a reason for hiding this comment

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

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

'expected "field_name: field_type"'
if self.options.python_version < (3, 6):
self.fail('NamedTuple class syntax is only supported in Python 3.6', defn)
return [], []
if len(defn.base_type_exprs) > 1:
self.fail('NamedTuple should be a single base', defn)
items = [] # type: List[str]
types = [] # type: List[Type]
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@elazarg elazarg Oct 15, 2016

Choose a reason for hiding this comment

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

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]

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

for stmt in defn.defs.body:
if isinstance(stmt, AssignmentStmt):
if len(stmt.lvalues) > 1 or not isinstance(stmt.lvalues[0], NameExpr):
self.fail(NAMEDTUP_CLASS_ERROR, stmt)
continue
name = stmt.lvalues[0].name
if not isinstance(stmt.rvalue, TempNode):
# 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)
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

types.append(AnyType())
else:
types.append(self.anal_type(stmt.type))
if name.startswith('_'):
self.fail('NamedTuple field name cannot start with an underscore: {}'
.format(name), stmt)
items.append(name)
else:
if (not isinstance(stmt, PassStmt) and
not (isinstance(stmt, ExpressionStmt) and
isinstance(stmt.expr, EllipsisExpr))):
self.fail(NAMEDTUP_CLASS_ERROR, stmt)
Copy link
Member

Choose a reason for hiding this comment

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

Why can't namedtuples have methods? Subclasses can.

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

return items, types

def analyze_namedtuple_classdef(self, defn: ClassDef) -> None:
node = self.lookup(defn.name, defn)
Copy link
Member

Choose a reason for hiding this comment

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

Careful, this may return None.

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.

node.kind = GDEF # TODO in process_namedtuple_definition also applies here
items, types = self.check_namedtuple_classdef(defn)
node.node = self.build_namedtuple_typeinfo(defn.name, items, types)

def visit_class_def(self, defn: ClassDef) -> None:
# special case for NamedTuple
Copy link
Member

Choose a reason for hiding this comment

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

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

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.

for base_expr in defn.base_type_exprs:
if isinstance(base_expr, NameExpr):
Copy link
Member

Choose a reason for hiding this comment

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

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

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.

sym = self.lookup_qualified(base_expr.name, base_expr)
if sym is None or sym.node is None:
continue
if sym.node.fullname() == 'typing.NamedTuple':
self.analyze_namedtuple_classdef(defn)
return
self.clean_up_bases_and_infer_type_variables(defn)
self.setup_class_def_analysis(defn)

Expand Down
1 change: 1 addition & 0 deletions mypy/test/testcheck.py
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@
'check-warnings.test',
'check-async-await.test',
'check-newtype.test',
'check-class-namedtuple.test',
'check-columns.test',
]

Expand Down
Loading