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

Setting nested field #2104

Closed
ntakouris opened this issue Jan 16, 2022 · 6 comments
Closed

Setting nested field #2104

ntakouris opened this issue Jan 16, 2022 · 6 comments
Labels

Comments

@ntakouris
Copy link

Bug Description

With those 2 definitions:

use pyo3::prelude::*;

#[pyclass]
#[derive(Debug, PartialEq, Clone)]
pub struct Beta {
    #[pyo3(get, set)]
    pub field: i32,
}

#[pymethods]
impl Beta {
    #[new]
    pub fn new(field: i32) -> Self {
        Beta { field }
    }
}

#[pyclass]
#[derive(Debug, PartialEq, Clone)]
pub struct Alpha {
    #[pyo3(get, set)]
    pub beta: Beta,
}

#[pymethods]
impl Alpha {
    #[new]
    pub fn new(beta: Beta) -> Self {
        Alpha { beta }
    }
}

You can do (in a python script):

import testlib

if __name__=="__main__":
    beta = testlib.Beta(field=1)
    alpha = testlib.Alpha(beta)

    print(alpha.beta.field) # prints 1

    alpha.beta.field = 2

    print(alpha.beta.field) # still prints 1!

The workaround would be to set alpha.beta into testlib.Beta(field=2), but this style does not scale well with multiple fields and more nested ones.

Steps to Reproduce

  1. Clone the repo ntakouris/pyo3-set-nested-field-bug and cd into it
  2. pip3 install .
  3. python3 usage.py should print 1 and 1 instead of 1 and 2
  4. View usage.py content

Backtrace

No response

Your operating system and version

Ubuntu 20.04.3 LTS

Your Python version (python --version)

Python 3.8.10

Your Rust version (rustc --version)

rustc 1.58.0 (02072b482 2022-01-11)

Your PyO3 version

0.15.1

How did you install python? Did you use a virtualenv?

apt

Additional Info

No response

@ntakouris ntakouris added the bug label Jan 16, 2022
@birkenfeld
Copy link
Member

birkenfeld commented Jan 16, 2022

This is not possible to change with the structs as defined. On attribute access, PyO3 must clone the member (in this case alpha.beta) and therefore the field is set on the copy. (Were PyO3 to give out a reference, it would have to borrow the struct but that does not work across language boundaries.)

If you need such hierarchies to work, you need to make at least the Alpha::beta field a Py<Beta>, so that PyO3 can hand out references without problems. This is akin to using Rc<RefCell<Beta>> in pure Rust.

@ntakouris
Copy link
Author

@birkenfeld so, no support for types that are copied by default like ints

@birkenfeld
Copy link
Member

Not sure what you mean here? An int is immutable anyway, so this is no issue for ints.

@ntakouris
Copy link
Author

In this case an int is passed by value, so there's no need to reference count / use Rc<RefCell<>> and/or Py<Beta>,for it be implemented. Unless I am missing something.

@birkenfeld
Copy link
Member

field itself is fine to stay as a simple int field. The problem lies in the intermediary Beta. The Python code

alpha.beta.field = 2

is the same as

tmp = alpha.beta
tmp.field = 2

and in the first line PyO3 clones the Beta for tmp, since it cannot hand out a Rust reference to it.

If Alpha has beta: Py<Beta>, the beta field is already a Python object and PyO3 can just hand out a new Python reference to it.

@davidhewitt
Copy link
Member

See also #1358. I'd love to make this more ergonomic one day!

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

No branches or pull requests

3 participants