powdr-labs / powdr

A modular stack for zkVMs, with a focus on productivity, security and performance.
Apache License 2.0
401 stars 80 forks source link

PIL analyzer doesn't like when a lambda parameter has the same name as a column in the outer scope #1859

Open lvella opened 3 weeks ago

lvella commented 3 weeks ago

Consider this Powdr-ASM code, in a function body:

        // Apply S-box
        let x3 = array::map(step_a, constr |x| { let x3; x3 = x * x * x; x3});
        let x7 = array::zip(x3, step_a, constr |x3, x| { let x7; x7 = x3 * x3 * x; x7 });

It fails with:

thread 'poseidon2_bb_test' panicked at pil-analyzer/src/expression_processor.rs:257:13:
Variable already defined: x3
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

But if I change x3 to x_3 in the second lambda:

        // Apply S-box
        let x3 = array::map(step_a, constr |x| { let x3; x3 = x * x * x; x3});
        let x7 = array::zip(x3, step_a, constr |x_3, x| { let x7; x7 = x_3 * x_3 * x; x7 });

the error is avoided.

chriseth commented 3 weeks ago

We could consider allowing point 5 in https://github.com/powdr-labs/powdr/issues/1227 again. Any opinions?

lvella commented 3 weeks ago

My opinion is that our language is too similar to Rust, and too heavily reliant on lambda functions to not allow reuse of identifiers across different scopes.

lvella commented 3 weeks ago

In terms of issue #1227, I think 4, 5 and 6 should be allowed, and wouldn't mind 3 either.

chriseth commented 3 weeks ago

Let's allow 5!