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

Allow subclassing NamedTuple as a nested class #4419

Closed
wants to merge 6 commits into from
Closed
Show file tree
Hide file tree
Changes from all 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
18 changes: 15 additions & 3 deletions mypy/semanal.py
Original file line number Diff line number Diff line change
Expand Up @@ -671,6 +671,7 @@ def analyze_class_body(self, defn: ClassDef) -> Iterator[bool]:
if self.analyze_typeddict_classdef(defn):
yield False
return
self.setup_class_def_analysis(defn)
Copy link
Collaborator

@emmatyping emmatyping Jan 7, 2018

Choose a reason for hiding this comment

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

While this works, I'm not sure if this is the best change that could be made. As we can see in analyze_namedtuple_classdef:
(referencing this TODO),
the root of the issue is that the analysis and processing of namedtuples is unconditionally global. If I understand things correctly, your change adds the node early to the symbol table so it is coerced into being a local definition by setup_class_def_analysis. That works, but is rather brittle.

Copy link
Contributor Author

@elliott-beach elliott-beach Jan 7, 2018

Choose a reason for hiding this comment

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

I agree that this is incomplete because we need to handle local definitions of NamedTuple in a few other places, but I would still argue that we should call setup_class_def_analysis for NamedTuples in this way. Local classes are added to the symbol table in setup_class_def_analysis, and there is no reason not to add local NamedTuples at that point as well.

This doesn't seem early to me because semantic analysis appears to me to proceed in two steps of (1) adding nodes with basic info to the symbol table and then (2) adding more complex type info. I think here is the right place to do step (1) for NamedTuples.

Copy link
Contributor Author

@elliott-beach elliott-beach Jan 7, 2018

Choose a reason for hiding this comment

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

The TODO comments by @JukkaL appear to be referring to assigning node.kind as GDEF or LDEF, which I think would be simple to fix.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Okay I think this is acceptable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great, Let me fix the todos.

Copy link
Contributor Author

@elliott-beach elliott-beach Jan 12, 2018

Choose a reason for hiding this comment

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

Actually, my understanding is LDEF and GDEF for NamedTuples, TypedDicts, and Enums is a longstanding issue that is not the cause of the specific problem this PR is solving, and I am not too confident on the solution. So I won't address those.

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
Expand Down Expand Up @@ -704,7 +705,6 @@ def analyze_class_body(self, defn: ClassDef) -> Iterator[bool]:
if key not in named_tuple_info.names or key != '__doc__'
})
else:
self.setup_class_def_analysis(defn)
self.analyze_base_classes(defn)
self.analyze_metaclass(defn)
defn.info.is_protocol = is_protocol
Expand Down Expand Up @@ -956,6 +956,10 @@ def get_all_bases_tvars(self, defn: ClassDef, removed: List[int]) -> TypeVarList
tvars.extend(base_tvars)
return remove_dups(tvars)

def is_namedtuple_classdef(self, defn: ClassDef) -> bool:
base_exprs = defn.base_type_exprs
return any(getattr(b, 'fullname', None) == 'typing.NamedTuple' for b in base_exprs)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This duplicates similar logic in analyze_namedtuple_classdef. There should be only one implementation of looking for a typing.NamedTuple base class.

Style nit: We avoid the use of getattr if there's a reasonable way around it, since it can't be type checked.


def analyze_namedtuple_classdef(self, defn: ClassDef) -> Optional[TypeInfo]:
# special case for NamedTuple
for base_expr in defn.base_type_exprs:
Expand All @@ -964,10 +968,14 @@ def analyze_namedtuple_classdef(self, defn: ClassDef) -> Optional[TypeInfo]:
if base_expr.fullname == 'typing.NamedTuple':
node = self.lookup(defn.name, defn)
if node is not None:
if self.type or self.is_func_scope():
name = defn.name + '@' + str(defn.line)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The name has to be mangled so that, for incremental mode, it matches the name of type (but it makes sense anyway so it does not leak outside the containing scope).

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is different from how we do name mangling for other kinds of types, such as enums. We should only do it in a function scope.

else:
name = defn.name
node.kind = GDEF # TODO in process_namedtuple_definition also applies here
items, types, default_items = self.check_namedtuple_classdef(defn)
info = self.build_namedtuple_typeinfo(
defn.name, items, types, default_items)
name, items, types, default_items)
node.node = info
defn.info.replaced = info
defn.info = info
Expand Down Expand Up @@ -1046,7 +1054,11 @@ def setup_class_def_analysis(self, defn: ClassDef) -> None:
local_name = defn.info._fullname + '@' + str(defn.line)
defn.info._fullname = self.cur_mod_id + '.' + local_name
defn.fullname = defn.info._fullname
self.globals[local_name] = node
if self.type and self.is_namedtuple_classdef(defn):
Copy link
Collaborator

Choose a reason for hiding this comment

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

This special casing of named tuples here is kind ugly. Why do we need this?

Copy link
Contributor Author

@elliott-beach elliott-beach Feb 8, 2018

Choose a reason for hiding this comment

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

The problem is that, inside a class method, local namedtuples are stored under the classes's symbol table (and are expected to be stored there in other parts of the code) but local classes are stored under the global symbol table. Storing subclasses of namedtuple in the global table breaks incremental mode.

As you suggested a little refactoring might be more effective than this haphazard fix, I would suggest storing local classes under the class symbol table. Then we wouldn't need this. IIRC local TypedDicts and other special types (???) are also stored in the class symbol table, so it be most consistent to always store local classes under the local symbol table.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I went a little it more into the technical details in #4419 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would be delighted to make that change if you agree.

# Special case for NamedTuple.
self.type.names[local_name] = node
else:
self.globals[local_name] = node

def analyze_base_classes(self, defn: ClassDef) -> None:
"""Analyze and set up base classes.
Expand Down
8 changes: 8 additions & 0 deletions test-data/unit/check-class-namedtuple.test
Original file line number Diff line number Diff line change
Expand Up @@ -667,3 +667,11 @@ class HasStaticMethod(NamedTuple):
return 4

[builtins fixtures/property.pyi]

[case testNamedTupleLocalScope]
from typing import NamedTuple, Any, Tuple

def function() -> Tuple:
class InnerNamedTuple(NamedTuple):
x: int
return InnerNamedTuple(x=0)
15 changes: 15 additions & 0 deletions test-data/unit/check-incremental.test
Original file line number Diff line number Diff line change
Expand Up @@ -1925,6 +1925,21 @@ tmp/crash.py:17: error: Revealed type is 'crash.B@13[builtins.int*]'
main:2: error: Revealed type is 'crash.A@5'
main:3: error: Revealed type is 'crash.B@13[builtins.int*]'

[case testIncrementalSubclassNamedTuple]
from a import C
reveal_type(C().a)
[file a.py]
from typing import NamedTuple
class C:
def __init__(self) -> None:
class A(NamedTuple):
x: int
self.a = A(1)
[out1]
main:2: error: Revealed type is 'Tuple[builtins.int, fallback=a.C.A@4]'
[out2]
main:2: error: Revealed type is 'Tuple[builtins.int, fallback=a.C.A@4]'

[case testGenericMethodRestoreMetaLevel]
from typing import Dict

Expand Down
42 changes: 42 additions & 0 deletions test-data/unit/semanal-namedtuple.test
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,48 @@ MypyFile:1(
__main__.N@2)
PassStmt:2()))

[case testNamedTupleDirectSubclass]
from typing import NamedTuple
class A(NamedTuple):
x: int
Copy link
Collaborator

Choose a reason for hiding this comment

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

Style nit: indent should be 4 spaces.


[out]
MypyFile:1(
ImportFrom:1(typing, [NamedTuple])
ClassDef:2(
A
TupleType(
Tuple[builtins.int])
BaseType(
builtins.tuple[builtins.int])
AssignmentStmt:3(
NameExpr(x [m])
TempNode:-1(
Any)
builtins.int)))

[case testLocalNamedTupleDirectSubclass]
from typing import NamedTuple
def foo():
class A(NamedTuple):
x: int
Copy link
Collaborator

Choose a reason for hiding this comment

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

Style nit: indent should be 4 spaces.

[out]
MypyFile:1(
ImportFrom:1(typing, [NamedTuple])
FuncDef:2(
foo
Block:2(
ClassDef:3(
A
TupleType(
Tuple[builtins.int])
BaseType(
builtins.tuple[builtins.int])
AssignmentStmt:4(
NameExpr(x [m])
TempNode:-1(
Any)
builtins.int)))))
-- Errors

[case testNamedTupleWithTooFewArguments]
Expand Down