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.

Sign up for GitHub

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

Allow subclassing NamedTuple as a nested class #4419

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
a295319
tests for local NamedTuple subclass (#4349)
elliott-beach Jan 2, 2018
dc180e7
Allow subclassing NamedTuple in function (#4349)
elliott-beach Jan 2, 2018
ea573e2
direct test for inner namedtuple
elliott-beach Jan 3, 2018
6eae524
add test for incremental namedtuple
elliott-beach Jan 14, 2018
abf5a0a
handle caching namedtuple
elliott-beach Jan 14, 2018
b398542
fix lint errors
elliott-beach Jan 14, 2018
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
2 changes: 1 addition & 1 deletion mypy/semanal.py
View file
Open in desktop
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 •
edited
Loading

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 •
edited
Loading

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 •
edited
Loading

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 •
edited
Loading

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
8 changes: 8 additions & 0 deletions test-data/unit/check-class-namedtuple.test
View file
Open in desktop
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)
42 changes: 42 additions & 0 deletions test-data/unit/semanal-namedtuple.test
View file
Open in desktop
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