Skip to content

Fix issue #3912 : invalid docstring generation of std::array <-> list caster #3916

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

Closed

Conversation

Skylion007
Copy link
Collaborator

Description

Suggested changelog entry:

* Fixes doc generation for std::array-list caster. Previously, signatures included the size of the list in a non-standard, non-spec compliant way.

@Skylion007 Skylion007 requested review from rwgk, wjakob and henryiii April 29, 2022 15:54
@wjakob
Copy link
Member

wjakob commented Apr 29, 2022

This PR achieves MyPy compliance in exchange for potentially very confusing situations where a pybind11-bound function does not accept an input array, and the error message provides zero context why that is (input has the wrong size)

@wjakob
Copy link
Member

wjakob commented Apr 29, 2022

These signatures in pybind11 precede the time where MyPy was even a thing, so "non-standard non-spec compliant" seems a bit strong of a qualifier.

@Skylion007
Copy link
Collaborator Author

@wjacob, in >=3.9 we can use the Annotated typing extension to specify the length of the list. Hmm, not sure if there is a standard compliant way to specify the type for anything besides Tuples though.

const_name("[") + const_name<Size>()
+ const_name("]"))
+ const_name("]"));
PYBIND11_TYPE_CASTER(ArrayType, const_name("List[") + value_conv::name + const_name("]"));
Copy link
Contributor

Choose a reason for hiding this comment

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

I think if you make the string typing.Annotated[List[type], size] then pybind11-stubgen will generate the correct thing from this (and if it doesn't then you can just modify it to do so).

Copy link
Contributor

Choose a reason for hiding this comment

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

Also note, it's just a string. You don't need typing extensions in pybind11, that's the responsibility of whoever interprets the string.

Copy link

Choose a reason for hiding this comment

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

Annotated[List[type], size] appears to be the intended standard way to hint the length of a list. PEP 593 shows this use case as an example.

I would just remove typing. from your suggestion since List is not prefixed either.

This PR achieves MyPy compliance in exchange for potentially very confusing situations where a pybind11-bound function does not accept an input array, and the error message provides zero context why that is (input has the wrong size)

@wjakob My understanding is that this PR only changes the type hint and does not affect the error message otherwise. Is there a situation where Annotated[List[str], 2] would be inferior to List[str[2]]?

Copy link
Contributor

Choose a reason for hiding this comment

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

@felixvd implemented as #4679

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a situation where Annotated[List[str], 2] would be inferior to List[str[2]]?

According to #4679 (comment) , plain 2 does not look good

@cielavenir
Copy link
Contributor

@Skylion007 @rwgk can we close this one?

@rwgk
Copy link
Collaborator

rwgk commented May 25, 2023

Closing after #4679 was merged.

@rwgk rwgk closed this May 25, 2023
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.

[BUG]: Invalid typing for std::array?
6 participants