Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add SequenceProtocol and MappingProtocol descriptions to the guide #1546
Add SequenceProtocol and MappingProtocol descriptions to the guide #1546
Changes from 4 commits
0c02146
4b675cc
b1aae93
88849bd
e8a277e
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
This differs from https://docs.python.org/3/reference/datamodel.html#object.__getitem__, which says that negative integers are are passed directly to
__getitem__
which then can interpret the negative numbers as it likes?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.
I presume PyO3 plugs the
__getitem__
method implementation into thePySequenceMethods.sq_item
slot. The C function in that slot has a slightly different interface from the Python-native__getitem__()
method, as described in the C-API docs linked above.PyObject_GetItem()
andPySequence_GetItem()
handle the negative indexes themselves when__len__()
is also implemented.My implementations of
__getitem__()
always start withassert!(idx >= 0);
because I also implement__len__()
, and they indeed work with the negative indexes.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.
Ah yes, this is a really good point. Perhaps in the documentation here we should link all these methods to their relevant slot information?
All the relationships between these methods and their slots is defined in https://github.com/PyO3/pyo3/blob/main/pyo3-macros-backend/src/defs.rs
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.
I like this idea, but the mapping between methods and slots looks not so straightforward. There are methods that do not have a C function slot like
__bytes__()
,__format__()
and__reversed__()
. And then there are methods that share the same slot like__setitem__()
and__delitem__()
. I think we could add a symbolic tag likePySequenceMethods.sq_item
to each trait method.Another interesting issue here is the implicit slot method probing order: for example the evaluation of
obj[i]
Python expression probesPyMappingMethods.mp_subscript
beforePySequenceMethods.sq_item
. This is relevant when someone implements both of these traits for some reason.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.
Hmm we should probably move
__reversed__
to thePyIterProtocol
trait? (I can do it in a separate PR.)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.
Well,
__reversed__()
is a standalone method, not aPyMappingMethods
slot, so there is no particular reason it should be a part ofPyMappingProtocol
.However, implementing
__reversed__()
for sequences is not very useful, because the default implementation seems sufficient for most cases.I personally do not like the idea of adding anything to
PyIterProtocol
though. It's already semantically overloaded as it encompasses bothIntoIterator
andIterator
Rust traits. But then the user is expected to implement a different subset of methods for each of the intended uses (as an iterator or as an iterable).Currently, we also have to write mandatory boilerplate code like
for every custom iterator class.
Most Rust programmers would expect a blanket implementation like
from the stdlib to exist instead.
I'm sorry about the long rant, but
PyIterProtocol
was the biggest road bump for me when working with PyO3.I really wish it was two distinct traits, like for example
PyIterProtocol
andPyIntoIterProtocol
, so that anyone who knows how Rust iterators work could easily implement Python iterators without scratching their head about "why do I need to returnPyRef<Self>
insteadSelf
here?"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.
Thanks, I completely agree that the current situation is confusing in many ways. I wonder if alternatively here I should remove
__reversed__
from#[pyproto]
completely. In my opinion#[pymethods]
are very easy to learn, and implementing__reversed__
in#[pymethods]
will already work.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.
I like this idea very much. There are other non-slotted methods that are included in the protocol traits, like
__bytes__()
. And then there are C function slots that have no correspondingPyObjectProtocol
method liketp_call
aka__call__()
.I'm all for implementing true non-slotted methods as
#[pymethods]
. This way we can avoid surprises like #1465 in the future.