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.

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

Remove get_refcnt from public api #4065

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

lfn3
Copy link
Contributor

@lfn3 lfn3 commented Apr 10, 2024

This is an attempt to fix #3357.
The function is used relatively frequently in tests, so have left it with #[cfg(test)] and pub(crate).
This avoids coating the tests with unsafe, and something that doesn't indicate intent as well.
(As has been done in test_inheiritance.rs)

TODO:

  • Fix docs that refer to get_refcnt as an example - is there another function I can use instead?

This is an attempt to fix PyO3#3357.
The function is used relatively frequently in tests,
so have left it with #[cfg(test)] and pub(crate).
This avoids coating the tests with `unsafe`,
and something that doesn't indicate intent as well.

TODO:
[ ] Fix docs that refer to `get_refcnt` as an example -
    is there another function I can use instead?
Copy link
Contributor

@LilyFoote LilyFoote left a comment

Choose a reason for hiding this comment

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

Looks like a good start to me.

I had a look at the failing tests and found these places we'll need more changes:

  • We'll need a different example (maybe is_truthy?):

    pyo3/guide/src/class.md

    Lines 855 to 859 in d2dca21

    // Take a GIL-indepedent reference when you want to store the reference elsewhere.
    #[pyfunction]
    fn print_refcnt(my_class: Py<MyClass>, py: Python<'_>) {
    println!("{}", my_class.get_refcnt(py));
    }
  • We'll need a different example in

    pyo3/guide/src/migration.md

    Lines 1537 to 1558 in d2dca21

    ### All `PyObject` and `Py<T>` methods now take `Python` as an argument
    <details>
    <summary><small>Click to expand</small></summary>
    Previously, a few methods such as `Object::get_refcnt` did not take `Python` as an argument (to
    ensure that the Python GIL was held by the current thread). Technically, this was not sound.
    To migrate, just pass a `py` argument to any calls to these methods.
    Before:
    ```rust,compile_fail
    # pyo3::Python::with_gil(|py| {
    py.None().get_refcnt();
    # })
    ```
    After:
    ```rust
    # pyo3::Python::with_gil(|py| {
    py.None().get_refcnt(py);
    # })
    ```
    </details>
    or to mark it as compile_fail too. @davidhewitt do you have a preference here?

Comment on lines +245 to +254
assert_eq!(unsafe { pyo3::ffi::Py_REFCNT(dict_sub.as_ptr()) }, 1);

let item = &py.eval_bound("object()", None, None).unwrap();
assert_eq!(item.get_refcnt(), 1);
assert_eq!(unsafe { pyo3::ffi::Py_REFCNT(item.as_ptr()) }, 1);

dict_sub.bind(py).set_item("foo", item).unwrap();
assert_eq!(item.get_refcnt(), 2);
assert_eq!(unsafe { pyo3::ffi::Py_REFCNT(item.as_ptr()) }, 2);

drop(dict_sub);
assert_eq!(item.get_refcnt(), 1);
assert_eq!(unsafe { pyo3::ffi::Py_REFCNT(item.as_ptr()) }, 1);
Copy link
Contributor

Choose a reason for hiding this comment

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

What was the reasoning for changing these? If I understand the issue, we want to keep get_refcnt for internal tests like this.

Copy link

Choose a reason for hiding this comment

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

The tests in tests are integration tests, they are compiled as a separate crate, and thus don't have access to private functions internal to the crate.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah! That makes sense!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refcounting changes after PEP 683
3 participants