fix(frontend)!: Restrict capturing mutable variable in lambdas

[vezenovm]

Description

Problem*

Resolves #4795

Summary*

We now restrict capturing mutable variables that are not mutable references in lambdas.
This is now banned:

fn main() {
    let mut x = 3;

    let f = || {
        x += 2;
    };

    f();
    assert(x == 5);
}

A mutable reference must be used for x instead.

We can still capture a mutable variable if it is only ever used immutably.
e.g. this code is still allowed:

fn main() {
        let mut x = 3;
        let f = || x;
        let _x2 = f();
        assert(x == 3);
   }

Additional Context

Documentation*

Check one:

• No documentation needed.
• Documentation included in this PR.
• [For Experimental Features] Documentation to be submitted in a separate PR.

PR Checklist*

• I have tested the changes locally.
• I have formatted the changes with Prettier and/or cargo fmt on default settings.

[jfecher]

Is it necessary to issue an error here? The original intent for lambda captures was not to prevent capturing a mutable variable completely, but rather that any mutable variables should be captured by value and thus be immutable within the lambda. E.g. the equivalent of let mut x = Foo(); bar(x) where fn bar(y: Foo) { ... }

[vezenovm]

It is not. This is what we currently allow. Taking the example from #4795 (comment):

fn main() {
    let mut x = 3;

    let f = || {
        x += 2;
    };

    f();
    assert(x == 5);
}

This example will fail on master. assert(x == 3); passes.

[vezenovm]

I thought we desired to have an error, but I could just have this be a warning.

Otherwise, lambdas are working as expected and we just need to provide greater documentation/education.

[vezenovm]

If we wanted to capture a mutable variable immutably it would require devs to reassign their variable before capturing in a lambda. This is a bit cumbersome in my opinion.

[jfecher]

I'm aware of the original issue, but maybe I was misunderstanding this check. I was suggesting that the original intent was to allow captures of x still but capture it immutably by value instead. So if you just did a print(x) in f that'd be allowed even if x was declared as let mut x = 3;

Can you add a test to ensure

fn main() {
        let mut x = 3;
        let f = || x;
        let _x2 = f();
        assert(x == 3);
    }

(or similar) is valid?

[vezenovm]

Ah I see. I misunderstood as I thought from the original issue you desired this restrictive of a check. I'll update and add this test.

[vezenovm]

Looks like the dispatch aztec-nr macro is going to need updating.

Fix here: AztecProtocol/aztec-packages#12311

[asterite]

I didn't try it but I thought of a way to avoid collecting the capture IDs into a Vec:

Suggested change

	let capture_ids =
	lambda_context.captures.iter().map(|var| var.ident.id).collect::<Vec<_>>();
	let (id, name, location) = self.get_lvalue_error_info(&lvalue);
	let typ = self.interner.definition_type(id);
	for capture_id in capture_ids {
	if capture_id == id && !typ.is_mutable_ref() {
	self.push_err(TypeCheckError::MutableCaptureWithoutRef {
	name: name.clone(),
	location,
	});
	}
	}
	let (id, name, location) = self.get_lvalue_error_info(&lvalue);
	let typ = self.interner.definition_type(id);
	if !typ.is_mutable_ref() && lambda_context.captures.iter().any(|var| var.ident.id == id)
	{
	let name = name.clone();
	self.push_err(TypeCheckError::MutableCaptureWithoutRef { name, location });
	}

[asterite]

I think this doesn't completely solve the issue, though it solves one case of it.

For example I think this code should also be banned:

fn main() {
    let mut x = 3;

    let f = || mutate(&mut x);

    f();
    assert(x == 3);
}

fn mutate(x: &mut Field) {
    *x = 5;
}

And this one:

struct Foo {
    value: Field,
}

impl Foo {
    fn mutate(&mut self) {
        self.value = 2;
    }
}

fn main() {
    let mut foo = Foo { value: 1 };

    let f = || foo.mutate();

    f();

    println(foo);
}

(no need to fix those in this PR, though!)

[jfecher]

I was thinking instead of checking on assignment or mutable-ref creation or method calls we could go to the source of where a lambda capture is introduced into a lambda's scope and change that variable to be immutable. Is that possible?

[jfecher]

Ok, I looked over the lambda capture code some more and we don't create any new definition ids.

So instead of changing the definition (which we can't since it is not a copy and would change the original to be immutable), we should have a single function to verify whether something is mutable and have the check only in that function. I looked across the codebase and found that we actually have already accidentally duplicated this code. &mut x calls check_can_mutate while obj.mutating_method() calls verify_mutable_reference. Assignment uses elaborate_lvalue` which returns a mutable boolean, although that method operates on an LValue rather than an ExprId so it is a bit different.

We should unify these methods into one (probably keeping elaborate_lvalue) and add the mutable capture check on the Ident case for this shared method. elaborate_lvalue should also call a new helper method to check the capture mutability on its Ident case as well.