-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
[needs tests] TypedDict: Recognize declaration of TypedDict('Point', {'x': int, 'y': int}). #2206
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
Changes from all commits
4cc4dc9
a2012fd
2189f58
33f6591
d5fadd0
18c1f91
56f901c
f995b45
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -60,7 +60,7 @@ | |
FuncExpr, MDEF, FuncBase, Decorator, SetExpr, TypeVarExpr, NewTypeExpr, | ||
StrExpr, BytesExpr, PrintStmt, ConditionalExpr, PromoteExpr, | ||
ComparisonExpr, StarExpr, ARG_POS, ARG_NAMED, MroError, type_aliases, | ||
YieldFromExpr, NamedTupleExpr, NonlocalDecl, SymbolNode, | ||
YieldFromExpr, NamedTupleExpr, TypedDictExpr, NonlocalDecl, SymbolNode, | ||
SetComprehension, DictionaryComprehension, TYPE_ALIAS, TypeAliasExpr, | ||
YieldExpr, ExecStmt, Argument, BackquoteExpr, ImportBase, AwaitExpr, | ||
IntExpr, FloatExpr, UnicodeExpr, EllipsisExpr, | ||
|
@@ -1127,6 +1127,7 @@ def visit_assignment_stmt(self, s: AssignmentStmt) -> None: | |
self.process_newtype_declaration(s) | ||
self.process_typevar_declaration(s) | ||
self.process_namedtuple_definition(s) | ||
self.process_typeddict_definition(s) | ||
|
||
if (len(s.lvalues) == 1 and isinstance(s.lvalues[0], NameExpr) and | ||
s.lvalues[0].name == '__all__' and s.lvalues[0].kind == GDEF and | ||
|
@@ -1498,9 +1499,9 @@ def get_typevar_declaration(self, s: AssignmentStmt) -> Optional[CallExpr]: | |
if not isinstance(s.rvalue, CallExpr): | ||
return None | ||
call = s.rvalue | ||
if not isinstance(call.callee, RefExpr): | ||
return None | ||
callee = call.callee | ||
if not isinstance(callee, RefExpr): | ||
return None | ||
if callee.fullname != 'typing.TypeVar': | ||
return None | ||
return call | ||
|
@@ -1579,10 +1580,9 @@ def process_namedtuple_definition(self, s: AssignmentStmt) -> None: | |
# Yes, it's a valid namedtuple definition. Add it to the symbol table. | ||
node = self.lookup(name, s) | ||
node.kind = GDEF # TODO locally defined namedtuple | ||
# TODO call.analyzed | ||
node.node = named_tuple | ||
|
||
def check_namedtuple(self, node: Expression, var_name: str = None) -> TypeInfo: | ||
def check_namedtuple(self, node: Expression, var_name: str = None) -> Optional[TypeInfo]: | ||
"""Check if a call defines a namedtuple. | ||
|
||
The optional var_name argument is the name of the variable to | ||
|
@@ -1596,9 +1596,9 @@ def check_namedtuple(self, node: Expression, var_name: str = None) -> TypeInfo: | |
if not isinstance(node, CallExpr): | ||
return None | ||
call = node | ||
if not isinstance(call.callee, RefExpr): | ||
return None | ||
callee = call.callee | ||
if not isinstance(callee, RefExpr): | ||
return None | ||
fullname = callee.fullname | ||
if fullname not in ('collections.namedtuple', 'typing.NamedTuple'): | ||
return None | ||
|
@@ -1607,9 +1607,9 @@ def check_namedtuple(self, node: Expression, var_name: str = None) -> TypeInfo: | |
# Error. Construct dummy return value. | ||
return self.build_namedtuple_typeinfo('namedtuple', [], []) | ||
else: | ||
# Give it a unique name derived from the line number. | ||
name = cast(StrExpr, call.args[0]).value | ||
if name != var_name: | ||
# Give it a unique name derived from the line number. | ||
name += '@' + str(call.line) | ||
info = self.build_namedtuple_typeinfo(name, items, types) | ||
# Store it as a global just in case it would remain anonymous. | ||
|
@@ -1620,7 +1620,7 @@ def check_namedtuple(self, node: Expression, var_name: str = None) -> TypeInfo: | |
|
||
def parse_namedtuple_args(self, call: CallExpr, | ||
fullname: str) -> Tuple[List[str], List[Type], bool]: | ||
# TODO Share code with check_argument_count in checkexpr.py? | ||
# TODO: Share code with check_argument_count in checkexpr.py? | ||
args = call.args | ||
if len(args) < 2: | ||
return self.fail_namedtuple_arg("Too few arguments for namedtuple()", call) | ||
|
@@ -1777,6 +1777,114 @@ def analyze_types(self, items: List[Expression]) -> List[Type]: | |
result.append(AnyType()) | ||
return result | ||
|
||
def process_typeddict_definition(self, s: AssignmentStmt) -> None: | ||
"""Check if s defines a TypedDict; if yes, store the definition in symbol table.""" | ||
if len(s.lvalues) != 1 or not isinstance(s.lvalues[0], NameExpr): | ||
return | ||
lvalue = s.lvalues[0] | ||
name = lvalue.name | ||
typed_dict = self.check_typeddict(s.rvalue, name) | ||
if typed_dict is None: | ||
return | ||
# Yes, it's a valid TypedDict definition. Add it to the symbol table. | ||
node = self.lookup(name, s) | ||
node.kind = GDEF # TODO locally defined TypedDict | ||
node.node = typed_dict | ||
|
||
def check_typeddict(self, node: Expression, var_name: str = None) -> Optional[TypeInfo]: | ||
"""Check if a call defines a TypedDict. | ||
|
||
The optional var_name argument is the name of the variable to | ||
which this is assigned, if any. | ||
|
||
If it does, return the corresponding TypeInfo. Return None otherwise. | ||
|
||
If the definition is invalid but looks like a TypedDict, | ||
report errors but return (some) TypeInfo. | ||
""" | ||
if not isinstance(node, CallExpr): | ||
return None | ||
call = node | ||
callee = call.callee | ||
if not isinstance(callee, RefExpr): | ||
return None | ||
fullname = callee.fullname | ||
if fullname != 'mypy_extensions.TypedDict': | ||
return None | ||
items, types, ok = self.parse_typeddict_args(call, fullname) | ||
if not ok: | ||
# Error. Construct dummy return value. | ||
return self.build_typeddict_typeinfo('TypedDict', [], []) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wonder if this isn't going to cause a cascade of spurious errors. Might be better to just return AnyType(). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When I alter this line's twin in check_namedtuple() to return None, which means that something other than a NamedTuple was found (as opposed to an invalid NamedTuple), the error detected in the test suite (semanal-namedtuple.test:136) seems less helpful. Old error: Tuple expected as NamedTuple() field Honestly I don't think either error message is particularly good but at least the first one mentions that is has something to do with a NamedTuple. Based on this regression in error message quality I've left this line as-is. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It might be possible to return an AnyType with a more-invasive change, as I'd need to alter the signature of check_namedtuple and its callers. I'd prefer to tread lightly. |
||
else: | ||
name = cast(StrExpr, call.args[0]).value | ||
if name != var_name: | ||
# Give it a unique name derived from the line number. | ||
name += '@' + str(call.line) | ||
info = self.build_typeddict_typeinfo(name, items, types) | ||
# Store it as a global just in case it would remain anonymous. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, this sounds risky. Consider:
Now it looks like the global foo will be overwritten for no reason. Maybe only do that if it's anonymous? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't fully understand the implications of changing this line. I will however note that if I remove the line entirely from check_namedtuple() and check_typeddict() then the test suite continues to pass. Agreed that it seems weird that a type declaration performed anywhere would want to be injected into the global scope. I can remove this line if someone else can confirm that I'm not going to break some subtle preexisting behavior. |
||
self.globals[name] = SymbolTableNode(GDEF, info, self.cur_mod_id) | ||
call.analyzed = TypedDictExpr(info) | ||
call.analyzed.set_line(call.line, call.column) | ||
return info | ||
|
||
def parse_typeddict_args(self, call: CallExpr, | ||
fullname: str) -> Tuple[List[str], List[Type], bool]: | ||
# TODO: Share code with check_argument_count in checkexpr.py? | ||
args = call.args | ||
if len(args) < 2: | ||
return self.fail_typeddict_arg("Too few arguments for TypedDict()", call) | ||
if len(args) > 2: | ||
return self.fail_typeddict_arg("Too many arguments for TypedDict()", call) | ||
# TODO: Support keyword arguments | ||
if call.arg_kinds != [ARG_POS, ARG_POS]: | ||
return self.fail_typeddict_arg("Unexpected arguments to TypedDict()", call) | ||
if not isinstance(args[0], (StrExpr, BytesExpr, UnicodeExpr)): | ||
return self.fail_typeddict_arg( | ||
"TypedDict() expects a string literal as the first argument", call) | ||
if not isinstance(args[1], DictExpr): | ||
return self.fail_typeddict_arg( | ||
"TypedDict() expects a dictionary literal as the second argument", call) | ||
dictexpr = args[1] | ||
items, types, ok = self.parse_typeddict_fields_with_types(dictexpr.items, call) | ||
return items, types, ok | ||
|
||
def parse_typeddict_fields_with_types(self, dict_items: List[Tuple[Expression, Expression]], | ||
context: Context) -> Tuple[List[str], List[Type], bool]: | ||
items = [] # type: List[str] | ||
types = [] # type: List[Type] | ||
for (field_name_expr, field_type_expr) in dict_items: | ||
if isinstance(field_name_expr, (StrExpr, BytesExpr, UnicodeExpr)): | ||
items.append(field_name_expr.value) | ||
else: | ||
return self.fail_typeddict_arg("Invalid TypedDict() field name", field_name_expr) | ||
try: | ||
type = expr_to_unanalyzed_type(field_type_expr) | ||
except TypeTranslationError: | ||
return self.fail_typeddict_arg('Invalid field type', field_type_expr) | ||
types.append(self.anal_type(type)) | ||
return items, types, True | ||
|
||
def fail_typeddict_arg(self, message: str, | ||
context: Context) -> Tuple[List[str], List[Type], bool]: | ||
self.fail(message, context) | ||
return [], [], False | ||
|
||
def build_typeddict_typeinfo(self, name: str, items: List[str], | ||
types: List[Type]) -> TypeInfo: | ||
strtype = self.named_type('__builtins__.str') # type: Type | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (Not your fault, but don't you find the difference between named_type() and named_type_or_none() jarring? I have to carefully read the implementation every time I see this... :-( ) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't really know what the difference is. Presumably named_type_or_none() can fail but there's no documentation explaining what None means in that context. The underlying lookup_fully_qualified() and lookup_fully_qualified_or_none() have identical documentation even though the latter can presumably fail in some way. Also named_type_or_none() and lookup_fully_qualified_or_none() can return Optional[...] but aren't marked as such. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We're missing many Optional annotations, so not to worry about those. The difference that was jarring to me is that one starts the lookup in the local namespace (hence |
||
dictype = (self.named_type_or_none('builtins.dict', [strtype, AnyType()]) | ||
or self.object_type()) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm guessing this 'or' branch is when the test fixtures don't define dict? It's a fine hack but might deserve a comment. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Perhaps. It's the same pattern that build_namedtuple_typeinfo() uses. I'm not qualified to leave a comment explaining the pattern. |
||
fallback = dictype | ||
|
||
info = self.basic_new_typeinfo(name, fallback) | ||
info.is_typed_dict = True | ||
|
||
# (TODO: Store {items, types} inside "info" somewhere for use later. | ||
# Probably inside a new "info.keys" field which | ||
# would be analogous to "info.names".) | ||
|
||
return info | ||
|
||
def visit_decorator(self, dec: Decorator) -> None: | ||
for d in dec.decorators: | ||
d.accept(self) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
from typing import Dict, Type, TypeVar | ||
|
||
T = TypeVar('T') | ||
|
||
|
||
def TypedDict(typename: str, fields: Dict[str, Type[T]]) -> Type[dict]: ... |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -154,9 +154,11 @@ N = namedtuple('N', 1) # E: List or tuple literal expected as the second argumen | |
from collections import namedtuple | ||
N = namedtuple('N', ['x', 1]) # E: String literal expected as namedtuple() item | ||
|
||
[case testNamedTupleWithInvalidArgs] | ||
-- NOTE: The following code works at runtime but is not yet supported by mypy. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Honestly, in Python 3.6 I'd love NamedTuple to take keyword args so you can say
It requires Python 3.6+ because that promises to preserve keyword arg order. But this would require support in typing too. (@ilevkivskyi?) Not for this PR obviously... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, it would be easy to implement in typing.py There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed that keyword args would be a nice syntax to use for |
||
-- Keyword arguments may potentially be supported in the future. | ||
[case testNamedTupleWithNonpositionalArgs] | ||
from collections import namedtuple | ||
N = namedtuple('N', x=['x']) # E: Unexpected arguments to namedtuple() | ||
N = namedtuple(typename='N', field_names=['x']) # E: Unexpected arguments to namedtuple() | ||
|
||
[case testInvalidNamedTupleBaseClass] | ||
from typing import NamedTuple | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Micro-optimization: you can move this before the isinstance() check -- it's still the callee even if we don't like it. :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Applied globally.