Can't use same for loop var

[katat]

given:

fn main(pub public_input: Field, private_input: [Field; 3]) {
    let mut sum = 0;

    for ii in 0..3 {
        sum = sum + private_input[ii];
    
    }
    for ii in 0..3 {
        sum = sum + private_input[ii];
    }

    assert_eq(sum, public_input);
}

it throws error at the second for loop:

   ╭─[for_loop:7:1]
 7 │     }
 8 │     for ii in 0..3 {
   ·         ─┬
   ·          ╰── here
 9 │         sum = sum + private_input[ii];
   ╰────
  help: the variable `ii` is declared twice

The cause is the TypedFnEnv use var name as key to store vars, but it doesn't remove the vars that are out of scope.

[Prabhat1308]

In mod.rs there is same instance of DuplicateDefinition being thrown out for constants . For variables , a check on the type_info.disabled does the job. Is that constant type-check being done here ? because the error is still being thrown after fixing for vars case.

[katat]

Thanks for looking into this.

The type_info.disabled is meant to forbid variable shadowing. This works fine for a function without a for-loop block. When there is a for-loop block, the variables remain in the map, even they are out of scope.

So I think one way to fix this is to remove the variables declared in the for-loop block after it finished the checks.

[mimoo]

I'm wondering why we didn't do that in the first place though, maybe the intention was to not allow the same variable names even across scopes so that when you read code further away in the logic and you see a variable name, then you know it always has to be the same variable you saw further above? Which sounds like a nice thing to have no? For example:

{
  let a = 3;
  /* do something with a that takes 1k LOC */
}

{
  let a = 5; // this would be forbidden
  /* do something with a */ // <-- as this might be a large block of logic and we might not understand that `a` was reset at this point
}

One could write that for example:

fn main(pub public_input: Field, private_input: [Field; 3]) {
    let mut sum = 0;

    for i1 in 0..3 {
        sum = sum + private_input[i1];
    
    }
    for i2 in 0..3 {
        sum = sum + private_input[i2];
    }

    assert_eq(sum, public_input);
}

it does seem a bit arbitrary to forbid developers to write the original example though...

[Prabhat1308]

It does seem that way but I would argue that getting a new variable name everytime might get messy and would affect code readability. Lets take for example this piece of code.

let shop_items: Vec<&str> = Vec::new();

for item in shop_items.iter() {
        // do something with the item
    }
    
 /// now if i want to use these in an array again , i would have to rename it.

    for ITEM in shop_items.iter() {
        // do something with the ITEM
    }

This issue might irritate the developers to choose some names close to what they want without losing the context for the variables breaking casing convections and all . Although this issue can be tackled by breaking the code in separate files , This might affect the dev experience . Most of the languages support it for the same reason.

[mimoo]

yeah I can agree with that, so now the question is, should we remove this disabled field and just remove a var when it goes out of scope? Or was this used with any other purpose? (in which case it would be bad)

[Prabhat1308]

I think there is no need for disabled flag considering we can always check for shadowing even with the hashmap . And removing the variable when we are out of a scope fixes the error of this issue. From the code and docs shadowing is the only intention i could see of this variable .

[mimoo]

let's do it then!

[Prabhat1308]

Fixed the issue in my PR . can be checked using

cargo run -- test --path examples/example.no --private-inputs '{"private_input": ["1","2","2"]}' --public-inputs '{"public_input": "10"}' --debug

Please review !

[mimoo]

thanks again @Prabhat1308 !