RFC: Provide a special purpose FromPyObject impl for byte slices

[adamreichold]

This enables efficiently and safely getting a byte slice from either bytes or byte arrays.

The main issue I see here is discoverability, i.e. should this be mention in the docs of PyBytes and PyByteArray or in the guide?

It is also not completely clear whether this really fixes the issue.

Closes #2888

[mejrs]

LGTM, please add some tests though 🙂

[adamreichold]

please add some tests though

Certainly, and also a changelog entry. I just wanted to discuss whether we really want this hack and where we need to document it.

The doc comment on the impl would only be visible in the implementors section of FromPyObject AFAIU which is not easily discoverable IMHO.

[davidhewitt]

Sneaky! I think this seems reasonable, perhaps we should also add into-py conversions for Cow<'_, [u8]> for consistency?

I think I would also try to make this discoverable in the guide, maybe both from https://pyo3.rs/v0.18.0/conversions/tables and on a new page discussing performance techniques? (I've been thinking about adding a performance page for ages anyway.)

I wonder, should we also ban Vec<u8> from #[pyfunction] arguments and return values? That would help avoid the performance footgun and reserve them for specialization later without breaking user code at that time. (I mean in a separate PR, not here.)

[adamreichold]

I wonder, should we also ban Vec from #[pyfunction] arguments and return values? That would help avoid the performance footgun and reserve them for specialization later without breaking user code at that time. (I mean in a separate PR, not here.)

You specifically mean banning Vec<u8>, not Vec<T>? How would we do this? Try to parse the argument types in the proc macros? Is that realistically possible on a syntactical level and considering arbitrarily complex types containing Vec<u8>?

(I think raising an error in FromPyObject is not possible due to the missing T: 'static bound in the impl for Vec<T> and making this a runtime error would also be poor ergonomics.)

[mejrs]

I wonder, should we also ban Vec from #[pyfunction] arguments and return values? T

This seems more worthy of a lint than a compile error.

How would we do this? Try to parse the argument types in the proc macros? Is that realistically possible on a syntactical level and considering arbitrarily complex types containing Vec?

We'd check whether the type looks like "Vec<u8>". Which is rather messy, I feel.

[adamreichold]

I think this seems reasonable, perhaps we should also add into-py conversions for Cow<'_, [u8]> for consistency?

So IntoPy<Py<PyAny>> for Cow<'_, [u8]>? That seems reasonable and could be done symmetrically, i.e. Borrowed to bytes and Owned to bytearray. The symmetry is limited as we have to copy the values in both cases, but at least this gives us a "type-driven" way to produce bytes and bytearray.

I think I would also try to make this discoverable in the guide, maybe both from https://pyo3.rs/v0.18.0/conversions/tables and on a new page discussing performance techniques? (I've been thinking about adding a performance page for ages anyway.)

I will adjust the table. I would ask for the performance section to be a separate PR which I will try to produce in reasonable amount of time (if no one else does so first). The reason is that I expect the wording to be somewhat contentions as we probably all care very much about performance and the techniques in question can be subtle and their applicability depend on a lot of context.

[davidhewitt]

Potentially also ToPyObject? Really need to work out those traits 😂

The symmetry is an interesting feature. It's quite pretty, but I think it has a risk of surprising users? They are likely not going to think too hard what variant their Cow is on and suddenly their Python type changes. Maybe from different branches of the same function. Seems like the sort of thing which will cause bug reports by people who don't read documentation.

As an alternative, how about changing the Cow::Owned branches so that it accepts any byte buffer (using PyBuffer::get instead of downcast to PyByteArray), and always return PyBytes? Users who want a PyByteArray can get better performance constructing directly from a &[u8] without an extra allocation to have a Cow::Owned, and I'm not convinced we need round tripping here.

I feel more strongly about narrowing the to-python value to be PyBytes always than I do about widening the input to accept all byte buffers.

[davidhewitt]

And yes, @mejrs is right on what I was thinking to do for Vec. It's not pretty, but we do do it already for Python and Option.

[adamreichold]

Potentially also ToPyObject? Really need to work out those traits joy

Done.

As an alternative, how about changing the Cow::Owned branches so that it accepts any byte buffer (using PyBuffer::get instead of downcast to PyByteArray), and always return PyBytes? Users who want a PyByteArray can get better performance constructing directly from a &[u8] without an extra allocation to have a Cow::Owned, and I'm not convinced we need round tripping here.

Implemented, but this is conditional not using the limited API or Python 3.11 or later. It also not clear how to include this in the guide.

And yes, @mejrs is right on what I was thinking to do for Vec. It's not pretty, but we do do it already for Python and Option.

I would still prefer a lint then, especially if this is imprecise due to working with syntax only. (I would also suggest a separate PR as well since this one seems to sprawl somewhat.)

One related idea that is a bit out in the woods: Since for #[pyfunction] we are not in generic context, but in macro context, Deref-based specialization might be an option to efficiently handle Vec<[u8]>. However, this would mean stepping outside of our FromPyObject trait hierarchy and doing something in the generated code that cannot be done just by invoking FromPyObject, so I am not sure whether this would be worth it. But it would open up a general avenue towards specialization without compiler support.

[davidhewitt]

I would still prefer a lint then, especially if this is imprecise due to working with syntax only.

Note that there's no stable way to do lints from macros, so any lints are a clumsy hack like what we do with deprecations.

One related idea that is a bit out in the woods: Since for #[pyfunction] we are not in generic context, but in macro context, Deref-based specialization might be an option to efficiently handle Vec<[u8]>

It's an interesting thought and might work nicely, we could have PyFunctionInput and PyFunctionOutput traits, and document them with users even? Worth considering in another PR, I agree.

Implemented, but this is conditional not using the limited API or Python 3.11 or later. It also not clear how to include this in the guide.

Ah, I had overlooked PyBuffer was unavailable in all contexts. Yes, need to consider what feels better for users...

[birkenfeld]

Note that there's no stable way to do lints from macros,

I was just thinking about that too. Maybe time for an RFC?

[davidhewitt]

@adamreichold what's your feeling about this currently? I think I regret my suggestion of PyBuffer, I'm thinking we should narrow the input types back to just bytes and bytearray to remove the conditionality. Keep the output type as bytes only?

After that, probably good to merge this?

(We could leave an issue in the backlog to remind ourselves to widen this conversion in a few years when 3.10 is dropped?)

[adamreichold]

@adamreichold what's your feeling about this currently? I think I regret my suggestion of PyBuffer, I'm thinking we should narrow the input types back to just bytes and bytearray to remove the conditionality. Keep the output type as bytes only?

Yes, I think we should start with the simplest possible version and extend it after we have gathered further feedback. I intentionally added the buffer protocol support as a separate commit which I will drop from the PR.

After that, probably good to merge this?

Yes, I think so.

(We could leave an issue in the backlog to remind ourselves to widen this conversion in a few years when 3.10 is dropped?)

Yes, will do.

[davidhewitt]

Looks great, thanks!

bors r+

[adamreichold]

Concerning the build error, I am not sure how the builtin isinstance can be missing on Python 3.8. Would it be sufficient if I just used our PyAny::is_instance_of::<PyBytes>() instead or would that reduce testing power too much?

[bors]

Canceled.

[adamreichold]

Added a commit to use native code instead of inline Python for the type assertions.

bors r=davidhewitt

[davidhewitt]

Concerning the build error, I am not sure how the builtin isinstance can be missing on Python 3.8. Would it be sufficient if I just used our PyAny::is_instance_of::<PyBytes>() instead or would that reduce testing power too much?

I think probably this is related to #2891 somehow. Needs some investigation. Using PyAny::is_instance_of seems fine for now.

[bors]

Build succeeded:

• conclusion