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

feat(types): add TypeVars / method generics typing #5167

Merged
merged 54 commits into from
Jun 25, 2024

Conversation

InvincibleRMC
Copy link
Contributor

@InvincibleRMC InvincibleRMC commented Jun 14, 2024

Description

Add supports for Typevars.
Requires __cpp_nontype_template_parameter_class feature.

To create the new a new typing::TypeVar use the following syntax.

typedef py::typing::TypeVar<"T"> TypeVarT;

This allows use to create type annotations like the following.

def foo(arg: list[T]) -> T: ...

Suggested changelog entry:

Adds support for python typing ``TypeVar`` to annotations.

@InvincibleRMC
Copy link
Contributor Author

InvincibleRMC commented Jun 14, 2024

TypeVars seem to be working for all versions of compilers with C++20 that support string literals in templates. Now I ask should the feature be dropped to maintain capability with older C++ standards. Or is there precedent for C++ version guarding features?

@InvincibleRMC InvincibleRMC marked this pull request as ready for review June 14, 2024 17:24
@InvincibleRMC InvincibleRMC changed the title typevar prototype Add TypeVars / method generics Jun 14, 2024
@InvincibleRMC InvincibleRMC changed the title Add TypeVars / method generics Add TypeVars / method generics typing Jun 14, 2024
@rwgk rwgk marked this pull request as draft June 14, 2024 19:02
@rwgk
Copy link
Collaborator

rwgk commented Jun 14, 2024

I converted this to Draft since this looks like you're still working on it.

Is the idea to add ifdef(C++20)?

@InvincibleRMC
Copy link
Contributor Author

Yeah. I wasn't sure if that was fine since some projects want equal support for different C++ standards.

@henryiii
Copy link
Collaborator

There are C++14 (py::overload_cast) and C++17 (std::variant and such) only features. Not sure if we have any C++20 features yet.

Copy link
Collaborator

@rwgk rwgk left a comment

Choose a reason for hiding this comment

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

@henryiii wrote:

Not sure if we have any C++20 features yet.

Me too; I don't think so. I'm only aware of accommodations for C++20 (module_ and PYBIND11_CPP20).

But I cannot think of reasons against adding conditional support for C++20 features. Are there concerns I'm missing?

include/pybind11/typing.h Outdated Show resolved Hide resolved
include/pybind11/typing.h Outdated Show resolved Hide resolved
include/pybind11/typing.h Show resolved Hide resolved
tests/test_pytypes.cpp Show resolved Hide resolved
tests/test_pytypes.py Outdated Show resolved Hide resolved
@InvincibleRMC InvincibleRMC marked this pull request as draft June 15, 2024 14:52
include/pybind11/typing.h Outdated Show resolved Hide resolved
include/pybind11/typing.h Outdated Show resolved Hide resolved
@InvincibleRMC InvincibleRMC marked this pull request as ready for review June 15, 2024 16:11
Copy link
Collaborator

@rwgk rwgk left a comment

Choose a reason for hiding this comment

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

Looks good to me.

Could you please make a pass through the PR description (quick and simple, I think): Is "Prototyping" still what you have in mind? And we need a Suggested changlog entry.

@InvincibleRMC InvincibleRMC marked this pull request as draft June 16, 2024 22:32
@InvincibleRMC
Copy link
Contributor Author

InvincibleRMC commented Jun 16, 2024

A potential problem I discovered in testing is stub generators like mypy's stubgen or pybind11-stubgen will need to recognize TypeVars from the compiled annotations to create the TypeVar objects themselves to then be used. I'm not sure of the best way of notifying the stub generators to create TypeVar instantiations. Another option would be to modify the order resolution of of the annotation generation to use the Python3.12 inline TypeVar declaration.

Python < 3.12

from typing import TypeVar
T = TypeVar('T')

def foo(arg0: T) -> T: ...

Python >= 3.12

def foo[T](arg0: T) -> T: ...

@rwgk
Copy link
Collaborator

rwgk commented Jun 18, 2024

A potential problem I discovered in testing is stub generators like mypy's stubgen or pybind11-stubgen will need to recognize TypeVars from the compiled annotations to create the TypeVar objects themselves to then be used.

I only have a very sketchy idea of what mypy's stubgen is doing. But while working on #4888 I learned that stubgen has fallback mechanisms if it cannot process some annotations.

Do you believe this PR will make things worse?

@InvincibleRMC
Copy link
Contributor Author

It is not ideal, but does not make anything worse. If that is acceptable I'm fine merging it in.

@InvincibleRMC InvincibleRMC marked this pull request as ready for review June 18, 2024 13:58
@rwgk
Copy link
Collaborator

rwgk commented Jun 18, 2024

It is not ideal, but does not make anything worse. If that is acceptable I'm fine merging it in.

I'm for merging this: it's clearly making pybind11 more complete/correct, with very little extra code. — The very minimal two-way collaboration between pybind11 and mypy stubgen is a bigger problem that hopefully will get addressed down the road (something like #4888).

@henryiii What's your opinion?

@henryiii henryiii merged commit aebcd70 into pybind:master Jun 25, 2024
83 checks passed
@henryiii henryiii changed the title Add TypeVars / method generics typing feat(types): add TypeVars / method generics typing Jun 25, 2024
@github-actions github-actions bot added the needs changelog Possibly needs a changelog entry label Jun 25, 2024
@henryiii henryiii removed the needs changelog Possibly needs a changelog entry label Jun 26, 2024
@InvincibleRMC InvincibleRMC deleted the typevars branch July 1, 2024 13:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants