Add support for NamedTuple methods #3081

JelleZijlstra · 2017-03-29T20:00:59Z

This is the mypy implementation of python/typing#352. cc @ilevkivskyi who wrote the runtime implementation.

Also fixes #3075.

This was pretty simple to implement.

gvanrossum

Pretty close to the target!

gvanrossum · 2017-03-29T20:38:57Z

mypy/semanal.py

+                    self.fail('Cannot overwrite NamedTuple attribute "{}"'.format(prohibited),
+                              named_tuple_info.names[prohibited].node)
+
+            named_tuple_info.names.update(nt_names)


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.

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).

gvanrossum · 2017-03-29T20:57:26Z

mypy/semanal.py

@@ -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


I'd rewrite this using positive logic -- if not X and not (Y and Z) and not Q is a little hard to understand.

gvanrossum · 2017-03-29T21:01:28Z

mypy/semanal.py

@@ -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


Call out in which function they are analyzed?

gvanrossum · 2017-03-29T21:03:54Z

test-data/unit/check-class-namedtuple.test

@@ -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


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

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.

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

gvanrossum · 2017-03-29T21:19:49Z

test-data/unit/check-class-namedtuple.test

+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__"


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).

gvanrossum · 2017-03-29T21:20:58Z

mypy/semanal.py

@@ -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__',


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

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

gvanrossum · 2017-03-29T21:22:14Z

mypy/semanal.py

-            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


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).

gvanrossum · 2017-03-29T21:31:17Z

PS. Would also like to see @ilevkivskyi's review.

JelleZijlstra · 2017-03-29T21:31:51Z

Thanks for the review! I'll push an update soon.

I realized we still disallow docstrings in namedtuples. Do you mind if I fix that in this PR too? Otherwise I can send another PR.

gvanrossum · 2017-03-29T21:49:15Z

I realized we still disallow docstrings in namedtuples. Do you mind if I fix that in this PR too?

Yes please.

ilevkivskyi

Just few more comments from me.

ilevkivskyi · 2017-03-29T22:15:10Z

mypy/semanal.py

@@ -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)


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__

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

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

__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.

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).

ilevkivskyi · 2017-03-29T22:16:59Z

test-data/unit/check-class-namedtuple.test

+
+class Base(NamedTuple):
+    x: int
+    def copy(self: T) -> T:


I would test another recently added feature: overloaded methods.

ilevkivskyi · 2017-03-29T22:19:38Z

test-data/unit/check-class-namedtuple.test

+
+class Child(Base):
+    def new_method(self) -> int:
+        return self.x


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

ilevkivskyi · 2017-03-29T22:35:04Z

mypy/semanal.py

+            nt_names = named_tuple_info.names
+            named_tuple_info.names = SymbolTable()
+
+            self.bind_class_type_vars(named_tuple_info)


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.

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?

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.

Agreed the save/restore trick feels unprincipled.

I agree that it's not ideal but an alternative implementation would probably be more complicated.

I was testing with an outdated version of typing. Replacing _source does work. I'm also OK with deciding that all of this is too obscure to support and we shouldn't care about special-casing __doc__ and _source.

ilevkivskyi · 2017-03-30T07:53:49Z

test-data/unit/check-class-namedtuple.test

@@ -583,7 +583,7 @@ class MagicalFields(NamedTuple):
    x: int
    def __slots__(self) -> None: pass  # E: Cannot overwrite NamedTuple attribute "__slots__"
    def __new__(cls) -> None: pass  # E: Cannot overwrite NamedTuple attribute "__new__"
-    def _source(self) -> int: pass  # E: Cannot overwrite NamedTuple attribute "_source"
+    def _source(self) -> int: pass  # _source is ok


I think this is an oversight in typing. _source should probably be prohibited. I will make a PR to typing now.

gvanrossum · 2017-03-31T04:26:29Z

I'll have another look when @ilevkivskyi is happy.

ilevkivskyi · 2017-03-31T23:45:06Z

test-data/unit/check-class-namedtuple.test

+    def __new__(cls) -> None: pass  # E: Cannot overwrite NamedTuple attribute "__new__"
+    def _source(self) -> int: pass  # E: Cannot overwrite NamedTuple attribute "_source"
+    __annotations__ = {'x': float}  # E: NamedTuple field name cannot start with an underscore: __annotations__ \
+        # E: Invalid statement in NamedTuple definition; expected "field_name: field_type" \


While you at it, maybe you could update the error message to use "field_name: field_type [= default]"?

ilevkivskyi · 2017-03-31T23:49:33Z

I'll have another look when @ilevkivskyi is happy.

I am almost happy now, I am only a bit worried about the trick with SymbolTable().

JelleZijlstra · 2017-04-28T15:36:24Z

There's a pretty big merge conflict now, I'll try to resolve it tonight.

gvanrossum · 2017-04-28T16:02:46Z

Before merging I want to be sure this passes with our internal code bases and also the quick and incremental tests that Jukka is developing.

Daenyth · 2017-05-03T20:46:51Z

Until this is merged, is there a way I can suppress this specific error so that I can use mypy on files with NamedTuples like this?

gvanrossum · 2017-05-03T20:48:37Z

Until this is merged, is there a way I can suppress this specific error so that I can use mypy on files with NamedTuples like this?

You can put a # type: ignore comment on the line where the unwanted error appears.

JelleZijlstra · 2017-05-03T20:58:09Z

It probably still won't recognize that the methods exist though.

Daenyth

Can you add a test case covering @classmethod?

It crashes in master but works in this branch

from typing import *

class Bar(NamedTuple):
    x: str

    @classmethod
    def new(cls, f: str) -> 'Bar':
        return Bar(x=f)

Also I still get a type error: foo.py:6: error: Invalid statement in NamedTuple definition; expected "field_name: field_type [= default]"

It's probably worth also checking @property and @staticmethod

JelleZijlstra · 2017-05-04T14:49:42Z

Added tests for all those and fixed some issues it uncovered. I needed another hack to make the cls argument to classmethods work.

Daenyth

Class level fields fail:

from typing import *
class Foo(NamedTuple):
    x: int
    y = 1  # Not one of the tuple fields but a class attribute

foo.py:4: error: Invalid statement in NamedTuple definition; expected "field_name: field_type [= default]"

JelleZijlstra · 2017-05-05T14:59:00Z

Wow, I hadn't thought of doing that. It does seem to work at runtime. I'd prefer to leave it out of this PR though, since the PR has been going on for long enough as is and I see some issues with that pattern. I think the fact that it works at runtime is probably an accident; it's not tested in typing and wasn't mentioned as far as I remember in the discussions about adding the feature there.

If you have a class variable, it becomes very awkward to annotate it. If I make

class Foo(NamedTuple):
    x = []
    y: int = 1

mypy will tell me that I need to specify a more precise type for the list. But if I do

class Foo(NamedTuple):
    x: List['Foo'] = []
    y: int = 1

x has become an instance field. I suppose

class Foo(NamedTuple):
    x = []  # type: List[Foo]
    y = 1

would work, but that just seems really confusing, since now you get radically different behavior depending on whether you used a type comment or a PEP 526-style annotation. Maybe we can figure out an acceptable way to handle this problem, but it seems complex enough that it's better to separate it out. Let's keep this PR focused on methods (and docstrings, which were trivial to implement).

Daenyth · 2017-05-05T15:01:05Z

That's entirely fair.

…

On Fri, May 5, 2017, 10:59 AM Jelle Zijlstra ***@***.***> wrote: Wow, I hadn't thought of doing that. It does seem to work at runtime. I'd prefer to leave it out of this PR though, since the PR has been going on for long enough as is and I see some issues with that pattern. I think the fact that it works at runtime is probably an accident; it's not tested in typing and wasn't mentioned as far as I remember in the discussions about adding the feature there. If you have a class variable, it becomes very awkward to annotate it. If I make class Foo(NamedTuple): x = [] y: int = 1 mypy will tell me that I need to specify a more precise type for the list. But if I do class Foo(NamedTuple): x: List['Foo'] = [] y: int = 1 x has become an instance field. I suppose class Foo(NamedTuple): x = [] # type: List[Foo] y = 1 would work, but that just seems really confusing, since now you get radically different behavior depending on whether you used a type comment or a PEP 526-style annotation. Maybe we can figure out an acceptable way to handle this problem, but it seems complex enough that it's better to separate it out. Let's keep this PR focused on methods (and docstrings, which were trivial to implement). — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#3081 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAA5NBuWTJKgS6YEArSjM3asfD5Vwb2Qks5r2zk3gaJpZM4Mte4d> .

ilevkivskyi · 2017-05-05T15:14:07Z

Wow, I hadn't thought of doing that. It does seem to work at runtime. I'd prefer to leave it out of this PR though, since the PR has been going on for long enough as is and I see some issues with that pattern

It works at runtime since it is not easy to prohibit everything at runtime :-)
But seriously, I don't think we need this. Named tuples are intended to be "minimalistic" classes, and possible syntactic solutions for class variables in named tuples seem confusing.

gvanrossum · 2017-05-10T22:12:00Z

Gah! I thought we had this feature already. What will it take to finish this?

ilevkivskyi · 2017-05-10T22:23:22Z

Gah! I thought we had this feature already. What will it take to finish this?

I think it is waiting for your review.

I'll have another look when @ilevkivskyi is happy.

I am practically happy with this, the SymbolTable() trick looks not too aesthetic to me, but this is probably just a matter of taste.

JelleZijlstra · 2017-05-10T22:26:52Z

You need to merge it. :)

As far as I'm aware, this code works and the only concern (brought up by you and Ivan above) is that the approach taken is hacky, since it has some strange manual manipulation of the symbol table. I don't know of an easy way to avoid the symtable manipulation, so my preference is to merge this as is and maybe later do a refactoring to make NamedTuples more like normal classes.

gvanrossum · 2017-05-11T01:31:23Z

Sorry about that! I'm trying to restore personal state that I've apparently lost. I will review this.

gvanrossum · 2017-05-16T01:15:25Z

Finally! Sorry again.

JelleZijlstra added 3 commits March 29, 2017 12:35

methods mostly work (TODO: more tests)

1077e65

more tests

44c6f06

add test for __annotations__

f00ef4c

JelleZijlstra changed the title ~~Implement NamedTuple methods~~ Add support for NamedTuple methods Mar 29, 2017

gvanrossum requested changes Mar 29, 2017

View reviewed changes

address Guido's comments

a490805

Also disallow _source and __annotations__

47852a6

ilevkivskyi reviewed Mar 29, 2017

View reviewed changes

JelleZijlstra added 3 commits March 29, 2017 15:46

allow docstrings (but not __doc__ assignment)

68d16c0

add some tests

6507736

allow overriding _source and __doc__

6728123

I was testing with an outdated version of typing. Replacing _source does work. I'm also OK with deciding that all of this is too obscure to support and we shouldn't care about special-casing __doc__ and _source.

ilevkivskyi reviewed Mar 30, 2017

View reviewed changes

disallow _source again

a80fe25

ilevkivskyi mentioned this pull request Mar 31, 2017

Allow docstring in NamedTuple subclasses #3043

Closed

ilevkivskyi reviewed Mar 31, 2017

View reviewed changes

pkch mentioned this pull request Apr 1, 2017

Refactor NamedTuple to put it in a stub #3102

Closed

update error message

ca6a0b5

This was referenced Apr 3, 2017

Support generic NamedTuples #685

Closed

[WIP] Move special-cased types from semanal to a separate module #3134

Closed

JelleZijlstra mentioned this pull request Apr 28, 2017

Release mypy 0.510 #3273

Closed

JelleZijlstra added 2 commits April 29, 2017 23:25

Merge remote-tracking branch 'upstream/master' into ntmethods

a40ce3a

fix tests post-merge

138d780

Daenyth mentioned this pull request May 4, 2017

Internal error with namedtuple+classmethod #3317

Closed

Daenyth reviewed May 4, 2017

View reviewed changes

add tests for classmethod, staticmethod, and property and fix

a03513e

Daenyth suggested changes May 5, 2017

View reviewed changes

Merge branch 'master' into ntmethods

9ff9b83

gvanrossum approved these changes May 16, 2017

View reviewed changes

gvanrossum merged commit 3589c08 into python:master May 16, 2017

JelleZijlstra mentioned this pull request Jul 20, 2017

Allow docstrings in Python 3.6+ class based TypedDict #3747

Closed

gvanrossum mentioned this pull request Jul 24, 2017

Fix crash on incompatible redefinition in named tuple #3760

Merged

JelleZijlstra deleted the ntmethods branch January 16, 2018 00:15

Add support for NamedTuple methods #3081

Add support for NamedTuple methods #3081

Conversation

JelleZijlstra commented Mar 29, 2017

gvanrossum left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gvanrossum commented Mar 29, 2017

JelleZijlstra commented Mar 29, 2017

gvanrossum commented Mar 29, 2017

ilevkivskyi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilevkivskyi Mar 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gvanrossum commented Mar 31, 2017

Choose a reason for hiding this comment

ilevkivskyi commented Mar 31, 2017

JelleZijlstra commented Apr 28, 2017

gvanrossum commented Apr 28, 2017

Daenyth commented May 3, 2017

gvanrossum commented May 3, 2017

JelleZijlstra commented May 3, 2017

Daenyth left a comment • edited Loading

Choose a reason for hiding this comment

JelleZijlstra commented May 4, 2017

Daenyth left a comment

Choose a reason for hiding this comment

JelleZijlstra commented May 5, 2017

Daenyth commented May 5, 2017 via email

ilevkivskyi commented May 5, 2017

gvanrossum commented May 10, 2017

ilevkivskyi commented May 10, 2017

JelleZijlstra commented May 10, 2017

gvanrossum commented May 11, 2017

gvanrossum commented May 16, 2017

ilevkivskyi Mar 29, 2017 •

edited

Loading

Daenyth left a comment •

edited

Loading