Implement default values on NamedTuple #2719
Conversation
| @@ -1855,8 +1862,14 @@ def add_method(funcname: str, | |||
|
|
|||
| add_method('_replace', ret=selftype, | |||
| args=[Argument(var, var.type, EllipsisExpr(), ARG_NAMED_OPT) for var in vars]) | |||
|
|
|||
| def make_init_arg(var: Var) -> Argument: | |||
| default = default_items.get(var.name(), None) | |||
There was a problem hiding this comment.
The default value could be itself None. So that I would use sentinel = object() and then default_items.get(var.name(), sentinel) and below if default is sentinel.
There was a problem hiding this comment.
The Python default value could be None, but as I understand it mypy would use a value of type mypy.types.NoneTyp internally. The previous code also passes None to indicate that the Argument had no default, so I think this is fine.
| reveal_type(X(1)) # E: Revealed type is 'Tuple[builtins.int, builtins.int, fallback=__main__.X]' | ||
| reveal_type(X(1, 2)) # E: Revealed type is 'Tuple[builtins.int, builtins.int, fallback=__main__.X]' | ||
|
|
||
| X(1, 'a') # E: Argument 2 to "X" has incompatible type "str"; expected "int" |
There was a problem hiding this comment.
I would add few more test cases. For example, the None default value mentioned above, user defined classes as field types, wrong types like x: str = 5, etc.
There was a problem hiding this comment.
Also it makes sense to add tests with classes inheriting form a named tuple with default values.
There was a problem hiding this comment.
Thanks for these suggestions! The x: str = 5 case actually wasn't checked, and I had some trouble coming up with a way to make that throw an error, because the body of the NamedTuple is currently almost completely ignored. I managed to fix it by typechecking the body like a normal class. This has the side effect that some errors (stuff like x[0]: int) produce multiple errors on the same line, but that doesn't seem like a big deal.
|
|
||
| class HasNone(NamedTuple): | ||
| x: int | ||
| y: Optional[int] = None |
There was a problem hiding this comment.
Thank you for writing more tests. I will be even more happy if you add few more tests for these (also with --strict-optional)
mypy/semanal.py
Outdated
| self.analyze_base_classes(defn) | ||
| self.analyze_metaclass(defn) | ||
| self.analyze_base_classes(defn) | ||
| self.analyze_metaclass(defn) | ||
|
|
||
| for decorator in defn.decorators: |
There was a problem hiding this comment.
Why do you need to analyze class decorators for named tuple?
There was a problem hiding this comment.
I don't (although it would be nice to support namedtuple decorators in the future). I'll push an update to change this once tests pass.
Also, I think it shouldn't be hard to implement python/typing#352 by further adjusting the behavior of this function—I might try that once this PR is approved.
There was a problem hiding this comment.
Also, I think it shouldn't be hard to implement python/typing#352 by further adjusting the behavior of this function—I might try that once this PR is approved.
I would be glad if you will do this.
Would be nice to support this but that's for a separate issue.
JelleZijlstra
force-pushed
the
namedtupledefault
branch
from
f166cea to
291c059
Compare
mypy/semanal.py
Outdated
|
|
||
| self.bind_class_type_vars(defn) | ||
| if not is_named_tuple: |
There was a problem hiding this comment.
Sorry for coming up with new comments, but I think that this will be cleaner if you refactor this as:
if self.analyze_namedtuple_classdef(defn):
# quick check
self.enter_class(defn)
defn.defs.accept(self)
self.leave_class()
else:
# full story
...
|
|
||
| class Y(X): | ||
| def method(self) -> str: | ||
| return self.x + cast(str, self.y) |
There was a problem hiding this comment.
To be honest, I don't like casts of unsafe expressions in tests, but formally this test works as it should.
Maybe you could modify it?
There was a problem hiding this comment.
I'll just change it to access self.y without using it. I did str(self.y) first but that doesn't work because the fixture doesn't have it.
|
@gvanrossum |
|
Sorry, I am way behind on reviews. Hopefully I'll catch up before the end
of the week.
|
|
|
||
| class Parameterized(NamedTuple): | ||
| x: int | ||
| y: List[int] = [1] + [2] |
There was a problem hiding this comment.
Add test case where the default value if [] and the type is List[int]. This will verify that the attribute type is used as type context.
| y: int = 3 | ||
|
|
||
| class Y(X): | ||
| def method(self) -> str: |
There was a problem hiding this comment.
Test calling base class __init__ in a subclass of a named tuple.
|
Also can you resolve the merge conflict? |
|
Thanks for implementing this! |
This implements python/typing#338 in mypy.
Should we have some special handling here to account for the fact that this will work in 3.6.1, but not in 3.6.0?