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.

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

[RFC] Render typed iterators in docstrings (alternative) #2371

Draft
wants to merge 8 commits into
base: master
Choose a base branch
from
Draft

[RFC] Render typed iterators in docstrings (alternative) #2371

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
91ca4e6
Render typed iterators (e.g. Iterator[int]) in docstrings
sizmailov May 14, 2020
8bbfeea
Introduce make_iterator_ng/make_key_iterator_ng,
sizmailov Aug 7, 2020
0e360b7
Revert `iterator_passthrough` test change
sizmailov Aug 7, 2020
036add2
Use make_iterator_ng/make_key_iterator_ng in tests
sizmailov Aug 7, 2020
96bdc5f
Use concrete return types instead of auto
sizmailov Aug 7, 2020
b1dbc30
fix: typo
sizmailov Aug 7, 2020
98763aa
Avoid deprecation warning in iterator_passthrough
sizmailov Aug 7, 2020
c5500d8
Make use of PYBIND11_DEPRECATED macro
sizmailov Aug 7, 2020
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
76 changes: 67 additions & 9 deletions include/pybind11/pybind11.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1712,6 +1712,22 @@ struct iterator_state {
bool first_or_done;
};

template<typename Iterator, typename Sentinel, return_value_policy Policy>
struct type_caster<iterator_state<Iterator, Sentinel, false, Policy>> :
type_caster_base<iterator_state<Iterator, Sentinel, false, Policy>> {
using ValueType = decltype(*std::declval<Iterator>());
public:
static constexpr auto name = _("Iterator[") + make_caster<ValueType>::name + _("]");
};

template<typename Iterator, typename Sentinel, return_value_policy Policy>
struct type_caster<iterator_state<Iterator, Sentinel, true, Policy>> :
type_caster_base<iterator_state<Iterator, Sentinel, true, Policy>> {
using ValueType = decltype((*std::declval<Iterator>()).first);
public:
static constexpr auto name = _("Iterator[") + make_caster<ValueType>::name + _("]");
};

PYBIND11_NAMESPACE_END(detail)

/// Makes a python iterator from a first and past-the-end C++ InputIterator.
Expand All @@ -1720,7 +1736,8 @@ template <return_value_policy Policy = return_value_policy::reference_internal,
typename Sentinel,
typename ValueType = decltype(*std::declval<Iterator>()),
typename... Extra>
iterator make_iterator(Iterator first, Sentinel last, Extra &&... extra) {
detail::iterator_state<Iterator, Sentinel, false, Policy>
make_iterator_ng(Iterator first, Sentinel last, Extra &&... extra) {
typedef detail::iterator_state<Iterator, Sentinel, false, Policy> state;

if (!detail::get_type_info(typeid(state), false)) {
Expand All @@ -1738,8 +1755,7 @@ iterator make_iterator(Iterator first, Sentinel last, Extra &&... extra) {
return *s.it;
}, std::forward<Extra>(extra)..., Policy);
}

return cast(state{first, last, true});
return state{first, last, true};
}

/// Makes an python iterator over the keys (`.first`) of a iterator over pairs from a
Expand All @@ -1749,7 +1765,8 @@ template <return_value_policy Policy = return_value_policy::reference_internal,
typename Sentinel,
typename KeyType = decltype((*std::declval<Iterator>()).first),
typename... Extra>
iterator make_key_iterator(Iterator first, Sentinel last, Extra &&... extra) {
detail::iterator_state<Iterator, Sentinel, true, Policy>
make_key_iterator_ng(Iterator first, Sentinel last, Extra &&... extra) {
typedef detail::iterator_state<Iterator, Sentinel, true, Policy> state;

if (!detail::get_type_info(typeid(state), false)) {
Expand All @@ -1768,23 +1785,64 @@ iterator make_key_iterator(Iterator first, Sentinel last, Extra &&... extra) {
}, std::forward<Extra>(extra)..., Policy);
}

return cast(state{first, last, true});
return state{first, last, true};
}

/// Makes a python iterator from a first and past-the-end C++ InputIterator.
template <return_value_policy Policy = return_value_policy::reference_internal,
typename Iterator,
typename Sentinel,
typename ValueType = decltype(*std::declval<Iterator>()),
typename... Extra>
[[deprecated("Superseded by make_iterator_ng")]]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Deprecation of long-standing function is a hostile action to users, since it would lead to warnings in user code, which would force everyone to use _ng version of functions.

This can be viewed as the cost of making the change fully backward-compatible.
Having two similar functions is not a good thing, so one should be marked as deprecated in favor of superior one.

I'm not 100% happy about this. Is there other viable option?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think if I were to consider this (using a suffix and deprecate the old) vs. #2244 (updating the function), my money would be still be on #2244 :(

In terms of incompatibility, I think that could be solved by using a (deprecated?) implicit conversion from py::detail::iterator_state<...> to py::iterator.

However, per @wjakob's points about complexity, I'm not sure if this PR avoids the complexity from before, due to the deprecation (which may lead to another name swap + deprecation from make_iterator_ng back to make_iterator?).

The best option (that I can think of) is to mirror the setup used in numpy.h for py::array and py::array_t:

Let iterator be the fully type-erased Python interface type, while iterator_t<ValueType> can add a (minimal) additional "C++ type-friendly" filter on top of iterator (e.g. the casting name, forced casting, etc.). If you do that, then perhaps you only need iterator_t<ValueType>, rather than carrying around type info in detail::iterator_state<Iterator, Sentinel, bool, Policy> and use all of the decltype(...) stuff? Then you can update the stl.h stuff to use iterator_t<ValueType> vs. just iterator or propogating types from there.
It may induce some additional non-functional indirection, but requires less type change?

Thoughts? @bstaletic or @wjakob if y'all are available?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think if I were to consider this (using a suffix and deprecate the old) vs. #2244 (updating the function), my money would be still be on #2244 :(

In terms of incompatibility, I think that could be solved by using a (deprecated?) implicit conversion from py::detail::iterator_state<...> to py::iterator.

However, per @ wjakob's points about complexity, I'm not sure if this PR avoids the complexity from before, due to the deprecation (which may lead to another name swap + deprecation from make_iterator_ng back to make_iterator?).

I agree with you up to this point.

The best option [...]

Your idea here doesn't sound bad to me, but I believe @wjakob should have the last word.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I also quite like the idea of fixing the actual problem, rather than adding onto the confusion. Then again, I can see the point that this "only" fixes the docstring (and you could already use py::arg_v to do so I believe?).

At any rate, if we're thinking about making a breaking 3.0.0 release, it would be cool if we could include a fixed version. A potential route would be to first use a new name + deprecation, then rename in a (distant future) pybind11 3.0.0?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Then again, I can see the point that this "only" fixes the docstring

This PR only fixes annotation of return type. Functions with typed iterator argument still need to use type-erased py::iterator. I don't think py::arg_v can help here.

I like the idea of py::iterator_t<ValueType>. The only issue I think of is overloads support. Dispatcher needs to tell the difference between f(py::iterator_t<int>) vs f(py::iterator_t<bool>). I think it's not possible to tell which is which until the actual iteration happens, unlike in case of py::array_t. This issue can be "avoided" by banning overloads that different only in py::iterator_t argument(s). Alternatively, we can make a scaffold to extract first element (which still might be not enough) to dispatch and then iterate using mocked-up iterator, but this sounds like an implementation nightmare and potential nasty debug sessions for users.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This PR only fixes annotation of return type. Functions with typed iterator argument still need to use type-erased py::iterator. I don't think py::arg_v can help here.

Right, jumped to conclusions too quickly, there! We don't actually have a py::ret_v, I suppose, to support a manual/custom return type annotation?

I like the idea of py::iterator_t<ValueType>. The only issue I think of is overloads support.

I thought we already have something like this (where the internal types are only checked at runtime), but I can't find it. This will always be a problem, though, so I wouldn't be opposed to just add it and document that it won't do anything for overloads (but it would do the casting for you, once you ask the next element). pybind11 has more corners where overloads aren't perfect and order matters, so ...
Extracting the first element would be reasonably unexpected, I believe. And as you say: it wouldn't provide any guarantees, so let's have users do that themselves, if they really want to disambiguate.

sizmailov marked this conversation as resolved.
Show resolved Hide resolved
iterator make_iterator(Iterator first, Sentinel last, Extra &&... extra) {
return cast(make_iterator_ng(first, last, std::forward<Extra>(extra)...));
}

/// Makes a python iterator from a first and past-the-end C++ InputIterator.
template <return_value_policy Policy = return_value_policy::reference_internal,
typename Iterator,
typename Sentinel,
typename ValueType = decltype(*std::declval<Iterator>()),
typename... Extra>
[[deprecated("Superseded by make_key_iterator_ng")]]
iterator make_key_iterator(Iterator first, Sentinel last, Extra &&... extra) {
return cast(make_key_iterator_ng(first, last, std::forward<Extra>(extra)...));
}

template <return_value_policy Policy = return_value_policy::reference_internal,
typename Type, typename... Extra>
detail::iterator_state<decltype(std::begin(std::declval<Type &>())), decltype(std::end(std::declval<Type &>())), false, Policy>
make_iterator_ng(Type &value, Extra &&... extra) {
return make_iterator_ng<Policy>(std::begin(value), std::end(value), std::forward<Extra>(extra)...);
}

/// Makes an iterator over the keys (`.first`) of a stl map-like container supporting
/// `std::begin()`/`std::end()`
template<return_value_policy Policy = return_value_policy::reference_internal,
typename Type, typename... Extra>
detail::iterator_state<decltype(std::begin(std::declval<Type&>())), decltype(std::end(std::declval<Type&>())), true, Policy>
make_key_iterator_ng(Type &value, Extra &&... extra) {
return make_key_iterator_ng<Policy>(std::begin(value), std::end(value), std::forward<Extra>(extra)...);
}

/// Makes an iterator over values of an stl container or other container supporting
/// `std::begin()`/`std::end()`
template <return_value_policy Policy = return_value_policy::reference_internal,
typename Type, typename... Extra> iterator make_iterator(Type &value, Extra&&... extra) {
return make_iterator<Policy>(std::begin(value), std::end(value), extra...);
typename Type, typename... Extra>
iterator make_iterator(Type &value, Extra&&... extra) {
return cast(make_iterator_ng<Policy>(value, std::forward<Extra>(extra)...));
}

/// Makes an iterator over the keys (`.first`) of a stl map-like container supporting
/// `std::begin()`/`std::end()`
template <return_value_policy Policy = return_value_policy::reference_internal,
typename Type, typename... Extra> iterator make_key_iterator(Type &value, Extra&&... extra) {
return make_key_iterator<Policy>(std::begin(value), std::end(value), extra...);
typename Type, typename... Extra>
iterator make_key_iterator(Type &value, Extra&&... extra) {
return cast(make_key_iterator<Policy>(value, std::forward<Extra>(extra)...));
}


template <typename InputType, typename OutputType> void implicitly_convertible() {
struct set_flag {
bool &flag;
Expand Down
8 changes: 4 additions & 4 deletions include/pybind11/stl_bind.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -313,7 +313,7 @@ void vector_accessor(enable_if_t<!vector_needs_copy<Vector>::value, Class_> &cl)

cl.def("__iter__",
[](Vector &v) {
return make_iterator<
return make_iterator_ng<
return_value_policy::reference_internal, ItType, ItType, T&>(
v.begin(), v.end());
},
Expand All @@ -340,7 +340,7 @@ void vector_accessor(enable_if_t<vector_needs_copy<Vector>::value, Class_> &cl)

cl.def("__iter__",
[](Vector &v) {
return make_iterator<
return make_iterator_ng<
return_value_policy::copy, ItType, ItType, T>(
v.begin(), v.end());
},
Expand Down Expand Up @@ -608,12 +608,12 @@ class_<Map, holder_type> bind_map(handle scope, const std::string &name, Args&&.
);

cl.def("__iter__",
[](Map &m) { return make_key_iterator(m.begin(), m.end()); },
[](Map &m) { return make_key_iterator_ng(m.begin(), m.end()); },
keep_alive<0, 1>() /* Essential: keep list alive while iterator exists */
);

cl.def("items",
[](Map &m) { return make_iterator(m.begin(), m.end()); },
[](Map &m) { return make_iterator_ng(m.begin(), m.end()); },
keep_alive<0, 1>() /* Essential: keep list alive while iterator exists */
);

Expand Down
2 changes: 1 addition & 1 deletion tests/test_opaque_types.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ TEST_SUBMODULE(opaque_types, m) {
.def("back", (std::string &(StringList::*)()) &StringList::back)
.def("__len__", [](const StringList &v) { return v.size(); })
.def("__iter__", [](StringList &v) {
return py::make_iterator(v.begin(), v.end());
return py::make_iterator_ng(v.begin(), v.end());
}, py::keep_alive<0, 1>());

class ClassWithSTLVecProperty {
Expand Down
16 changes: 8 additions & 8 deletions tests/test_sequences_and_iterators.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -189,7 +189,7 @@ TEST_SUBMODULE(sequences_and_iterators, m) {
})
.def("__len__", &Sequence::size)
/// Optional sequence protocol operations
.def("__iter__", [](const Sequence &s) { return py::make_iterator(s.begin(), s.end()); },
.def("__iter__", [](const Sequence &s) { return py::make_iterator_ng(s.begin(), s.end()); },
py::keep_alive<0, 1>() /* Essential: keep object alive while iterator exists */)
.def("__contains__", [](const Sequence &s, float v) { return s.contains(v); })
.def("__reversed__", [](const Sequence &s) -> Sequence { return s.reversed(); })
Expand Down Expand Up @@ -249,9 +249,9 @@ TEST_SUBMODULE(sequences_and_iterators, m) {
})
.def("__setitem__", &StringMap::set)
.def("__len__", &StringMap::size)
.def("__iter__", [](const StringMap &map) { return py::make_key_iterator(map.begin(), map.end()); },
.def("__iter__", [](const StringMap &map) { return py::make_key_iterator_ng(map.begin(), map.end()); },
py::keep_alive<0, 1>())
.def("items", [](const StringMap &map) { return py::make_iterator(map.begin(), map.end()); },
.def("items", [](const StringMap &map) { return py::make_iterator_ng(map.begin(), map.end()); },
py::keep_alive<0, 1>())
;

Expand All @@ -266,10 +266,10 @@ TEST_SUBMODULE(sequences_and_iterators, m) {
py::class_<IntPairs>(m, "IntPairs")
.def(py::init<std::vector<std::pair<int, int>>>())
.def("nonzero", [](const IntPairs& s) {
return py::make_iterator(NonZeroIterator<std::pair<int, int>>(s.begin()), NonZeroSentinel());
return py::make_iterator_ng(NonZeroIterator<std::pair<int, int>>(s.begin()), NonZeroSentinel());
}, py::keep_alive<0, 1>())
.def("nonzero_keys", [](const IntPairs& s) {
return py::make_key_iterator(NonZeroIterator<std::pair<int, int>>(s.begin()), NonZeroSentinel());
return py::make_key_iterator_ng(NonZeroIterator<std::pair<int, int>>(s.begin()), NonZeroSentinel());
}, py::keep_alive<0, 1>())
;

Expand Down Expand Up @@ -345,12 +345,12 @@ TEST_SUBMODULE(sequences_and_iterators, m) {
// test_iterator_passthrough
// #181: iterator passthrough did not compile
m.def("iterator_passthrough", [](py::iterator s) -> py::iterator {
return py::make_iterator(std::begin(s), std::end(s));
return py::cast(py::make_iterator_ng(std::begin(s), std::end(s)));
});

// test_iterator_rvp
// #388: Can't make iterators via make_iterator() with different r/v policies
static std::vector<int> list = { 1, 2, 3 };
m.def("make_iterator_1", []() { return py::make_iterator<py::return_value_policy::copy>(list); });
m.def("make_iterator_2", []() { return py::make_iterator<py::return_value_policy::automatic>(list); });
m.def("make_iterator_1", []() { return py::make_iterator_ng<py::return_value_policy::copy>(list); });
m.def("make_iterator_2", []() { return py::make_iterator_ng<py::return_value_policy::automatic>(list); });
}
29 changes: 29 additions & 0 deletions tests/test_stl_binders.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -277,3 +277,32 @@ def test_map_delitem():
del um['ua']
assert sorted(list(um)) == ['ub']
assert sorted(list(um.items())) == [('ub', 2.6)]


def test_map_docstrings(doc):
assert (doc(m.MapStringDouble.__iter__) ==
"__iter__(self: m.stl_binders.MapStringDouble)"
" -> Iterator[str]")
assert (doc(m.MapStringDouble.items) ==
"items(self: m.stl_binders.MapStringDouble)"
" -> Iterator[Tuple[str, float]]")
assert (doc(m.UnorderedMapStringDouble.__iter__) ==
"__iter__(self: m.stl_binders.UnorderedMapStringDouble)"
" -> Iterator[str]\n")
assert (doc(m.UnorderedMapStringDouble.items) ==
"items(self: m.stl_binders.UnorderedMapStringDouble)"
" -> Iterator[Tuple[str, float]]\n")


def test_vector_docstrings(doc):
assert (doc(m.VectorInt.__iter__) ==
"__iter__(self: m.stl_binders.VectorInt)"
" -> Iterator[int]\n")


@pytest.unsupported_on_pypy
@pytest.requires_numpy
def test_vector_docstring2(doc):
assert (doc(m.VectorStruct.__iter__) ==
"__iter__(self: m.stl_binders.VectorStruct)"
" -> Iterator[m.stl_binders.VStruct]")