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

Support pointwise addition for arrays and tuples #343

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

matthiasgoergens
Copy link

Resolves #342

Synopsis

We want to support examples like these:

struct StructRecursive {
    a: i32,
    b: [i32; 2],
    c: [[i32; 2]; 3],
    d: (i32, i32),
    e: ((u8, [i32; 3]), i32),
    f: ((u8, i32), (u8, ((i32, u64, ((u8, u8), u16)), u8))),
    g: i32,
}

struct TupleRecursive((i32, u8), [(i32, u8); 10]);

Supporting arrays and tuples inside of enums would also be useful, but that's not in this PR.

Solution

Overhaul some helper functions in add_helpers and make them recursive.

Checklist

  • Documentation is updated (if required)
  • Tests are added/updated (if required)
  • CHANGELOG entry is added (if required)

Fixes JelteF#342

We want to support examples like these:

```rust
struct StructRecursive {
    a: i32,
    b: [i32; 2],
    c: [[i32; 2]; 3],
    d: (i32, i32),
    e: ((u8, [i32; 3]), i32),
    f: ((u8, i32), (u8, ((i32, u64, ((u8, u8), u16)), u8))),
    g: i32,
}

struct TupleRecursive((i32, u8), [(i32, u8); 10]);
```

Supporting arrays and tuples inside of enums would also be useful, but
that's not in this PR.
@matthiasgoergens
Copy link
Author

Unfortunately, as implemented this breaks for

#[derive(Add)]
pub struct GenericArrayStruct<T> {
    pub a: [T; 2],
}

A workaround is:

#[derive(Add)]
pub struct GenericArrayStruct<T: Copy> {
    pub a: [T; 2],
}

But a more proper fix would be useful.

This alternative implementation uses iterators and vectors, but does not
require `Copy`.
@matthiasgoergens
Copy link
Author

The latest commit on this branch has an alternative that uses Vec and iterators, but does not require Copy. I'm not sure if that's better or worse?

quote! {
lhs #field_path.into_iter().zip(rhs #field_path.into_iter())
.map(|(lhs, rhs)| #fn_body)
.collect::<Vec<_>>()
Copy link
Collaborator

@tyranron tyranron Mar 26, 2024

Choose a reason for hiding this comment

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

@matthiasgoergens

The latest commit on this branch has an alternative that uses Vec and iterators, but does not require Copy. I'm not sure if that's better or worse?

Nah... this is not good, as is not zero-cost and forbids no_std usages.

I wonder whether it could be solved with some additional add-hoc type machinery, which can be put into the add module of the derive_more crate. We could provide either our custom Addable trait, or a #[repr(transparent)] wrapper type implementing the Add trait from core, and fill it with all the necessary blanket implementations for tuples and arrays. While in macro expansion, it would be only enough to call these implementations, which will do the job recursively and automatically.

Copy link
Collaborator

@tyranron tyranron Mar 26, 2024

Choose a reason for hiding this comment

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

Expanding this machinery to all types, we may support also the following case:

type MyArr = [i32; 2];
struct StructRecursive {
    a: i32,
    b: MyArr,
}

Which won't work if we try detecting the tuple/array type via macro.

However, I do think that the coherence won't allow this, as we will quickly hit the "upstream crates may add a new impl of trait core::ops::Add" error. Which, in turn, could be tried to overcome with autoref-based specialization.

Copy link
Collaborator

@tyranron tyranron Mar 26, 2024

Choose a reason for hiding this comment

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

Yeah... it seems that something like the following should work in general case, allowing us to consider type aliases naturally:

#[repr(transparent)]
struct Wrap<T>(T);

impl<T, const N: usize> Add for &Wrap<[T; N]> {
   // custom implementation for arrays
}
impl<T> Add for &Wrap<(T,)> {
   // and so on for other tuples...
}

impl<T: Add> Add for Wrap<T> {
    // piggy back implemetation to `core::ops::Add`
}

// and something like this when expanding the macro:
(&&Wrap::from(self)).add(&Wrap::from(rhs));
// where `Wrap::from` is `#[repr(transparent)]` transmuting for references

This way autoref-based specialization will try first to resolve our custom implementations for tuple and arrays, and fallback to core::ops::Add otherwise.

@JelteF JelteF modified the milestones: 1.0.0, 1.1.0 Jul 1, 2024
@JelteF
Copy link
Owner

JelteF commented Jul 1, 2024

Moved this to the 1.1.0 milestone as this isn't a breaking change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pointwise addition (and multiplication) for array struct members?
3 participants