Skip to content

docs: improved reference_internal documentation #5528

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 2 commits into from
Feb 20, 2025

Conversation

gentlegiantJGC
Copy link
Contributor

@gentlegiantJGC gentlegiantJGC commented Feb 17, 2025

Description

The documentation for the reference_internal return policy implies that py::keep_alive<0, 1>() is always applied but this isn't true.

There is quite a bit of confusion about the reference_internal return policy (myself included) stemming from this documentation.

Here are the related issues I have found.
#1764
#2618
#4124
#4236
#5046

These changes:

  1. Improve the wording of the reference_internal documentation to make it easier to understand.
  2. Add a note about the policy falling back to move if the return value is not an lvalue reference or pointer.

As documented thoroughtly in #4236 and #5046 the def_property family does not honor the keep_alive argument passed to it and #2618 also mentions it does not honor the call_guard argument. I think these issues should be fixed but should it be documented here until it is? The workaround is to use cpp_function and add those arguments there.

Fixes #1764
Fixes #2618
Fixes #4124

Suggested changelog entry:

Improved ``reference_internal`` policy documentation

Improved the wording to make it easier to understand and added a note about the policy falling back to move if the return value is not an lvalue reference or pointer.
@gentlegiantJGC
Copy link
Contributor Author

The test failure is unrelated to changes I made

| :enum:`return_value_policy::reference_internal` | If the return value is an lvalue reference or a pointer, the parent object |
| | (the implicit ``this``, or ``self`` argument of the called method or |
| | property) is kept alive for at least the lifespan of the return value. |
| | Otherwise this policy falls back to the policy |
Copy link
Collaborator

Choose a reason for hiding this comment

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

That's a great and much needed clarification!

What do you think about:

  • Making the "Otherwise ..." sentence bold, and ending it with "(see #5528)."

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@gentlegiantJGC
Copy link
Contributor Author

I considered adding a line pointing out that keep_alive doesn't work when explicitly passed to the def_property methods because that is the users natural next response but it felt beyond the scope of it.
I added a compiler error in #5529 but I don't particularly like that because it will break existing code even if it didn't work correctly.
What would your suggestion be?

Copy link
Collaborator

@rwgk rwgk left a comment

Choose a reason for hiding this comment

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

Thanks @gentlegiantJGC!

Sorry I'm too busy right now to carefully think about your ideas for a more ambitious fix. I'll merge this, it's definitely useful!

@rwgk rwgk merged commit 73825f3 into pybind:master Feb 20, 2025
76 checks passed
@github-actions github-actions bot added the needs changelog Possibly needs a changelog entry label Feb 20, 2025
@gentlegiantJGC gentlegiantJGC deleted the internal_reference_docs branch February 20, 2025 18:02
@henryiii henryiii changed the title Improved reference_internal documentation docs: improved reference_internal documentation Mar 31, 2025
@henryiii henryiii removed the needs changelog Possibly needs a changelog entry label Apr 1, 2025
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
3 participants