Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

allow generic calls within forloop #189

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

katat
Copy link
Collaborator

@katat katat commented Sep 20, 2024

This PR is to allow calling generic functions within forloops while preventing it from running into the case of unrolling.

For example, we want to allow generic function calls:

fn clone(const LEN: Field, val: Field) -> [Field; LEN] {
    let mut arr = [0; LEN];
    for idx in 0..LEN {
        arr[idx] = val;
    }
    return arr;
}

for idx in 0..2 {
    let cloned = clone(2, idx);
    arr3[idx] = cloned;
}

Because the argument LEN is not relating to the variables inside the for loop scope, it is allowed to call the generic function.

But if it is the following case, it won't be allowed:

for idx in 0..2 {
    let cloned = clone(idx, idx);
    arr3[idx] = cloned;
}

The checking logic relies on the states of TypedFnEnv. When it processes the Expr::Variable case during type check, TypedFnEnv will throw error if the condition isn't allowed to load the variable for a generic function call:

if fn_sig.require_monomorphization() && typed_fn_env.is_in_forloop() {
    for (observed_arg, expected_arg) in args.iter().zip(fn_sig.arguments.iter()) {
        // check if the arg involves generic vars
        if !expected_arg.extract_generic_names().is_empty() {
            let mut forbidden_env = typed_fn_env.clone();
            forbidden_env.forbid_forloop_scope();

            // rewalk the observed arg expression
            // it should throw an error if the arg contains generic vars relating to the variables in the forloop scope
            self.compute_type(observed_arg, &mut forbidden_env)?;

            // release the forbidden flag
            forbidden_env.allow_forloop_scope();
        }
    }
}

@katat katat requested a review from mimoo September 20, 2024 10:42
@katat katat mentioned this pull request Sep 27, 2024
20 tasks
Copy link
Contributor

@gio54321 gio54321 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice PR!
Left a comment on what I think is a small bug.
Another thing: shouldn't this behavior be implemented also for method calls?


// rewalk the observed arg expression
// it should throw an error if the arg contains generic vars relating to the variables in the forloop scope
self.compute_type(observed_arg, &mut forbidden_env)?;
Copy link
Contributor

@gio54321 gio54321 Oct 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there is a small bug in the handling of the for loop variables. Take this code (just modifying one of the examples)

fn clone(const LEN: Field, val: Field) -> [Field; LEN] {
    let mut arr = [0; LEN];
    for idx in 0..LEN {
        arr[idx] = val;
    }
    return arr;
}

fn main(pub xx: Field) {
    let mut arr3 = [[0; 2]; 2];
    for idx in 0..2 {
        let cloned = clone(2, xx + idx);
        arr3[idx] = cloned;
    }

    let fst = arr3[0];
    let snd = arr3[1];
    assert_eq(fst[0], 0);
    assert_eq(fst[1], 0);
    assert_eq(snd[0], 1);
    assert_eq(snd[1], 1);
}

I would expect that adding a nested loop of one iteration inside the one defined do not change any of the emitted constraints, and indeed it works.

    for idx in 0..2 {
        for unused in 0..1 {
            let cloned = clone(2, xx + idx);
            arr3[idx] = cloned;
        }
    }

However, if the LEN argument is replaced with the outer loop variable

    for idx in 0..2 {
        for unused in 0..1 {
            let cloned = clone(idx, xx + idx);
            arr3[idx] = cloned;
        }
    }

then the nice error about the call of generic function is not returned anymore and instead there is this panic

thread 'main' panicked at src/parser/types.rs:800:14:
generic value not assigned

I'm not really sure what is going on, but a bit of debugging shows that this compute_type call at this line of code does not return an error in the latter case (maybe it should?).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great spot. I have't thought about this case. Will check it out. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants