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

() to empty tuple conversion is confusing #4896

Open
mitsuhiko opened this issue Feb 8, 2025 · 5 comments
Open

() to empty tuple conversion is confusing #4896

mitsuhiko opened this issue Feb 8, 2025 · 5 comments

Comments

@mitsuhiko
Copy link
Contributor

I just managed to upgrade to the latest PyO3 and I was immediately bitten by the new () to empty tuple conversion. I was especially confused because there is a special case now that #[pyfunction] does actually still perform the old conversion, but into_py_any and friends do not.

I'm not sure what the motivation of this change was, but I now need to manually handle that case in a few places. Is there ever a situation where you want to treat () as an empty tuple?

@davidhewitt
Copy link
Member

Sorry this change bit you. Are you able to share any detail about the patterns where this ended up problematic?

We had some discussion of this change in #4041 (comment) and #2316 (comment). I think as maintainers we had all felt () -> None was a necessity that had come out of needing unit return functions to work, but that () -> () is ultimately the less surprising behaviour.

We did note the change on the migration guide, but maybe we underestimated impact.

Note that with the old IntoPy trait () implemented both conversions and type inference would drive the final value. Which left a lot of space for unintended results too 😬

@mitsuhiko
Copy link
Contributor Author

I have to admit that I don’t really think of () being an empty tuple in Rust. Tuples are not constructed dynamically and () is exclusively used to indicate what is None in Python.

Where it started being a problem is that I have runtime objects on the rust side that need mapping to the Python side. Those hold the equivalent of () but never to mean the empty tuple but None. Think things like JSON.

@birkenfeld
Copy link
Member

FWIW, I agree with Armin. None kind of plays two roles in Python, which are covered by None:: and () in Rust, while the empty Python tuple is seen rarely, and usually as a standin for "empty collection" that is better served by an empty list.

@davidhewitt
Copy link
Member

That's totally fair points, and I'm happy to be wrong here and change this back.

I wonder in a world where Rust has variadic generics based on tuples if () gets more complicated.

Let's perhaps let this issue sit for a bit to see if we can get more input for both sides of the reasoning.

@Icxolu
Copy link
Contributor

Icxolu commented Feb 12, 2025

Interesting view point. I have to admit that I still struggle a bit to imagine a case where this is a problem, but I can see this being confusing. Maybe this due to Pythons None being kind of overloaded, meaning either:

  • "nothing" in the sense of () (as a function return type)
  • or "maybe" in the sense of Option

For me storing () somewhere does not make a lot of sense, because it does not have a meaningful value (beyond type system trickery). And the same is true for using it in argument position if it turned into None, i.e. and argument that's always None is not really useful. It makes more sense to me to represent something like this with an Option which may or may not be None.

I also can see the other way around also being confusing. Going from a 0-length to a 1-length tuple on the Rust side to None/Tuple on the Python side is also not really obvious.

Another take could be to remove the explicit conversion entirely (we also don't implement FromPyObject) and only keep the macro specialization for the return type. This would force everybody to be explicit about it. From a consistency point of view I still kind of like the current way (but that might be just me being a bit biased because I implemented it)

Of course everything about this can be worked around by just defining your own ZST with a custom IntoPyObject implementation and using that instead.

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