python / mypy

Optional static typing for Python
https://www.mypy-lang.org/
Other
18.33k stars 2.81k forks source link

Mypy rejects valid nested class inside of a NamedTuple class definition #5362

Open gwk opened 6 years ago

gwk commented 6 years ago

I think I've found a bug in NamedTuple handling:

from enum import Enum
from typing import NamedTuple

class T(NamedTuple):
  class State(Enum):
    A =1
  state:State

print(T.State.A)
print(T(state=T.State.A))

This runs fine in Python3.7 but mypy master head issues the following error:

Invalid statement in NamedTuple definition; expected "field_name: field_type [= default]"

setup.cfg:

[mypy]
python_version = 3.6
cache_dir = _build/mypy_cache
mypy_path = ~/work/pithy:./lambda
check_untyped_defs = True
disallow_subclassing_any = True
disallow_untyped_calls = True
disallow_untyped_defs = False
ignore_missing_imports = False
show_column_numbers = True
show_none_errors = True
strict_boolean = False
strict_optional = True
warn_incomplete_stub = True
warn_no_return = True
warn_redundant_casts = True
warn_return_any = True
warn_unused_configs = True
warn_unused_ignores = True
ethanhs commented 6 years ago

It seems we miss this case in check_namedtuple_classdef here: https://github.com/python/mypy/blob/master/mypy/semanal_namedtuple.py#L57. I believe we would also need to handle adding this class to the generated typeinfo. @gwk would you be interested in trying to fix this?

JelleZijlstra commented 6 years ago

I'm not sure we should support this. Defining a class inside a NamedTuple class doesn't seem like the intended usage of NamedTuple.

ethanhs commented 6 years ago

I'm not sure we should support this.

I'm okay with that too. I agree it doesn't make a lot of sense in this context.

gwk commented 6 years ago

I'm a little surprised that you would deem legal usage undesirable so quickly.

Nested classes are a feature of the language that can be used in a variety of ways. NamedTuple is a way of conveniently generating classes with certain very desirable properties, namely immutability and sequential access. These two features compose well in Python, and for Mypy to prohibit that composition just makes Mypy less useful.

In my own humble opinion, "Intended usage" is perhaps not the only way that Mypy should consider the language. The language designers have not and cannot predict all of the ways that various features will be productively used or can interact with each other. I would think that the goal of the type checker is to prohibit pathological cases, and not ones that users are reporting as useful!

In any case, thank you for considering this. @ethanhs if I'm really the only one who sees value in this, I could take a stab at a patch.

asottile commented 5 years ago

If you're looking for a concrete use case, here's where I just hit this:

class PR(NamedTuple):
    date: datetime.date
    link_text: str
    link_url: str
    title: str

    class PRDisplay(NamedTuple):
        date: str
        link: str
        title: str

    @property
    def display(self) -> 'PR.PRDisplay':
        return PR.PRDisplay(
            self.date.isoformat(), f'[{self.link_text}]', self.title,
        )
NMertsch commented 1 year ago

Regarding

Defining a class inside a NamedTuple class doesn't seem like the intended usage of NamedTuple.

-- @JelleZijlstra

I agree. The docs say that typing.NamedTuple is a "Typed version of collections.namedtuple()". collections.namedtuple does not allow nested classes, so I guess neither should typing.NamedTuple (at least in a type-checking context).

(Sorry for being late to the party, but I stumbled across a similar use case.)

hmc-cs-mdrissi commented 1 year ago

Nested collections.namedtuple are possible and do happen in some libraries like here. That's main reason I hit this issue and made a pr to support.

A secondary reason to support is consistency with other type checkers. pyright supports nested namedtuples and pytype also seems fine with them.