feat: Support generic types in Derive statements

[wraitii]

Fix #2956

This implements derive support for all existing derive for structs with generic types.
I haven't done it for enums but can be added (not sure if those can take generic types?).

Also wondering if this should be actually tested in cairo somewhere ?

---

This change is 

[spapinistarkware]

Reviewable status: 0 of 2 files reviewed, 3 unresolved discussions (waiting on @wraitii)

---

crates/cairo-lang-plugins/src/plugins/derive.rs line 64 at r1 (raw file):

enum ExtraInfo {
    Enum(Vec<SmolStr>),
    Struct(Vec<SmolStr>, Vec<SmolStr>),

Please extract variables to a new struct with named members.

---

crates/cairo-lang-plugins/src/plugins/derive.rs line 72 at r1 (raw file):

}

fn extract_generics(db: &dyn SyntaxGroup, struct_ast: &ItemStruct) -> Vec<SmolStr> {

I suspect you'll need a more involved logic when you support generic impl params.

---

crates/cairo-lang-plugins/src/test_data/derive line 20 at r1 (raw file):

#[derive(Drop, Clone, PartialEq, Serde)]
struct TwoMemberGenericStruct<T, U> {

Please test also with impl generic params.

[spapinistarkware]

You can also add a pure Cairo test under bug_samples with the issue name.

[wraitii]

Done. Note that there are currently errors in some other cairo tests (seems they're also not run by default and one needs to cargo run --bin cairo-test)

Reviewable status: 0 of 4 files reviewed, 4 unresolved discussions (waiting on @spapinistarkware)

---

crates/cairo-lang-plugins/src/plugins/derive.rs line 64 at r1 (raw file):

Previously, spapinistarkware (Shahar Papini) wrote…

Please extract variables to a new struct with named members.

Done

---

crates/cairo-lang-plugins/src/plugins/derive.rs line 72 at r1 (raw file):

Previously, spapinistarkware (Shahar Papini) wrote…

I suspect you'll need a more involved logic when you support generic impl params.

Indeed

---

crates/cairo-lang-plugins/src/plugins/derive.rs line 86 at r2 (raw file):

                }
                ast::GenericParam::Const(c) => {
                    other_generics.push(c.as_syntax_node().get_text_without_trivia(db))

I believe these don't actually work well yet, but I think this should cover it anyways?

---

crates/cairo-lang-plugins/src/test_data/derive line 20 at r1 (raw file):

Previously, spapinistarkware (Shahar Papini) wrote…

Please test also with impl generic params.

Done, added a simpler test as well

[spapinistarkware]

It's running on the CI, so I'm ok with it.

Reviewed 4 of 4 files at r2, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @wraitii)

---

crates/cairo-lang-plugins/src/plugins/derive.rs line 86 at r2 (raw file):

Previously, wraitii (Lancelot de Ferrière) wrote…

I believe these don't actually work well yet, but I think this should cover it anyways?

Right.

---

crates/cairo-lang-plugins/src/plugins/derive.rs line 100 at r2 (raw file):

    f: impl Fn(&SmolStr) -> String,
) -> String {
    match (type_generics.is_empty(), other_generics.is_empty()) {

This logic can be simpler. There is no need to check for is_entpy() (abd even in general, I think checking for length 0 is usually smelly).
Note that you can end with a trailing comma
So, simply iterating over the generics and appending something and a comma, should cut it.

---

crates/cairo-lang-plugins/src/plugins/derive.rs line 115 at r2 (raw file):

}

fn format_generics(type_generics: &Vec<SmolStr>, other_generics: &Vec<String>) -> String {

Same.

---

tests/bug_samples/issue2964.cairo line 12 at r2 (raw file):

}

// This doesn't actually work, fails with 'unknown literal' on U

Right, like u said, constant generics don't work yet. You can remove this from the test for now, with a TODO to check it as well. Put the todo on me (spapini)

[wraitii]

Reviewable status: 0 of 4 files reviewed, 9 unresolved discussions (waiting on @spapinistarkware)

---

crates/cairo-lang-plugins/src/plugins/derive.rs line 100 at r2 (raw file):

Previously, spapinistarkware (Shahar Papini) wrote…

This logic can be simpler. There is no need to check for is_entpy() (abd even in general, I think checking for length 0 is usually smelly).
Note that you can end with a trailing comma
So, simply iterating over the generics and appending something and a comma, should cut it.

Simplified the logic, it leads to empty<> when deriving non-generic structs, but that seems OK to the cairo compiler

---

crates/cairo-lang-plugins/src/plugins/derive.rs line 115 at r2 (raw file):

Previously, spapinistarkware (Shahar Papini) wrote…

Same.

Done.

---

crates/cairo-lang-plugins/src/plugins/derive.rs line 228 at r3 (raw file):

                }).join("\n            "),
                generics = format_generics(type_generics, other_generics),
                generics_impl = format_generics_with_trait(type_generics, other_generics, |t| format!("impl {t}Clone: Clone<{t}>, impl {t}Destruct: Destruct<{t}>"))

It's unclear to me why Clone requires Destruct exactly (or rather, it's unclear to me what is getting destructed?). See comment in derive test below.

---

crates/cairo-lang-plugins/src/plugins/derive.rs line 314 at r3 (raw file):

                        }).join("\n        "),
                        generics = format_generics(type_generics, other_generics),
                        generics_impl = format_generics_with_trait(type_generics, other_generics, |t| format!("impl {t}PartialEq: PartialEq<{t}>, impl {t}Destruct: Destruct<{t}>"))

Also added Destruct here, takes ownership so for now this sorta makes sense to me

---

crates/cairo-lang-plugins/src/plugins/derive.rs line 368 at r3 (raw file):

                        member_names.iter().map(|member| format!("{member}: serde::Serde::deserialize(ref serialized)?,")).join("\n            "),
                        generics = format_generics(type_generics, other_generics),
                        generics_impl = format_generics_with_trait(type_generics, other_generics, |t| format!("impl {t}Serde: serde::Serde<{t}>, impl {t}Destruct: Destruct<{t}>"))

Likewise added Destruct

---

crates/cairo-lang-plugins/src/test_data/derive line 130 at r3 (raw file):

    fn clone(self: @TwoMemberGenericStruct<T, U, impl USomeTrait: SomeTrait<U, T>, >) -> TwoMemberGenericStruct<T, U, impl USomeTrait: SomeTrait<U, T>, > {
        TwoMemberGenericStruct {
            a: self.a.clone(),

This is where the compiler complains that self.a is dropped if I don't add the TDestruct impl, which is odd to me, but there's definitely a chance I'm just not fully understanding this linear type thing.

---

tests/bug_samples/issue2964.cairo line 12 at r2 (raw file):

Previously, spapinistarkware (Shahar Papini) wrote…

Right, like u said, constant generics don't work yet. You can remove this from the test for now, with a TODO to check it as well. Put the todo on me (spapini)

Done

---

tests/bug_samples/issue2964.cairo line 26 at r3 (raw file):

#[test]
fn main() {
    // This assumes that Drop implies Destruct and Copy implies Clone

I believe this is correct, but I've seen comments in the core lib that implies it might not stay that way?

[spapinistarkware]

Reviewed 4 of 4 files at r3, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @wraitii)

---

crates/cairo-lang-plugins/src/plugins/derive.rs line 228 at r3 (raw file):

Previously, wraitii (Lancelot de Ferrière) wrote…

It's unclear to me why Clone requires Destruct exactly (or rather, it's unclear to me what is getting destructed?). See comment in derive test below.

See below

---

crates/cairo-lang-plugins/src/plugins/derive.rs line 314 at r3 (raw file):

Previously, wraitii (Lancelot de Ferrière) wrote…

Also added Destruct here, takes ownership so for now this sorta makes sense to me

See below

---

crates/cairo-lang-plugins/src/plugins/derive.rs line 368 at r3 (raw file):

Previously, wraitii (Lancelot de Ferrière) wrote…

Likewise added Destruct

See below

---

crates/cairo-lang-plugins/src/test_data/derive line 130 at r3 (raw file):

Previously, wraitii (Lancelot de Ferrière) wrote…

This is where the compiler complains that self.a is dropped if I don't add the TDestruct impl, which is odd to me, but there's definitely a chance I'm just not fully understanding this linear type thing.

The member borrowing logic does not recognize snapshots right now. This leads to multiple issues.
Here, self.a.clone() can panic, when when it does, self.a and self.b are borrowed byt value, and need to be dropped.

The correct fix here is to fix the borrowing logic to include snapshot. This will also fix #2816.

For now, add a TODO on the derive, to remove the Destruct requirements.

---

tests/bug_samples/issue2964.cairo line 21 at r3 (raw file):

//#[derive(Drop)]
struct GenericStructConst<T, const U: felt252> {
    x: T, 

Remove extra whitespace at the end of the line.

Code quote:

·

---

tests/bug_samples/issue2964.cairo line 26 at r3 (raw file):

Previously, wraitii (Lancelot de Ferrière) wrote…

I believe this is correct, but I've seen comments in the core lib that implies it might not stay that way?

That's a nother topic:) You can assume this, it's ok.

[wraitii]

Reviewable status: 0 of 4 files reviewed, 1 unresolved discussion (waiting on @spapinistarkware)

---

crates/cairo-lang-plugins/src/test_data/derive line 130 at r3 (raw file):

Previously, spapinistarkware (Shahar Papini) wrote…

The member borrowing logic does not recognize snapshots right now. This leads to multiple issues.
Here, self.a.clone() can panic, when when it does, self.a and self.b are borrowed byt value, and need to be dropped.

The correct fix here is to fix the borrowing logic to include snapshot. This will also fix #2816.

For now, add a TODO on the derive, to remove the Destruct requirements.

Done

---

tests/bug_samples/issue2964.cairo line 21 at r3 (raw file):

Previously, spapinistarkware (Shahar Papini) wrote…

Remove extra whitespace at the end of the line.

Done.

[spapinistarkware]

Reviewed 4 of 4 files at r4, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @wraitii)

[spapinistarkware]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @wraitii)

a discussion (no related file):
Note that clippy fails.

---

---

crates/cairo-lang-plugins/src/plugins/derive.rs line 314 at r4 (raw file):

                }).join("\n        "),
                generics = format_generics(type_generics, other_generics),
                // TODO(spapini): Remove the Destruct requirement by changing member borrowing logic to recognize snapshots.

Keep lines under 100 chars.

[spapinistarkware]

Reviewed 1 of 2 files at r5, 1 of 1 files at r6.
Reviewable status: all files reviewed (commit messages unreviewed), 1 unresolved discussion (waiting on @wraitii)

[spapinistarkware]

Reviewed all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @wraitii)

[VeryDustyBot]

This item belongs to payment request #70D8DC on OnlyDust:

• from abdelhamidbakhta to wraitii
• 2 items included
• $500 for 1 day of work