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

bpo-43224: Implement substitution of unpacked TypeVarTuple #31800

Merged
merged 5 commits into from
Mar 11, 2022

Conversation

serhiy-storchaka
Copy link
Member

@serhiy-storchaka serhiy-storchaka commented Mar 10, 2022

Lib/typing.py Outdated
raise NotImplementedError(
"Type substitution for TypeVarTuples is not yet implemented"
)
if len(self.__parameters__) == 1 and isinstance(self.__parameters__[0], TypeVarTuple):
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure it's sufficient to only check for the case where len(self.__parameters__) == 1 - we could have e.g.:

T1 = TypeVar('T1')
T2 = TypeVar('T2')
Ts = TypeVarTuple('Ts')
class A(Generic[T1, T2, Unpack[Ts]]): pass
B = A[int, T2, Unpack[Ts]]
C = B[str, float]
print(C)

There, B.__parameters__ is (T2, Ts), causing a TypeError later on in the code.

Copy link
Member Author

Choose a reason for hiding this comment

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

You are right. The previous code only worked with existing tests. New code adds tests for generics with multiple type variables.

@mrahtz
Copy link
Contributor

mrahtz commented Mar 10, 2022

Thanks for looking into this!

@mrahtz
Copy link
Contributor

mrahtz commented Mar 12, 2022

Woah, I'm not sure merging this was a good idea. We discussed this extensively in #31021 (review) and decided against implementing it this way. There are too many edge cases (e.g. the merged implementation will happily assign an unpacked arbitrary-length tuple such as *tuple[int, ...] to a plain TypeVar, which is not legal - see the extensive tests in b9b1c80#diff-04d29c98076c2d6bb75921ea9becb26a862544d39b71db87b6e354c759b9305dL794), and @JelleZijlstra suspected it might cause issues with Annotated.

I'd strongly prefer this merge to be reversed in favour of #31804 and/or further discussion on what the right approach is.

@serhiy-storchaka
Copy link
Member Author

I think that list[T][int] should return list[int], and tuple[*Ts][int, str] should return tuple[int, str].

Workaround can be used in cases where this does not work.

@mrahtz
Copy link
Contributor

mrahtz commented Mar 13, 2022

If you're adamant about keeping this implementation, I don't feel happy about it, but I don't want this to become a blocker for us. I'll submit a new PR with extra test cases so we can be more confident we've covered the edge cases.

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.

4 participants