fix: Fix sequencing of side-effects in lvalue

[jfecher]

Description

Problem*

Resolves #8344

Summary*

I removed statements from the index part of an lvalue to sequence the side-effects consistently. This is done while converting the Ast to the Hir so the change is seen by both the comptime interpreter and SSA.

Additional Context

This changes output of nargo expand so I added a quick check to skip it if the expression trivially contains no side-effects (is already a variable or literal) to minimize changes but there are still several.

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]

Looks like

tests::nargo_expand_execution_success::test_aes128_encrypt
tests::nargo_expand_execution_success::test_encrypted_log_regression
tests::nargo_expand_execution_success::test_nested_array_dynamic

are failing - I wasn't aware we tested expanded code to ensure it runs but it's changing now because I'm creating a new variable i for each index de-sugared in this way. Internally there's no confusion since they have different DefinitionIds but nargo expand drops the $id suffix so it results in code that uses i, i, i where each i is meant to be different as the same i, leading to an assertion failure.

[asterite]

We could ignore those tests for now. In the long run, I'm not sure how nargo expand can work with these introduced variables.

[asterite]

Maybe the compiler could have a counter for temporary variables, then use "__var{index}" for these. During compilation it wouldn't change anything, but at least for nargo expand it would mean there's less chance of collisions.

[jfecher]

Maybe the compiler could have a counter for temporary variables, then use "__var{index}" for these. During compilation it wouldn't change anything, but at least for nargo expand it would mean there's less chance of collisions.

If we just want to reduce the chance of collisions we could add _{definition_id} as a suffix since that is already effectively a counter. I'll add that for this PR but there is still a chance of collision which is unsatisfactory.

[github-actions]

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark 'Execution Time'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.

Benchmark suite	Current: 051d626	Previous: a686856	Ratio
rollup-merge	0.004 s	0.003 s	1.33

This comment was automatically generated by workflow using github-action-benchmark.

CC: @TomAFrench