For loops with incorrect number of bindings

[madmann91]

The following code should not be accepted by Impala :

extern "C" {
    fn plausible_number() -> i32;
    fn print_number(i32) -> ();
}

fn this_function(a: i32, b: i32, c: fn (i32, i32) -> ()) -> () {
    c(a + b, plausible_number());
}

extern fn lolz(k: i32) -> () {
    for i in this_function(0, k) { // Should not be accepted : 'c' takes 2 parameters in function 'broken'
       print_number(i)
    }
}

[madmann91]

The issue is the following : during parsing, a parameter called continue is added to the parameter list of the for loop closure. It then looks like :

|i, continue| {
    print_number(i)
}

Which then correctly typechecks with fn(i: i32, continue: i32) -> ().

I think we should remove this behaviour completely. We should be consistent with what is done in while loops, which maintain local declarations for break and continue (in for loops, only the break statement is a local declaration) :

class WhileExpr : public StmtLikeExpr {
    /* ... */
    AutoPtr<const LocalDecl> break_decl_;
    AutoPtr<const LocalDecl> continue_decl_;
    /*... */
};

I could take care of this, but I want your opinion first, in case there is something I am missing.

[leissa]

This continue added to the lambda is actually how it works in for loops. Note that you can also have fn(int), for instance, as continue type. I like this behaviour and I wouldn't change this. However, I still think there is a bug:
When we add the magic continue parameter to the closure, we should also add the return type noret. Then, the type would be fn(int, int) -> ! which is incompatible with the signature.

p.s.: In the case you don't know: fn(int) -> ! is syntactic sugar for fn(int). You can also use this in function definitions, for example:

|a: int| -> ! ret(a)

[leissa]

In other words:

for x in f(y) {
    B
}

should be syntactic sugar for:

f(x, |y, continue| -> ! { B })

and not:

f(x, |y, continue| { B })

[madmann91]

The two impala strings:

f(x, |y, continue| -> ! { B })

and

f(x, |y, continue| { B })

Generate the same AST. It is even possible to write:

fn f(x: i32, h: fn(), g: fn (i32, fn ()) -> ()) -> () {
    g(x, h)
}

fn test() -> () {
    f(42, return, |y, continue| -> ! { print(y) })
}

That's of course because the AST node |a, b, c, ...| { ... } can be typechecked as either a continuation, or a function. This ambiguity is not a problem in general, just in the case of for loops.

A fix for this issue -without changing the current way the continue and break statements are handled- could be to make sure that the number of bindings in the for loop is the same as the number of parameters in the function type. I could also do this, but that wouldn't be a clean solution in my opinion.

Maybe we should just add a flag to the AST telling the typechecker than the AST node |a, b, c, ...| { ... } has to be a continuation ? We could then have the correct behaviour for the ! syntax as well.

[leissa]

yes, you are right: this will generate the same AST - and I consider this a bug. So the real fix is to include sth to Fn to mark it as a continuation and then deal with it correctly in the parser and later phases. There is parse_return_type which returns for both fn(...) o and fn(...) o -> ! a nullptr. With o I mean the current position of the parser. This would be the first step to fix this thing.

Speaking of this, we now have the following cases (where Te is the type of expr):

1. |...| expr ---> fn(..., fn(Te)) or fn(...)
2. |...| -> ! expr ---> fn(...)
3. |...| -> T ---> fn(..., fn(T))
4. |...| -> (T1, T2, T3) ---> fn(..., fn(T1, T2, T3))

Case 4 will be gone when we fix #12.

For named functions we have:

1. fn(...) ---> fn(...)
2. fn(...) -> ! ---> fn(...) (same as 1.)
3. fn(...) -> T ---> fn(..., fn(T))
4. fn(...) -> (T1, T2, T3) ---> fn(..., fn(T1, T2, T3))

Again, case 4 will be gone when we fix #12.
Regarding case 1: I think making fn(...) mean fn(...) -> () would be a better default but we can discuss that in another issue...