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

set_item consumes, but uses ToPyObject instead of IntoPyObject #480

Closed
m-ou-se opened this issue May 11, 2019 · 6 comments
Closed

set_item consumes, but uses ToPyObject instead of IntoPyObject #480

m-ou-se opened this issue May 11, 2019 · 6 comments

Comments

@m-ou-se
Copy link
Contributor

m-ou-se commented May 11, 2019

PyDict::set_item consumes the key and value, but uses ToPyObject. Shouldn't that be IntoPyObject?

For ToPyObject, taking a reference would be sufficient. But it seems more efficient to consume and use IntoPyObject.

(Same for PyList::set_item, etc.)

@PetrGlad
Copy link

PetrGlad commented Aug 9, 2019

Also set_item borrows mutates the dict. Should it borrow mutable?

@m-ou-se
Copy link
Contributor Author

m-ou-se commented Aug 11, 2019

@PetrGlad No, that's unreleated, and on purpose. Python objects can always have multiple owners, so Rust's mut is not used for mutability in PyO3: See https://docs.rs/pyo3/0.7.0/pyo3/#ownership-and-lifetimes

@davidhewitt
Copy link
Member

Marking to evaluate this as part of possible collections improvements for 0.15.

@davidhewitt davidhewitt added this to the 0.15 milestone Jul 4, 2021
@davidhewitt davidhewitt modified the milestones: 0.15, 0.16 Nov 7, 2021
@aviramha
Copy link
Member

@davidhewitt What would be the change here? I played a bit (tried changing ToPyObject -> IntoPy) and got into some errors. Is this supposed to be a tricky thing to solve?

@davidhewitt
Copy link
Member

Changing to IntoPy sounds like the right idea, but as it's breaking I have basically been sitting on this as I'm not sure how much it really helps. The semantics of IntoPy and ToPyObject are a little blurred...

@davidhewitt davidhewitt modified the milestones: 0.16, 0.17 Feb 26, 2022
@davidhewitt davidhewitt modified the milestones: 0.17, 0.18 Jul 3, 2022
@davidhewitt davidhewitt modified the milestones: 0.18, 0.19 Dec 27, 2022
@davidhewitt davidhewitt removed this from the 0.19 milestone Jun 16, 2023
@davidhewitt
Copy link
Member

Finally solved by the new IntoPyObject trait in #4493

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

No branches or pull requests

4 participants