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

PyO3: migrate to using IntoPyObject #21681

Merged
merged 15 commits into from
Nov 23, 2024

Conversation

tdyas
Copy link
Contributor

@tdyas tdyas commented Nov 22, 2024

As part of upgrading to PyO3 v2.23.x, migrate to using the new IntoPyObject conversion trait instead of the now deprecated IntoPy and ToPyObject traits. Deprecation warnings are no longer disabled for the affected files.

@tdyas tdyas added category:internal CI, fixes for not-yet-released features, etc. release-notes:not-required PR doesn't require mention in release notes labels Nov 22, 2024
@tdyas tdyas requested review from benjyw and huonw November 22, 2024 12:04
src/rust/engine/src/python.rs Outdated Show resolved Hide resolved
@tdyas tdyas mentioned this pull request Nov 22, 2024
4 tasks
.into_pyobject(py)?
.to_owned()
.into_any()
.unbind(),
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems like a lot of hoop-jumping to "convert" a Rust bool into a Python bool. Shouldn't this function (even before this change) return PyResult<bool> and then the formal conversion is a lot simpler?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The default branch of the match for py.NotImplemented is what is causing the rest of mess since all three branches have to return the same type. If the function returns PyResult<bool> then it cannot return NotImplemented.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have a solution in mind for a custom enum (with True, False, and NotImplemented variants) which would be convertible from Option<bool> and convert to the correct Python-side types. But that would be a follow-on PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ugh, I would have expected we could raise NotImplementedError via the PyErr in the PyResult, but I guess technically you are supposed to return the NotImplemented singleton in comparison operators, and not raise. So this is correct.

@tdyas tdyas merged commit 2e176f0 into pantsbuild:main Nov 23, 2024
24 checks passed
@tdyas tdyas deleted the pyo3/migrate-to-intopyobject branch November 23, 2024 03:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:internal CI, fixes for not-yet-released features, etc. release-notes:not-required PR doesn't require mention in release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants