-
-
Notifications
You must be signed in to change notification settings - Fork 31.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
PEP 585 and ForwardRef #85542
Comments
PEP-585 current implementation (3.10.0a0) differs from current Generic implementation about ForwardRef, as illustrated bellow: from dataclasses import dataclass, field
from typing import get_type_hints, List, ForwardRef
@dataclass
class Node:
children: list["Node"] = field(default_factory=list)
children2: List["Node"] = field(default_factory=list)
assert get_type_hints(Node) == {"children": list["Node"], "children2": List[Node]}
assert List["Node"].__args__ == (ForwardRef("Node"),)
assert list["Node"].__args__ == ("Node",) # No ForwardRef here, so no evaluation by get_type_hints There is indeed no kind of ForwardRef for He could be "fixed" in 2 lines in def _eval_type(t, globalns, localns, recursive_guard=frozenset()):
if isinstance(t, str):
t = ForwardRef(t)
if isinstance(t, ForwardRef):
... but it's kind of hacky/dirty. It's true that this issue will not concern legacy code, 3.9 still being not released. So developers of libraries using get_type_hints could add in their documentation that By the way, Guido has quickly given his opinion about it in PR 21553: "We probably will not ever support this: importing ForwardRef from the built-in generic alias code would be problematic, and once from __future__ import annotations is always on there's no need to quote the argument anyway." (So feel free to close this issue) |
I think mentioning this in the docs is the best we can do in 3.9, and for 3.10 the point will be moot. The next release is release candidate 1, so we're well past the point where we can implement new functionality. |
However, PEP-563 will not solve the recursive type alias issue like |
Hm, recursive type aliases are an interesting issue. We may be able to do better there for 3.10, even if we can't fix it for 3.9 (or at least not for 3.9.0). But in the meantime maybe you can come up with a PR that adds a note to the typing docs in 3.10 explaining that |
I'm running into this issue right now. Can anyone provide a rationale as to why you think this is acceptable/expected behaviour? Do we expect developers to semi-rely on get_type_hints(), but than still having to manually resolve forward references in PEP-585 generic aliases? That seems broken to me. Thanks, |
Niklas, can you show a brief example showing the issue you're running into? Is it just that list["Node"].__args__ is just ("Node",), not (ForwardRef("Node"),)? Or is it more complex? |
Guido, sorry for the late response on this. I have a work around, but it involves passing along my own "context" from which to resolve strings on the go as they are encountered while decomposing the type hint. NiklasRosenstein/python-databind@960da61 I'm using I understand the documentation has been updated to reflect this behaviour, but it was an issue for me because it broke it's original API contract.
Arguably the same has happened when |
I asked for a brief example that explains your issue to me. Instead you sent me links to production code and patches to it. Sorry, but that doesn't help me understand your problem. Is there really no way that you can write a little story that goes along the lines of In our production code, we use the pattern foo blah blah: a lot. As you can see, the get_type_hints call fails |
You're right, let me trim it down: In production we use What I ask myself is what motivated the decision to change the behaviour for PEP-585 generics in |
When PEP-585 was discussed and implemented we did not expect people to care as much about runtime types as they did. I already explained that making list['Node'] incorporate a ForwardRef instance is unrealistic (we'd have to reimplement ForwardRef in C first). It might be possible to change get_type_hints() to recognize strings, and deprecate ForwardRef altogether. But I suspect that that would break something else, since ForwardRef is documented (I had intended it to remain an internal detail but somehow it got exposed, I don't recall why). Please stop asking why the decision was made (it sounds rather passive-aggressive to me) and start explaining the problem you are having in a way that we can actually start thinking about a solution. I have boiled down the original example to a slightly simpler one (dataclasses are a red herring): >>> from typing import get_type_hints, List
>>> class N:
... c1: list["N"]
... c2: List["N"]
...
>>> N.__annotations__
{'c1': list['N'], 'c2': typing.List[ForwardRef('N')]}
>>> get_type_hints(N)
{'c1': list['N'], 'c2': typing.List[__main__.N]} The key problem here is that the annotation list['N'] is not expanded to list[N]. What can we do to make get_type_hint() produce list[N] instead here? |
It was not my intention to sound passive agressive. Thanks for providing the context around the time PEP-585 was discussed. Frankly, I believe I have explained the problem quite well. But I would like to propose a solution. I can't judge in what circumstance a Assuming this is indeed a case to be taken into account, I would propose that Of course this would break cases that have come to strictly expect strings in PEP-585 generic |
Ah, I see the issue. I stepped through get_type_hints() using pdb, and it does have a special case for when it encounters a string: it wraps it in a ForwardRef and proceeds from there: Lines 1806 to 1807 in cef0a54
But list['N'] isn't a string, so it doesn't trigger this case. If you were to use "list[N]" instead, it works: >>> from typing import get_type_hints
>>> class N:
... c: "list[N]"
...
>>> get_type_hints(N)
{'c': list[__main__.N]}
>>> But I suppose you have a reason you (or your users) don't want to do that. We could probably add a special case where it checks for types.GenericAlias and goes through __args__, replacing strings by ForwardRefs. But would that be enough? The algorithm would have to recursively dive into __args__ to see if there's a string hidden deep inside, e.g. list[tuple[int, list["N"]]]. And what if the user writes something hybrid, like List[list["N"]]? What other cases would we need to cover? And can we sell this as a bugfix for 3.10, or will this be a new feature in 3.11? How will it interact with from __future__ import annotations? |
Here's a patch that doesn't do it right but illustrates the point: diff --git a/Lib/typing.py b/Lib/typing.py
index 972b8ba24b..4616db60c3 100644
--- a/Lib/typing.py
+++ b/Lib/typing.py
@@ -1807,6 +1807,12 @@ def get_type_hints(obj, globalns=None, localns=None, include_extras=False):
value = type(None)
if isinstance(value, str):
value = ForwardRef(value, is_argument=False, is_class=True)
+ elif isinstance(value, types.GenericAlias):
+ args = tuple(
+ ForwardRef(arg) if isinstance(arg, str) else args
+ for arg in value.__args__
+ )
+ value = value.__origin__[(*args,)]
value = _eval_type(value, base_globals, base_locals)
hints[name] = value |
Interesting! Representing the entire type hint as a string is something I haven't thought about, but it makes sense that it works. It is my understanding that >>> from typing import get_type_hints, Mapping, List
>>>
>>> class N:
... a: Mapping['str', list[List['N']]]
...
>>> get_type_hints(N)
{'a': typing.Mapping[str, list[typing.List[__main__.N]]]} Upon closer inspection of the diff --git a/Lib/typing.py b/Lib/typing.py
index e3e098b1fc..ac56b545b4 100644
--- a/Lib/typing.py
+++ b/Lib/typing.py
@@ -331,6 +331,12 @@ def _eval_type(t, globalns, localns, recursive_guard=frozenset()):
if isinstance(t, ForwardRef):
return t._evaluate(globalns, localns, recursive_guard)
if isinstance(t, (_GenericAlias, GenericAlias, types.UnionType)):
+ if isinstance(t, GenericAlias):
+ args = tuple(
+ ForwardRef(arg) if isinstance(arg, str) else arg
+ for arg in t.__args__
+ )
+ t = t.__origin__[(*args,)]
ev_args = tuple(_eval_type(a, globalns, localns, recursive_guard) for a in t.__args__)
if ev_args == t.__args__:
return t Testcase: >>> from typing import get_type_hints, Mapping, List
>>> class N:
... a: Mapping['str', List[list['N']]]
...
>>> get_type_hints(N)
{'a': typing.Mapping[str, typing.List[list[__main__.N]]]} I believe that this would be enough, but then again I haven't yet had enough time to crack at other implications this might have.
I've never used this future, but from my current, possibly limited, understanding it should have no side effects on how
I will throw in my personal opinion that this could be a bugfix, but I'm obviously biased as being on the "experiencing end" of this behaviour we're trying to change. |
I've started a pull request here: #30900 |
I think this is fixed now as the PR was merged but will leave closing this issue to @gvanrossum |
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:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: