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

Remove ObjectProtocol #911

Merged
merged 1 commit into from
May 8, 2020
Merged

Conversation

davidhewitt
Copy link
Member

@davidhewitt davidhewitt commented May 7, 2020

This is a reworked version of #892 which does not include the builtin_methods idea.

Summary of changes:

  • ObjectProtocol methods have been moved to PyAny.
  • All native types implement Deref<Target = PyAny>
  • PyCell<T> implements Deref<Target = <T::BaseType as PyTypeInfo>::AsRefType>. This allows deref to super types, and because PyAny is always the last base type, this means PyCell<T> can always access the PyAny methods too. This actually provides a lot of the solution for A safe API for upcasting #787 Changed so that PyCell<T> now just implements Deref<Target = PyAny>, thanks for feedback.

I wanted to add AsRef<PyAny> to PyRef and PyRefMut, but this is more disruptive change because at the moment py_ref.as_ref() always gives the super type. If I add a second AsRef impl, then sometimes type inference fails where it did not before.

I think I can improve that by creating PyRef::as_super, but still wanted to check what people think before I do that.

@kngwyu
Copy link
Member

kngwyu commented May 7, 2020

Nice, thank you. I'll give a detailed review later.

PyCell implements Deref<Target = <T::BaseType as PyTypeInfo>::AsRefType>

Casting PyCell<Super> to PyCell<Base> is not always safe because of dict and weakref slots.
So I think Target = PyAny is sufficient.

I wanted to add AsRef to PyRef and PyRefMut, but this is more disruptive change because at the moment py_ref.as_ref() always gives the super type.

Good catch 👍 , and I have another concern about borrow flags.
For example, this code panics because it validates PyCell's borrowing rule.

#[pyclass] 
struct Class {
    field: usize
}
let ref_: PyRef<Class> = ~;
ref_.setattr("field", 5).unwrap();

So I think providing safe and good APIs for PyRef/PyRefMut conversion is difficult.
And, since it's not so common usage, I don't think we need it.

@davidhewitt
Copy link
Member Author

davidhewitt commented May 7, 2020

Casting PyCell to PyCell is not always safe because of dict and weakref slots.
So I think Target = PyAny is sufficient.

Oh, can you explain this to me? I thought because PyCellInner<Super> has PyCellInner<Base> as the first field, this upcast is ok.

@kngwyu
Copy link
Member

kngwyu commented May 7, 2020

Oh, can you explain this to me?

When a class with #[pyclass(dict)] or #[pyclass(weakref)] is extended only the subclass has dict/weakref slots. So if we get PyClass<Base> from PyClass<Super>, it doesn't have dict/weakref slots. But this can't be actually problematic so... please don't mind about this unsafety.
Anyway, I still think that Deref<Target=PyAny> is better, because we can get no useful methods by Deref<Target=PyClass<Base>>.

@davidhewitt
Copy link
Member Author

Hmm I think if casting PyCell<Sub> to PyCell<Base> is not safe, we have bigger problems, as downcast already makes this possible:

let gil = Python::acquire_gil();
let py = gil.python();

let sub_any = PyCell::new(py, (SubClass {}, BaseClass { value: 120 })).unwrap().to_object(py);

let base_cell = sub_any.as_ref(py).downcast::<PyCell<BaseClass>>().unwrap();
println!("base: {:?}", base_cell as *const _);

let sub_cell = sub_any.as_ref(py).downcast::<PyCell<SubClass>>().unwrap();
println!("sub: {:?}", sub_cell as *const _);

// Output is the same address:
// base: 0x1f556db67f0
// sub: 0x1f556db67f0

@davidhewitt
Copy link
Member Author

because we can get no useful methods by Deref<Target=PyClass>.

We get access to PyClass<Base>::borrow etc. See this line in test_pycell_deref where I can borrow PyRefMut<BaseClass> from a PyCell<SubClass>.

You sure you don't want this? If you're sure, I am happy to change it to just Deref<Target = PyAny> 👍

@kngwyu
Copy link
Member

kngwyu commented May 7, 2020

We get access to PyClass::borrow etc

Ah I missed that, sorry. So now we have two ways to get PyRef<Base>?

// 1.
let base = cell.borrow().into_super();
// 2.
let base: PyRef<Base> = PyCell::borrow(cell);

Hmm... 🤔
I don't think that the later one is completely useless, but isn't it difficult to notice for users?

@davidhewitt
Copy link
Member Author

Hmm... 🤔
I don't think that the later one is completely useless, but isn't it difficult to notice for users?

Yeah, I would be glad to have ideas how to make the documentation better here.

In my opinion this new way to borrow Base is more powerful, because you can even borrow a grandparent in the same way:

let cell: &PyCell<SubSubClass> = ...;

// Current way to get PyRef<Base>
// As one line: cell.borrow().into_super().as_ref()
let sub_sub_ref: PyRef<SubSubClass> = cell.borrow();
let sub_ref: PyRef<SubClass> = sub_sub_ref.into_super();
let base_ref: &PyRef<Base> = sub_ref.as_ref();

// New way to get PyRef<Base>
let base_ref: PyRef<Base> = PyCell::borrow(cell);

@kngwyu
Copy link
Member

kngwyu commented May 7, 2020

In my opinion this new way to borrow Base is more powerful, because you can even borrow a grandparent in the same way:

Yeah, but I think simpleness is more important here than such kind of powerfulness, since it's rare to have grandparents.

@davidhewitt
Copy link
Member Author

That's fair 👍 . I will change PyCell<T> to Deref<Target = PyAny>.

@davidhewitt davidhewitt force-pushed the remove-objectprotocol branch 3 times, most recently from 228d4d8 to 38a5edb Compare May 7, 2020 16:08
@davidhewitt
Copy link
Member Author

I've pushed that change 😄

Copy link
Member

@kngwyu kngwyu left a comment

Choose a reason for hiding this comment

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

Almost LGTM 👍

src/types/any.rs Show resolved Hide resolved
guide/src/types.md Outdated Show resolved Hide resolved
@davidhewitt davidhewitt force-pushed the remove-objectprotocol branch from 38a5edb to a5ebef4 Compare May 8, 2020 09:05
@davidhewitt
Copy link
Member Author

I pushed a change to types.md:

  • Swapped the order around. Previously was PyObject, Py<T>, PyAny, PyList. Now is PyAny, PyList, PyObject, Py<T>. I think this is better as PyAny is the type we want users to use most imo.
  • Changed the conversion bullet points into doc-tested examples.

@kngwyu
Copy link
Member

kngwyu commented May 8, 2020

Thank you!

@kngwyu kngwyu merged commit d5eb8f0 into PyO3:master May 8, 2020
@davidhewitt davidhewitt deleted the remove-objectprotocol branch August 10, 2021 07:19
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.

3 participants