fixed closure type dependencies so it does not depend on the number o…

[TomerStarkware]

…f captured variables or their order

[reviewable-StarkWare]

This change is 

[orizi]

Reviewed 2 of 4 files at r1.
Reviewable status: 2 of 5 files reviewed, 2 unresolved discussions (waiting on @ilyalesokhin-starkware)

---

crates/cairo-lang-semantic/src/items/imp.rs line 1898 at r2 (raw file):

        chain!(
            closure_type_long
                .captured_types

add a specific test that the destruct properly works.

---

crates/cairo-lang-lowering/src/lower/block_builder.rs line 308 at r1 (raw file):

                .iter()
                .eq(inputs.iter().map(|var_usage| &ctx.variables[var_usage.var_id].ty))
        );

should we really add this?

Code quote:

        let TypeLongId::Closure(closure_id) = expr.ty.lookup_intern(ctx.db) else {
            unreachable!("Closure Expr should have a Closure type.");
        };

        // Assert that the closure type matches the input we pass to it.
        assert!(
            closure_id
                .captured_types
                .iter()
                .eq(inputs.iter().map(|var_usage| &ctx.variables[var_usage.var_id].ty))
        );

[ilyalesokhin-starkware]

crates/cairo-lang-semantic/src/items/imp.rs line 1900 at r2 (raw file):

                .captured_types
                .iter()
                .sorted_by_key(|ty| ty.as_intern_id())

I don't think this is stable

Code quote:

 ty.as_intern_id(

[ilyalesokhin-starkware]

tests/bug_samples/issue7234.cairo line 9 at r2 (raw file):

}

fn main() {

what was the issue before this fix?

Code quote:

fn main() {

[orizi]

Reviewed 2 of 3 files at r3, all commit messages.
Reviewable status: 3 of 6 files reviewed, 3 unresolved discussions (waiting on @ilyalesokhin-starkware and @TomerStarkware)

---

crates/cairo-lang-semantic/src/items/imp.rs line 1900 at r2 (raw file):

Previously, ilyalesokhin-starkware wrote…

I don't think this is stable

It isn't - but it is consistent within an instance, and these are basically just impl bounds.

---

tests/bug_samples/issue7234.cairo line 9 at r2 (raw file):

Previously, ilyalesokhin-starkware wrote…

what was the issue before this fix?

Compiler crash.

[TomerStarkware]

Reviewable status: 3 of 6 files reviewed, 3 unresolved discussions (waiting on @ilyalesokhin-starkware and @orizi)

---

crates/cairo-lang-lowering/src/lower/block_builder.rs line 308 at r1 (raw file):

Previously, orizi wrote…

should we really add this?

I want some coupling between the sierra's number fo struct members, and arguments we pass to its constructor.

---

crates/cairo-lang-semantic/src/items/imp.rs line 1898 at r2 (raw file):

Previously, orizi wrote…

add a specific test that the destruct properly works.

Done.

---

crates/cairo-lang-semantic/src/items/imp.rs line 1900 at r2 (raw file):

Previously, ilyalesokhin-starkware wrote…

I don't think this is stable

do we need it to be stable, its just the list of impl params

---

tests/bug_samples/issue7234.cairo line 9 at r2 (raw file):

Previously, ilyalesokhin-starkware wrote…

what was the issue before this fix?

the closure's sierra type had only one member (since captured types contained only one bytearray)

[orizi]

Reviewed 1 of 4 files at r1, 1 of 1 files at r2, 1 of 3 files at r3.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @ilyalesokhin-starkware)

[TomerStarkware]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @ilyalesokhin-starkware)

---

crates/cairo-lang-semantic/src/items/imp.rs line 1900 at r2 (raw file):

Previously, TomerStarkware wrote…

do we need it to be stable, its just the list of impl params

Done.

---

tests/bug_samples/issue7234.cairo line 9 at r2 (raw file):

Previously, TomerStarkware wrote…

the closure's sierra type had only one member (since captured types contained only one bytearray)

Done.

[ilyalesokhin-starkware]

tests/bug_samples/issue7234.cairo line 9 at r2 (raw file):

Previously, TomerStarkware wrote…

Done.

"the closure's sierra type had only one member (since captured types contained only one bytearray)"
why is that an issue?

what was the compiler crash?

[orizi]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @ilyalesokhin-starkware)

---

tests/bug_samples/issue7234.cairo line 9 at r2 (raw file):

Previously, ilyalesokhin-starkware wrote…

"the closure's sierra type had only one member (since captured types contained only one bytearray)"
why is that an issue?

what was the compiler crash?

the number of parameters for the construction of the closure was wrong. (as well as their order).
so depending on the specific configuration there was a failing zip_eq or a later function specialization in sierra gen error.

[ilyalesokhin-starkware]

Reviewed 2 of 3 files at r3, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @TomerStarkware)

---

crates/cairo-lang-semantic/src/items/imp.rs line 1900 at r3 (raw file):

                .captured_types
                .iter()
                .sorted_by_key(|ty| ty.as_intern_id())

can you add a todo to make this stable.

I'm worried that we would get an unstable compilation.

Code quote:

       .sorted_by_key(|ty| ty.as_intern_id())

[orizi]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @ilyalesokhin-starkware)

---

crates/cairo-lang-semantic/src/items/imp.rs line 1900 at r3 (raw file):

Previously, ilyalesokhin-starkware wrote…

can you add a todo to make this stable.

I'm worried that we would get an unstable compilation.

no need for to do - gave simple code suggestion.

---

crates/cairo-lang-semantic/src/items/imp.rs line 1900 at r3 (raw file):

                .captured_types
                .iter()
                .sorted_by_key(|ty| ty.as_intern_id())

a bit pricier - but always stable.

Suggestion:

                .unique()

[ilyalesokhin-starkware]

crates/cairo-lang-semantic/src/items/imp.rs line 1900 at r3 (raw file):

Previously, orizi wrote…

a bit pricier - but always stable.

How can it always be stable? where is the order coming from?

[ilyalesokhin-starkware]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @orizi)

---

tests/bug_samples/issue7234.cairo line 9 at r2 (raw file):

Previously, orizi wrote…

the number of parameters for the construction of the closure was wrong. (as well as their order).
so depending on the specific configuration there was a failing zip_eq or a later function specialization in sierra gen error.

I was unable to reproduce the crash after reverting the other canges

[orizi]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @ilyalesokhin-starkware)

---

crates/cairo-lang-semantic/src/items/imp.rs line 1900 at r3 (raw file):

Previously, ilyalesokhin-starkware wrote…

How can it always be stable? where is the order coming from?

from the order of the capture - this is provided by the usage algorithm - we should be using an OrderedHashMap

[orizi]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @ilyalesokhin-starkware)

---

tests/bug_samples/issue7234.cairo line 9 at r2 (raw file):

Previously, ilyalesokhin-starkware wrote…

I was unable to reproduce the crash after reverting the other canges

what are "the other changes"?
the crash was fixed by removing the dedup.

the readded dedup was required to make the implementation of Destruct workable (otherwise we would have had multiple Destruct implementations - therefore the additional test)

[TomerStarkware]

Reviewable status: 5 of 6 files reviewed, 3 unresolved discussions (waiting on @ilyalesokhin-starkware and @orizi)

---

crates/cairo-lang-semantic/src/items/imp.rs line 1900 at r3 (raw file):

Previously, orizi wrote…

from the order of the capture - this is provided by the usage algorithm - we should be using an OrderedHashMap

Done.

[ilyalesokhin-starkware]

tests/bug_samples/issue7234.cairo line 9 at r2 (raw file):

Previously, orizi wrote…

what are "the other changes"?
the crash was fixed by removing the dedup.

the readded dedup was required to make the implementation of Destruct workable (otherwise we would have had multiple Destruct implementations - therefore the additional test)

"the other changes" == changes to .rs files

[ilyalesokhin-starkware]

Reviewable status: 5 of 6 files reviewed, 3 unresolved discussions (waiting on @orizi)

---

crates/cairo-lang-semantic/src/items/imp.rs line 1900 at r3 (raw file):

Previously, TomerStarkware wrote…

Done.

Ok, makes sense.

[orizi]

Reviewable status: 5 of 6 files reviewed, 4 unresolved discussions (waiting on @ilyalesokhin-starkware)

---

tests/bug_samples/issue7234.cairo line 9 at r2 (raw file):

Previously, ilyalesokhin-starkware wrote…

"the other changes" == changes to .rs files

indeed - was missing #[test] as sierra-gen must be called for the crash.

good catch.

---

tests/bug_samples/issue7234.cairo line 9 at r4 (raw file):

}

fn main() {

Suggestion:

#[test]
fn main() {

[orizi]

Reviewed all commit messages.
Reviewable status: 5 of 6 files reviewed, 3 unresolved discussions (waiting on @ilyalesokhin-starkware and @TomerStarkware)

[orizi]

Reviewed 1 of 1 files at r4.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @ilyalesokhin-starkware and @TomerStarkware)

[orizi]

Reviewed 2 of 2 files at r5, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @ilyalesokhin-starkware)

[TomerStarkware]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @ilyalesokhin-starkware)

---

crates/cairo-lang-semantic/src/items/imp.rs line 1900 at r3 (raw file):

Previously, ilyalesokhin-starkware wrote…

Ok, makes sense.

Done.

---

crates/cairo-lang-semantic/src/items/imp.rs line 1900 at r3 (raw file):

Previously, orizi wrote…

no need for to do - gave simple code suggestion.

Done.

---

tests/bug_samples/issue7234.cairo line 9 at r2 (raw file):

Previously, orizi wrote…

indeed - was missing #[test] as sierra-gen must be called for the crash.

good catch.

Done.

[ilyalesokhin-starkware]

Reviewed 1 of 2 files at r5.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @TomerStarkware)

[ilyalesokhin-starkware]

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @TomerStarkware)

---

tests/bug_samples/issue7234.cairo line 9 at r2 (raw file):

Previously, TomerStarkware wrote…

Done.

does not pass the ci now