Add opt-in generic traits for generated SoA types #73

mobiusklein · 2025-01-16T20:48:31Z

This adds SoASlice, SoASliceMut, and SoAVec traits and adds a new attribute #[generate_traits] to automatically implement them on the generated types.

Because I saw a specific Rust version pinned in the test suite that predated the stabilization of GATs, I had to feature-gate the traits on both the internal and public-facing crates. Unfortunately, the mixed root + internal crates workspace setup didn't seem to support traits on the crate at the repository root, so I had to re-structure repository to a uniform workspace. The traits still exist when the generic_traits feature is disabled, but they have no methods or associated types so the source code is still Rust 1.63 compatible.

Hopefully, that means there are no breaking changes unless the feature is enabled.

I couldn't implement a clean "root trait" that tied all of the different types and interfaces together. I could not find a way to do so while not generating a type system query overflow or cycle or cease to function as lifetime dependencies tangled. I'm still not satisfied with the API, but I've convinced myself that as the language stands today I can't make it any better.

…oncrete methods

…ays`

mobiusklein · 2025-01-27T03:30:07Z

Added an additional optional type-level attribute macro soa_crate(::path::to::soa_derive) for locating the soa_derive crate if it is not imported from ::soa_derive directly. This is so that it can be vendor'd or aliased.

Luthaf · 2025-01-27T11:26:01Z

Added an additional optional type-level attribute macro soa_crate(::path::to::soa_derive) for locating the soa_derive crate if it is not imported from ::soa_derive directly. This is so that it can be vendor'd or aliased.

This is unrelated to the generic trait, right? Could you split it into a separate PR?

Luthaf · 2025-01-27T11:26:57Z

Regarding the main feature: it is quite a lot of new code, so I'll need to find the time to review it properly!

Luthaf · 2025-01-27T11:30:04Z

From a very quick look, could you explain why we need a generic_traits feature? Could this not be always enabled?

Also, please do not reorganize the code while contributing a feature. You moved the soa_derive crate around, but it is not needed. Can you move it back to the root of the repository?

mobiusklein · 2025-01-27T16:45:14Z

Added an additional optional type-level attribute macro soa_crate(::path::to::soa_derive) for locating the soa_derive crate if it is not imported from ::soa_derive directly. This is so that it can be vendor'd or aliased.

This is unrelated to the generic trait, right? Could you split it into a separate PR?

Technically no, it just required making changes to all generated code, including the newly added trait code. If you still want it split into another PR, I can revert the changes on this branch and then re-add them afterwards in a separate PR if this one is accepted.

From a very quick look, could you explain why we need a generic_traits feature? Could this not be always enabled?

I feature-gated this behavior because it requires GATs and you had explicitly called out a Rust version without GAT support in the CI configuration. If you want to enable the behavior all the time, while retaining the added generate_traits attribute to actually opt-in to trait impls, I'll need to remove the pinned 1.63 Rust version in CI.

Also, please do not reorganize the code while contributing a feature. You moved the soa_derive crate around, but it is not needed. Can you move it back to the root of the repository?

I wasn't able to get Cargo to recognize a feature on soa_derive itself when the Cargo.toml that configured the workspace was also the one for soa_derive itself. If you want me to remove the feature gate, I can also restore the original project layout.

mobiusklein · 2025-01-28T22:29:24Z

Just so I know whether to get started, do you agree I should remove the pre-GAT version of Rust from CI and restore the project layout?

Thank you

Luthaf · 2025-01-29T13:22:57Z

It looks like we only need to update the MSRV to 1.65 to get GAT, right? If so, I'm happy to update the MSRV here, remove the feature gate & code re-organization!

EDIT: this would mean updating the pinned rustc in CI to 1.65. If 1.65 has other issues/missing parts, we could also update further, but I'm trying to make sure soa_derive is not forcing downstream projects to upgrade their rustc version.

Luthaf · 2025-01-29T13:26:17Z

Technically no, it just required making changes to all generated code, including the newly added trait code. If you still want it split into another PR, I can revert the changes on this branch and then re-add them afterwards in a separate PR if this one is accepted.

Yes please!

If you want to enable the behavior all the time, while retaining the added generate_traits attribute to actually opt-in to trait impls

I guess this is because generate_traits will increase compile time a lot? Here as well I would prefer if the trait implementation was generated in all cases, unless the slowdown is very high.

commit a1cd5b6 Author: Joshua Klein <[email protected]> Date: Sun Feb 2 13:31:10 2025 -0500 make trait implementation automatic

mobiusklein · 2025-02-02T18:46:01Z

I have reverted changes to the project layout, made trait implementation automatic, and removed the crate name option

mobiusklein · 2025-02-02T18:47:19Z

It appears the minimum Rust version required to run the test suite is 1.78. The code itself might compile on an earlier Rust version though using it in a generic context would likely be constrained.

mobiusklein · 2025-02-05T15:28:49Z

@Luthaf just to make sure this is clear, the current state of the PR is ready for review, or at least to have the MSRV change requirement rejected so I can revert all the changes and restore the feature gate or attempt some other means of achieving the same idea without forcing the project layout to change.

Luthaf · 2025-02-12T09:35:17Z

Hey @mobiusklein! Sorry for the delayed answer, both personal and work life have been taking a lot of my time recently. I'll likely have to continue dealing with it for a couple more weeks, so this might take some time, sorry in advance.

Just to be clear, 1.78 is only required for the tests of the new traits, right? I'd like to keep the code in a state that can compile on recent-ish ubuntu, which is currently using 1.75. So for now bumping CI to 1.78 would be fine for me, and I can add another CI job that checks with an older version that the code still compiles, excluding the tests for the new traits.

mobiusklein · 2025-02-12T19:05:32Z

Thank you for the warning, not a problem. I am currently using this for prototyping, but if it advances, I may need to release a fork since I don't think vendoring works with proc-macros. This was a great way to learn about procedural macros and if I am not careful I'll rewrite more of my traits this way.

Yes, that's correct. I had also looked into how hard it would be to introduce conditional compilation for a specific Rust version, but unfortunately that requires adding a build script and/or extra dependencies. At one point the GATs were actually causing a compiler crash on Rust 1.80 or 1.82, but I don't think that version of the code is being used here.

Luthaf

This looks good overall! There are a couple of places where I did not understand the reason behind the code, and what looks like a couple of leftover from development.

example/Cargo.toml

soa-derive-internal/Cargo.toml

Luthaf · 2025-02-25T22:43:22Z

soa-derive-internal/src/iter.rs

@@ -124,7 +125,7 @@ pub fn derive(input: &Input) -> TokenStream {
            /// Get an iterator over the
            #[doc = #ref_doc_url]
            /// in this vector
-            pub fn iter(&self) -> #iter_name {
+            pub fn iter(&self) -> <#name as ::soa_derive::SoAIter>::Iter {


I don't understand why this is required here?

For readbility, I find referring to the type directly better, instead of going through associated types of a trait

I was likely tying the type system in balloon animals at one point and that made the system happy, and after reverting that particular design, it remained happy with those aliases so they remained. I'll restored the generated names

I think this still needs updating

soa-derive-internal/src/iter.rs

soa-derive-internal/src/lib.rs

src/lib.rs

Luthaf · 2025-02-25T22:51:38Z

src/lib.rs

+//! # mod cheese {
+//! # use soa_derive::{StructOfArray, prelude::*};
+//! #[derive(StructOfArray)]
+//! #[generate_traits]


not sure if this is up to date, but what would be the cost of always generating the trait implementations?

And if the cost is too high to always enable them, then this should be scoped with something like #[soa_derive(generate_traits)]

Luthaf · 2025-02-25T22:53:23Z

src/lib.rs

+pub mod prelude {
+    pub use super::{SoAVec, SoAIter, SoASlice, SoASliceMut, SoAPointers, StructOfArray, ToSoAVec, IntoSoAIter, SoAAppendVec};
+}


This is everything defined by this crate, isn't it? I don't think it's worth having a separate prelude in this case, the same can be achieved with use soa_derive::* instead of use soa_derive::prelude::*

That'd also pull the soa_zip macro and its helper, soa_zip_impl, along with SoAIndex, which isn't necessary to use the generic traits or the types themselves.

The prelude also means that if in the future you were to add more things to the crate root, they would only be pulled by a wildcard import if they were also added to the prelude. This is probably overly defensive future-proofing though.

If you still think this is acceptable, I'm fine with removing the prelude module.

tests/particles/mod.rs

Co-authored-by: Guillaume Fraux <[email protected]>

mobiusklein · 2025-02-26T00:27:36Z

There were a lot of things left over from development, thank you for noting that. I had tried several different overlapping designs to try to find a way out of the corners I was painting myself into with the type system. At one point it was even crashing rustc 1.82.

I'll go through the documentation again now that the implementation is more-or-less acceptable. Thank you.

mobiusklein · 2025-03-16T13:34:37Z

Can you please re-iterate your ruling on the prelude module? If you still want it removed, I'll remove it and then do a final pass over the documentation.

Luthaf · 2025-03-16T17:23:22Z

Let's remove the prelude for now, we can always add it back down the line if it is needed, but for now I don't think it is worth doing.

Thanks for your patience with this! I'll try to do a final review tomorrow, but I think the code is in a great shape overall!

Luthaf

Thanks a lot for this work! I'll try to make the code work on CI with an earlier rustc, and then make a new release with this

Luthaf · 2025-03-19T22:04:29Z

soa-derive-internal/src/refs.rs

+        impl<'a>  From<#ref_name<'a>> for #name where #( for<'b> #fields_types: Clone, )* {
+            fn from(value: #ref_name<'a>) -> #name {
+                value.to_owned()
+            }
+        }
+
+        impl<'a>  From<&'a #ref_name<'a>> for #name where #( for<'b> #fields_types: Clone, )* {
+            fn from(value: &'a #ref_name<'a>) -> #name {
+                value.to_owned()
+            }
+        }
+


ok, this makes sense. Thanks for the detailed explanation!

Luthaf · 2025-03-19T22:46:07Z

Now published as v0.14.0 on crates.io!

mobiusklein added 22 commits January 10, 2025 10:04

feat: add traits for sequence-like behaviors already implemented as c…

7841e87

…oncrete methods

remove dead code

ff43c47

checkpoint

a96d8ff

Switch to lifetime-bound traits not directly attached to `StructOfArr…

a82e41f

…ays`

checkpoint two lifetimes

38029d9

Fix most of the lifetime mess, polish interface, add documentation

b69298e

checkpoint

d679b90

checkpoint

4566a50

checkpoint: rewrite

0e0348d

checkpoint

9ba2358

unbundle traits

3c21049

checkpoint

aab3848

restructure crate for features

0fc43d5

guard traits

5f3e766

manage dead code warnings

498d5de

docs

f752525

Remove commented out impl

ad073f7

remove traits from example

3cc69fd

exclude doctest

8db660b

add supporting traits for to_vec and into_iter

3aecfd0

undo bundling, introduces problematic lifetime net

f200fa0

feature: add soa_crate type-level attribute for vendoring

8fdcb3a

chore: slightly better error message

a95fce8

mobiusklein added 8 commits February 2, 2025 13:02

checkpoint

dec18c9

restore original layout

9daab55

Squashed commit of the following:

c4989b5

commit a1cd5b6 Author: Joshua Klein <[email protected]> Date: Sun Feb 2 13:31:10 2025 -0500 make trait implementation automatic

remove feature flag

cac64ac

commence CI-based version search

1c3f227

update version range

1f18602

min version found

7ae3b44

minimum version found

221e26e

Luthaf reviewed Feb 25, 2025

View reviewed changes

mobiusklein and others added 3 commits February 25, 2025 18:31

Update soa-derive-internal/src/lib.rs

511787d

Co-authored-by: Guillaume Fraux <[email protected]>

Update example/Cargo.toml

ae7ce4c

Co-authored-by: Guillaume Fraux <[email protected]>

Apply suggestions from code review

333d233

Co-authored-by: Guillaume Fraux <[email protected]>

mobiusklein added 2 commits February 25, 2025 22:34

Apply more suggestions from code review, update documentation

461ba24

Apply more suggestions from code review

7dccb57

remove the prelude and alluded to in the review

ee3f77e

Luthaf approved these changes Mar 19, 2025

View reviewed changes

Luthaf merged commit 6a66d43 into lumol-org:master Mar 19, 2025
5 checks passed

Luthaf pushed a commit that referenced this pull request Mar 19, 2025

Add opt-in generic traits for generated SoA types (#73)

7182fd3

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add opt-in generic traits for generated SoA types #73

Add opt-in generic traits for generated SoA types #73

mobiusklein commented Jan 16, 2025

mobiusklein commented Jan 27, 2025

Luthaf commented Jan 27, 2025

Luthaf commented Jan 27, 2025 •

edited

Loading

Luthaf commented Jan 27, 2025 •

edited

Loading

mobiusklein commented Jan 27, 2025

mobiusklein commented Jan 28, 2025

Luthaf commented Jan 29, 2025 •

edited

Loading

Luthaf commented Jan 29, 2025

mobiusklein commented Feb 2, 2025

mobiusklein commented Feb 2, 2025

mobiusklein commented Feb 5, 2025 •

edited

Loading

Luthaf commented Feb 12, 2025

mobiusklein commented Feb 12, 2025

Luthaf left a comment

Luthaf Feb 25, 2025

mobiusklein Feb 26, 2025

Luthaf Mar 5, 2025

Luthaf Feb 25, 2025

Luthaf Feb 25, 2025

mobiusklein Feb 26, 2025

mobiusklein commented Feb 26, 2025

mobiusklein commented Mar 16, 2025

Luthaf commented Mar 16, 2025

Luthaf left a comment

Luthaf Mar 19, 2025

Luthaf commented Mar 19, 2025

Add opt-in generic traits for generated SoA types #73

Add opt-in generic traits for generated SoA types #73

Conversation

mobiusklein commented Jan 16, 2025

mobiusklein commented Jan 27, 2025

Luthaf commented Jan 27, 2025

Luthaf commented Jan 27, 2025 • edited Loading

Luthaf commented Jan 27, 2025 • edited Loading

mobiusklein commented Jan 27, 2025

mobiusklein commented Jan 28, 2025

Luthaf commented Jan 29, 2025 • edited Loading

Luthaf commented Jan 29, 2025

mobiusklein commented Feb 2, 2025

mobiusklein commented Feb 2, 2025

mobiusklein commented Feb 5, 2025 • edited Loading

Luthaf commented Feb 12, 2025

mobiusklein commented Feb 12, 2025

Luthaf left a comment

Choose a reason for hiding this comment

Luthaf Feb 25, 2025

Choose a reason for hiding this comment

mobiusklein Feb 26, 2025

Choose a reason for hiding this comment

Luthaf Mar 5, 2025

Choose a reason for hiding this comment

Luthaf Feb 25, 2025

Choose a reason for hiding this comment

Luthaf Feb 25, 2025

Choose a reason for hiding this comment

mobiusklein Feb 26, 2025

Choose a reason for hiding this comment

mobiusklein commented Feb 26, 2025

mobiusklein commented Mar 16, 2025

Luthaf commented Mar 16, 2025

Luthaf left a comment

Choose a reason for hiding this comment

Luthaf Mar 19, 2025

Choose a reason for hiding this comment

Luthaf commented Mar 19, 2025

Luthaf commented Jan 27, 2025 •

edited

Loading

Luthaf commented Jan 27, 2025 •

edited

Loading

Luthaf commented Jan 29, 2025 •

edited

Loading

mobiusklein commented Feb 5, 2025 •

edited

Loading