Add support for arbitrary arrays #1128

c410-f3r · 2020-08-30T19:47:18Z

Extends my previous work on #778 and an additional PR will soon be provided to remove the Default bound restriction.

If rust-lang/rust#75644 is going to be accepted, then the auxiliary functions won't be necessary in the near future.

kngwyu · 2020-08-31T08:38:37Z

Thank you, but is there any strong reason we should have this nightly feature?
I think we can wait for stabilization.

davidhewitt · 2020-08-31T11:04:15Z

Agreed in that I was hoping the goal was to remove the nightly feature. Maybe instead of providing this in the pyo3 core, we can extend the new FromPyObject derive to be able to plug in custom converters similar to #[serde(deserialize_with = ...)], and then the array support could be provided in a separate crate?

birkenfeld · 2020-08-31T11:14:39Z

TBH I'd rather this to be kept behind a feature flag for longer, than get merged without the second that const-generics are stabilized (#1076 calling).

davidhewitt · 2020-08-31T11:46:27Z

Agreed whatever we do we should give time for MSRV support to propagate.

c410-f3r · 2020-09-01T13:43:34Z

I did include it within the nightly feature because... you know... it is currently only available in the nightly version of Rustc.

Constant generics is now behind its own opt-in flag and all Default and Copy bounds were removed

src/types/sequence.rs

kngwyu · 2020-09-01T14:22:18Z

I did include it within the nightly feature because... you know... it is currently only available in the nightly version of Rustc.

What I meant as feature is what this PR does and not the feature flag itself. My intention was keeping the code base reasonably small.

Anyway, let's consider merging after the upstream PR is merged.

kngwyu · 2020-09-01T14:22:56Z

Ah, and I realized that now we don't have any tests for nightly feature.

davidhewitt · 2020-09-01T17:20:03Z

Ah, and I realized that now we don't have any tests for nightly feature.

As the nightly feature is just adding optimizations, there's not much testing to be added other than an extra CI job which runs with the flag enabled.

I'd prefer to move away from the pyo3 core crate having any CI which requires nightly, as it consumes contributor time (e.g. PRs spuriously failing because something changed in nightly). Hence my question above whether we can make this easy to integrate via a separate crate for now.

davidhewitt · 2020-11-12T18:53:04Z

I have been thinking further on this topic, and I wanted to propose that we do support this PR, behind a new feature. I think it's beneficial to the Rust ecosystem that we support this because it enables more users to experiment with const generics. This can help the feature to eventually stabilize and also benefits immediately our users on nightly Rust.

(Even serde has an unstable feature for things like this: https://github.com/serde-rs/serde/blob/e3d871ff7bf10dadf10bdc234a55692228358d0e/serde/Cargo.toml#L45)

I'd prefer to keep away from having a single nightly feature, because I want users to know what they're opting in to and because I want to name it clearly that it's not guaranteed to be supported forever.

So I propose to move forward on this PR with a new feature named unstable-const-generics.

c410-f3r · 2020-11-17T20:13:02Z

@davidhewitt Maybe wait for the 1.50 release?

rust-lang/rust#79135

c410-f3r · 2021-03-25T17:25:14Z

The time has come.
Are you guys still using the latest stable compiler on master? Maybe I should put back the cons-generics feature flag.

davidhewitt · 2021-03-25T18:13:18Z

We support all the way back to Rust 1.41, so this will need to go behind a const-generics feature flag, yes.

davidhewitt · 2021-03-25T18:14:24Z

An alternative to a user-facing feature flag is we could detect the compiler version in the build script, and then conditionally enable const generics based on that.

c410-f3r · 2021-03-25T18:18:52Z

We support all the way back to Rust 1.41, so this will need to go behind a const-generics feature flag, yes.

I miss the good ol' days when nightly was the only thing available for this project

An alternative to a user-facing feature flag is we could detect the compiler version in the build script, and then conditionally enable const generics based on that.

Sure, this is a good approach. I will update everything accordingly. Thanks!

davidhewitt · 2021-03-25T18:23:51Z

👍 you might be interested in anyhow's build.rs which enables backtrace support in this fashion. We might be able to follow a similar pattern here.

https://github.com/dtolnay/anyhow/blob/master/build.rs

c410-f3r · 2021-03-25T19:39:52Z

Clippy failure is not related to this PR

davidhewitt

Thanks very much for the continued refinements! This is looking great.

Afraid I have another round of comments; I think we're really close now, very excited to see this land! 😅

As well as the comments below, I have two additional points:

When I suggested moving code into conversions/array.rs, I meant all of the existing array_impls! macros and tests too. If it's all together in one place it's much easier for reviewers to audit what's tested and also maintain in the future once we don't need to support older rustc < 1.51.
This still needs a CHANGELOG entry. I propose the following:
```
### Added
- Add conversion for `[T; N]` for all `N` on Rust 1.51 and up. [#1128](https://github.com/PyO3/pyo3/pull/1128)
```
Happy for you to choose alternative wording if you think there's a better phrase out there.

src/conversions/array.rs

src/conversions/mod.rs

src/types/sequence.rs

kngwyu

Thank you for your continuous works, happy to see this works actually! 🎉

build.rs

src/conversions/array.rs

kngwyu · 2021-03-31T13:06:30Z

src/conversions/array.rs

+        let initialized_part = core::ptr::slice_from_raw_parts_mut(self.dst, *self.initialized);
+        unsafe {
+            core::ptr::drop_in_place(initialized_part);
+        }


Why do we need to drop the initialized slice?

Guard drops the initialized part when the closure panics to prevent leaks

Ah, OK, I'm sorry but I was only thinking of the situation where T is a number or so (then dropping the slice does not mean so much...).
How about using [MaybeUninit<T>; N] instead?
Then, for me, it is clearer that each element should be dropped.
Some examples (https://doc.rust-lang.org/std/mem/union.MaybeUninit.html#initializing-an-array-element-by-element and https://doc.rust-lang.org/src/core/mem/maybe_uninit.rs.html#1042) use this style.
Also, please consider moving ArrayGuard definition in the try_create_array, since it is not used in other places.

This data structure was used by previous releases of the nightly compiler and is battle tested by many people around the globe. Please consider avoid changing its original formulation.

@kngwyu note that eventually an API for this might be added to std: rust-lang/rust#75644

Personally I'm happy as long as we're sure the implementation we use is sound.

davidhewitt

Whew, this PR is turning out to be quite fiddly! Thank you for all the effort you've put into this.

I'm apologetic to say I've got another round of comments. In particular I have a concern that we might have some UB inside the try_create_array function.

src/conversions/array.rs

src/buffer.rs

src/utils.rs

src/conversions/array.rs

davidhewitt · 2021-04-24T06:21:13Z

@c410-f3r thanks very much for all the work you've done getting this PR through to this point.

I'm doing some tidying up of pending items ahead of the 0.14 release so I'm shortly going to rebase this PR and push a commit to finish it off so that we can include it in the upcoming release. Hope that's ok.

c410-f3r · 2021-04-24T13:00:49Z

Hey @davidhewitt Thanks for finishing this PR!

kngwyu

Thanks! I have some minor requests for tests

src/conversions/array.rs

c410-f3r closed this Sep 1, 2020

c410-f3r force-pushed the array branch from 5e5e96d to 90b14fb Compare September 1, 2020 13:40

c410-f3r reopened this Sep 1, 2020

c410-f3r commented Sep 1, 2020

View reviewed changes

src/types/sequence.rs Outdated Show resolved Hide resolved

sr-gi mentioned this pull request Dec 1, 2020

Arbitrary fixed length arrays sr-gi/ldk-python#1

Open

c410-f3r mentioned this pull request Feb 1, 2021

Add 'core::array::from_fn' and 'core::array::try_from_fn' rust-lang/rust#75644

Merged

c410-f3r force-pushed the array branch from 5a8ef29 to c2995ec Compare February 12, 2021 11:35

Base automatically changed from master to main March 16, 2021 22:09

c410-f3r force-pushed the array branch from 8ef47c9 to 0e35860 Compare March 25, 2021 17:53

c410-f3r changed the title ~~Support for arbitrary arrays~~ Add support for arbitrary arrays Mar 25, 2021

c410-f3r force-pushed the array branch 4 times, most recently from 2afaa93 to 5370406 Compare March 25, 2021 19:07

davidhewitt reviewed Mar 30, 2021

View reviewed changes

src/conversions/array.rs Outdated Show resolved Hide resolved

src/conversions/array.rs Outdated Show resolved Hide resolved

src/conversions/mod.rs Outdated Show resolved Hide resolved

src/types/sequence.rs Outdated Show resolved Hide resolved

c410-f3r force-pushed the array branch 3 times, most recently from 926f70a to 8f47a76 Compare March 31, 2021 12:11

kngwyu requested changes Mar 31, 2021

View reviewed changes

c410-f3r force-pushed the array branch 2 times, most recently from eb0720e to 3ba4a05 Compare March 31, 2021 16:44

davidhewitt reviewed Mar 31, 2021

View reviewed changes

davidhewitt mentioned this pull request Mar 31, 2021

buffer: tidy up exceptions #1534

Merged

davidhewitt added this to the 0.14 milestone Apr 13, 2021

davidhewitt mentioned this pull request Apr 19, 2021

0.14 Release #1570

Closed

7 tasks

davidhewitt force-pushed the array branch 2 times, most recently from 1de9d37 to 7814c2a Compare April 24, 2021 07:46

davidhewitt mentioned this pull request Apr 24, 2021

build: refactor for simplicity #1574

Merged

davidhewitt requested a review from kngwyu April 24, 2021 14:05

c410-f3r and others added 2 commits April 26, 2021 08:22

Add support for arbitrary arrays

9bc5089

array: safer implementation of try_create_array

7ead166

davidhewitt force-pushed the array branch from 5cfea26 to 7ead166 Compare April 26, 2021 07:23

davidhewitt mentioned this pull request Apr 26, 2021

Misleading error: Missing trait IntoPyCallbackOutput #1500

Closed

mejrs mentioned this pull request Apr 27, 2021

documentation for IntoPy #1578

Merged

array: improve test coverage

068e311

davidhewitt force-pushed the array branch from 4fac205 to 068e311 Compare April 27, 2021 22:41

array: fix sequence lengths in error message

a084cd5

davidhewitt approved these changes Apr 28, 2021

View reviewed changes

kngwyu requested changes Apr 29, 2021

View reviewed changes

src/conversions/array.rs Outdated Show resolved Hide resolved

src/conversions/array.rs Outdated Show resolved Hide resolved

array: rearrange file for fewer #[cfg]s

29dbd99

davidhewitt force-pushed the array branch from 5e29846 to 29dbd99 Compare May 1, 2021 06:48

davidhewitt merged commit 3ee84a3 into PyO3:main May 1, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for arbitrary arrays #1128

Add support for arbitrary arrays #1128

c410-f3r commented Aug 30, 2020

kngwyu commented Aug 31, 2020

davidhewitt commented Aug 31, 2020

birkenfeld commented Aug 31, 2020

davidhewitt commented Aug 31, 2020

c410-f3r commented Sep 1, 2020

kngwyu commented Sep 1, 2020

kngwyu commented Sep 1, 2020

davidhewitt commented Sep 1, 2020

davidhewitt commented Nov 12, 2020

c410-f3r commented Nov 17, 2020

c410-f3r commented Mar 25, 2021

davidhewitt commented Mar 25, 2021

davidhewitt commented Mar 25, 2021

c410-f3r commented Mar 25, 2021 •

edited

Loading

davidhewitt commented Mar 25, 2021

c410-f3r commented Mar 25, 2021

davidhewitt left a comment

kngwyu left a comment

kngwyu Mar 31, 2021

c410-f3r Mar 31, 2021

kngwyu Mar 31, 2021

c410-f3r Mar 31, 2021

davidhewitt Mar 31, 2021

davidhewitt left a comment

davidhewitt commented Apr 24, 2021

c410-f3r commented Apr 24, 2021

kngwyu left a comment

Add support for arbitrary arrays #1128

Add support for arbitrary arrays #1128

Conversation

c410-f3r commented Aug 30, 2020

kngwyu commented Aug 31, 2020

davidhewitt commented Aug 31, 2020

birkenfeld commented Aug 31, 2020

davidhewitt commented Aug 31, 2020

c410-f3r commented Sep 1, 2020

kngwyu commented Sep 1, 2020

kngwyu commented Sep 1, 2020

davidhewitt commented Sep 1, 2020

davidhewitt commented Nov 12, 2020

c410-f3r commented Nov 17, 2020

c410-f3r commented Mar 25, 2021

davidhewitt commented Mar 25, 2021

davidhewitt commented Mar 25, 2021

c410-f3r commented Mar 25, 2021 • edited Loading

davidhewitt commented Mar 25, 2021

c410-f3r commented Mar 25, 2021

davidhewitt left a comment

Choose a reason for hiding this comment

kngwyu left a comment

Choose a reason for hiding this comment

kngwyu Mar 31, 2021

Choose a reason for hiding this comment

c410-f3r Mar 31, 2021

Choose a reason for hiding this comment

kngwyu Mar 31, 2021

Choose a reason for hiding this comment

c410-f3r Mar 31, 2021

Choose a reason for hiding this comment

davidhewitt Mar 31, 2021

Choose a reason for hiding this comment

davidhewitt left a comment

Choose a reason for hiding this comment

davidhewitt commented Apr 24, 2021

c410-f3r commented Apr 24, 2021

kngwyu left a comment

Choose a reason for hiding this comment

c410-f3r commented Mar 25, 2021 •

edited

Loading