Skip to content
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 649 and ForwardRef caching #128593

Open
Viicos opened this issue Jan 7, 2025 · 1 comment
Open

PEP 649 and ForwardRef caching #128593

Viicos opened this issue Jan 7, 2025 · 1 comment
Labels
topic-typing type-bug An unexpected behavior, bug, or error

Comments

@Viicos
Copy link
Contributor

Viicos commented Jan 7, 2025

Bug report

Bug description:

Any typing construct using typing._GenericAlias (and not types.GenericAlias) will convert the arguments to ForwardRef instances at runtime:

from typing import List

class A[T]:
    pass

List['F']
#> List[ForwardRef('F')]
A['F']
#> A[ForwardRef('F')]

Because _GenericAlias.__getitem__ calls are cached with the _tp_cache() decorator, we end up with the same ForwardRef instance used:

alias1 = List['F']
#> List[ForwardRef('F')]
alias2 = List['F']
#> List[ForwardRef('F')]

alias1.__args__[0] is alias2.__args__[0]
#> True

And this becomes an issue as ForwardRef._evaluate() calls are also cached per instance. Consider the following setup:

# mod1.py

def func(a: List['Forward']): pass

Forward = int

typing.get_type_hints(func)
# {'a': List[int]}

# mod2.py

def func(a: List['Forward']): pass

Forward = str

typing.get_type_hints(func)
# {'a': List[int]}

Note that this is already an issue on Python < 3.14. However, the impact is somewhat limited as afaik this only happens with functions. The reason is that there is a really old cache invalidation mechanism seemingly introduced to avoid a leak issue in ForwardRef._evaluate. This logic would force the evaluation of the forward reference (even if _evaluate was already called) if the provided localns is different from the globalns (on L920 the evaluation logic goes on):

cpython/Lib/typing.py

Lines 916 to 920 in dae5b16

def _evaluate(self, globalns, localns, type_params=None, *, recursive_guard):
if self.__forward_arg__ in recursive_guard:
return self
if not self.__forward_evaluated__ or localns is not globalns:
if globalns is None and localns is None:

Using typing.get_type_hints, this condition is false when getting annotations of functions, because functions don't have locals and locals are set to globals if this is the case:

cpython/Lib/typing.py

Lines 2441 to 2442 in 8f93dd8

elif localns is None:
localns = globalns


However, the implementation of PEP 649 removed this check and the cached evaluated value is unconditionally used:

def evaluate(self, *, globals=None, locals=None, type_params=None, owner=None):
"""Evaluate the forward reference and return the value.
If the forward reference cannot be evaluated, raise an exception.
"""
if self.__forward_evaluated__:
return self.__forward_value__

While the described bug above is pretty uncommon as it only occurs with functions, it also happens in classes with 3.14:

# mod1.py

class A:
    'a': List['Forward']

Forward = int

typing.get_type_hints(A)
# {'a': List[int]}

# mod2.py

class A:
    'a': List['Forward']

Forward = str

typing.get_type_hints(func)
# {'a': List[int]}

And I believe this is going to cause some issues with existing code bases, especially the ones using runtime typing libraries. In Pydantic, we already had issues like this one, as we recently changed our global/local namespace logic (and as such, we had a report of the above issue with classes: cloudflare/cloudflare-python#116 (comment)).

While we might argue that string annotations are no longer needed in 3.14, it is still relevant for libraries which need to keep support for older Python versions.

One possible solution would be to have _tp_cache() skip string arguments, so that we don't end up reusing the same ForwardRef instances. Not sure how big the impact will be.

CPython versions tested on:

3.14, CPython main branch

Operating systems tested on:

No response

@Viicos Viicos added the type-bug An unexpected behavior, bug, or error label Jan 7, 2025
@Viicos
Copy link
Contributor Author

Viicos commented Jan 7, 2025

cc @JelleZijlstra

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic-typing type-bug An unexpected behavior, bug, or error
Projects
None yet
Development

No branches or pull requests

2 participants