prevent capturing mutable variables and allow accessing copiable vari…

[TomerStarkware]

…ables post closure usage

---

This change is 

[orizi]

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

---

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

                self.semantics.captured.insert(member.clone(), var_usage.var_id);
            }
        }

Suggestion:

            .collect();

        let var_usage = generators::StructConstruct { inputs, ty: expr.ty, location }
            .add(ctx, &mut self.statements);
        let closure_info = ClosureInfo { members, snapshot_types };

        for (var_usage, member) in zip!(inputs, usage.usage.keys()) {
            if ctx.variables[var_usage.var_id].copyable.is_ok() && !usage.changes.contains_key(member) {
                self.semantics.copiable_captured.insert(member.clone(), var_usage.var_id);
            } else {
                self.semantics.captured.insert(member.clone(), var_usage.var_id);
            }
        }

---

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

    fn var(&self, var: VariableId) -> &Variable {
        &self.ctx.variables[var]
    }

this isn't used.

Code quote:

    fn var(&self, var: VariableId) -> &Variable {
        &self.ctx.variables[var]
    }

---

crates/cairo-lang-semantic/src/diagnostic.rs line 858 at r1 (raw file):

            }
            SemanticDiagnosticKind::MutableCapturedVariable => {
                "Mutable variables cannot be captured by closures".into()

Suggestion:

                "Mutable variables capture by closure is not supported".into()

[ilyalesokhin-starkware]

Reviewable status: 3 of 7 files reviewed, 4 unresolved discussions (waiting on @orizi and @TomerStarkware)

---

crates/cairo-lang-semantic/src/expr/compute.rs line 1648 at r1 (raw file):

    // TODO(TomerStarkware): Add support for capturing mutable variables when then we have borrow.
    for (captured_var, expr) in usage.usage.iter().chain(usage.snap_usage.iter()) {

what about usage. changes? do we support that?

Code quote:

for (captured_var, expr) in usage.usage.iter().chain(usage.snap_usage.iter()) {

[ilyalesokhin-starkware]

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

            }
        }
    }

Shouldn't we skip the update if it is a copiable capture?

Code quote:

     self.update(ctx, path, new_var).unwrap();

[TomerStarkware]

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

---

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

                self.semantics.captured.insert(member.clone(), var_usage.var_id);
            }
        }

Done.

---

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

Previously, orizi wrote…

this isn't used.

Done.

---

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

Previously, ilyalesokhin-starkware wrote…

Shouldn't we skip the update if it is a copiable capture?

yes

---

crates/cairo-lang-semantic/src/diagnostic.rs line 858 at r1 (raw file):

            }
            SemanticDiagnosticKind::MutableCapturedVariable => {
                "Mutable variables cannot be captured by closures".into()

Done.

---

crates/cairo-lang-semantic/src/expr/compute.rs line 1648 at r1 (raw file):

Previously, ilyalesokhin-starkware wrote…

what about usage. changes? do we support that?

no it has the same problem

Code snippet:

let mut x = 0;
let c = || {x=x+1;};
x=1
c();

[orizi]

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

[orizi]

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

[ilyalesokhin-starkware]

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

    ) -> Option<()> {
        if self.copiable_captured.get(path).is_some() {
            // Note, this can only right now if we take a snapshot of the variable (as the snapshot

fix comment.

Code quote:

this can only right now if we take a snapshot

[ilyalesokhin-starkware]

crates/cairo-lang-semantic/src/diagnostic.rs line 858 at r3 (raw file):

            }
            SemanticDiagnosticKind::MutableCapturedVariable => {
                "Mutable variables capture by closure is not supported".into()

Suggestion:

Capture of mutable variables in a closure is not supported

[ilyalesokhin-starkware]

crates/cairo-lang-semantic/src/expr/compute.rs line 1648 at r1 (raw file):

Previously, TomerStarkware wrote…

no it has the same problem

so you should go over usage.chages as well, right?

[ilyalesokhin-starkware]

Reviewed 2 of 7 files at r1, 1 of 5 files at r3.
Reviewable status: 5 of 7 files reviewed, 3 unresolved discussions (waiting on @orizi and @TomerStarkware)

[TomerStarkware]

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

---

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

Previously, ilyalesokhin-starkware wrote…

fix comment.

Done.

---

crates/cairo-lang-semantic/src/diagnostic.rs line 858 at r3 (raw file):

            }
            SemanticDiagnosticKind::MutableCapturedVariable => {
                "Mutable variables capture by closure is not supported".into()

Done.

---

crates/cairo-lang-semantic/src/expr/compute.rs line 1648 at r1 (raw file):

Previously, ilyalesokhin-starkware wrote…

so you should go over usage.chages as well, right?

isn't changes contained in usage?

[ilyalesokhin-starkware]

Reviewable status: 2 of 7 files reviewed, 3 unresolved discussions (waiting on @orizi and @TomerStarkware)

---

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

Previously, TomerStarkware wrote…

Done.

I don't see it

[ilyalesokhin-starkware]

crates/cairo-lang-semantic/src/expr/compute.rs line 1648 at r1 (raw file):

Previously, TomerStarkware wrote…

isn't changes contained in usage?

no

[TomerStarkware]

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

---

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

Previously, ilyalesokhin-starkware wrote…

I don't see it

Done.

---

crates/cairo-lang-semantic/src/expr/compute.rs line 1648 at r1 (raw file):

Previously, ilyalesokhin-starkware wrote…

no

Done.

[ilyalesokhin-starkware]

Reviewed 4 of 5 files at r4, 2 of 2 files at r5.
Reviewable status: all files reviewed (commit messages unreviewed), all discussions resolved (waiting on @TomerStarkware)

[orizi]

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

[orizi]

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

---

crates/cairo-lang-semantic/src/expr/compute.rs line 1667 at r6 (raw file):

            ctx.diagnostics.report(expr.stable_ptr(), MutableCapturedVariable);
        }
    }

Suggestion:

    let mut reported = UnorderedHashSet::<_>::default();
    // TODO(TomerStarkware): Add support for capturing mutable variables when then we have borrow.
    for (captured_var, expr) in chain!(&usage.usage, &usage.snap_usage, &usage.changes)
    {
        let Some(var) = ctx.semantic_defs.get(&captured_var.base_var()) else {
            // if the variable is not found in the semantic defs, it is closure parameter.
            continue;
        };

        if var.is_mut() && reported.insert(expr.stable_ptr()) {
            ctx.diagnostics.report(expr.stable_ptr(), MutableCapturedVariable);
        }
    }

[TomerStarkware]

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

---

crates/cairo-lang-semantic/src/expr/compute.rs line 1667 at r6 (raw file):

            ctx.diagnostics.report(expr.stable_ptr(), MutableCapturedVariable);
        }
    }

Done.

[orizi]

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

---

crates/cairo-lang-semantic/src/expr/compute.rs line 1658 at r7 (raw file):

    for (captured_var, expr) in
        chain!(usage.usage.iter(), usage.snap_usage.iter(), usage.changes.iter())
    {

this doesn't work?

Suggestion:

    for (captured_var, expr) in chain!(&usage.usage, &usage.snap_usage, &usage.changes) {

[TomerStarkware]

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

---

crates/cairo-lang-semantic/src/expr/compute.rs line 1658 at r7 (raw file):

Previously, orizi wrote…

this doesn't work?

Done.

[orizi]

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