-
Notifications
You must be signed in to change notification settings - Fork 2.2k
fix(types): type hints from future python versions #5693
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
fix(types): type hints from future python versions #5693
Conversation
Signed-off-by: Michael Carlstrom <rmc@carlstrom.com>
Signed-off-by: Michael Carlstrom <rmc@carlstrom.com>
Signed-off-by: Michael Carlstrom <rmc@carlstrom.com>
Signed-off-by: Michael Carlstrom <rmc@carlstrom.com>
Signed-off-by: Michael Carlstrom <rmc@carlstrom.com>
Signed-off-by: Michael Carlstrom <rmc@carlstrom.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great! Those match all stubgen errors concerning new Python features I found while working on #5678.
There is one additional typing issue I found though:
pybind11/include/pybind11/cast.h
Lines 1450 to 1453 in 1dd85ef
template <> | |
struct handle_type_name<weakref> { | |
static constexpr auto name = const_name("weakref"); | |
}; |
weakref
is not the correct type but just the module name.I think this should be
weakref.ReferenceType
(docs)Maybe we can squeeze this in here as well?
Your solution for X | Y
looks good to me as well.
For testing I am not so sure about all those if sys.version_info >= (3, XX):
with double asserts.
Would it maybe make sense to somehow move that into a common place as well (like in conftest.py
?
Signed-off-by: Michael Carlstrom <rmc@carlstrom.com>
Signed-off-by: Michael Carlstrom <rmc@carlstrom.com>
I forgot that |
Signed-off-by: Michael Carlstrom <rmc@carlstrom.com>
That would make the tests a lot simpler, so I think this is a good idea. How is the test output when the I can have a deeper look tomorrow evening on your latest changes. |
tests/test_opaque_types.py
Outdated
@@ -1,6 +1,7 @@ | |||
from __future__ import annotations | |||
|
|||
import pytest | |||
from conftest import across_version_type_hint_checker |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please, never import from conftest.py. You can put fixtures, etc. there, but don't import from it. It causes all sorts of issues (such as the issue holding up cibuildwheel 3.0 in numpy right now!); it's not meant to be an importable module for users. We sadly have one case (tests/test_docs_advanced_cast_custom.py
), but I'd like to get rid of that, not add more. Edit: it's only at type check time!
Instead of adding a test/utils
directory just for this, and requiring PyTest 7+, this can be wrapped in a fixture for now.
Since we do have one case of importing from conftest, I'm also okay to leave this in for now and add the test utils in a followup later, as anything broken by it will already have workarounds.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this also reduce the quality of assertion rewriting?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI, wrapping it in a fixture could look like:
@pytest.fixture
def backport_typehints() -> Callable[[str], str]:
d = {}
if sys.version_info < (3, 13):
d["typing.TypeIs"] = "typing_extensions.TypeIs"
d["types.CapsuleType"] = "typing_extensions.CapsuleType"
if sys.version_info < (3, 12):
d["collections.abc.Buffer"] = "typing_extensions.Buffer"
if sys.version_info < (3, 11):
d["typing.Never"] = "typing_extensions.Never"
if sys.version_info < (3, 10):
d["typing.TypeGuard"] = "typing_extensions.TypeGuard"
def backport(text: str) -> str:
for old, new in d.items():
text = text.replace(old, new)
return text
return backport
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please, never import from conftest.py. You can put fixtures, etc. there, but don't import from it. It causes all sorts of issues (such as the issue holding up cibuildwheel 3.0 in numpy right now!); it's not meant to be an importable module for users.
We sadly have one case(tests/test_docs_advanced_cast_custom.py
), but I'd like to get rid of that, not add more. Edit: it's only at type check time!
Just to clarify and to learn from it (since I added that conftest.py import in tests/test_docs_advanced_cast_custom.py
):
Would you also discourage from importing conftest.py while using if TYPE_CHECKING
?
Generally, I like having type hints for function arguments for better code readability and navigation.
Vscode/Pylance is capable of matching test arguments to fixtures and show gray type hints without manual type annotations, so I could see not having those in tests.
Signed-off-by: Michael Carlstrom <rmc@carlstrom.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two minor comments, otherwise this looks good to me.
Signed-off-by: Michael Carlstrom <rmc@carlstrom.com>
Signed-off-by: Michael Carlstrom <rmc@carlstrom.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR modernizes type hint generation by switching from typing.Union
/Optional
syntax to PEP 604 (‘|’) across C++ headers and Python tests, and adds a pytest fixture to backport new hints on older Python versions.
- Replace legacy
typing.Union[...]
/typing.Optional[...]
strings with the|
operator in C++io_name
andhandle_type_name
. - Introduce
backport_typehints
fixture inconftest.py
to swaptyping_extensions
hints to built-intyping
on older Pythons. - Update test assertions throughout
tests/
to expect the new union-syntax and usebackport_typehints
where needed.
Reviewed Changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
tests/test_stl.py | Add docstring checks for reference-sensitive and filesystem types |
tests/test_pytypes.py | Update expected doc outputs to use ` |
tests/test_pytypes.cpp | Change io_name for RealNumber to `"float |
tests/test_opaque_types.py | Add backport_typehints fixture to pointer error message tests |
tests/test_buffers.py | Simplify buffer docstring test and use new hint syntax |
tests/conftest.py | Introduce backport_typehints fixture for hint backporting |
include/pybind11/typing.h | Use union_concat and ` |
include/pybind11/stl/filesystem.h | Switch filesystem caster to `os.PathLike |
include/pybind11/stl.h | Simplify optional_caster /variant_caster to pipe syntax |
include/pybind11/detail/descr.h | Implement union_concat and `operator |
include/pybind11/detail/common.h | Add macros for typing_extensions vs. built-in hints |
include/pybind11/cast.h | Update CapsuleType and anyset names to use compatibility macros |
Description
As shown in #5678 some types need to use typing_extensions if they don't exists yet. Similar to #5663 but, for the rest of the types.
Suggested changelog entry:
typing_extensions
alternatives for all types that need them