Add support for iterators #169

gio54321 · 2024-08-27T10:06:20Z

This PR aims at addressing issue #132 by introducing the possibility to iterate over arrays.
This makes it possible to write a circuit like this

fn zip(a1: [Field; 3], a2: [Field; 3]) -> [[Field; 2]; 3] {
    let mut result = [[a1[0], a1[0]], [a1[0], a1[0]], [a1[0], a1[0]]];
    for index in 0..3 {
        result[index] = [a1[index], a2[index]];
    }
    return result;
}

fn main(pub arr: [Field; 3]) -> Field {
    let expected = [1, 2, 3];
    for pair in zip(arr, expected) {
        assert_eq(pair[0], pair[1]);
    }

    return arr[0];
}

This also improves the iterate.no test as mentioned in the issue

fn House.windows(house: House) -> Field {
    let mut windows_count = 0;
    for room in house.rooms {
        windows_count = windows_count + room.windows();
    }

    return windows_count;
}

katat

Looks great!

Since this is also highly related to the const generic feature , so I think it'd better to be based on the generic PR branch #147.

katat · 2024-08-28T03:08:57Z

src/parser/types.rs

-                // for i in 0..5 { ... }
-                //             ^
-                let (end, end_span) = match tokens.bump(ctx) {
+                // here we decide if this for loop is over a range or is an iterator,


This is not always the case once the generic feature is introduced. For example:

fn range(const START: Field, const END: Field) { for ii in START..END { // START / END are const generic variables, which are not in numeric form in the syntax. ... } }

So I think the safer way to detect whether it is range or iterator can be based on the presence of .., which indicates an range.

katat · 2024-08-28T03:15:26Z

src/type_checker/checker.rs

+                // for now this means that the expression should have type `Array`
+                let iterator_typ = self.compute_type(iterator, typed_fn_env)?;
+                let iterator_typ =
+                    iterator_typ.expect("Could not compute type of iterator (TODO: better error)");


this can be an error thrown with a span, so it can better inform the users where the error is originated in the code.

katat · 2024-08-28T03:15:31Z

src/type_checker/checker.rs

+                    // exit the scope
+                    typed_fn_env.pop();
+                } else {
+                    panic!("Iterator should be an array (TODO: better error)");


katat · 2024-08-28T03:16:18Z

src/circuit_writer/writer.rs

@@ -5,6 +5,7 @@ use kimchi::circuits::wires::Wire;
 use num_bigint::BigUint;
 use num_traits::Num as _;
 use serde::{Deserialize, Serialize};
+use toml::value::Array;


seems unused

gio54321 · 2024-08-28T14:50:53Z

Should have addressed most of the things, now merging into rfc-0-impl

One thing to note is that for now the iterator variable is immutable, maybe we could consider adding also mutable iterator variables? For example this code would add one to every element of arr

for mut elem in array {
    elem = elem + 1
}

though I don't necessarily think that this is a good pattern and I tend to think this should not be allowed, since to me it is not clear if this is mutating the underlying structure or just the local variable elem.

katat

Nice. I think that is a good spot. We may come back to this mutable feature if the practical projects really demands it.

There are just a couple of minor issues to fix. The main one is to add the new iterator example to tests. Maybe rename the iterators.no to generic_iterator.no as it involves generic and helps differentiate from the existing iterator.no.

katat · 2024-08-29T06:52:10Z

examples/iterators.no

@@ -0,0 +1,16 @@
+fn zip(a1: [Field; LEN], a2: [Field; LEN]) -> [[Field; 2]; LEN] {


this new example should be added as a new test in the tests/examples.rs

katat · 2024-08-29T07:03:28Z

src/circuit_writer/writer.rs

+                iterator,
+                body,
+            } => {
+                let iterator_expr = self


maybe it is better to call it as iterator_var, since it is of VarOrRef after the expression has been computed.

katat · 2024-08-29T07:06:01Z

src/mast/mod.rs

+
+            mono_fn_env.store_type(
+                &var.value,
+                &MTypeInfo::new(&array_element_type, var.span, None),


unnecessary & prefix

katat · 2024-08-29T07:34:17Z

src/type_checker/checker.rs

+                    TyKind::Array(element_type, _len) => *element_type,
+                    TyKind::GenericSizedArray(element_type, _size) => *element_type,
+                    _ => {
+                        return Err(self.error(


maybe add a negative test (negative_tests.rs) to cover this scenario, this would help remind this type check should be covered in the tast phase if it happens to refactor at later stage :D

katat

Awesome work!

This feature is a great fit with the generic arrays. The generic feature enable working with varying length of arrays, while this iterator feature simplifies the use of these generic array and make it practical.

mimoo · 2024-09-02T19:31:17Z

src/parser/types.rs

+        var: Ident,
+        iterator: Box<Expr>,
+        body: Vec<Stmt>,
+    },


late to the party, but wouldn't it be cleaner (and perhaps safer since forloops have annoying logic for generics) to have a common structure for them?

ForLoop { var: Ident, range: RangeOrIterator, body: Vec<Stmt>, }

gio54321 added 3 commits August 27, 2024 11:53

Add initial support for iterators

bf75898

Add iterators example

5eb5812

Update tests

8d0b414

katat reviewed Aug 28, 2024

View reviewed changes

gio54321 changed the base branch from main to rfc-0-impl August 28, 2024 08:20

gio54321 added 6 commits August 28, 2024 11:20

Merge branch 'rfc-0-impl' of github.com:zksecurity/noname into iterators

4c45219

Fix parsing and monomorphization for iterators

cfef3bd

Implement iterator type checking for generic sized arrays

a519c0b

Update iterators example to use generic sized arrays

4ce48ab

Remove some unused imports

c35479d

Fix iterator name resolution

79e8bb4

katat reviewed Aug 29, 2024

View reviewed changes

gio54321 added 4 commits August 30, 2024 10:55

Add iterator tests

2a32591

Add more iterator negative tests

26a1a09

Refactor variable names

570d7ea

Fix formatting

2635507

katat approved these changes Sep 2, 2024

View reviewed changes

katat merged commit fd1d827 into zksecurity:rfc-0-impl Sep 2, 2024
1 check passed

mimoo reviewed Sep 2, 2024

View reviewed changes

This was referenced Sep 2, 2024

Merge forloop structure into one #176

Closed

Ensure no generic function call within iterator loop #177

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for iterators #169

Add support for iterators #169

gio54321 commented Aug 27, 2024 •

edited

Loading

katat left a comment

katat Aug 28, 2024

katat Aug 28, 2024

katat Aug 28, 2024

katat Aug 28, 2024

gio54321 commented Aug 28, 2024

katat left a comment •

edited

Loading

katat Aug 29, 2024

katat Aug 29, 2024

katat Aug 29, 2024

katat Aug 29, 2024

katat left a comment

mimoo Sep 2, 2024

mimoo Sep 3, 2024

		@@ -0,0 +1,16 @@
		fn zip(a1: [Field; LEN], a2: [Field; LEN]) -> [[Field; 2]; LEN] {

Add support for iterators #169

Add support for iterators #169

Conversation

gio54321 commented Aug 27, 2024 • edited Loading

katat left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gio54321 commented Aug 28, 2024

katat left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

katat left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gio54321 commented Aug 27, 2024 •

edited

Loading

katat left a comment •

edited

Loading