Skip to content

Add typing and collections.abc module prefix #5663

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 25 commits into
base: master
Choose a base branch
from

Conversation

gentlegiantJGC
Copy link
Contributor

@gentlegiantJGC gentlegiantJGC commented May 14, 2025

Description

Add typing. and collections.abc prefix to remove ambiguity.

Suggested changelog entry:

Fix typing and collections.abc type hint ambiguity.

gentlegiantJGC and others added 2 commits May 14, 2025 09:58
These type hints are invalid in Python 3.8.
Add `typing.` prefix to remove ambiguity.
@gentlegiantJGC
Copy link
Contributor Author

The tests need updating to reflect these changes.
Please give an indication if this is something you would consider merging and I will update the tests.

@henryiii
Copy link
Collaborator

@InvincibleRMC, thoughts?

@InvincibleRMC
Copy link
Contributor

InvincibleRMC commented May 14, 2025

I think this is a good idea. This was discussed a little in and around #5566 (comment). Adding the module prefix will allow users to create their own Sequence class for example and not have the type hints conflict with typing.Sequence see issues like sizmailov/pybind11-stubgen#241.

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, but do we actually still want to support Python 3.8?

Python 3.8 EOL was 2024-10-07

@@ -239,6 +239,27 @@
# define PYBIND11_SUBINTERPRETER_SUPPORT
#endif

// 3.9 Compatibility
#if 0x03090000 <= PY_VERSION_HEX
# define PYBIND11_ITERABLE_TYPE_HINT "collections.abc.Iterable"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggestion to make these

PYBIND11_TYPE_HINT_...

so they line up nicely and are easier to pin-point.

@@ -239,6 +239,27 @@
# define PYBIND11_SUBINTERPRETER_SUPPORT
#endif

// 3.9 Compatibility
Copy link
Collaborator

Choose a reason for hiding this comment

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

// Python 3.9+ Compatibility

?
(The + is the most important aspect of my suggestion.)

@gentlegiantJGC
Copy link
Contributor Author

do we actually still want to support Python 3.8?

You still have test runners for 3.8 so I assumed you still supported it.
I couldn't find anything saying what your minimum supported version was.
If you want to drop support then it would simplify this.

@rwgk
Copy link
Collaborator

rwgk commented May 14, 2025

If you want to drop support then it would simplify this.

Me: yes

But I'll go with @henryiii's judgement.

@henryiii
Copy link
Collaborator

Let's keep it for 3.0. We can drop it for 3.1.

@henryiii
Copy link
Collaborator

I couldn't find anything saying what your minimum supported version was.

It's in setup.cfg (or pyproject.toml after #5598)

@gentlegiantJGC
Copy link
Contributor Author

I think I am going to scale back this pull request to just adding the module prefix.
Fixing the issue in 3.8 is going to require a lot of changes to the tests which seems pointless for a version that has reached end of life.
I will leave the 3.8 macros but commented out so someone can add them later if they want.

Fixing this issue in Python 3.8 will require updating lots of tests. This can be added in a further pull request.
@gentlegiantJGC gentlegiantJGC changed the title Fix Python 3.8 type hints and add module prefix Add typing and collections.abc module prefix May 15, 2025
@gentlegiantJGC
Copy link
Contributor Author

I have fixed the tests.
This only includes adding the module prefixes for typing and collections.abc.
I have no personal interest in supporting Python 3.8 so I will leave that to someone else.
I have left the type hint macros for 3.8 in common.h but commented out to make it easier if someone does want to fix the 3.8 issues. This should just be a case of uncommenting the macros and updating the tests with a switch based on python version.

@henryiii
Copy link
Collaborator

If you want it to go into 3.0, 3.8 needs to work (where "work" is very loosely defined as "not be worse that it is now"). If you are fine to wait till after 3.0 (like a week), then no need to worry about 3.8. I'd personally go for the latter.

@gentlegiantJGC
Copy link
Contributor Author

This can wait for 3.1. I have removed the 3.8 compatibility macros and moved the strings inline.

@gentlegiantJGC
Copy link
Contributor Author

I don't know why that test is failing

@rwgk
Copy link
Collaborator

rwgk commented May 17, 2025

All tests pass.

@gentlegiantJGC do you want to merge this now (looks good to me)? Or wait until after the v3.0.0 release?

@gentlegiantJGC
Copy link
Contributor Author

This can't be merged yet because it changes the following which are incompatible with Python 3.8
(typing.)Iterable -> collections.abc.Iterable
(typing.)Iterator -> collections.abc.Iterator
(typing.)Callable -> collections.abc.Callable
(typing.)Sequence -> collections.abc.Sequence

I decided not to support 3.8 because it would make the codebase more complex for something which is getting removed which felt pointless.

@rwgk
Copy link
Collaborator

rwgk commented May 17, 2025

I decided not to support 3.8 because it would make the codebase more complex for something which is getting removed which felt pointless.

Sounds good. I'm just curious: What exactly is incompatible? Certain type checkers when running under 3.8? We're not testing that here? Could/should we?

@henryiii henryiii added this to the v3.1 milestone May 18, 2025
@henryiii
Copy link
Collaborator

Aren't these strings? Those are valid at type-check time, just not turning them into non-string annotations at runtime. I think that's not terrible, though? We might even have examples elsewhere? Our annotations are very much not runnable at runtime already.

@gentlegiantJGC
Copy link
Contributor Author

You are correct that this changes nothing about the runtime behaviour.
It only changes the type hints that pybind11 embeds in the docstrings that can be used to generate stub files which static type checkers can use.

The collections.abc variants were added in 3.9 so a static type checker would correctly error that they don't exist.
That being said typing.h currently uses tuple[], dict[], list[] and set[] which were not subscriptable in Python 3.8 so static type checking in 3.8 is already broken if any of those are used. This behaviour was also implemented in 3.9.

@gentlegiantJGC
Copy link
Contributor Author

We're not testing that here? Could/should we?

It probably wouldn't hurt. You can use pybind11-stubgen to generate stubs for your test suite and then run mypy on the result.

It would catch issues like this in the future.

@henryiii
Copy link
Collaborator

Type checkers shouldn't care if a .pyi file is not runnable. Just like they don't care about from __future__ import annotations, or if you put the string "list[int]" into a type annotation. Runtime cares, but runtime can't access a .pyi file anyway.

Generally for type hints, if new syntax ([] or |, for example) gets added, type hints can update to it unless they are runtime evaluated (which the default in <3.14, but strings, future, and pyi are not).

@gentlegiantJGC
Copy link
Contributor Author

Upon further research it seems that mypy has backported class subcription and collection.abc classes to 3.7 so I see no issue merging this now. I assumed they would use the functionality of that version.

I still think it is worth validating the generated type hints. It would have caught issues like #5662 which did raise errors in mypy in older versions.

# type_check.pyi
from __future__ import annotations

from collections.abc import Buffer

def return_buffer() -> Buffer: ...
# mypy -m type_check
type_check.pyi:3: error: Module "collections.abc" has no attribute "Buffer"  [attr-defined]
Found 1 error in 1 file (checked 1 source file)

@timohl
Copy link
Contributor

timohl commented May 19, 2025

Good change (also thanks for the typing.Buffer PR 👍).

This can't be merged yet because it changes the following which are incompatible with Python 3.8 (typing.)Iterable -> collections.abc.Iterable (typing.)Iterator -> collections.abc.Iterator (typing.)Callable -> collections.abc.Callable (typing.)Sequence -> collections.abc.Sequence

I decided not to support 3.8 because it would make the codebase more complex for something which is getting removed which felt pointless.

If using the collections.abc.* variants over typing.* creates problems with type checkers in Python 3.8, you could still use the typing.* variants here and when 3.8 is dropped, it could be switched to collections.abc.* (should be a simple search and replace).
Well, IF pybind should have correct type hints for 3.8.

Looking over the current state of type hints:

  • typing.Annotated is also a candidate that breaks 3.8 type checking right now (was added in 3.9).
  • types.CapsuleType was added in 3.13, maybe this should be handled similar to typing.Buffer?

In my opinion, pybind v3 should support correct type hints for 3.9+.
Previously, type hints were broken in several places for all Python versions anyway.
If support for 3.8 (which officially reached end-of-life in October 2024) is going to be dropped anyway, type hints might as well stay partially broken until then (unless somebody finds this important and wants to put in the work).

For 3.9+ type hints, we could also think about replacing typing.Union[X, Y] with X | Y and typing.Optional[X] with X | None.
Not an important change but makes things more compact and "Using that [Union] shorthand is recommended." (Python docs)

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.

5 participants