Repository: cython/cython
Pull request: Execute AlignFunctionDefinitions before MarkClosureTransform.
This PR is fixing crashing cython when generator expressions are
used in cdef functions in pure python mode. Fixes #3477.


Problem statement:
variable 'referenced before assignment' warning, error in all() with .pxd but not .pyx
The following is a standalone example of a problem I've run into trying to accelerate some code:

given allExt.py (takes a set of words and checks if 3rd letter in each is a vowel):
```
class a3v(object):
    def __init__(self, inset):
        self.myStrSet = inset   # set input word list as an attribute on  class instance

    def all3Vowels(self):     # test if 3rd letter of each word is a vowel
        vndx = 2                    # local variable for 3rd letter index
        inStrSet = self.myStrSet    # local variable for class instance word list
        if all(
            ltr in ("a", "e", "i", "o", "u")
            for ltr in (w[vndx] for w in inStrSet)
        ):
            return True
        return False
```
and tst.py:
```
#!/usr/local/bin/python3
import allExt

a3vL = allExt.a3v(["the", "bright", "flower"])

if a3vL.all3Vowels():
   print('yes')
else:
   print('no')
```
The code runs without issue, and fits with examples I have found in the cython docs.  However, when augmented with allExt.pxd: 
```
import cython

cdef class a3v:
     cdef public list myStrSet

     @cython.locals(vndx=int, inStrSet=list, ltr=str)
     cpdef bint all3Vowels(self)
```
Upon compiling as shown I get:
```
rob@nova-370:01 14:45:55 atmNdx $ cythonize -3 -f -i allExt.py allExt.pxd
[1/1] Cythonizing /Users/rob/sync/rgithub/cython/atmNdx/allExt.py
warning: allExt.py:12:26: local variable 'vndx' referenced before assignment

Error compiling Cython file:
------------------------------------------------------------
...
    def all3Vowels(self):
        vndx = 2
        inStrSet = self.myStrSet
        if all(
            ltr in ("a", "e", "i", "o", "u")
            for ltr in (w[vndx] for w in inStrSet)
                                        ^
------------------------------------------------------------

allExt.py:12:41: local variable 'inStrSet' referenced before assignment
Traceback (most recent call last):
```
OTOH, the equivalent (?) pyx version allExt.pyx:
```
class a3v(object):
    def __init__(self, inset):
        self.myStrSet = inset

    def all3Vowels(self):
        cdef int vndx = 2
        cdef list inStrSet = self.myStrSet
        cdef str ltr
        if all(
            ltr in ("a", "e", "i", "o", "u")
            for ltr in (w[vndx] for w in inStrSet)
        ):
            return True
        return False
```
compiles and runs without issue.

Have I missed something in the docs or some other simple issue?
Thank you.

Hints from issue discussion:
Thanks for the report. There seems to be something wrong with the nested generator expressions here and the way the type information is applied from the decorator (or maybe _when_ it is applied).

You didn't mention which Cython version you were using, so I guess it's a recent release. Could you try the latest master branch, and also PR #3323?

BTW, you don't need to declare `inStrSet` here because its type should get inferred from the assignment, since the attribute is typed.
Thanks for the follow up.
This was with Cython-0.29.16, but I have now reproduced with both 3.0a0
master and https://github.com/da-woods/cython.git (#3323).


On Fri, Apr 3, 2020 at 8:45 AM Stefan Behnel <notifications@github.com>
wrote:

> Thanks for the report. There seems to be something wrong with the nested
> generator expressions here and the way the type information is applied from
> the decorator (or maybe *when* it is applied).
>
> You didn't mention which Cython version you were using, so I guess it's a
> recent release. Could you try the latest master branch, and also PR #3323
> <https://github.com/cython/cython/pull/3323>?
>
> BTW, you don't need to declare inStrSet here because its type should get
> inferred from the assignment, since the attribute is typed.
>
> —
> You are receiving this because you authored the thread.
> Reply to this email directly, view it on GitHub
> <https://github.com/cython/cython/issues/3477#issuecomment-608283260>, or
> unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AAZW3QDL4WTTE5FOZWIMHALRKWHZRANCNFSM4LY7IUIA>
> .
>

I have spend several hours investigating the bug and here are my findings.

## Bug details
The issue can be easilly reproduced by any construct using `for ... in` (maybe except list and dict comprehension) when .pxd file is used:
```python
$ cat test.py
def foo():
    a = (x for x in range(5))
foo()

$ cat test.pxd
cdef foo()
```
but also `all()`, `any()`, etc... are causing crashing cython. The example above is crashing with following exception:
```python
Traceback (most recent call last):
  File "/home/matus/dev/cython39/bin/cythonize", line 33, in <module>
    sys.exit(load_entry_point('Cython', 'console_scripts', 'cythonize')())
  File "/home/matus/dev/cython/Cython/Build/Cythonize.py", line 224, in main
    cython_compile(path, options)
  File "/home/matus/dev/cython/Cython/Build/Cythonize.py", line 68, in cython_compile
    ext_modules = cythonize(
  File "/home/matus/dev/cython/Cython/Build/Dependencies.py", line 1115, in cythonize
    cythonize_one(*args)
  File "/home/matus/dev/cython/Cython/Build/Dependencies.py", line 1261, in cythonize_one
    result = compile_single(pyx_file, options, full_module_name=full_module_name)
  File "/home/matus/dev/cython/Cython/Compiler/Main.py", line 575, in compile_single
    return run_pipeline(source, options, full_module_name)
  File "/home/matus/dev/cython/Cython/Compiler/Main.py", line 503, in run_pipeline
    err, enddata = Pipeline.run_pipeline(pipeline, source)
  File "/home/matus/dev/cython/Cython/Compiler/Pipeline.py", line 370, in run_pipeline
    data = run(phase, data)
  File "/home/matus/dev/cython/Cython/Compiler/Pipeline.py", line 350, in run
    return phase(data)
  File "/home/matus/dev/cython/Cython/Compiler/Pipeline.py", line 52, in generate_pyx_code_stage
    module_node.process_implementation(options, result)
  File "/home/matus/dev/cython/Cython/Compiler/ModuleNode.py", line 171, in process_implementation
    self.generate_c_code(env, options, result)
  File "/home/matus/dev/cython/Cython/Compiler/ModuleNode.py", line 451, in generate_c_code
    self.body.generate_function_definitions(env, code)
  File "/home/matus/dev/cython/Cython/Compiler/Nodes.py", line 400, in generate_function_definitions
    stat.generate_function_definitions(env, code)
  File "/home/matus/dev/cython/Cython/Compiler/Nodes.py", line 1839, in generate_function_definitions
    self.generate_lambda_definitions(lenv, code)
  File "/home/matus/dev/cython/Cython/Compiler/Nodes.py", line 374, in generate_lambda_definitions
    node.generate_function_definitions(env, code)
  File "/home/matus/dev/cython/Cython/Compiler/Nodes.py", line 4432, in generate_function_definitions
    super(GeneratorDefNode, self).generate_function_definitions(env, code)
  File "/home/matus/dev/cython/Cython/Compiler/Nodes.py", line 3341, in generate_function_definitions
    FuncDefNode.generate_function_definitions(self, env, code)
  File "/home/matus/dev/cython/Cython/Compiler/Nodes.py", line 1886, in generate_function_definitions
    if self.needs_closure:
AttributeError: 'ClosureScope' object has no attribute 'scope_class'
```

## The cause of the bug

I have compared the version above with following .pyx version:
```python
cdef foo():
    a = (x for x in range(5))

foo()
```

It seems that the problem is in the following part:
```python
> /home/matus/dev/cython/Cython/Compiler/ParseTreeTransforms.py(2879)visit_FuncDefNode()
-> if node.needs_closure or self.path:
(Pdb) l
2874        def visit_FuncDefNode(self, node):
2875            if self.in_lambda:
2876                self.visitchildren(node)
2877                return node
2878            breakpoint()
2879 ->         if node.needs_closure or self.path:
2880                self.create_class_from_scope(node, self.module_scope)
2881                self.path.append(node)
2882                self.visitchildren(node)
2883                self.path.pop()
2884            return node
```
In working .pyx case node.needs_closure is evaluated as `True` and the block is executed. In broken .py case it is evaluated as `False`, the block is not executed and crash is followed. I did experiment and during breakpoint I have executed following:
```python
(Pdb) l
2874        def visit_FuncDefNode(self, node):
2875            if self.in_lambda:
2876                self.visitchildren(node)
2877                return node
2878            breakpoint()
2879 ->         if node.needs_closure or self.path:
2880                self.create_class_from_scope(node, self.module_scope)
2881                self.path.append(node)
2882                self.visitchildren(node)
2883                self.path.pop()
2884            return node
(Pdb) p node.needs_closure
False
(Pdb) node.needs_closure = True
(Pdb) c
```
After that the build was finished successfully. Hence, from that point, I focused on investigating `needs_closure`.

## needs_closure details

And I have found out the following:
**1. case with .pyx file which is not crashing:**
```python
> /home/matus/dev/cython/Cython/Compiler/ParseTreeTransforms.py(2678)visit_FuncDefNode()
-> print('3', self.needs_closure)
(Pdb) l
2673        def visit_FuncDefNode(self, node):
2674            print('2', False)
2675            self.needs_closure = False
2676            self.visitchildren(node)
2677            breakpoint()
2678 ->         print('3', self.needs_closure)
2679            print('3', True)
2680            node.needs_closure = self.needs_closure
2681            self.needs_closure = True
2682
2683            collector = YieldNodeCollector()
(Pdb) p node
<Cython.Compiler.Nodes.DefNode object at 0x7f53cf6a0490>
(Pdb) p self
<Cython.Compiler.ParseTreeTransforms.MarkClosureVisitor object at 0x7f53c8c0f400>
(Pdb) c
3 False
3 True
5 True
5 True
> /home/matus/dev/cython/Cython/Compiler/ParseTreeTransforms.py(2724)visit_CFuncDefNode()
-> print('4', self.needs_closure)
(Pdb) p node
<Cython.Compiler.Nodes.CFuncDefNode object at 0x7f53cf5e4310>                   # Node object Address is 0x7f53cf5e4310
(Pdb) p self
<Cython.Compiler.ParseTreeTransforms.MarkClosureVisitor object at 0x7f53c8c0f400>
(Pdb) c
4 True
4 True
> /home/matus/dev/cython/Cython/Compiler/ParseTreeTransforms.py(2878)visit_FuncDefNode()
-> if node.needs_closure or self.path:
(Pdb) p node
<Cython.Compiler.Nodes.CFuncDefNode object at 0x7f53cf5e4310>                   # Node object Address is 0x7f53cf5e4310
(Pdb)
```
Here can be seen that `node` object has needs_closure is set to True (in the line 2724, both nodes are `CFuncDefNode`) and it was set as displayed above (node objects has same address).

**2. case with .py file which is crashing:**
```python
> /home/matus/dev/cython/Cython/Compiler/ParseTreeTransforms.py(2678)visit_FuncDefNode()
-> print('3', self.needs_closure)
(Pdb) p node
<Cython.Compiler.Nodes.DefNode object at 0x7f6016a82c10>
(Pdb) p self
<Cython.Compiler.ParseTreeTransforms.MarkClosureVisitor object at 0x7f6016a866a0>
(Pdb) c
3 False
3 True
5 True
5 True
> /home/matus/dev/cython/Cython/Compiler/ParseTreeTransforms.py(2678)visit_FuncDefNode()
-> print('3', self.needs_closure)
<Cython.Compiler.Nodes.DefNode object at 0x7f6016a9e040>
(Pdb) p self
<Cython.Compiler.ParseTreeTransforms.MarkClosureVisitor object at 0x7f6016a866a0>
(Pdb) c
3 True
3 True
> /home/matus/dev/cython/Cython/Compiler/ParseTreeTransforms.py(2878)visit_FuncDefNode()
-> if node.needs_closure or self.path:
(Pdb) p node
<Cython.Compiler.Nodes.CFuncDefNode object at 0x7f600fdfd640>
```
Here can be seen that `node` object has needs_closure is set to False because there is no object with the address `0x7f600fdfd640` - the previous nodes are `DefNode` not `CFuncDefNode`. This is causing that the statement `if node.needs_closure or self.path` is evaluated as false causing later on crash as showed before.

@scoder I would like to fix this bug, but now I am slightly stuck. Could you give me a point which part should I look into?
@matusvalo The node will be converted into a `CFuncDefNode` in `AlignFunctionDefinitions`

https://github.com/cython/cython/blob/66eb66f2f0c0cfa22942853659d444b6f4ecb799/Cython/Compiler/ParseTreeTransforms.py#L2537.

It looks like that gets run just after `MarkClosureTransform`

https://github.com/cython/cython/blob/66eb66f2f0c0cfa22942853659d444b6f4ecb799/Cython/Compiler/Pipeline.py#L191-L192

You might be able to get away with just swapping their order. If that doesn't work then you might have to do something cleverer to repair the relevant links in `AlignFunctionDefinitions`.
Thank you @da-woods 
I have swapped the order and it works!
```
matus@L3505101303:~/dev/cython/Cython$ git diff
diff --git a/Cython/Compiler/Pipeline.py b/Cython/Compiler/Pipeline.py
index 4e8e98c71..342481c32 100644
--- a/Cython/Compiler/Pipeline.py
+++ b/Cython/Compiler/Pipeline.py
@@ -188,8 +188,8 @@ def create_pipeline(context, mode, exclude_classes=()):
         ParallelRangeTransform(context),
         AdjustDefByDirectives(context),
         WithTransform(context),
-        MarkClosureVisitor(context),
         _align_function_definitions,
+        MarkClosureVisitor(context),
         RemoveUnreachableCode(context),
         ConstantFolding(),
         FlattenInListTransform(),
```

Now the file is compiling without error:
```
$ rm *.c; cythonize -3ib test.py
rm: cannot remove '*.c': No such file or directory
Compiling /home/matus/dev/test/test.py because it changed.
[1/1] Cythonizing /home/matus/dev/test/test.py
running build_ext
building 'test' extension
creating /home/matus/dev/test/tmp8cp6hu35/home
creating /home/matus/dev/test/tmp8cp6hu35/home/matus
creating /home/matus/dev/test/tmp8cp6hu35/home/matus/dev
creating /home/matus/dev/test/tmp8cp6hu35/home/matus/dev/test
x86_64-linux-gnu-gcc -pthread -Wno-unused-result -Wsign-compare -DNDEBUG -g -fwrapv -O2 -Wall -g -fstack-protector-strong -Wformat -Werror=format-security -g -fstack-protector-strong -Wformat -Werror=format-security -Wdate-time -D_FORTIFY_SOURCE=2 -fPIC -I/home/matus/dev/cython39/include -I/usr/include/python3.9 -c /home/matus/dev/test/test.c -o /home/matus/dev/test/tmp8cp6hu35/home/matus/dev/test/test.o
x86_64-linux-gnu-gcc -pthread -shared -Wl,-O1 -Wl,-Bsymbolic-functions -Wl,-Bsymbolic-functions -Wl,-z,relro -Wl,-Bsymbolic-functions -Wl,-z,relro -g -fstack-protector-strong -Wformat -Werror=format-security -Wdate-time -D_FORTIFY_SOURCE=2 /home/matus/dev/test/tmp8cp6hu35/home/matus/dev/test/test.o -o /home/matus/dev/test/test.cpython-39-x86_64-linux-gnu.so
```
I have checked also the functionality and the generated library is working correctly (at least for my simple example)!
Good! When you have time feel free to submit it as a PR (with your small example test integrated into https://github.com/cython/cython/blob/master/tests/run/pure_pxd.srctree).

Hopefully it won't break anything else, but the full lot of tests will get run on the PR so we'll find out
PR created. I will do further validation in larger code base.

Commit message: Inherit AutoCpdefFunctionDefinitions from AlignFunctionDefinitions

Patch to review:
diff --git a/Cython/Compiler/ParseTreeTransforms.py b/Cython/Compiler/ParseTreeTransforms.py
index 33020884068..eeb7f774001 100644
--- a/Cython/Compiler/ParseTreeTransforms.py
+++ b/Cython/Compiler/ParseTreeTransforms.py
@@ -2496,8 +2496,6 @@ class AlignFunctionDefinitions(CythonTransform):
 
     def visit_ModuleNode(self, node):
         self.scope = node.scope
-        self.directives = node.directives
-        self.imported_names = set()  # hack, see visit_FromImportStatNode()
         self.visitchildren(node)
         return node
 
@@ -2535,16 +2533,32 @@ def visit_DefNode(self, node):
                     error(pxd_def.pos, "previous declaration here")
                 return None
             node = node.as_cfunction(pxd_def)
+        return node
+
+    def visit_ExprNode(self, node):
+        # ignore lambdas and everything else that appears in expressions
+        return node
+
+
+class AutoCpdefFunctionDefinitions(AlignFunctionDefinitions):
+
+    def visit_ModuleNode(self, node):
+        self.directives = node.directives
+        self.imported_names = set()  # hack, see visit_FromImportStatNode()
+        return super(AutoCpdefFunctionDefinitions, self).visit_ModuleNode(node)
+
+    def visit_DefNode(self, node):
+        pxd_def = self.scope.lookup(node.name)
+        if pxd_def and (not pxd_def.scope or not pxd_def.scope.is_builtin_scope):
+            pass
         elif (self.scope.is_module_scope and self.directives['auto_cpdef']
               and node.name not in self.imported_names
               and node.is_cdef_func_compatible()):
-            # FIXME: cpdef-ing should be done in analyse_declarations()
             node = node.as_cfunction(scope=self.scope)
-        # Enable this when nested cdef functions are allowed.
-        # self.visitchildren(node)
         return node
 
     def visit_FromImportStatNode(self, node):
+        # return node
         # hack to prevent conditional import fallback functions from
         # being cdpef-ed (global Python variables currently conflict
         # with imports)
@@ -2553,10 +2567,6 @@ def visit_FromImportStatNode(self, node):
                 self.imported_names.add(name)
         return node
 
-    def visit_ExprNode(self, node):
-        # ignore lambdas and everything else that appears in expressions
-        return node
-
 
 class RemoveUnreachableCode(CythonTransform):
     def visit_StatListNode(self, node):
diff --git a/Cython/Compiler/Pipeline.py b/Cython/Compiler/Pipeline.py
index 4e8e98c713f..37e68147a46 100644
--- a/Cython/Compiler/Pipeline.py
+++ b/Cython/Compiler/Pipeline.py
@@ -149,7 +149,7 @@ def create_pipeline(context, mode, exclude_classes=()):
     from .ParseTreeTransforms import ExpandInplaceOperators, ParallelRangeTransform
     from .ParseTreeTransforms import CalculateQualifiedNamesTransform
     from .TypeInference import MarkParallelAssignments, MarkOverflowingArithmetic
-    from .ParseTreeTransforms import AdjustDefByDirectives, AlignFunctionDefinitions
+    from .ParseTreeTransforms import AdjustDefByDirectives, AlignFunctionDefinitions, AutoCpdefFunctionDefinitions
     from .ParseTreeTransforms import RemoveUnreachableCode, GilCheck
     from .FlowControl import ControlFlowAnalysis
     from .AnalysedTreeTransforms import AutoTestDictTransform
@@ -188,8 +188,9 @@ def create_pipeline(context, mode, exclude_classes=()):
         ParallelRangeTransform(context),
         AdjustDefByDirectives(context),
         WithTransform(context),
-        MarkClosureVisitor(context),
         _align_function_definitions,
+        MarkClosureVisitor(context),
+        AutoCpdefFunctionDefinitions(context),
         RemoveUnreachableCode(context),
         ConstantFolding(),
         FlattenInListTransform(),
diff --git a/tests/run/pure_pxd.srctree b/tests/run/pure_pxd.srctree
index 59c71cdf62b..fcba49ac089 100644
--- a/tests/run/pure_pxd.srctree
+++ b/tests/run/pure_pxd.srctree
@@ -55,6 +55,17 @@ def func(a, b, c):
     """
     return a + b + c
 
+def sum_generator_expression(a):
+    # GH-3477 - closure variables incorrectly captured in functions transformed to cdef
+    return sum(i for i in range(a))
+
+def run_sum_generator_expression(a):
+    """
+    >>> run_sum_generator_expression(5)
+    10
+    """
+    return sum_generator_expression(a)
+
 
 def test(module):
     import os.path
@@ -95,3 +106,6 @@ cdef class TypedMethod:
 
 
 cpdef int func(x, int y, z) except? -1  # argument names should not matter, types should
+
+
+cdef int sum_generator_expression(int a)


Code review comments:
diff_hunk: @@ -2535,16 +2533,32 @@ def visit_DefNode(self, node):
                     error(pxd_def.pos, "previous declaration here")
                 return None
             node = node.as_cfunction(pxd_def)
+        return node
+
+    def visit_ExprNode(self, node):
+        # ignore lambdas and everything else that appears in expressions
+        return node
+
+
+class AutoCpdefFunctionDefinitions(AlignFunctionDefinitions):
+
+    def visit_ModuleNode(self, node):
+        self.directives = node.directives
+        self.imported_names = set()  # hack, see visit_FromImportStatNode()
+        return super(AutoCpdefFunctionDefinitions, self).visit_ModuleNode(node)
+
+    def visit_DefNode(self, node):
+        pxd_def = self.scope.lookup(node.name)
+        if pxd_def and (not pxd_def.scope or not pxd_def.scope.is_builtin_scope):
+            pass
         elif (self.scope.is_module_scope and self.directives['auto_cpdef']
               and node.name not in self.imported_names
               and node.is_cdef_func_compatible()):
-            # FIXME: cpdef-ing should be done in analyse_declarations()
             node = node.as_cfunction(scope=self.scope)
-        # Enable this when nested cdef functions are allowed.
-        # self.visitchildren(node)
         return node
 
     def visit_FromImportStatNode(self, node):
+        # return node
path: Cython/Compiler/ParseTreeTransforms.py
text: @user1:
?

@author:
This is debug code I have missed to delete.