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

Add support for NamedTuple methods #3081

Merged
merged 14 commits into from May 16, 2017
67 changes: 48 additions & 19 deletions mypy/semanal.py
Expand Up @@ -151,6 +151,11 @@
FUNCTION_FIRST_PHASE_POSTPONE_SECOND = 1 # Add to symbol table but postpone body
FUNCTION_SECOND_PHASE = 2 # Only analyze body

# Matches "_prohibited" in typing.py
NAMEDTUPLE_PROHIBITED_NAMES = ('__new__', '__init__', '__slots__', '__getnewargs__',
Copy link
Member

Choose a reason for hiding this comment

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

Maybe you don't need this? There's already an error when you define a field starting with underscore.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's only for fields, but we also need to disallow methods with these names.

'_fields', '_field_defaults', '_field_types',
'_make', '_replace', '_asdict')


class SemanticAnalyzer(NodeVisitor):
"""Semantically analyze parsed mypy files.
Expand Down Expand Up @@ -655,23 +660,40 @@ def visit_class_def(self, defn: ClassDef) -> None:
self.clean_up_bases_and_infer_type_variables(defn)
if self.analyze_typeddict_classdef(defn):
return
if self.analyze_namedtuple_classdef(defn):
# just analyze the class body so we catch type errors in default values
self.enter_class(defn)
named_tuple_info = self.analyze_namedtuple_classdef(defn)
if named_tuple_info is not None:
# temporarily clear the names dict so we don't get errors about duplicate names that
Copy link
Member

Choose a reason for hiding this comment

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

While you're at it I prefer comments to start with a capital letter and ended with proper punctuation (except for non-whole-sentence inline comments).

# were set in build_namedtuple_typeinfo
nt_names = named_tuple_info.names
named_tuple_info.names = SymbolTable()

self.bind_class_type_vars(named_tuple_info)
Copy link
Member

Choose a reason for hiding this comment

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

There are few old issues to enable generic named tuples, see e.g. #685
The fact that you are binding an empty symbol table here and then repopulating it later, may complicate the implementation of generic named tuples in future.

Copy link
Member Author

Choose a reason for hiding this comment

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

Generic namedtuples would be nice, but I'm not sure there's anything actionable here. Or should we approach support for methods in a different way?

Copy link
Member

Choose a reason for hiding this comment

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

Or should we approach support for methods in a different way?

I don't have a strong opinion here, but if there is another possible way in view, I would prefer not to trick SymbolTable.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed the save/restore trick feels unprincipled.

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 agree that it's not ideal but an alternative implementation would probably be more complicated.

self.enter_class(named_tuple_info)

defn.defs.accept(self)

self.leave_class()
self.unbind_class_type_vars()

# make sure we didn't use illegal names, then reset the names in the typeinfo
for prohibited in NAMEDTUPLE_PROHIBITED_NAMES:
if prohibited in named_tuple_info.names:
self.fail('Cannot overwrite NamedTuple attribute "{}"'.format(prohibited),
named_tuple_info.names[prohibited].node)

named_tuple_info.names.update(nt_names)
Copy link
Member

Choose a reason for hiding this comment

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

Could you assert here that the sets of keys merged here are distinct? ISTM if there's a key being overwritten by this update() call it's problematic.

Copy link
Member Author

Choose a reason for hiding this comment

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

The sets will actually always overlap, because build_namedtuple_typeinfo inserts names for the namedtuple fields, and visiting the class body will insert those names again. I think that's safe because build_namedtuple_typeinfo is very restrictive in the kind of statements it accepts within a class body, but I'll add some more tests to verify that it's correct (you already suggested some below).

else:
self.setup_class_def_analysis(defn)

self.bind_class_type_vars(defn)
self.bind_class_type_vars(defn.info)

self.analyze_base_classes(defn)
self.analyze_metaclass(defn)

for decorator in defn.decorators:
self.analyze_class_decorator(defn, decorator)

self.enter_class(defn)
self.enter_class(defn.info)

# Analyze class body.
defn.defs.accept(self)
Expand All @@ -683,13 +705,13 @@ def visit_class_def(self, defn: ClassDef) -> None:

self.unbind_class_type_vars()

def enter_class(self, defn: ClassDef) -> None:
def enter_class(self, info: TypeInfo) -> None:
# Remember previous active class
self.type_stack.append(self.type)
self.locals.append(None) # Add class scope
self.block_depth.append(-1) # The class body increments this to 0
self.postpone_nested_functions_stack.append(FUNCTION_BOTH_PHASES)
self.type = defn.info
self.type = info

def leave_class(self) -> None:
""" Restore analyzer state. """
Expand All @@ -698,14 +720,14 @@ def leave_class(self) -> None:
self.locals.pop()
self.type = self.type_stack.pop()

def bind_class_type_vars(self, defn: ClassDef) -> None:
def bind_class_type_vars(self, info: TypeInfo) -> None:
""" Unbind type variables of previously active class and bind
the type variables for the active class.
"""
if self.bound_tvars:
disable_typevars(self.bound_tvars)
self.tvar_stack.append(self.bound_tvars)
self.bound_tvars = self.bind_class_type_variables_in_symbol_table(defn.info)
self.bound_tvars = self.bind_class_type_variables_in_symbol_table(info)

def unbind_class_type_vars(self) -> None:
""" Unbind the active class' type vars and rebind the
Expand Down Expand Up @@ -882,7 +904,7 @@ def remove_dups(self, tvars: List[T]) -> List[T]:
all_tvars.remove(t)
return new_tvars

def analyze_namedtuple_classdef(self, defn: ClassDef) -> bool:
def analyze_namedtuple_classdef(self, defn: ClassDef) -> Optional[TypeInfo]:
# special case for NamedTuple
for base_expr in defn.base_type_exprs:
if isinstance(base_expr, RefExpr):
Expand All @@ -892,15 +914,17 @@ def analyze_namedtuple_classdef(self, defn: ClassDef) -> bool:
if node is not None:
node.kind = GDEF # TODO in process_namedtuple_definition also applies here
items, types, default_items = self.check_namedtuple_classdef(defn)
node.node = self.build_namedtuple_typeinfo(
info = self.build_namedtuple_typeinfo(
defn.name, items, types, default_items)
return True
return False
node.node = info
defn.info = info
return info
return None

def check_namedtuple_classdef(
self, defn: ClassDef) -> Tuple[List[str], List[Type], Dict[str, Expression]]:
NAMEDTUP_CLASS_ERROR = ('Invalid statement in NamedTuple definition; '
'expected "field_name: field_type"')
'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 [], [], {}
Expand All @@ -912,9 +936,11 @@ def check_namedtuple_classdef(
for stmt in defn.defs.body:
if not isinstance(stmt, AssignmentStmt):
# Still allow pass or ... (for empty namedtuples).
# Also allow methods.
if (not isinstance(stmt, PassStmt) and
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 rewrite this using positive logic -- if not X and not (Y and Z) and not Q is a little hard to understand.

not (isinstance(stmt, ExpressionStmt) and
isinstance(stmt.expr, EllipsisExpr))):
not (isinstance(stmt, ExpressionStmt) and
isinstance(stmt.expr, EllipsisExpr)) and
not isinstance(stmt, FuncBase)):
self.fail(NAMEDTUP_CLASS_ERROR, stmt)
elif len(stmt.lvalues) > 1 or not isinstance(stmt.lvalues[0], NameExpr):
# An assignment, but an invalid one.
Expand Down Expand Up @@ -2128,6 +2154,7 @@ def add_field(var: Var, is_initialized_in_class: bool = False,
add_field(Var('_field_types', dictype), is_initialized_in_class=True)
add_field(Var('_field_defaults', dictype), is_initialized_in_class=True)
add_field(Var('_source', strtype), is_initialized_in_class=True)
add_field(Var('__annotations__', ordereddictype), is_initialized_in_class=True)
Copy link
Member

Choose a reason for hiding this comment

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

This could be unrelated to this PR, but despite the fact that you define it here as ordereddicttype it is revealed as dict in tests. Also, __annotations__ is still just a dict (although it is ordered in 3.6+) and does not have OrderedDict methods like move_to_end().

Also I am not sure why you need it here. Latest typeshed stubs define object.__annotations__

Copy link
Member Author

Choose a reason for hiding this comment

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

ordereddictype and dictype are actually the same thing here (they're initialized the same way). Maybe I should just have cleaned up ordereddictype.

Copy link
Member Author

Choose a reason for hiding this comment

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

Also, mypy really does think that NamedTuples don't have __annotations__ if I remove this line.

Copy link
Member Author

Choose a reason for hiding this comment

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

__annotations__ is in fact an OrderedDict at runtime; see _make_nmtuple in typing. I tried to actually specify OrderedDict in the code here, but couldn't get it work, maybe because this code runs early enough in semantic analysis that we don't have other modules imported yet.

Copy link
Member

Choose a reason for hiding this comment

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

Also, mypy really does think that NamedTuples don't have __annotations__ if I remove this line.

This looks like a bug (although unrelated to this PR).


tvd = TypeVarDef('NT', 1, [], info.tuple_type)
selftype = TypeVarType(tvd)
Expand Down Expand Up @@ -3359,7 +3386,7 @@ def visit_class_def(self, cdef: ClassDef) -> None:
self.process_nested_classes(cdef)

def process_nested_classes(self, outer_def: ClassDef) -> None:
self.sem.enter_class(outer_def)
self.sem.enter_class(outer_def.info)
for node in outer_def.defs.body:
if isinstance(node, ClassDef):
node.info = TypeInfo(SymbolTable(), node, self.sem.cur_mod_id)
Expand Down Expand Up @@ -3488,8 +3515,10 @@ def visit_func_def(self, fdef: FuncDef) -> None:
self.errors.pop_function()

def visit_class_def(self, tdef: ClassDef) -> None:
for type in tdef.info.bases:
self.analyze(type)
# NamedTuple base classes are special; we don't have to check them again here
Copy link
Member

Choose a reason for hiding this comment

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

Call out in which function they are analyzed?

if not tdef.info.is_named_tuple:
for type in tdef.info.bases:
self.analyze(type)
# Recompute MRO now that we have analyzed all modules, to pick
# up superclasses of bases imported from other modules in an
# import loop. (Only do so if we succeeded the first time.)
Expand Down
76 changes: 75 additions & 1 deletion test-data/unit/check-class-namedtuple.test
Expand Up @@ -296,6 +296,7 @@ class X(NamedTuple):
reveal_type(X._fields) # E: Revealed type is 'Tuple[builtins.str, builtins.str]'
reveal_type(X._field_types) # E: Revealed type is 'builtins.dict[builtins.str, Any]'
reveal_type(X._field_defaults) # E: Revealed type is 'builtins.dict[builtins.str, Any]'
reveal_type(X.__annotations__) # E: Revealed type is 'builtins.dict[builtins.str, Any]'

[builtins fixtures/dict.pyi]

Expand Down Expand Up @@ -345,7 +346,7 @@ from typing import NamedTuple
class X(NamedTuple):
x: int
y = z = 2 # E: Invalid statement in NamedTuple definition; expected "field_name: field_type"
def f(self): pass # E: Invalid statement in NamedTuple definition; expected "field_name: field_type"
def f(self): pass

[case testNewNamedTupleWithInvalidItems2]
# flags: --python-version 3.6
Expand Down Expand Up @@ -482,3 +483,76 @@ Y(y=1, x='1').method()
class CallsBaseInit(X):
def __init__(self, x: str) -> None:
super().__init__(x)

[case testNewNamedTupleWithMethods]
# flags: --python-version 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.

Do you still need this flag? IIRC mypy now defaults to 3.6 everywhere?

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 don't feel strongly about it, but it seems mildly useful to call out that this code will only work in 3.6 and not earlier versions.

Copy link
Member

Choose a reason for hiding this comment

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

But we're not doing consistently, so I don't think it adds value.

from typing import NamedTuple

class XMeth(NamedTuple):
x: int
def double(self) -> int:
return self.x
async def asyncdouble(self) -> int:
return self.x

class XRepr(NamedTuple):
x: int
y: int = 1
def __str__(self) -> str:
return 'string'
def __add__(self, other: XRepr) -> int:
return 0

reveal_type(XMeth(1).double()) # E: Revealed type is 'builtins.int'
reveal_type(XMeth(1).asyncdouble()) # E: Revealed type is 'typing.Awaitable[builtins.int]'
reveal_type(XMeth(42).x) # E: Revealed type is 'builtins.int'
reveal_type(XRepr(42).__str__()) # E: Revealed type is 'builtins.str'
reveal_type(XRepr(1, 2).__add__(XRepr(3))) # E: Revealed type is 'builtins.int'

[case testNewNamedTupleMethodInheritance]
# flags: --python-version 3.6
from typing import NamedTuple, TypeVar

T = TypeVar('T')

class Base(NamedTuple):
x: int
def copy(self: T) -> T:
Copy link
Member

Choose a reason for hiding this comment

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

I would test another recently added feature: overloaded methods.

return self
def good_override(self) -> int:
return self.x
def bad_override(self) -> int:
return self.x

class Child(Base):
def new_method(self) -> int:
return self.x
Copy link
Member

Choose a reason for hiding this comment

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

I would add tests checking for self[0] inside a method, its type, access, and error on assignment.

def good_override(self) -> int:
return 0
def bad_override(self) -> str: # E: Return type of "bad_override" incompatible with supertype "Base"
return 'incompatible'

def takes_base(base: Base) -> int:
return base.x

reveal_type(Base(1).copy()) # E: Revealed type is 'Tuple[builtins.int, fallback=__main__.Base]'
reveal_type(Child(1).copy()) # E: Revealed type is 'Tuple[builtins.int, fallback=__main__.Child]'
reveal_type(Base(1).good_override()) # E: Revealed type is 'builtins.int'
reveal_type(Child(1).good_override()) # E: Revealed type is 'builtins.int'
reveal_type(Base(1).bad_override()) # E: Revealed type is 'builtins.int'
reveal_type(takes_base(Base(1))) # E: Revealed type is 'builtins.int'
reveal_type(takes_base(Child(1))) # E: Revealed type is 'builtins.int'

[case testNewNamedTupleIllegalNames]
# flags: --python-version 3.6
from typing import NamedTuple

class XMethBad(NamedTuple):
x: int
def _fields(self): # E: Cannot overwrite NamedTuple attribute "_fields"
return 'no chance for this'

class MagicalFields(NamedTuple):
x: int
def __slots__(self) -> None: ... # E: Cannot overwrite NamedTuple attribute "__slots__"
def __new__(cls) -> None: ... # E: Cannot overwrite NamedTuple attribute "__new__"
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 like to see a test that shows you can't have both a method and a field with the same name (the update() call I pointed out made me nervous about that).