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

Rename PyObjectRef with PyAny #388

Merged
merged 5 commits into from
Mar 18, 2019
Merged

Rename PyObjectRef with PyAny #388

merged 5 commits into from
Mar 18, 2019

Conversation

kngwyu
Copy link
Member

@kngwyu kngwyu commented Mar 4, 2019

See #356 (comment) and around comments for the motivation.
Some related ideas:

  • Maybe should we remove FromPyObject rather than renaming it? This trait remains unchanged and doesn't fit to our semantics
  • We should also rename PyTryFrom with PyDowncast?

@kngwyu kngwyu requested a review from konstin March 4, 2019 13:09
@pganssle
Copy link
Member

pganssle commented Mar 4, 2019

As someone who doesn't really understand what PyObjectRef does at the moment, I will say that changing this name to PyAny doesn't help me much, if at all.

I wonder if it might be a good idea to start by adding clear documentation about the difference between PyObject and PyObjectRef, and then propose a change to the name?

@konstin
Copy link
Member

konstin commented Mar 4, 2019

@pganssle I'm convinced that this is a better name and, in combination with more documentation, it will make more sense than PyObjectRef.

For short, the new name means that

from typing import List, Any


def foo(x: List, y: Any):
    ...

translates into

fn foo(x: &PyList, y: &PyAny) {
    unimplemented!()
}

Copy link
Member

@konstin konstin left a comment

Choose a reason for hiding this comment

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

Big 👍

@konstin
Copy link
Member

konstin commented Mar 4, 2019

Maybe should we remove FromPyObject rather than renaming it? This trait remains unchanged and doesn't fit to our semantics

It should be replaced by FromPy at some point.

We should also rename PyTryFrom with PyDowncast?

I like the idea, but that should be discussed separately.

@konstin
Copy link
Member

konstin commented Mar 4, 2019

I'd say we wait a bit before merging so everyone has time to add comments.

Oh, and @kngwyu could you please add a changelog entry?

@pganssle
Copy link
Member

pganssle commented Mar 4, 2019

@konstin

For short, the new name means that
...
translates into
...

Yes, this is is a nice parallel, but this is very specific to the Python typing module. The equivalent for things other than annotations would be "derived from PyObject". I think you don't want to start using "mixed metaphors", where the Rust type system parallels type annotations in some respects, and parallels the C API / inheritance model in other respects. It will prevent newcomers from forming a useful mental model of how to use pyo3 types.

I haven't looked into this closely, but it may be worth considering other parts of the type system to see if the parallels break down. or if they hold up. What about Callable? Sequence?

I think in the end you should only adopt this Any if the idea is that PyO3's type system will generally try to have a fairly consistent relationship to the typing type system, and that you want to abandon the idea of inheritance.

@konstin
Copy link
Member

konstin commented Mar 4, 2019

As I've said in the issue thread, PyObject would indeed be the ideal name. Unfortunately that is already taken twice (for the ffi type and for our wrapper) and PyAny was second best I could come up with. If you have a better name, I'm happy to discuss that.

Callable isn't implemented yet, PySequence exists. The types module doc page has a good overview, and I'd say it's kinda consistent.

I think in the end you should only adopt this Any if the idea is that PyO3's type system will generally try to have a fairly consistent relationship to the typing type system, and that you want to abandon the idea of inheritance.

I don't want to base pyo3 on typing, I just want to use the resemblance to make it easier understandable (I'd also say that "any" makes sense even without typing). I don't see how this related to inheritance, especially given that pyo3 can only emulate inheritance.

@kngwyu
Copy link
Member Author

kngwyu commented Mar 5, 2019

@pganssle
I'm not the author, but I think the reason why both PyObject and PyObjectRef exist is the mostly historical one.
PyObject remains unchanged since rust-cpython age and PyObjectRef is added after this project started.
PyObjectRef behaves like other python native types like PyDict and mostly we use it as &'gil PyObjectRef. It has a life time same as GIL, which is forced by our object stack.
Other than that, PyObject behaves like Py<PyObjectRef>. It's used as PyObject and not has a lifetime same as GIL, though it also does refcnt -= 1 when a GILGuard drops.

So while renaming PyObjectRef with PyObject is good, it needs a lots of refactoring, especially around ToPyObject and IntoPyObject.
But I think it's still possible and worth trying, though how to change ToPyObject is unclear.

@konstin konstin mentioned this pull request Mar 18, 2019
1 task
@konstin konstin merged commit 3e475b0 into PyO3:master Mar 18, 2019
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.

None yet

3 participants