Repository: cython/cython
Pull request: Fix iteration over cpp input ranges
Hello wonderful people!

This PR resolves #6578.
This change allows CppIteratorNode to insert it's own 'for' statement that uses standard C for loop capabilities.

```
from libcpp.vector cimport vector

cpdef test():
    cdef vector[int] v
    for i in v:
        print(i)
```

Before:
```
  __pyx_t_1 = __pyx_v_v.begin();
                      /*              <- CppIteratorNode.generate_result_code (6, 13) */
  for (;;) {
                        /*              -> NextNode.generate_result_code (6, 13) */
    if (!(__pyx_t_1 != __pyx_v_v.end())) break;
    __pyx_t_2 = *__pyx_t_1;
    ++__pyx_t_1;
                        /*              <- NextNode.generate_result_code (6, 13) */
    __pyx_v_i = __pyx_t_2;
```

After:
```
  __pyx_t_1 = __pyx_v_v.begin();
                      /*              <- CppIteratorNode.generate_result_code (6, 13) */
  for (                    /*          -> CppIteratorNode.generate_for_loop_header (6, 13) */
; __pyx_t_1 != __pyx_v_v.end(); ++__pyx_t_1                    /*          <- CppIteratorNode.generate_for_loop_header (6, 13) */
  ) {
                        /*              -> NextNode.generate_result_code (6, 13) */
    __pyx_t_2 = *__pyx_t_1;
                        /*              <- NextNode.generate_result_code (6, 13) */
    __pyx_v_i = __pyx_t_2;

```


Problem statement:
[BUG] 'for in' loop doesn't generate correct code for C++ ranges with input iterator category
### Describe the bug

I'm trying to write a pxd for the popular c++ simdjson library. simdjson has an `array` type with `input iterator` representing json array. I have the following code in pxd:

```
    cppclass sj_array_iterator "simdjson::ondemand::array_iterator":
        sj_array_iterator()

        operator++()
        bint operator ==(sj_array_iterator)
        bint operator !=(sj_array_iterator)
        sj_value operator *()

    cdef cppclass sj_array "simdjson::ondemand::array":
        ...
        sj_array_iterator begin()
        sj_array_iterator end()

```

The issue is, for the following cython code incorrect C++ code is generated:
```
cdef sj_array arr = ...
cdef sj_value arr_element
for arr_element in arr:
      # Some code for arr_element
       ...
```

The generated C++ code: 
1. Check that iterator does not point to arr.end()
2. Dereference iterator
**3. Increment iterator**
**4. Execute user code (everything after #Some code for arr_element)**

The correct would be to swap steps 3 and 4. This is what c++ range-based for loop does. Also this is how C/C++ for loop works. Increment part of `for(...;...;++i)` is executed after inner user code is complete and we are ready to proceed to the next iteration.

In simdjson iterators and `value` object don't really point to a particular part of json. Instead they share the reference to the current parser state. Incrementing iterator actually changes what previously dereferenced value points at.


### Code to reproduce the behaviour:

Any standard container can be used to generate incorrect iteration code. But for the most containers it will work anyway:

```cython

from libcpp.vector cimport vector

cdef test1():
    cdef:
        vector[int] v
        int i

    for i in v:
        print(i)

```

```
/*
 *         int i
 * 
 *     for i in v:             # <<<<<<<<<<<<<<
 *         print(i)
 * 
 */
  __pyx_t_1 = __pyx_v_v.begin();
  for (;;) {
    if (!(__pyx_t_1 != __pyx_v_v.end())) break;
    __pyx_t_2 = *__pyx_t_1;
    ++__pyx_t_1;
    __pyx_v_i = __pyx_t_2;

    /* 
 * 
 *     for i in v:
 *         print(i)             # <<<<<<<<<<<<<<
 * 
 */
    __pyx_t_3 = __Pyx_PyInt_From_int(__pyx_v_i); if (unlikely(!__pyx_t_3)) __PYX_ERR(1, 435, __pyx_L1_error)
```


### Expected behaviour

Increment 
```
    ++__pyx_t_1;
```
should happen after executing user code `print(i)`

### OS

all

### Python version

all

### Cython version

I think all, but I checked 3.0.11

### Additional context

I wonder if it would be possible to just rely on the standard behavior of for loop?
```
 for (;__pyx_t_1 != __pyx_v_v.end();__pyx_t_1++) {
     __pyx_t_2 = *__pyx_t_1;
```

I would also avoid calling end() on each iteration to be fully compliant with C++ range-based for loop:
```
auto /* begin */ = /* begin-expr */;
auto /* end */ = /* end-expr */;
for ( ; /* begin */ != /* end */; ++/* begin */)
{
     item-declaration = */* begin */;
     statement
}
```

Hints from issue discussion:

Commit message: Fix failing tests

Patch to review:
diff --git a/Cython/Compiler/ExprNodes.py b/Cython/Compiler/ExprNodes.py
index c931a55f583..9cc3ecc02f0 100644
--- a/Cython/Compiler/ExprNodes.py
+++ b/Cython/Compiler/ExprNodes.py
@@ -2985,6 +2985,7 @@ class IteratorNode(ScopedExprNode):
     reversed = False      # currently only used for list/tuple types (see Optimize.py)
     is_async = False
     has_local_scope = False
+    has_custom_for_loop = False
 
     subexprs = ['sequence']
 
@@ -3218,6 +3219,7 @@ class CppIteratorNode(ExprNode):
     extra_dereference = ""
     is_temp = True
     reversed = False
+    has_custom_for_loop = True
 
     subexprs = ['sequence']
 
@@ -3274,7 +3276,7 @@ def analyse_types(self, env):
 
     def generate_result_code(self, code):
         sequence_type = self.sequence.type
-        begin_name, _ = self.get_iterator_func_names()
+        begin_name, end_name = self.get_iterator_func_names()
         # essentially 3 options:
         if self.sequence.is_simple():
             # 1) Sequence can be accessed directly, like a name;
@@ -3308,21 +3310,24 @@ def generate_result_code(self, code):
                     self.cpp_attribute_op,
                     begin_name))
 
-    def generate_iter_next_result_code(self, result_name, code):
         # end call isn't cached to support containers that allow adding while iterating
         # (much as this is usually a bad idea)
-        _, end_name = self.get_iterator_func_names()
-        code.putln("if (!(%s%s != %s%s%s())) break;" % (
+
+        code.putln("for (; %s%s != %s%s%s(); ++%s%s) {" % (
                         self.extra_dereference,
                         self.result(),
                         self.cpp_sequence_cname or self.sequence.result(),
                         self.cpp_attribute_op,
-                        end_name))
+                        end_name,
+                        self.extra_dereference,
+                        self.result()
+        ))
+
+    def generate_iter_next_result_code(self, result_name, code):
         code.putln("%s = *%s%s;" % (
                         result_name,
                         self.extra_dereference,
                         self.result()))
-        code.putln("++%s%s;" % (self.extra_dereference, self.result()))
 
     def generate_subexpr_disposal_code(self, code):
         if not self.cpp_sequence_cname:
@@ -3412,6 +3417,7 @@ class AsyncIteratorNode(ScopedExprNode):
     type = py_object_type
     is_temp = 1
     has_local_scope = False
+    has_custom_for_loop = False
 
     def infer_type(self, env):
         return py_object_type
diff --git a/Cython/Compiler/Nodes.py b/Cython/Compiler/Nodes.py
index 7721c9eac1f..8556168779f 100644
--- a/Cython/Compiler/Nodes.py
+++ b/Cython/Compiler/Nodes.py
@@ -7707,7 +7707,8 @@ def generate_execution_code(self, code):
         code.mark_pos(self.pos)
         old_loop_labels = code.new_loop_labels()
         self.iterator.generate_evaluation_code(code)
-        code.putln("for (;;) {")
+        if not self.iterator.has_custom_for_loop:
+            code.putln("for (;;) {")
         self.item.generate_evaluation_code(code)
         self.target.generate_assignment_code(self.item, code)
         code.write_trace_line(self.pos)


Code review comments:
diff_hunk: @@ -7707,7 +7707,8 @@ def generate_execution_code(self, code):
         code.mark_pos(self.pos)
         old_loop_labels = code.new_loop_labels()
         self.iterator.generate_evaluation_code(code)
-        code.putln("for (;;) {")
+        if not self.iterator.has_custom_for_loop:
path: Cython/Compiler/Nodes.py
text: @user1:
I wonder if it'd be better to do

```
code.put("for (")
self.iterator.generate_for_loop_header(code)
code.put(") {")
```

with the default version being

```
def generate_for_loop_header(self, code):
    code.put(";;")
```
?

What I don't really like about your current version is that it's a little unclear where the `for (...) {` is generated in the `has_custom_for_loop` case.

@author:
Totally agree, but I'm not sure where to put `generate_for_loop_header` default implementation. 
The common ancestor of IteratorNode, CppIteratorNode, AsyncIteratorNode is ExprNode. Is it ok if we put it there?


@user1:
`ExprNode` is too general I think.

Given how simple the default implementation is, you could just duplicate it on `IteratorNode` and `AsyncIteratorNode` and not worry about inheritance.