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.

Sign up for GitHub

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
wants to merge 1 commit into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 1 addition & 6 deletions include/pybind11/stl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -251,12 +251,7 @@ struct array_caster {
return l.release();
}

PYBIND11_TYPE_CASTER(ArrayType,
const_name("List[") + value_conv::name
+ const_name<Resizable>(const_name(""),
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

};

template <typename Type, size_t Size>
Expand Down
4 changes: 2 additions & 2 deletions tests/test_stl.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,8 +39,8 @@ def test_array(doc):
assert m.load_array(lst)
assert m.load_array(tuple(lst))

assert doc(m.cast_array) == "cast_array() -> List[int[2]]"
assert doc(m.load_array) == "load_array(arg0: List[int[2]]) -> bool"
assert doc(m.cast_array) == "cast_array() -> List[int]"
assert doc(m.load_array) == "load_array(arg0: List[int]) -> bool"


def test_valarray(doc):
Expand Down