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

Allow more methods to take interned arguments #2312

Merged
merged 9 commits into from
May 2, 2022
Merged

Conversation

mejrs
Copy link
Member

@mejrs mejrs commented Apr 17, 2022

  • I have yet to add some docs to these methods
  • Changelog

Comment on lines +301 to +307
impl<'a> IntoPy<Py<PyString>> for &'a str {
#[inline]
fn into_py(self, py: Python<'_>) -> Py<PyString> {
PyString::new(py, self).into()
}
}

Copy link
Member Author

Choose a reason for hiding this comment

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

This is a (mild) breaking change because of inference errors, but not too bad.

Copy link
Member

Choose a reason for hiding this comment

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

Just to make sure I understand things correctly: The comment is informational as the other changes are breaking anyway and this is therefore targeting 0.17.0?

Copy link
Member

Choose a reason for hiding this comment

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

Is the previous instance into PyObject even useful anymore?

Copy link
Member

Choose a reason for hiding this comment

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

Is the previous instance into PyObject even useful anymore?

I think so as Py<PyString> does not automatically "decay" into Py<PyAny> and the existing impl might be used in generic context to produce PyObject for stashing away.

Copy link
Member

Choose a reason for hiding this comment

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

Easy to get PyObject from a Py though.

Copy link
Member

Choose a reason for hiding this comment

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

Easy to get PyObject from a Py though.

I think that in generic context, one needs to be able to express this in a single trait bound, e.g. calling into_py twice or into_py followed by into to go from T to Py<S> to PyObject is not necessarily the same as going from T to PyObject.

Copy link
Member Author

Choose a reason for hiding this comment

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

Just to make sure I understand things correctly: The comment is informational as the other changes are breaking anyway and this is therefore targeting 0.17.0?

Mainly it means that things like let thing = "thing".into_py(py); stop compiling

Copy link
Member

Choose a reason for hiding this comment

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

To avoid making lots of annoying inference errors we could potentially combine this PR with #2316, which would enable us to select on types which have IntoPyObject<Target = PyString> instead of IntoPy<Py<PyString>>.

@birkenfeld
Copy link
Member

Very nice!

@mejrs mejrs force-pushed the intern-more branch 8 times, most recently from eb814f4 to 5154778 Compare April 20, 2022 23:15
Copy link
Member

@adamreichold adamreichold left a comment

Choose a reason for hiding this comment

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

Looks good to me.

One remaining concern that I see is that we have made several methods "trivially generic" which were not generic before. Should we aim to reduce code bloat and factor out the non-generic parts into inner help functions? On the other hand, the functions in question do not appear to be huge, so I am unsure whether this would be worth the extra effort.

src/types/any.rs Outdated Show resolved Hide resolved
src/types/any.rs Show resolved Hide resolved
@mejrs
Copy link
Member Author

mejrs commented Apr 21, 2022

Should we aim to reduce code bloat and factor out the non-generic parts into inner help functions?

They are pretty small, I think it's fine.

Would it make sense to unify them?

Done 👍

@davidhewitt
Copy link
Member

Looks good to me too. The only question I have outstanding is whether we want to merge this now or wait until we can also merge with #2316 so that we don't cause users to go through extra churn of type inference failures in the interim.

(FWIW I have a bad feeling that #2316 gets a little upset with some of the more gnarly #[pyproto] code, so that might be blocked until 0.18 when we can kill the feature off entirely.)

@mejrs
Copy link
Member Author

mejrs commented Apr 23, 2022

I'm feeling that what we're doing with traits is a bit ad-hoc.

Maybe we need to sit down and think about what we don't like, what we would like, and what a good path to getting there is.

@mejrs mejrs requested a review from davidhewitt April 25, 2022 21:36
@davidhewitt
Copy link
Member

I'm feeling that what we're doing with traits is a bit ad-hoc.

Maybe we need to sit down and think about what we don't like, what we would like, and what a good path to getting there is.

I agree very much with this sentiment. I'll write my current backlog of ideas up in a separate issue as a starting point.

As for this PR... looks good to me! The concern I have with merging before we make a decision on #2316 is that this may cause users to have to add : PyObject annotations to keep existing .into_py() uses compiling. If we then merge #2316 these annotations may then become type errors. (I guess #2316 will probably break a bit of existing code too, so maybe that's not actually a reason to hold back this PR?) What's your opinion on this?

unsafe {
let args = args.into_py(py).into_ptr();
let kwargs = kwargs.into_ptr();
let ptr = ffi::PyObject_GetAttr(self.as_ptr(), name.to_object(py).as_ptr());
Copy link
Member Author

@mejrs mejrs Apr 26, 2022

Choose a reason for hiding this comment

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

Sorry this is a bit messy since resolving the merge conflict.

How do you feel about:

pyo3::ffi::Something(name.to_object(py).as_ptr());

versus

let ptr = name.to_object(py).into_ptr();
pyo3::ffi::Something(ptr);
ffi::Py_DECREF(ptr);

(or something else?)

Personally I prefer the latter. IMO the former is error prone and gives a bad example.

Copy link
Member

Choose a reason for hiding this comment

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

Why do you feel the former is error prone? I think the second is more error prone because missing the Py_DECREF call is leaking memory just like missing free() in C.

Perhaps a middle ground is to make it obvious we're using RAII through Py?

let name: PyObject = name.to_object(py);
pyo3::ffi::Something(name.as_ptr());

Copy link
Member Author

Choose a reason for hiding this comment

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

Why do you feel the former is error prone? I think the second is more error prone because missing the Py_DECREF call is leaking memory just like missing free() in C.

I'm mostly worried about someone reading it and and writing it slightly differently:

let ptr = name.to_object(py).as_ptr();
pyo3::ffi::Something(ptr);

which produces a dangling pointer, as the python object is dropped at the end of the statement (I should add documentation about this).

Perhaps a middle ground is to make it obvious we're using RAII through Py?

let name: PyObject = name.to_object(py);
pyo3::ffi::Something(name.as_ptr());

Sure 👍

Copy link
Member

Choose a reason for hiding this comment

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

'm mostly worried about someone reading it and and writing it slightly differently:

This is an excellent point which I'd totally missed. IMO the middle ground above is probably the only sensible way to arrange this then. If you like, I'll submit a PR to fix the whole codebase to use this pattern once this PR merged? That way you can just worry about the modified functions here without another merge conflict.

@mejrs
Copy link
Member Author

mejrs commented Apr 26, 2022

What's your opinion on this?

I think that's fine, it's breaking either way.

Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

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

Ok, let's head towards merging this in 0.17 and maybe thinking about IntoPyObject for 0.18. I'll try to write up my thoughts on traits tomorrow.

Got to confess I'm really not a fan of calling Py_DECREF or Py_XDECREF whenever we can just let Py do it in Drop implementation.

unsafe {
let args = args.into_py(py).into_ptr();
let kwargs = kwargs.into_ptr();
let ptr = ffi::PyObject_GetAttr(self.as_ptr(), name.to_object(py).as_ptr());
Copy link
Member

Choose a reason for hiding this comment

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

Why do you feel the former is error prone? I think the second is more error prone because missing the Py_DECREF call is leaking memory just like missing free() in C.

Perhaps a middle ground is to make it obvious we're using RAII through Py?

let name: PyObject = name.to_object(py);
pyo3::ffi::Something(name.as_ptr());

src/instance.rs Outdated Show resolved Hide resolved
src/instance.rs Show resolved Hide resolved
src/types/any.rs Outdated Show resolved Hide resolved
Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

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

👍 Looks great, thanks very much for all the tidy ups!

A final thought: as we're committing to merging this and producing extra inference errors in PyO3 0.17, can we please add a section to migration.md to explain this to users?

@mejrs
Copy link
Member Author

mejrs commented Apr 28, 2022

A final thought: as we're committing to merging this and producing extra inference errors in PyO3 0.17, can we please add a section to migration.md to explain this to users?

Sure.

I'm also concerned about some missing codecov here, so I want to add a couple of tests as well.

@mejrs
Copy link
Member Author

mejrs commented Apr 30, 2022

Should be done now :)

@mejrs mejrs merged commit dce4377 into PyO3:main May 2, 2022
@davidhewitt
Copy link
Member

Thanks for merging, looks great, sorry I had a busy last few days and didn't get around to looking at this sooner. 👍

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

4 participants