Details
-
Type:
RFC
-
Status: Implemented
-
Resolution: Done
-
Component/s: DM
-
Labels:None
Description
I'd like to propose one major and two minor changes to the pybind11 style guide, all related to the recommendation on return value policies. These changes are to address special cases that came up when writing the pybind11 wrappers for DM-17981.
Major change
I propose that we add the following paragraph between "in C++ than Python)." and "In rare cases":
When a C++ method on a container/collection returns non-const references or pointers to the collection's elements, it MAY use a policy other than reference_internal. reference_internal may be unsafe in this case because Python could keep a reference to a collection element after it was deleted, leading to a segmentation fault. Developers should choose a return value policy appropriate for the properties of the collection and the expected way it will be used from Python (e.g., whether elements need to be modified in-place).
Minor change 1
I propose that the phrases "(smart) pointer" in the current text have their parentheses removed, and the phrase "pointers" in the proposed new paragraph above be qualified with "smart pointers". According to the pybind11 documentation, using the automatic policy with a function that returns a C-style (raw) pointer will cause pybind11 to assume the pointer points to a new object that needs to be managed, which would be disastrous if the pointer actually points to an already-managed internal object.
We discussed this issue on #dm-pybind11 and concluded that we never use raw pointers, so I don't think we need new text to cover that case. We just need to not imply that automatic would be safe if somebody did return a raw pointer.
Minor change 2
I propose that the phrases "data member" in the current text be replaced with "internal state". The current wording suggests that it matters whether the reference is to a direct member (aka field, attribute, etc.) of the class or to state held more indirectly (e.g., as part of a subobject). However, such a distinction is an implementation detail that may change with refactoring, and, more importantly, the behavior of an internal reference/pointer is exactly the same in both cases.
Attachments
Issue Links
Activity
I suggest removing the rule. The pybind11 documentation is sufficiently clear for return value policy. However, if you insist on updating instead of deleting then your changes as far as what to call pointers seem wise to me.
I also like Russell Owen's proposal even better.
Russell Owen the issue (and the reason Kian-Tat Lim asked me to to propose the paragraph) is that the rules as written required us to petition K-T for an exception.
On the other hand, maybe we could avoid adding the new paragraph and simply downgrade the rule from a SHALL to a SHOULD. Then developers could just let the pybind11 docs take precedence. Does that work for everybody?
Edit: I misunderstood and thought only the proposed new paragraph was to be deleted.
Krzysztof Findeisen
added a comment - - edited Russell Owen the issue (and the reason Kian-Tat Lim asked me to to propose the paragraph) is that the rules as written required us to petition K-T for an exception.
On the other hand, maybe we could avoid adding the new paragraph and simply downgrade the rule from a SHALL to a SHOULD. Then developers could just let the pybind11 docs take precedence. Does that work for everybody?
Edit: I misunderstood and thought only the proposed new paragraph was to be deleted. That's okay with me. My preference is still for dropping the whole thing, but everything on the table is a marked improvement from what's there right now.
Adopting as "remove the return value policy rule completely", as recommended above.
Krzysztof Findeisen
added a comment - Adopting as "remove the return value policy rule completely", as recommended above. I approve of removing this rule, but it might be nice to add a mention of/reference to the pybind11 RVP documentation somewhere in the "advanced" part of the how-to document (which is not change-controlled).
Generally thumbs up on all of this. A tiny quibble about Minor Change 2: the key factor, I think, is whether the reference is to something guaranteed to have the same lifetime as the containing object (data member or subobject member) or something not so guaranteed (like a collection member that might disappear).