python / cpython

The Python programming language
https://www.python.org
Other
62.59k stars 30.03k forks source link

typing: Unexpected result with value of instance of class inherited from typing.NamedTuple #77258

Open ghost opened 6 years ago

ghost commented 6 years ago
BPO 33077
Nosy @gvanrossum, @rhettinger, @ericvsmith, @ilevkivskyi
Files
  • python_test.py: Python unittest
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields: ```python assignee = None closed_at = None created_at = labels = ['3.7', 'type-bug', 'library', 'docs'] title = 'typing: Unexpected result with value of instance of class inherited from typing.NamedTuple' updated_at = user = None ``` bugs.python.org fields: ```python activity = actor = 'levkivskyi' assignee = 'docs@python' closed = False closed_date = None closer = None components = ['Documentation', 'Library (Lib)'] creation = creator = '\xd0\x95\xd0\xb2\xd0\xb3\xd0\xb5\xd0\xbd\xd0\xb8\xd0\xb9 \xd0\x9c\xd0\xb0\xd1\x85\xd0\xbc\xd1\x83\xd0\xb4\xd0\xbe\xd0\xb2' dependencies = [] files = ['47487'] hgrepos = [] issue_num = 33077 keywords = [] message_count = 10.0 messages = ['313843', '313848', '313916', '313918', '313919', '313920', '313925', '313945', '313957', '314168'] nosy_count = 6.0 nosy_names = ['gvanrossum', 'rhettinger', 'eric.smith', 'docs@python', 'levkivskyi', '\xd0\x95\xd0\xb2\xd0\xb3\xd0\xb5\xd0\xbd\xd0\xb8\xd0\xb9 \xd0\x9c\xd0\xb0\xd1\x85\xd0\xbc\xd1\x83\xd0\xb4\xd0\xbe\xd0\xb2'] pr_nums = [] priority = 'normal' resolution = None stage = 'needs patch' status = 'open' superseder = None type = 'behavior' url = 'https://bugs.python.org/issue33077' versions = ['Python 3.6', 'Python 3.7'] ```

    ghost commented 6 years ago

    Overwriting of default values not working, and used default value of base class. Unittest file if attachment described a problem.

    gvanrossum commented 6 years ago

    Thanks Евгений Махмудов for the report!

    The crux is this:

    class A(NamedTuple):
        value: bool = True
    
    class B(A):
        value: bool = False

    B(True).value # Expected True, but is False B(True)[0] # True as expected

    If we add NamedTuple to B's bases or make its metaclass NamedTupleMeta, it works as expected.

    Introspecting the classes a bit more suggests a cause: the class variable A.value is a \<property ...>, but B.value is just False, and adding the extra base class or metaclass corrects this.

    Ivan, you can probably tell what's wrong from this. Maybe it's hard to fix, because NamedTuple doesn't appear in A.__mro__? (IIRC there was a question about that somewhere recently too?)

    ilevkivskyi commented 6 years ago

    Yes, this is because subclassing typing.NamedTuple is not an actual subclassing, but is just a syntactic sugar for calling collections.namedtuple. A discussion about allowing subclassing/extending named tuples previously appeared in https://github.com/python/typing/issues/427 but was subsequently closed as wontfix. In short, the argument was that we should keep feature set and implementation of named tuples simple/minimalistic, and instead recommend dataclasses for all more complex use cases. Note however, that this decision was never added to the typing.NamedTuple documentation. I think it totally makes sense to clarify this in the docs.

    gvanrossum commented 6 years ago

    I wonder if it's too late to conclude that NamedTuple in this context should have been a class decorator rather than a base class. With a class decorator it's more understandable that the effect doesn't automatically apply to subclasses.

    ericvsmith commented 6 years ago

    I once thought of building NamedTuple functionality into dataclasses, either as a parameter to @dataclass or as a new decorator. But it didn't get very far.

    It would have to return a different class, like NamedTuple does, and this didn't fit in well with the original "keep @dataclass simple" mantra.

    It would also be especially confusing with frozen already existing.

    ilevkivskyi commented 6 years ago

    I would say it is too late. typing.NamedTuple is out there for more than a year and is quite actively used. A search on GitHub shows thousands of files that import typing.NamedTuple and a macroscopic fraction of those use it with the class syntax. I think the damage from breaking working code outweighs the potential bugs here. A clear example in the docs that explains how it works would be sufficient I think.

    (Also a camel case decorator would look weird.)

    gvanrossum commented 6 years ago

    Apart from the fact that it's too late, if you had to do it over again, could it be done as a class decorator?

    Anyway, let's keep this issue open but reclassify it as a docs issue.

    ilevkivskyi commented 6 years ago

    Apart from the fact that it's too late, if you had to do it over again, could it be done as a class decorator?

    Yes, this could be done as a decorator which would replace the original class with a named tuple after inspecting __annotations__ and __dict__. (But again, the name would be different, since @NamedTuple looks weird to me.)

    rhettinger commented 6 years ago

    Would it be worthwhile to show an example of a subclass that overrides or extends __new__?

    Elsewhere in Python, the usual technique for changing method defaults is for a subclass to override or extend the method in question.

        class A:
            def somemeth(self, value: bool = True):
                print(value)
    
        class B(A):
            def somemeth(self, value: bool = False):
                super().somemeth(value)
    ilevkivskyi commented 6 years ago

    Would it be worthwhile to show an example of a subclass that overrides or extends __new__?

    I think yes. I would actually add few examples what could (and maybe also couldn't) be done with named tuples.

    JelleZijlstra commented 2 years ago

    This still reproduces on 3.11. #90130 covers similar confusing NamedTuple behavior.

    gvanrossum commented 2 years ago

    I thought the conclusion was to just document this? And that other issue seems something quite unrelated.