python / cpython

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

`__future__` annotations breaks `TypedDict` `__required/optional_keys__` #97727

Open scop opened 2 years ago

scop commented 2 years ago

Bug report

from __future__ import annotations appears to break TypedDict required/optional keys ending up in __required_keys__ and __optional_keys__. mypy works as expected though.

Using the example from https://peps.python.org/pep-0655/#usage-in-python-3-11 as the base

$ cat t.py
from __future__ import annotations

from typing_extensions import NotRequired, TypedDict

class Dog(TypedDict):
    name: str
    owner: NotRequired[str]

print("required", Dog.__required_keys__)
print("optional", Dog.__optional_keys__)
$ python3 t.py
required frozenset({'name', 'owner'})
optional frozenset()

With the __future__ import removed, works as expected:

$ cat t.py
from typing_extensions import NotRequired, TypedDict

class Dog(TypedDict):
    name: str
    owner: NotRequired[str]

print("required", Dog.__required_keys__)
print("optional", Dog.__optional_keys__)
$ python3 t.py
required frozenset({'name'})
optional frozenset({'owner'})

Note: breaks across different variations of total and Required/NotRequired and typing_extensions vs typing imports, above is just one example.

https://peps.python.org/pep-0655/#how-to-teach-this contains an example with the __future__ annotations import in place with no mention that it would not cause __required_keys__ and __optional_keys__ becoming populated as expected, so I'm assuming this is a bug.

Your environment

JelleZijlstra commented 2 years ago

This was also reported in python/typing_extensions#55. As discussed there, it's hard to find a solution for this problem that doesn't cause new problems, because the most obvious solution (evaluating the annotations when the TypedDict is defined) would break various use cases for forward references.

martindemello commented 2 years ago

from a pragmatic point of view, i wonder if we can add a pure string parsing function that unwraps the outer container (if any) of a forward ref. then typeddict here could consider the tuple (ForwardRef('NotRequired'), ForwardRef('str')) and possibly choose to resolve the first element, or simply apply some heuristics to see if it would resolve to typing_extensions.NotRequired

hmc-cs-mdrissi commented 2 years ago

The main issue with string heuristics is deciding which of these cases you want to handle,

from typing import Required as R, NotRequired as NR, Annotated
import typing as t

class Foo(TypedDict):
  x: R[int]
  y: NR[str]
  z: Annotated[NR[str], ...]
  w: Annotated[t.Required[str], ...]

Dataclasses does do a similar thing for ClassVar, so you could pick which cases to support and document it. I'd view this as an improvement over status quo, although it'd stay a subtle thing.

One way to avoid most breakage involving forward refs would be delaying get_type_hints call to time __required_keys__ is accessed by having it be like a class property. Using get_annotations maybe to just have same behavior of letting globals be modules namespace and locals be class namespace. Rough code idea,

from __future__ import annotations

from inspect import get_annotations
from typing import get_origin

from typing_extensions import NotRequired, Required

class LazyTypedDict:
    @classmethod
    def required_keys(cls):
        explicit_required_keys = {k for k, v in get_annotations(cls, eval_str=True).items() if get_origin(v) is Required}
        return explicit_required_keys

class Foo(LazyTypedDict):
    a: Required[int]
    b: int
    c: NotRequired[int]

print(Foo.required_keys()) # Prints {"a"}

This is enough to handle stuff like recursive/mutual recursion just fine too.

class RecA(LazyTypedDict):
    b: Required[ReqB]

class ReqB(LazyTypedDict):
    a: Required[RecA]
    s: int

print(ReqB.required_keys()) # Prints {"a"} as wanted.

I'd be interested in doing a pr for either approach. Would string heuristics + caveats or lazy evaluation be preferred?

JelleZijlstra commented 2 years ago

I'm hoping that the SC will soon come to a decision on whether to go with PEP 563 vs. 649. Once that's done we, may get more clarity on the right approach to problems like this one.

scop commented 2 years ago

Thanks for considering. Perhaps in the meantime it would be good to add a caveat note to https://peps.python.org/pep-0655/#how-to-teach-this about the possible breakage.

JelleZijlstra commented 1 year ago

PEP-649 has now been accepted, rendering from __future__ import annotations essentially deprecated. Therefore, I think we should close this as "won't fix" instead of introducing any string-based hacks.

AlexWaygood commented 1 year ago

PEP-649 has now been accepted, rendering from __future__ import annotations essentially deprecated. Therefore, I think we should close this as "won't fix" instead of introducing any string-based hacks.

I think it's fine to say we won't fix this on the basis that it's too complex. I don't think PEP-649's acceptance is a good rationale to say we won't fix this, however. For code bases that want to support all non-EOL Python versions, from __future__ import annotations will likely continue to be used for a long time after PEP 649 has been implemented in Python 3.13. Differences of behaviour under PEP-563 are therefore likely to impact a large number of users for a while yet.