fixed loop capture of snapshoted variables

[TomerStarkware]

This change is 

[orizi]

Reviewed 20 of 20 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ilyalesokhin-starkware and @TomerStarkware)

---

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

        for var in &usage.introductions {
            write!(usages_str, "{:?}, ", var.debug(&expr_formatter)).unwrap();
        }

Suggestion:

        write!(usages_str, "  Usage:").unwrap();
        for (_, expr) in usage.usage.iter() {
            write!(usages_str, " {:?},", expr.debug(&expr_formatter)).unwrap();
        }
        writeln!(usages_str).unwrap();
        write!(usages_str, "  Changes:").unwrap();
        for (_, expr) in usage.changes.iter() {
            write!(usages_str, " {:?},", expr.debug(&expr_formatter)).unwrap();
        }
        writeln!(usages_str).unwrap();
        write!(usages_str, "  Snapshot_Usage:").unwrap();
        for (_, expr) in usage.snap_usage.iter() {
            write!(usages_str, " {:?},", expr.debug(&expr_formatter)).unwrap();
        }
        writeln!(usages_str).unwrap();
        write!(usages_str, "  Introductions:").unwrap();
        for var in &usage.introductions {
            write!(usages_str, " {:?},", var.debug(&expr_formatter)).unwrap();
        }

[TomerStarkware]

Reviewable status: 18 of 20 files reviewed, 1 unresolved discussion (waiting on @ilyalesokhin-starkware and @orizi)

---

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

        for var in &usage.introductions {
            write!(usages_str, "{:?}, ", var.debug(&expr_formatter)).unwrap();
        }

Done.

[orizi]

Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @ilyalesokhin-starkware)

[orizi]

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

[orizi]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ilyalesokhin-starkware and @TomerStarkware)

a discussion (no related file):
Add a test for point 4 at #2816.

---

[orizi]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ilyalesokhin-starkware and @TomerStarkware)

a discussion (no related file):

Previously, orizi wrote…

Add a test for point 4 at #2816.

#2816 (comment)

---

[TomerStarkware]

Reviewable status: 20 of 22 files reviewed, 1 unresolved discussion (waiting on @ilyalesokhin-starkware and @orizi)

a discussion (no related file):

Previously, orizi wrote…

#2816 (comment)

Done.

---

[ilyalesokhin-starkware]

crates/cairo-lang-lowering/src/lower/context.rs line 306 at r3 (raw file):

                type_id: exprs
                    .first()
                    .map_or(TypeLongId::Missing(skip_diagnostic()).intern(ctx.db), |expr| {

why skip_diagnostic?

Code quote:

skip_diagnostic())

[ilyalesokhin-starkware]

crates/cairo-lang-lowering/src/lower/mod.rs line 1121 at r3 (raw file):

    let lowered = lower_expr(ctx, builder, expr.inner)?;

    // If the inner expression is already a snapshot of the same type, we can just return it.

why?
explain that this happens because in the loop we change the type of the member path variable.

Code quote:

 // If the inner expression is already a snapshot of the same type, we can just return it.

[orizi]

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

---

tests/bug_samples/issue2816.cairo line 2 at r3 (raw file):

fn contains<T,impl TPartialEq: PartialEq<T>,+Drop<T>,+Copy<T>>(ref self: Array<T>, item: T) -> bool {

do note that cairo fmt still fails.

[orizi]

Reviewable status: 20 of 22 files reviewed, 4 unresolved discussions (waiting on @ilyalesokhin-starkware and @TomerStarkware)

a discussion (no related file):

Previously, TomerStarkware wrote…

Done.

as discussed f2f - some parts are still missing.

---

[ilyalesokhin-starkware]

crates/cairo-lang-lowering/src/lower/mod.rs line 1124 at r3 (raw file):

    if expr.ty == lowered.ty(ctx) {
        return Ok(lowered);
    }

remove

Code quote:

    if expr.ty == lowered.ty(ctx) {
        return Ok(lowered);
    }

[ilyalesokhin-starkware]

crates/cairo-lang-lowering/src/lower/mod.rs line 1126 at r3 (raw file):

    }
    if let LoweredExpr::Member(member_path, _) = &lowered {
        if let Some(var) = builder.get_ref(ctx, member_path) {

add something like:

Suggestion:

  // Inside a closure, 'member\_path' might refer to a snapshot of the original variable.
  // If a snapshot is requested inside the closure, we should ignore it.
  if let Some(var) = builder.get_ref(ctx, member_path) {

[ilyalesokhin-starkware]

crates/cairo-lang-lowering/src/lower/test_data/loop line 1009 at r3 (raw file):

    let t = T { s: S {} };
    loop {
        t.s.f1oo();

use(@t.s)

Code quote:

 t.s.f1oo();

[ilyalesokhin-starkware]

crates/cairo-lang-lowering/src/lower/test_data/loop line 1008 at r3 (raw file):

fn foo() {
    let t = T { s: S {} };
    loop {

add a test where u use T inside the loop

Code quote:

 loop {

[orizi]

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

[TomerStarkware]

Reviewable status: 7 of 23 files reviewed, 8 unresolved discussions (waiting on @ilyalesokhin-starkware, @orizi, and @t)

a discussion (no related file):

Previously, orizi wrote…

as discussed f2f - some parts are still missing.

Done.

---

---

crates/cairo-lang-lowering/src/lower/context.rs line 306 at r3 (raw file):

Previously, ilyalesokhin-starkware wrote…

why skip_diagnostic?

Done.

---

crates/cairo-lang-lowering/src/lower/mod.rs line 1121 at r3 (raw file):

Previously, ilyalesokhin-starkware wrote…

why?
explain that this happens because in the loop we change the type of the member path variable.

changed

---

crates/cairo-lang-lowering/src/lower/mod.rs line 1124 at r3 (raw file):

Previously, ilyalesokhin-starkware wrote…

remove

Done.

---

crates/cairo-lang-lowering/src/lower/mod.rs line 1126 at r3 (raw file):

Previously, ilyalesokhin-starkware wrote…

add something like:

added a comment for other solution.

---

crates/cairo-lang-lowering/src/lower/test_data/loop line 1008 at r3 (raw file):

Previously, ilyalesokhin-starkware wrote…

add a test where u use T inside the loop

Done.

---

crates/cairo-lang-lowering/src/lower/test_data/loop line 1009 at r3 (raw file):

Previously, ilyalesokhin-starkware wrote…

use(@t.s)

Done.

---

tests/bug_samples/issue2816.cairo line 2 at r3 (raw file):

Previously, orizi wrote…

do note that cairo fmt still fails.

Done.

[orizi]

Reviewed 11 of 16 files at r4, all commit messages.
Reviewable status: 18 of 23 files reviewed, 9 unresolved discussions (waiting on @ilyalesokhin-starkware, @t, and @TomerStarkware)

a discussion (no related file):
Why were the contract improvements reverted? (What changed algorithmically)

---

---

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

    }

    pub fn update_snap_ref(&mut self, member_path: &ExprVarMemberPath, var: VariableId) {

Doc

---

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

        self.snapped_semantics.insert(member_path.into(), var);
    }
    pub fn get_snap_ref(

Doc

[TomerStarkware]

Reviewable status: 17 of 23 files reviewed, 9 unresolved discussions (waiting on @ilyalesokhin-starkware, @orizi, and @t)

a discussion (no related file):

Previously, orizi wrote…

Why were the contract improvements reverted? (What changed algorithmically)

which contract?

---

---

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

Previously, orizi wrote…

Doc

Done.

---

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

Previously, orizi wrote…

Doc

Done.

[orizi]

Reviewed 4 of 16 files at r4, 1 of 1 files at r5, all commit messages.
Reviewable status: 22 of 23 files reviewed, 9 unresolved discussions (waiting on @ilyalesokhin-starkware, @t, and @TomerStarkware)

a discussion (no related file):

Previously, TomerStarkware wrote…

which contract?

all the reverted ones. (all the erc20 based)

---

---

crates/cairo-lang-lowering/src/lower/mod.rs line 1478 at r5 (raw file):

                .and_then(
                    |var| {
                        if ctx.variables[var.var_id].ty == param.ty() { Some(var) } else { None }

Suggestion:

                        (ctx.variables[var.var_id].ty == param.ty()).then_some(var)

---

crates/cairo-lang-lowering/src/lower/mod.rs line 1486 at r5 (raw file):

                    } else {
                        None
                    }

Suggestion:

                    let var = builder.get_snap_ref(ctx, &param)?;
                    (ctx.variables[var.var_id].ty == param.ty()).then_some(var)

[orizi]

Reviewed 1 of 16 files at r4, 1 of 1 files at r6, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @ilyalesokhin-starkware, @t, and @TomerStarkware)

[TomerStarkware]

Reviewable status: 22 of 23 files reviewed, 7 unresolved discussions (waiting on @ilyalesokhin-starkware and @orizi)

---

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

Previously, ilyalesokhin-starkware wrote…

Where is this generic function defined? why is it generic?

its get method of OrderedHashMap

---

crates/cairo-lang-lowering/src/lower/mod.rs line 1150 at r11 (raw file):

Previously, ilyalesokhin-starkware wrote…

consider moving inside the match and removing the early returns.

but the other arms of the match should also reach this part if get_snap_ref fails

---

crates/cairo-lang-lowering/src/lower/test_data/loop line 1350 at r11 (raw file):

Previously, orizi wrote…

simplify (as you don't actually assert for this test)

Done.

[orizi]

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

[ilyalesokhin-starkware]

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

Previously, TomerStarkware wrote…

its get method of OrderedHashMap

Why do you need to generic param: isn't the type of self.snapped_semantics known here?

[ilyalesokhin-starkware]

crates/cairo-lang-lowering/src/lower/mod.rs line 1481 at r13 (raw file):

                })
                .ok_or_else(|| {
                    LoweringFlowError::Failed(ctx.diagnostics.report(stable_ptr, MemberPathLoop))

when does this happen do we have a test for that?

Code quote:

LoweringFlowError::Failed(ctx.diagnostics.report(stable_ptr, MemberPathLoop))

[TomerStarkware]

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

---

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

Previously, ilyalesokhin-starkware wrote…

Why do you need to generic param: isn't the type of self.snapped_semantics known here?

yes but the get method receives a generic argument that only needs to be Equivalent so into alone does not solve it

---

crates/cairo-lang-lowering/src/lower/mod.rs line 1481 at r13 (raw file):

Previously, ilyalesokhin-starkware wrote…

when does this happen do we have a test for that?

We do not have a test, I think it can only happen if a compiler bug occurs

[ilyalesokhin-starkware]

Reviewed 1 of 4 files at r7, 1 of 1 files at r11, 1 of 1 files at r13, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @TomerStarkware)

---

crates/cairo-lang-lowering/src/lower/mod.rs line 1481 at r13 (raw file):

Previously, TomerStarkware wrote…

We do not have a test, I think it can only happen if a compiler bug occurs

Add a todo to remove this error.

[ilyalesokhin-starkware]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @TomerStarkware)

[ilyalesokhin-starkware]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @TomerStarkware)

[orizi]

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

---

crates/cairo-lang-lowering/src/lower/mod.rs line 1481 at r14 (raw file):

                })
                .ok_or_else(|| {
                    // TODO(TomerStaskware): remove this error.

if so - why does it exist?

[orizi]

Reviewed 1 of 1 files at r15, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @TomerStarkware)

[orizi]

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

---

-- commits line 5 at r15:
rebase

[TomerStarkware]

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

---

-- commits line 5 at r15:

Previously, orizi wrote…

rebase

Done.

---

crates/cairo-lang-lowering/src/lower/mod.rs line 1481 at r14 (raw file):

Previously, orizi wrote…

if so - why does it exist?

looks like legacy error

[orizi]

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

---

-- commits line 5 at r15:

Previously, TomerStarkware wrote…

Done.

i still see 2 commits.

---

crates/cairo-lang-lowering/src/lower/mod.rs line 1481 at r14 (raw file):

Previously, TomerStarkware wrote…

looks like legacy error

make the TODO more operative.

[TomerStarkware]

Reviewable status: 22 of 23 files reviewed, 2 unresolved discussions (waiting on @orizi)

---

crates/cairo-lang-lowering/src/lower/mod.rs line 1481 at r14 (raw file):

Previously, orizi wrote…

make the TODO more operative.

Done.

[orizi]

Reviewed 1 of 1 files at r16, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @TomerStarkware)

[TomerStarkware]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @orizi)

---

-- commits line 5 at r15:

Previously, orizi wrote…

i still see 2 commits.

Done.

[orizi]

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