Skip to content

gh-130870 Fix _eval_type Handling for GenericAlias with Unflattened Arguments and Union Types #130897

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

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

sharktide
Copy link
Contributor

@sharktide sharktide commented Mar 5, 2025

Fixes the handling of GenericAlias types in _eval_type by correctly unflattening callable arguments when necessary. Also improves the resolution of Union types by evaluating their arguments properly.

Tests (View comments below):
#130897 (comment)

@bedevere-app
Copy link

bedevere-app bot commented Mar 5, 2025

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

@@ -0,0 +1,6 @@
Type: Bugfix
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is too much text; we don't need the "type" and "title" (we get that from the linked issue number). We don't have to mention that the tests pass: if they didn't, we wouldn't merge the change.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this is screaming AI-generated to me, as well as the PR description.

@sharktide, it's OK to use AI for some cases, but please refer to the devguide for the rules on using LLMs to generate PRs.

Copy link
Contributor Author

@sharktide sharktide Mar 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was running out of time as I was at school and I had a short break so I asked chatgpt to make the news entry and pr desc. The first few lines of the news entry I basically copied from my previous prs as I don’t have much experience

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is too much text; we don't need the "type" and "title" (we get that from the linked issue number). We don't have to mention that the tests pass: if they didn't, we wouldn't merge the change.

I’ll change it when I get a chance

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No worries, feel free to ask us for help or clarification on the process.

Copy link
Contributor Author

@sharktide sharktide Mar 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed it to this

Issue: :gh:`130870`

Detailed changes:
This change improves the handling of ``GenericAlias`` and ``Union`` types in ``_eval_type``, ensuring that callable arguments for ``GenericAlias`` are unflattened and that ``Union`` types are properly evaluated.

@@ -486,7 +486,9 @@ def _eval_type(t, globalns, localns, type_params=_sentinel, *, recursive_guard=f
if ev_args == t.__args__:
return t
if isinstance(t, GenericAlias):
return GenericAlias(t.__origin__, ev_args)
if _should_unflatten_callable_args(t, ev_args):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the motivation for calling _should_unflatten_callable_args here?

Please add tests demonstrating the new behavior.

Copy link
Contributor Author

@sharktide sharktide Mar 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why call _should_unflatten_callable_args:

_should_unflatten_callable_args is used to make sure that the arguments of a callable type are handled properlt sometimes, the arguments are grouped together like in a list and need to be unpacked into separate arguments. I'll link a modified version of the test script here and then also link the results

*Sorry my capitalization sucks

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I made a mistake. The appropriate example should be

Callable[[arg1, arg2], result]

Copy link
Contributor Author

@sharktide sharktide Mar 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is my test script:

import typing
from typing import ForwardRef, Union, List, Tuple, Dict, Callable, Any
import functools
import operator
from typing import _eval_type

class MyType:
    pass

# Test 1
def test_forward_ref():
    fwd_ref = ForwardRef('MyType')
    result = _eval_type(fwd_ref, globals(), locals())
    assert result is MyType, f"Expected MyType, got {result}"

# Test 2
def test_generic_alias():
    fwd_ref = ForwardRef('MyType')
    generic_list = List[fwd_ref]
    result = _eval_type(generic_list, globals(), locals())
    assert result == List[MyType], f"Expected List[MyType], got {result}"

# Test 3
def test_union():
    fwd_ref_1 = ForwardRef('MyType')
    fwd_ref_2 = ForwardRef('int')
    union_type = Union[fwd_ref_1, fwd_ref_2]
    
    result = _eval_type(union_type, globals(), locals())
    assert result == Union[MyType, int], f"Expected Union[MyType, int], got {result}"

# Test 4
def test_recursive_forward_ref():
    recursive_ref = ForwardRef('RecursiveType')
    globals()['RecursiveType'] = recursive_ref

    recursive_type = Dict[str, List[recursive_ref]]
    
    result = _eval_type(recursive_type, globals(), locals(), recursive_guard={recursive_ref})
    
    assert result == Dict[str, List[recursive_ref]], f"Expected Dict[str, List[RecursiveType]], got {result}"


# Test 5
def test_callable_unpacking():
    fwd_ref = ForwardRef('MyType')
    callable_type = Callable[[fwd_ref, int], str]
    result = _eval_type(callable_type, globals(), locals())
    
    assert result == Callable[[MyType, int], str], f"Expected Callable[[MyType, int], str], got {result}"

# Test 6
def test_unpacked_generic():
    fwd_ref = ForwardRef('MyType')
    generic_type = Tuple[fwd_ref, int]
    
    result = _eval_type(generic_type, globals(), locals())
    assert result == Tuple[MyType, int], f"Expected Tuple[MyType, int], got {result}"

# Test 7
def test_preservation_of_type():
    fwd_ref_1 = ForwardRef('MyType')
    fwd_ref_2 = ForwardRef('int')
    complex_type = Dict[str, Union[fwd_ref_1, fwd_ref_2]]
    
    result = _eval_type(complex_type, globals(), locals())
    assert result == Dict[str, Union[MyType, int]], f"Expected Dict[str, Union[MyType, int]], got {result}"

# Test 8 (As per request of @JelleZijlstra)
def test_callable_unflattening():
    callable_type = Callable[[int, str], bool]
    result = _eval_type(callable_type, globals(), locals(), type_params=())
    assert result == Callable[[int, str], bool], f"Expected Callable[[int, str], bool], got {result}"

    callable_type_packed = Callable[[int, str], bool]  # Correct format for callable
    result = _eval_type(callable_type_packed, globals(), locals(), type_params=())
    assert result == Callable[[int, str], bool], f"Expected Callable[[int, str], bool], got {result}"



def run_tests():
    test_forward_ref()
    test_generic_alias()
    test_union()
    test_recursive_forward_ref()
    test_callable_unpacking()
    test_unpacked_generic()
    test_preservation_of_type()
    test_callable_unflattening()
    print("All tests passed!")

# Run the tests
run_tests()

And result:

PS C:\Users\---\Documents\GitHub\cpython> .\PCbuild\amd64\python_d.exe test.py
C:\Users\---\Documents\GitHub\cpython\test.py:16: DeprecationWarning: Failing to pass a value to the 'type_params' parameter of 'typing._eval_type' is deprecated, as it leads to incorrect behaviour when calling typing._eval_type on a stringified annotation that references a PEP 695 type parameter. It will be disallowed in Python 3.15.
  result = _eval_type(fwd_ref, globals(), locals())
C:\Users\---\Documents\GitHub\cpython\test.py:24: DeprecationWarning: Failing to pass a value to the 'type_params' parameter of 'typing._eval_type' is deprecated, as it leads to incorrect behaviour when calling typing._eval_type on a stringified annotation that references a PEP 695 type parameter. It will be disallowed in Python 3.15.
  result = _eval_type(generic_list, globals(), locals())
C:\Users\---\Documents\GitHub\cpython\test.py:33: DeprecationWarning: Failing to pass a value to the 'type_params' parameter of 'typing._eval_type' is deprecated, as it leads to incorrect behaviour when calling typing._eval_type on a stringified annotation that references a PEP 695 type parameter. It will be disallowed in Python 3.15.
  result = _eval_type(union_type, globals(), locals())
C:\Users\---\Documents\GitHub\cpython\test.py:47: DeprecationWarning: Failing to pass a value to the 'type_params' parameter of 'typing._eval_type' is deprecated, as it leads to incorrect behaviour when calling typing._eval_type on a stringified annotation that references a PEP 695 type parameter. It will be disallowed in Python 3.15.
  result = _eval_type(recursive_type, globals(), locals(), recursive_guard={recursive_ref})
C:\Users\---\Documents\GitHub\cpython\test.py:57: DeprecationWarning: Failing to pass a value to the 'type_params' parameter of 'typing._eval_type' is deprecated, as it leads to incorrect behaviour when calling typing._eval_type on a stringified annotation that references a PEP 695 type parameter. It will be disallowed in Python 3.15.
  result = _eval_type(callable_type, globals(), locals())
C:\Users\---\Documents\GitHub\cpython\test.py:66: DeprecationWarning: Failing to pass a value to the 'type_params' parameter of 'typing._eval_type' is deprecated, as it leads to incorrect behaviour when calling typing._eval_type on a stringified annotation that references a PEP 695 type parameter. It will be disallowed in Python 3.15.
  result = _eval_type(generic_type, globals(), locals())
C:\Users\---\Documents\GitHub\cpython\test.py:76: DeprecationWarning: Failing to pass a value to the 'type_params' parameter of 'typing._eval_type' is deprecated, as it leads to incorrect behaviour when calling typing._eval_type on a stringified annotation that references a PEP 695 type parameter. It will be disallowed in Python 3.15.
  result = _eval_type(complex_type, globals(), locals())
All tests passed!

Note: The DeprecationWarning is a problem with my test script rather than the modified function.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test cases should go in test_typing.py.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will do

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JelleZijlstra Done added class TestGenericAliasHandling to test_typing.py

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JelleZijlstra Ready for review

@bedevere-app
Copy link

bedevere-app bot commented Mar 6, 2025

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@sharktide
Copy link
Contributor Author

I have made the requested changes; please review again

@bedevere-app
Copy link

bedevere-app bot commented Mar 6, 2025

Thanks for making the requested changes!

@JelleZijlstra: please review the changes made to this pull request.

@sharktide
Copy link
Contributor Author

@JelleZijlstra

HI! Sorry to disturb you, but could you please re-review this pull request? TiA

@sharktide
Copy link
Contributor Author

@JelleZijlstra or @AlexWaygood Could you please check this PR again? It has been over a month I have been waiting.

@JelleZijlstra
Copy link
Member

The test cases don't use any public APIs so as far as I can see this PR doesn't change any behaviors that we care about.

@sharktide
Copy link
Contributor Author

sharktide commented Apr 12, 2025

The test cases don't use any public APIs so as far as I can see this PR doesn't change any behaviors that we care about.

@JelleZijlstra
While the changes might seem to address internal behaviors, They also contribute to more robust handling of GenericAlias and Union types within the _eval_type function (imo), ensuring that edge cases like callable argument unpacking and recursive type evaluation are properly managed. This improvement could indirectly enhance the reliability of features relying on these internal implementations.

It took me so long to write that :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants