Repository: cython/cython
Pull request: Fixes CodeWriter for `cdef` functions aka. CFuncDefNode.
Resolves #3513.

Problem statement:
CodeWriter doesn't work for 'CFuncDefNode'
```python
>>> writer = CodeWriter()

>>> writer.write(TreeFragment("def fn(): \n    pass").root)
<Cython.CodeWriter.LinesResult at 0x121704a90>

>>> writer.write(TreeFragment("cdef fn(): \n    pass").root)
Compiler crash traceback from this point on:
  File "Cython/Compiler/Visitor.py", line 180, in Cython.Compiler.Visitor.TreeVisitor._visit
  File "/usr/local/lib/python3.7/site-packages/Cython/CodeWriter.py", line 243, in visit_FuncDefNode
    self.startline(u"def %s(" % node.name)
AttributeError: 'CFuncDefNode' object has no attribute 'name'
```

Hints from issue discussion:
Interesting to see that people are actually using this. :)

PR welcome.
> Interesting to see that people are actually using this. :)

Is the `CodeWriter` the only way to dump Cython AST into syntastically-correct Cython code?

> PR welcome.

PR #3514 will fix that.
> Is the CodeWriter the only way to dump Cython AST into syntastically-correct Cython code?

The intended way, yes.
@scoder It is quite wired that `CodeWriter` cannot handle some kinkds of experssions, for example, the `PrimaryCmpNode`, however we have a `visit_PrimaryCmpNode` in `ExpressionWriter`.

The `CodeWriter` seems never been used at other place in Cython repo, but due to missing such visitors we cannot have a `Writer` that can print whole program. Should we make `CodeWriter` to inherits from `ExpressionWriter` as well?

Edit: to make it more clear, I mean, should we treat `CodeWriter` as the "union" of `DeclarationWriter` and `ExpressionWriter` then we get a full-functionality "Writer" ?
@scoder Any comments? (polite ping 😆 
@sighingnow good idea. I just pushed #3528.
> good idea. I just pushed #3528.

Thanks a lot! Visitor for some kinds of ast `Node` are still missing, I will continue to push to #3514 to make the `CodeWriter` more complete. Will ping you when it ready for review and merging.

Commit message: Fix for the special case for `CSimpleBaseTypeNode` (when name is None).

Signed-off-by: Tao He <linzhu.ht@alibaba-inc.com>

Patch to review:
diff --git a/Cython/CodeWriter.py b/Cython/CodeWriter.py
index 24d57626b67..f09dd62d010 100644
--- a/Cython/CodeWriter.py
+++ b/Cython/CodeWriter.py
@@ -1,7 +1,6 @@
 """
 Serializes a Cython code tree to Cython code. This is primarily useful for
 debugging and testing purposes.
-
 The output is in a strict format, no whitespace or comments from the input
 is preserved (and it could not be as it is not present in the code tree).
 """
@@ -10,6 +9,7 @@
 
 from .Compiler.Visitor import TreeVisitor
 from .Compiler.ExprNodes import *
+from .Compiler.Nodes import CNameDeclaratorNode, CSimpleBaseTypeNode
 
 
 class LinesResult(object):
@@ -80,6 +80,14 @@ def comma_separated_list(self, items, output_rhs=False):
                     self.visit(item.default)
                 self.put(u", ")
             self.visit(items[-1])
+            if output_rhs and items[-1].default is not None:
+                self.put(u" = ")
+                self.visit(items[-1].default)
+
+    def visit_indented(self, node):
+        self.indent()
+        self.visit(node)
+        self.dedent()
 
     def visit_Node(self, node):
         raise AssertionError("Node not handled by serializer: %r" % node)
@@ -96,9 +104,7 @@ def visit_CDefExternNode(self, node):
         else:
             file = u'"%s"' % node.include_file
         self.putline(u"cdef extern from %s:" % file)
-        self.indent()
-        self.visit(node.body)
-        self.dedent()
+        self.visit_indented(node.body)
 
     def visit_CPtrDeclaratorNode(self, node):
         self.put('*')
@@ -133,13 +139,12 @@ def visit_CSimpleBaseTypeNode(self, node):
                 self.put("short " * -node.longness)
             elif node.longness > 0:
                 self.put("long " * node.longness)
-        self.put(node.name)
+        if node.name is not None:
+            self.put(node.name)
 
     def visit_CComplexBaseTypeNode(self, node):
-        self.put(u'(')
         self.visit(node.base_type)
         self.visit(node.declarator)
-        self.put(u')')
 
     def visit_CNestedBaseTypeNode(self, node):
         self.visit(node.base_type)
@@ -225,9 +230,7 @@ def visit_CClassDefNode(self, node):
             self.put(node.base_class_name)
             self.put(u")")
         self.endline(u":")
-        self.indent()
-        self.visit(node.body)
-        self.dedent()
+        self.visit_indented(node.body)
 
     def visit_CTypeDefNode(self, node):
         self.startline(u"ctypedef ")
@@ -241,14 +244,49 @@ def visit_FuncDefNode(self, node):
         self.startline(u"def %s(" % node.name)
         self.comma_separated_list(node.args)
         self.endline(u"):")
-        self.indent()
-        self.visit(node.body)
-        self.dedent()
+        self.visit_indented(node.body)
+
+    def visit_CFuncDefNode(self, node):
+        if node.overridable:
+            self.startline(u'cpdef ')
+        else:
+            self.startline(u'cdef ')
+        if node.modifiers:
+            self.put(' '.join(node.modifiers))
+            self.put(' ')
+        if node.visibility != 'private':
+            self.put(node.visibility)
+            self.put(u' ')
+        if node.api:
+            self.put(u'api ')
+
+        if node.base_type:
+            self.visit(node.base_type)
+            if node.base_type.name is not None:
+                self.put(u' ')
+
+        # visit the CFuncDeclaratorNode, but put a `:` at the end of line
+        self.visit(node.declarator.base)
+        self.put(u'(')
+        self.comma_separated_list(node.declarator.args)
+        self.endline(u'):')
+
+        self.visit_indented(node.body)
 
     def visit_CArgDeclNode(self, node):
-        if node.base_type.name is not None:
+        # for `CSimpleBaseTypeNode`, the variable type may been parsed as type.
+        # for other types node, its `name` is always None.
+        if not isinstance(node.base_type, CSimpleBaseTypeNode) or \
+                node.base_type.name is not None:
             self.visit(node.base_type)
-            self.put(u" ")
+
+            # once we print something for node.base_type, we may need to print
+            # an extra ` `.
+            #
+            # a special case: if `node.declarator` is a `CNameDeclaratorNode`,
+            # its "name" might be empty string, for example, for `cdef f(x)`.
+            if node.declarator.declared_name():
+                self.put(u" ")
         self.visit(node.declarator)
         if node.default is not None:
             self.put(u" = ")
@@ -350,14 +388,25 @@ def visit_IfStatNode(self, node):
             self.startline("elif ")
             self.visit(clause.condition)
             self.endline(":")
-            self.indent()
-            self.visit(clause.body)
-            self.dedent()
+            self.visit_indented(clause.body)
         if node.else_clause is not None:
             self.line("else:")
-            self.indent()
-            self.visit(node.else_clause)
-            self.dedent()
+            self.visit_indented(node.else_clause)
+
+    def visit_WhileStatNode(self, node):
+        self.startline(u"while ")
+        self.visit(node.condition)
+        self.endline(u":")
+        self.visit_indented(node.body)
+        if node.else_clause is not None:
+            self.line("else:")
+            self.visit_indented(node.else_clause)
+
+    def visit_ContinueStatNode(self, node):
+        self.line(u"continue")
+
+    def visit_BreakStatNode(self, node):
+        self.line(u"break")
 
     def visit_SequenceNode(self, node):
         self.comma_separated_list(node.args)  # Might need to discover whether we need () around tuples...hmm...
@@ -382,9 +431,7 @@ def visit_WithStatNode(self, node):
             self.put(u" as ")
             self.visit(node.target)
         self.endline(u":")
-        self.indent()
-        self.visit(node.body)
-        self.dedent()
+        self.visit_indented(node.body)
 
     def visit_TryFinallyStatNode(self, node):
         self.line(u"try:")
@@ -392,9 +439,7 @@ def visit_TryFinallyStatNode(self, node):
         self.visit(node.body)
         self.dedent()
         self.line(u"finally:")
-        self.indent()
-        self.visit(node.finally_clause)
-        self.dedent()
+        self.visit_indented(node.finally_clause)
 
     def visit_TryExceptStatNode(self, node):
         self.line(u"try:")
@@ -415,9 +460,7 @@ def visit_ExceptClauseNode(self, node):
             self.put(u", ")
             self.visit(node.target)
         self.endline(":")
-        self.indent()
-        self.visit(node.body)
-        self.dedent()
+        self.visit_indented(node.body)
 
     def visit_ReturnStatNode(self, node):
         self.startline("return ")
@@ -480,12 +523,18 @@ def comma_separated_list(self, items):
     def visit_Node(self, node):
         raise AssertionError("Node not handled by serializer: %r" % node)
 
-    def visit_NameNode(self, node):
-        self.put(node.name)
+    def visit_IntNode(self, node):
+        self.put(node.value)
+
+    def visit_FloatNode(self, node):
+        self.put(node.value)
 
     def visit_NoneNode(self, node):
         self.put(u"None")
 
+    def visit_NameNode(self, node):
+        self.put(node.name)
+
     def visit_EllipsisNode(self, node):
         self.put(u"...")
 
@@ -756,12 +805,13 @@ def __call__(self, node):
         return node
 
     def visit_CFuncDefNode(self, node):
-        if 'inline' in node.modifiers:
-            return
         if node.overridable:
             self.startline(u'cpdef ')
         else:
             self.startline(u'cdef ')
+        if node.modifiers:
+            self.put(' '.join(node.modifiers))
+            self.put(' ')
         if node.visibility != 'private':
             self.put(node.visibility)
             self.put(u' ')
diff --git a/Cython/Tests/TestCodeWriter.py b/Cython/Tests/TestCodeWriter.py
index a793dd6c65e..a29562aa93a 100644
--- a/Cython/Tests/TestCodeWriter.py
+++ b/Cython/Tests/TestCodeWriter.py
@@ -21,6 +21,7 @@ def test_print(self):
         self.t(u"""
                     print(x + y ** 2)
                     print(x, y, z)
+                    print(x + y, x + y * z, x * (y + z))
                """)
 
     def test_if(self):
@@ -46,6 +47,20 @@ def f(x = 34, y = 54, z):
                         pass
                """)
 
+    def test_cdef(self):
+        self.t(u"""
+                    cdef f(x, y, z):
+                        pass
+                    cdef public void (x = 34, y = 54, z):
+                        pass
+                    cdef f(int *x, void *y, Value *z):
+                        pass
+                    cdef f(int **x, void **y, Value **z):
+                        pass
+                    cdef inline f(int &x, Value &z):
+                        pass
+               """)
+
     def test_longness_and_signedness(self):
         self.t(u"def f(unsigned long long long long long int y):\n    pass")
 
@@ -75,6 +90,14 @@ def test_for_loop(self):
                         print(43)
                 """)
 
+    def test_while_loop(self):
+        self.t(u"""
+                    while True:
+                        while True:
+                            while True:
+                                continue
+                """)
+
     def test_inplace_assignment(self):
         self.t(u"x += 43")
 


Code review comments:
diff_hunk: @@ -241,14 +244,49 @@ def visit_FuncDefNode(self, node):
         self.startline(u"def %s(" % node.name)
         self.comma_separated_list(node.args)
         self.endline(u"):")
-        self.indent()
-        self.visit(node.body)
-        self.dedent()
+        self.visit_indented(node.body)
+
+    def visit_CFuncDefNode(self, node):
+        if node.overridable:
+            self.startline(u'cpdef ')
+        else:
+            self.startline(u'cdef ')
path: Cython/CodeWriter.py
text: @user1:
```suggestion
        self.startline(u'cpdef ' if node.overridable else u'cdef ')
```

diff_hunk: @@ -80,6 +80,14 @@ def comma_separated_list(self, items, output_rhs=False):
                     self.visit(item.default)
                 self.put(u", ")
             self.visit(items[-1])
+            if output_rhs and items[-1].default is not None:
+                self.put(u" = ")
+                self.visit(items[-1].default)
+
+    def visit_indented(self, node):
path: Cython/CodeWriter.py
text: @user1:
Ah, sorry, my fault. `.visit_*()` is the naming convention for methods that match node types. Just give it a private name then:
```suggestion
    def _visit_indented(self, node):
```

diff_hunk: @@ -241,14 +244,49 @@ def visit_FuncDefNode(self, node):
         self.startline(u"def %s(" % node.name)
         self.comma_separated_list(node.args)
         self.endline(u"):")
-        self.indent()
-        self.visit(node.body)
-        self.dedent()
+        self.visit_indented(node.body)
+
+    def visit_CFuncDefNode(self, node):
+        if node.overridable:
+            self.startline(u'cpdef ')
+        else:
+            self.startline(u'cdef ')
+        if node.modifiers:
+            self.put(' '.join(node.modifiers))
+            self.put(' ')
+        if node.visibility != 'private':
+            self.put(node.visibility)
+            self.put(u' ')
+        if node.api:
+            self.put(u'api ')
+
+        if node.base_type:
+            self.visit(node.base_type)
+            if node.base_type.name is not None:
+                self.put(u' ')
+
+        # visit the CFuncDeclaratorNode, but put a `:` at the end of line
+        self.visit(node.declarator.base)
+        self.put(u'(')
+        self.comma_separated_list(node.declarator.args)
+        self.endline(u'):')
+
+        self.visit_indented(node.body)
 
     def visit_CArgDeclNode(self, node):
-        if node.base_type.name is not None:
+        # for `CSimpleBaseTypeNode`, the variable type may been parsed as type.
+        # for other types node, its `name` is always None.
path: Cython/CodeWriter.py
text: @user1:
```suggestion
        # For "CSimpleBaseTypeNode", the variable type may have been parsed as type.
        # For other node types, the "name" is always None.
```

diff_hunk: @@ -382,19 +431,15 @@ def visit_WithStatNode(self, node):
             self.put(u" as ")
             self.visit(node.target)
         self.endline(u":")
-        self.indent()
-        self.visit(node.body)
-        self.dedent()
+        self.visit_indented(node.body)
 
     def visit_TryFinallyStatNode(self, node):
         self.line(u"try:")
         self.indent()
         self.visit(node.body)
         self.dedent()
path: Cython/CodeWriter.py
text: @user1:
```suggestion
        self._visit_indented(node.body)
```

diff_hunk: @@ -241,14 +244,49 @@ def visit_FuncDefNode(self, node):
         self.startline(u"def %s(" % node.name)
         self.comma_separated_list(node.args)
         self.endline(u"):")
-        self.indent()
-        self.visit(node.body)
-        self.dedent()
+        self.visit_indented(node.body)
+
+    def visit_CFuncDefNode(self, node):
+        if node.overridable:
+            self.startline(u'cpdef ')
+        else:
+            self.startline(u'cdef ')
+        if node.modifiers:
+            self.put(' '.join(node.modifiers))
+            self.put(' ')
+        if node.visibility != 'private':
+            self.put(node.visibility)
+            self.put(u' ')
+        if node.api:
+            self.put(u'api ')
+
+        if node.base_type:
+            self.visit(node.base_type)
+            if node.base_type.name is not None:
+                self.put(u' ')
+
+        # visit the CFuncDeclaratorNode, but put a `:` at the end of line
+        self.visit(node.declarator.base)
+        self.put(u'(')
+        self.comma_separated_list(node.declarator.args)
+        self.endline(u'):')
+
+        self.visit_indented(node.body)
 
     def visit_CArgDeclNode(self, node):
-        if node.base_type.name is not None:
+        # for `CSimpleBaseTypeNode`, the variable type may been parsed as type.
+        # for other types node, its `name` is always None.
+        if not isinstance(node.base_type, CSimpleBaseTypeNode) or \
+                node.base_type.name is not None:
             self.visit(node.base_type)
-            self.put(u" ")
+
+            # once we print something for node.base_type, we may need to print
+            # an extra ` `.
+            #
+            # a special case: if `node.declarator` is a `CNameDeclaratorNode`,
+            # its "name" might be empty string, for example, for `cdef f(x)`.
path: Cython/CodeWriter.py
text: @user1:
```suggestion
            # If we printed something for "node.base_type", we may need to print an extra ' '.
            #
            # Special case: if "node.declarator" is a "CNameDeclaratorNode",
            # its "name" might be an empty string, for example, for "cdef f(x)".
```