Implement `FromPyObject` and `IntoPy<PyObject>` traits for arrays (up to 32) #778

c410-f3r · 2020-02-29T01:45:11Z

Fixes #775.

Copy + Default bounds to initialize memory for copy_to_slice_impl and avoid unsafe (MaybeUninit and ptr friends). Copy bound is also necessary due to the lack of a "real" IntoIterator implementation for arrays.

c410-f3r · 2020-02-29T01:47:08Z

src/types/list.rs

+    create_tests!(array);
+    create_tests!(vec vec!);


Not sure if this approach is desired. If so, I will also do the same in sequence.rs.

I think this is a good idea, thanks for adding comprehensive tests!

kngwyu · 2020-02-29T03:19:12Z

src/types/list.rs

+macro_rules! array_impls {
+    ($($N:expr),+) => {
+        $(
+            impl<T> ToPyObject for [T; $N]


As these imples are not faster than one for slices(impl ~ for &[T]), I don't think we need this.

I agree for ToPyObject but not for IntoPy<PyObject> - see below.

kngwyu · 2020-02-29T03:26:27Z

src/types/sequence.rs

@@ -282,6 +325,22 @@ where
    Ok(v)
 }

+fn extract_sequence_into_slice<'s, T>(obj: &'s PyAny, slice: &mut [T]) -> PyResult<()>


How about extract_sequence_inplace?

I tend to think _inplace as referring to the first argument, so I would vote against that name.

extract_sequence_into_slice is fine imo.

kngwyu · 2020-02-29T03:27:25Z

Thank you!
FromPyObject impl looks good but I don't think we need IntoPy<PyObject> and ToPyObject impls for arrays.

davidhewitt · 2020-02-29T11:35:29Z

Also thanks from me!

FromPyObject impl looks good but I don't think we need IntoPy<PyObject> and ToPyObject impls for arrays.

I slightly disagree with this. ToPyObject taking &[T; N] maybe doesn't offer benefit over &[T] but I believe IntoPy<PyObject> offers value.

For example, the following function will not compile without the IntoPy<PyObject> implementation:

#[pyfunction]
fn make_array()-> [i32; 2] {
    [0, 0]
}

davidhewitt · 2020-02-29T11:40:50Z

CHANGELOG.md

@@ -9,6 +9,8 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.

 ## [0.9.0]

+* Implement `*Py` traits for arrays (up to 32).


Can you please change the line to read the following and also move it to the Added section of the 0.9.0 changelog.

Suggested change

* Implement `*Py` traits for arrays (up to 32).

* Implement `FromPyObject` and `IntoPy<PyObject>` traits for arrays (up to 32). [#778](https://github.com/PyO3/pyo3/pull/778)

davidhewitt · 2020-02-29T11:43:10Z

src/types/list.rs

+    create_tests!(array);
+    create_tests!(vec vec!);


I think this is a good idea, thanks for adding comprehensive tests!

c410-f3r · 2020-02-29T13:18:38Z

Comments addressed. Thank you guys

kngwyu · 2020-02-29T17:55:16Z

For example, the following function will not compile without the IntoPy implementation:

That makes sense, thanks 👍

kngwyu · 2020-02-29T17:55:53Z

src/types/list.rs

+        $(
+            impl<T> IntoPy<PyObject> for [T; $N]
+            where
+                T: Copy + IntoPy<PyObject>,


Let's change this bound to T: ToPyObject.
Then we can use self.as_ref().to_object(py) for implementation.

kngwyu · 2020-02-29T18:28:57Z

src/types/sequence.rs

+    T: FromPyObject<'s>,
+{
+    let seq = <PySequence as PyTryFrom>::try_from(obj)?;
+    if (seq.len()? as usize) != slice.len() {


Suggested change

if (seq.len()? as usize) != slice.len() {

if seq.len()? as usize != slice.len() {

kngwyu · 2020-02-29T18:35:03Z

src/types/sequence.rs

@@ -330,6 +389,9 @@ impl<'v> PyTryFrom<'v> for PySequence {

 #[cfg(test)]
 mod test {
+    macro_rules! create_tests {


Here not all tests are for FromPyObject.
Do we need to repeat all of them?
I think what we really need is array version of test_extract_bytearray_to_vec.

kngwyu · 2020-02-29T18:36:38Z

src/types/list.rs

@@ -205,6 +232,9 @@ where

 #[cfg(test)]
 mod test {


Same as below.
Don't need to repeat all tests.

kngwyu · 2020-03-02T08:32:04Z

Thank you for addressing comments, but as I commented, I think current macro-generated tests seem to make it unclear what is tested here.
For FromPyObject implementation, we just need a test that calls let arr: [u32; N] = obj.extract().unwrap(). Most of tests like test_seq_contains are not related to this PR at all.

We can say the same thing for IntoPy<PyObject> implementation. Most of the tests are not array-specific and use ToPyObject for &[T] implementation(e.g., test_new).
What we need is simply a test that calls [1, 2, 3].into_py(py).

c410-f3r · 2020-03-02T21:26:26Z

I was waiting for David but looks like these tests are actually unwanted

davidhewitt · 2020-03-02T22:25:40Z

Thanks - sorry about the extra work there. I thought your macro test implementation was nice, but @kngwyu has been a maintainer of pyo3 for much longer than I so I'm still deferring to his opinion when we differ.

I wasn't going to ask you to undo the work though - was thinking about pushing a change myself. Thanks for doing so.

kngwyu · 2020-03-03T04:09:42Z

@c410-f3r
Thank you for completing the PR!
@davidhewitt

so I'm still deferring to his opinion when we differ.

You don't have to do so. We sometimes make mistakes, but fortunately, we can help us with each other. When our opinion differs, it just means we need a discussion.

c410-f3r commented Feb 29, 2020

View reviewed changes

kngwyu requested changes Feb 29, 2020

View reviewed changes

davidhewitt reviewed Feb 29, 2020

View reviewed changes

c410-f3r changed the title ~~Implement *Py traits to arrays (up to 32)~~ Implement *Py traits for arrays (up to 32) Feb 29, 2020

c410-f3r changed the title ~~Implement *Py traits for arrays (up to 32)~~ Implement FromPyObject and IntoPy<PyObject> traits for arrays (up to 32) Feb 29, 2020

c410-f3r force-pushed the array-impls branch from 079029d to 4d5241d Compare February 29, 2020 13:17

davidhewitt approved these changes Feb 29, 2020

View reviewed changes

kngwyu reviewed Feb 29, 2020

View reviewed changes

kngwyu requested changes Feb 29, 2020

View reviewed changes

c410-f3r force-pushed the array-impls branch from f980d2c to e2bcc2a Compare March 2, 2020 00:43

Implement *Py* traits for arrays (up to 32)

6b78f1b

c410-f3r force-pushed the array-impls branch from e2bcc2a to 6b78f1b Compare March 2, 2020 21:25

kngwyu approved these changes Mar 3, 2020

View reviewed changes

Merge branch 'master' into array-impls

dc8e0c4

Make rustfmt happy

402c382

kngwyu merged commit f470154 into PyO3:master Mar 3, 2020

c410-f3r mentioned this pull request Aug 30, 2020

Add support for arbitrary arrays #1128

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement `FromPyObject` and `IntoPy<PyObject>` traits for arrays (up to 32) #778

Implement `FromPyObject` and `IntoPy<PyObject>` traits for arrays (up to 32) #778

c410-f3r commented Feb 29, 2020 •

edited

Loading

c410-f3r Feb 29, 2020 •

edited

Loading

davidhewitt Feb 29, 2020

kngwyu Feb 29, 2020

davidhewitt Feb 29, 2020

kngwyu Feb 29, 2020

davidhewitt Feb 29, 2020

kngwyu commented Feb 29, 2020

davidhewitt commented Feb 29, 2020

davidhewitt Feb 29, 2020

davidhewitt Feb 29, 2020

c410-f3r commented Feb 29, 2020

kngwyu commented Feb 29, 2020

kngwyu Feb 29, 2020 •

edited

Loading

kngwyu Feb 29, 2020

kngwyu Feb 29, 2020 •

edited

Loading

kngwyu Feb 29, 2020

kngwyu commented Mar 2, 2020 •

edited

Loading

c410-f3r commented Mar 2, 2020

davidhewitt commented Mar 2, 2020

kngwyu commented Mar 3, 2020

		@@ -9,6 +9,8 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.

		## [0.9.0]

		* Implement `*Py` traits for arrays (up to 32).

	* Implement `*Py` traits for arrays (up to 32).
	* Implement `FromPyObject` and `IntoPy<PyObject>` traits for arrays (up to 32). [#778](https://github.com/PyO3/pyo3/pull/778)

	if (seq.len()? as usize) != slice.len() {
	if seq.len()? as usize != slice.len() {

Implement FromPyObject and IntoPy<PyObject> traits for arrays (up to 32) #778

Implement FromPyObject and IntoPy<PyObject> traits for arrays (up to 32) #778

Conversation

c410-f3r commented Feb 29, 2020 • edited Loading

c410-f3r Feb 29, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kngwyu commented Feb 29, 2020

davidhewitt commented Feb 29, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

c410-f3r commented Feb 29, 2020

kngwyu commented Feb 29, 2020

kngwyu Feb 29, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kngwyu Feb 29, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kngwyu commented Mar 2, 2020 • edited Loading

c410-f3r commented Mar 2, 2020

davidhewitt commented Mar 2, 2020

kngwyu commented Mar 3, 2020

Implement `FromPyObject` and `IntoPy<PyObject>` traits for arrays (up to 32) #778

Implement `FromPyObject` and `IntoPy<PyObject>` traits for arrays (up to 32) #778

c410-f3r commented Feb 29, 2020 •

edited

Loading

c410-f3r Feb 29, 2020 •

edited

Loading

kngwyu Feb 29, 2020 •

edited

Loading

kngwyu Feb 29, 2020 •

edited

Loading

kngwyu commented Mar 2, 2020 •

edited

Loading