chore: introduce for_loop

[asterite]

Description

Problem

For #7119 (comment)

Summary

Additional Context

I'm not convinced about the name for_loop so open to suggestions.

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.

[github-actions]

Changes to Brillig bytecode sizes

Generated at commit: 2fa824df83569c441ec86e1f9c330f61a667fefb, compared to commit: 966d8a607436f371079f2e7fa3774046fb9785a5

🧾 Summary (10% most significant diffs)

Program	Brillig opcodes (+/-)	%
no_predicates_numeric_generic_poseidon_inliner_max	-490 ✅	-67.31%
fold_numeric_generic_poseidon_inliner_max	-490 ✅	-67.31%
poseidon2_inliner_max	-273 ✅	-84.78%
bench_2_to_17_inliner_max	-296 ✅	-93.38%
fold_2_to_17_inliner_max	-538 ✅	-96.24%

---

Full diff report 👇

Program	Brillig opcodes (+/-)	%
poseidon2_inliner_min	504 (+130)	+34.76%
bench_2_to_17_inliner_min	495 (+127)	+34.51%
fold_2_to_17_inliner_min	527 (+127)	+31.75%
no_predicates_numeric_generic_poseidon_inliner_min	823 (+187)	+29.40%
fold_numeric_generic_poseidon_inliner_min	845 (+187)	+28.42%
keccak256_inliner_min	1,668 (+153)	+10.10%
regression_5252_inliner_min	3,688 (+127)	+3.57%
hashmap_inliner_min	12,905 (+311)	+2.47%
uhashmap_inliner_min	9,872 (+67)	+0.68%
keccak256_inliner_zero	1,695 (+7)	+0.41%
uhashmap_inliner_zero	7,531 (+29)	+0.39%
hashmap_inliner_zero	7,972 (+25)	+0.31%
keccak256_inliner_max	2,096 (+6)	+0.29%
regression_5252_inliner_zero	3,318 (-39)	-1.16%
regression_5252_inliner_max	4,190 (-245)	-5.52%
hashmap_inliner_max	18,471 (-1,148)	-5.85%
fold_numeric_generic_poseidon_inliner_zero	502 (-71)	-12.39%
no_predicates_numeric_generic_poseidon_inliner_zero	502 (-71)	-12.39%
poseidon2_inliner_zero	250 (-45)	-15.25%
uhashmap_inliner_max	9,968 (-2,177)	-17.93%
fold_2_to_17_inliner_zero	324 (-75)	-18.80%
bench_2_to_17_inliner_zero	222 (-67)	-23.18%
no_predicates_numeric_generic_poseidon_inliner_max	238 (-490)	-67.31%
fold_numeric_generic_poseidon_inliner_max	238 (-490)	-67.31%
poseidon2_inliner_max	49 (-273)	-84.78%
bench_2_to_17_inliner_max	21 (-296)	-93.38%
fold_2_to_17_inliner_max	21 (-538)	-96.24%

[asterite]

Hm, it seems modifying something via a closure doesn't work:

fn main() {
    let mut array = [1, 2, 3];
    callback(|i| { array[i] = 0; });
    println(array);
}

fn callback<Env>(f: fn[Env](u32) -> ()) {
    f(1);
}

The above prints [1, 2, 3]. I don't know if this is expected or it's a bug.

For now I'll close this PR. Maybe this could be done with a macro instead but it would make the code much harder to read.

[jfecher]

Hm, it seems modifying something via a closure doesn't work:

It's expected. You need to explicitly capture the reference instead: let array = &mut [1, 2, 3];

[jfecher]

@asterite to expand on the above, we have a decision to either capture by ref or by copy by default. If we copy by ref by default, then we can't actually just copy the T into the closure environment, we need an implicit &mut T since this crosses a function boundary. Moreover, the code for how to actually pass a copy if by-ref is the default becomes less obvious:

let mut x = ...;
let y = x;

let f = || y;

Someone may see y and try to replace it with x where the reverse if copy is the default is more obvious:

let mut x = ...;
let y = &mut x; // clearly needed so that f has a mutable reference to x

let f = || y;

It used to be that within the closure we'd mark the captured variable as immutable by default so we'd issue an error if the user tried to do:

let mut x = ...;
|| {
    x = 3; // error
}

But I think when closure conversion was being worked on quite a while back this was causing some issue and all variables in closure's env were marked as mutable by default instead... If anything I think this is the issue we should fix - to make these immutable so that we can issue a proper error again.

This issue appears to be the same one from here: #4795 which @michaeljklein has a draft PR open for which maybe we can revisit.

[nventuro]

But I think when closure conversion was being worked on quite a while back this was causing some issue and all variables in closure's env were marked as mutable by default instead... If anything I think this is the issue we should fix - to make these immutable so that we can issue a proper error again.

Please. I'd never in a million years expect for this to be the default, and had no idea either syntax for mutable references even existed, since as far as I knew Noir didn't even expose references as a concept at all. And without an error I'd also not realize I was accidentally requiring captured variables to be mutable.

[jfecher]

@nventuro it's definitely not meant to work the way it currently does (silently allowing code which only looks like it does the right thing). Once #4795 is fixed and we can issue an error again it should hopefully be more clear.

[TomAFrench]

@asterite do you want to reopen this and use explicit mutable references?

[asterite]

I started doing this earlier today but I wasn't sure, it made the code looks a bit harder to read. I'll try it anyway and see how it looks and if we get any benefit to it.

[asterite]

Reopening...

[asterite]

It seems some external projects fail. The problem is, because we don't get an error if we incorrectly mutate something in a closure it's very hard to debug this, and that's why I'm hesitant about including it now. We could maybe try it again after we report an error in this case.

[nventuro]

It seems some external projects fail. The problem is, because we don't get an error if we incorrectly mutate something in a closure it's very hard to debug this, and that's why I'm hesitant about including it now. We could maybe try it again after we report an error in this case.

fwiw the stdlib already has closures (e.g. BoundedVec::map), so that ship has sailed. I guess you could make the argument that people are more likely to want to mutate stuff in for_each than in map.