Repository: berkerpeksag/astor

Issue:
Support for Python 3.6 Constant nodes
In Python 3.6, the new [`ast.Constant`](https://docs.python.org/3/whatsnew/3.6.html#ast) node was introduced, ostensibly for the purposes of constant folding. However, in [Python 3.8](https://docs.python.org/3.8/whatsnew/3.8.html#deprecated), it appears that it is being used to replace `ast.Str`, `ast.Bytes`, `ast.Num`, `ast.Ellipsis`, and `ast.NameConstant`. 

Since astor doesn't include a `visit_Constant` method on its `SourceGenerator`, attempting to emit code generated with `ast.Constant` causes a lot of errors.

 * CPython 3.6 [issue](https://bugs.python.org/issue26146) adding `ast.Constant`
 * CPython 3.8 [issue](https://bugs.python.org/issue32892) and [commit](https://github.com/python/cpython/pull/9445/files) switching to using `ast.Constant`



Discussion:
berkerpeksag: Thanks for the PR! It looks pretty good to me. I think your organization of the code seems reasonable. I will post my nitpicks later today :)

Perhaps we could try to generate something like `spam = 'eggs'` from an AST tree generated by Python 3.8?
chrisrink10: @berkerpeksag I added a few test cases (which are currently failing) to illustrate how I was thinking about testing this change. I'll try to look into why they are failing a little later.
chrisrink10: Alright @berkerpeksag I fixed the tests now. The only thing we might want to do is add Python 3.8-dev into the Travis test suite, but I'll leave that up to you.
berkerpeksag: Thank you very much! I think adding 3.8-dev into Travis is a good idea.
serhiy-storchaka: For most values the code is just `self.write(repr(value))`. You need to handle just few special values: `float` and `complex` numbers with `inf` and `nan` and `Ellipsis`. For some reasons strings need special handling too.
chrisrink10: Thanks for the comments @serhiy-storchaka ! I just want to clarify - do you want me to remove the private handlers and just use `self.write(repr(value))` except for numbers and strings?
serhiy-storchaka: I would write the `Constant` visitor as
```python
    def visit_Constant(self, node):
        value = node.value
        if isinstance(value, (float, complex)):
            self._handle_numeric_constant(value)
        elif isinstance(value, str):
            precedence = self.get__pp(node)
            self._handle_string_constant(value, precedence)
        else:
            self.write(repr(value))
```
and make the `NameConstant` handler using `repr()` instead of `str()` for uniformity.
berkerpeksag: Serhiy's suggestion sounds good to me.
chrisrink10: Thanks for the comments @berkerpeksag and @serhiy-storchaka . I've implemented the changes you suggested. 
chrisrink10: Thank you for reviewing again @serhiy-storchaka ! I pushed some changes which addressed your latest round of comments.
chrisrink10: Thank you @serhiy-storchaka ! I addressed your latest round of comments.
jmfrank63: I just commented over at https://github.com/berkerpeksag/astor/issues/120#issuecomment-462339851 and could need this.
I am using astor to generate code for https://github.com/python-trio/trio/pull/805
Is there an ETA this PR is getting merged?
berkerpeksag: @jmfrank63 thank you for the ping. I will try to get this PR in later this week. Sorry for the delay, @chrisrink10!
Kodiologist: @berkerpeksag Here's another poke. This seems to be a blocker for support for Python 3.8 features in Hy.
chrisrink10: @berkerpeksag I fixed the merge conflicts. It would be great if we could get this merged soon. Thanks!
chrisrink10: Just pushed the changelog update. Thank you!
berkerpeksag: Sorry, I forgot to say this earlier, but could you add your name to https://github.com/berkerpeksag/astor/blob/master/AUTHORS?
chrisrink10: Done!

Patch:
diff --git a/AUTHORS b/AUTHORS
index 80996f7..831dc6b 100644
--- a/AUTHORS
+++ b/AUTHORS
@@ -15,3 +15,4 @@ And with some modifications based on Armin's code:
 * Radomír Bosák <radomir.bosak@gmail.com>
 * Kodi Arfer <git@arfer.net>
 * Felix Yan <felixonmars@archlinux.org>
+* Chris Rink <chrisrink10@gmail.com>
diff --git a/astor/code_gen.py b/astor/code_gen.py
index 005749f..94d22ed 100644
--- a/astor/code_gen.py
+++ b/astor/code_gen.py
@@ -398,8 +398,9 @@ class SourceGenerator(ExplicitNodeVisitor):
         self.write(node.context_expr)
         self.conditional_write(' as ', node.optional_vars)
 
+    # deprecated in Python 3.8
     def visit_NameConstant(self, node):
-        self.write(str(node.value))
+        self.write(repr(node.value))
 
     def visit_Pass(self, node):
         self.statement(node, 'pass')
@@ -528,11 +529,24 @@ class SourceGenerator(ExplicitNodeVisitor):
     def visit_Name(self, node):
         self.write(node.id)
 
-    def visit_JoinedStr(self, node):
-        self.visit_Str(node, True)
+    # ast.Constant is new in Python 3.6 and it replaces ast.Bytes,
+    # ast.Ellipsis, ast.NameConstant, ast.Num, ast.Str in Python 3.8
+    def visit_Constant(self, node):
+        value = node.value
+
+        if isinstance(value, (float, complex)):
+            self._handle_numeric_constant(value)
+        elif isinstance(value, str):
+            self._handle_string_constant(node, node.value)
+        elif value is Ellipsis:
+            self.write('...')
+        else:
+            self.write(repr(value))
 
-    def visit_Str(self, node, is_joined=False):
+    def visit_JoinedStr(self, node):
+        self._handle_string_constant(node, None, is_joined=True)
 
+    def _handle_string_constant(self, node, value, is_joined=False):
         # embedded is used to control when we might want
         # to use a triple-quoted string.  We determine
         # if we are in an assignment and/or in an expression
@@ -555,8 +569,9 @@ class SourceGenerator(ExplicitNodeVisitor):
             current_line[0] = current_line[0][str_index:]
         current_line = ''.join(current_line)
 
-        if is_joined:
+        has_ast_constant = sys.version_info >= (3, 6)
 
+        if is_joined:
             # Handle new f-strings.  This is a bit complicated, because
             # the tree can contain subnodes that recurse back to JoinedStr
             # subnodes...
@@ -574,6 +589,8 @@ class SourceGenerator(ExplicitNodeVisitor):
                             if value.format_spec is not None:
                                 self.write(':')
                                 recurse(value.format_spec)
+                    elif has_ast_constant and isinstance(value, ast.Constant):
+                        self.write(value.value)
                     else:
                         kind = type(value).__name__
                         assert False, 'Invalid node %s inside JoinedStr' % kind
@@ -589,7 +606,8 @@ class SourceGenerator(ExplicitNodeVisitor):
             uni_lit = False  # No formatted byte strings
 
         else:
-            mystr = node.s
+            assert value is not None, "Node value cannot be None"
+            mystr = value
             uni_lit = self.using_unicode_literals
 
         mystr = self.pretty_string(mystr, embedded, current_line, uni_lit)
@@ -603,39 +621,51 @@ class SourceGenerator(ExplicitNodeVisitor):
         if lf:
             self.colinfo = len(result) - 1, lf
 
+    # deprecated in Python 3.8
+    def visit_Str(self, node):
+        self._handle_string_constant(node, node.s)
+
+    # deprecated in Python 3.8
     def visit_Bytes(self, node):
         self.write(repr(node.s))
 
+    def _handle_numeric_constant(self, value):
+        x = value
+
+        def part(p, imaginary):
+            # Represent infinity as 1e1000 and NaN as 1e1000-1e1000.
+            s = 'j' if imaginary else ''
+            if math.isinf(p):
+                if p < 0:
+                    return '-1e1000' + s
+                return '1e1000' + s
+            if math.isnan(p):
+                return '(1e1000%s-1e1000%s)' % (s, s)
+            return repr(p) + s
+
+        real = part(x.real if isinstance(x, complex) else x, imaginary=False)
+        if isinstance(x, complex):
+            imag = part(x.imag, imaginary=True)
+            if x.real == 0:
+                s = imag
+            elif x.imag == 0:
+                s = '(%s+0j)' % real
+            else:
+                # x has nonzero real and imaginary parts.
+                s = '(%s%s%s)' % (real, ['+', ''][imag.startswith('-')], imag)
+        else:
+            s = real
+        self.write(s)
+
     def visit_Num(self, node,
                   # constants
                   new=sys.version_info >= (3, 0)):
         with self.delimit(node) as delimiters:
-            x = node.n
-
-            def part(p, imaginary):
-                # Represent infinity as 1e1000 and NaN as 1e1000-1e1000.
-                s = 'j' if imaginary else ''
-                if math.isinf(p):
-                    if p < 0:
-                        return '-1e1000' + s
-                    return '1e1000' + s
-                if math.isnan(p):
-                    return '(1e1000%s-1e1000%s)' % (s, s)
-                return repr(p) + s
-
-            real = part(x.real if isinstance(x, complex) else x, imaginary=False)
-            if isinstance(x, complex):
-                imag = part(x.imag, imaginary=True)
-                if x.real == 0:
-                    s = imag
-                elif x.imag == 0:
-                    s = '(%s+0j)' % real
-                else:
-                    # x has nonzero real and imaginary parts.
-                    s = '(%s%s%s)' % (real, ['+', ''][imag.startswith('-')], imag)
-            else:
-                s = real
-            self.write(s)
+            self._handle_numeric_constant(node.n)
+
+            # We can leave the delimiters handling in visit_Num
+            # since this is meant to handle a Python 2.x specific
+            # issue and ast.Constant exists only in 3.6+
 
             # The Python 2.x compiler merges a unary minus
             # with a number.  This is a premature optimization
diff --git a/docs/changelog.rst b/docs/changelog.rst
index 07d5821..c03b5c3 100644
--- a/docs/changelog.rst
+++ b/docs/changelog.rst
@@ -8,7 +8,12 @@ Release Notes
 New features
 ~~~~~~~~~~~~
 
-*
+* Support ``ast.Constant`` nodes being emitted by Python 3.8 (and initially
+  created in Python 3.6).
+  (Reported and fixed by Chris Rink in `Issue 120`_ and `PR 121`_.)
+
+.. _`Issue 120`: https://github.com/berkerpeksag/astor/issues/120
+.. _`PR 121`: https://github.com/berkerpeksag/astor/pull/121
 
 Bug fixes
 ~~~~~~~~~
diff --git a/tox.ini b/tox.ini
index 5f6ddf0..c3c02b6 100644
--- a/tox.ini
+++ b/tox.ini
@@ -1,5 +1,5 @@
 [tox]
-envlist = py27, py34, py35, py36, pypy, pypy3.5
+envlist = py27, py34, py35, py36, py37, py38, pypy, pypy3.5
 skipsdist = True
 
 [testenv]


Test patch:
diff --git a/tests/test_code_gen.py b/tests/test_code_gen.py
index fc11b75..f050e10 100644
--- a/tests/test_code_gen.py
+++ b/tests/test_code_gen.py
@@ -38,6 +38,9 @@ class Comparisons(object):
         dmp2 = astor.dump_tree(ast2)
         self.assertEqual(dmp1, dmp2)
 
+    def assertAstEqualsSource(self, tree, source):
+        self.assertEqual(self.to_source(tree).rstrip(), source)
+
     def assertAstRoundtrips(self, srctxt):
         """This asserts that the reconstituted source
            code can be compiled into the exact same AST
@@ -442,6 +445,105 @@ class CodegenTestCase(unittest.TestCase, Comparisons):
         """
         self.assertSrcRoundtripsGtVer(source, (3, 6))
 
+    @unittest.skipUnless(sys.version_info <= (3, 3),
+                         "ast.Name used for True, False, None until Python 3.4")
+    def test_deprecated_constants_as_name(self):
+        self.assertAstEqualsSource(
+            ast.Assign(targets=[ast.Name(id='spam')], value=ast.Name(id='True')),
+            "spam = True")
+
+        self.assertAstEqualsSource(
+            ast.Assign(targets=[ast.Name(id='spam')], value=ast.Name(id='False')),
+            "spam = False")
+
+        self.assertAstEqualsSource(
+            ast.Assign(targets=[ast.Name(id='spam')], value=ast.Name(id='None')),
+            "spam = None")
+
+    @unittest.skipUnless(sys.version_info >= (3, 4),
+                         "ast.NameConstant introduced in Python 3.4")
+    def test_deprecated_name_constants(self):
+        self.assertAstEqualsSource(
+            ast.Assign(targets=[ast.Name(id='spam')], value=ast.NameConstant(value=True)),
+            "spam = True")
+
+        self.assertAstEqualsSource(
+            ast.Assign(targets=[ast.Name(id='spam')], value=ast.NameConstant(value=False)),
+            "spam = False")
+
+        self.assertAstEqualsSource(
+            ast.Assign(targets=[ast.Name(id='spam')], value=ast.NameConstant(value=None)),
+            "spam = None")
+
+    def test_deprecated_constant_nodes(self):
+        self.assertAstEqualsSource(
+            ast.Assign(targets=[ast.Name(id='spam')], value=ast.Num(3)),
+            "spam = 3")
+
+        self.assertAstEqualsSource(
+            ast.Assign(targets=[ast.Name(id='spam')], value=ast.Num(-93)),
+            "spam = -93")
+
+        self.assertAstEqualsSource(
+            ast.Assign(targets=[ast.Name(id='spam')], value=ast.Num(837.3888)),
+            "spam = 837.3888")
+
+        self.assertAstEqualsSource(
+            ast.Assign(targets=[ast.Name(id='spam')], value=ast.Num(-0.9877)),
+            "spam = -0.9877")
+
+        self.assertAstEqualsSource(ast.Ellipsis(), "...")
+
+        if sys.version_info >= (3, 0):
+            self.assertAstEqualsSource(
+                ast.Assign(targets=[ast.Name(id='spam')], value=ast.Bytes(b"Bytes")),
+                "spam = b'Bytes'")
+
+        self.assertAstEqualsSource(
+            ast.Assign(targets=[ast.Name(id='spam')], value=ast.Str("String")),
+            "spam = 'String'")
+
+    @unittest.skipUnless(sys.version_info >= (3, 6),
+                         "ast.Constant introduced in Python 3.6")
+    def test_constant_nodes(self):
+        self.assertAstEqualsSource(
+            ast.Assign(targets=[ast.Name(id='spam')], value=ast.Constant(value=3)),
+            "spam = 3")
+
+        self.assertAstEqualsSource(
+            ast.Assign(targets=[ast.Name(id='spam')], value=ast.Constant(value=-93)),
+            "spam = -93")
+
+        self.assertAstEqualsSource(
+            ast.Assign(targets=[ast.Name(id='spam')], value=ast.Constant(value=837.3888)),
+            "spam = 837.3888")
+
+        self.assertAstEqualsSource(
+            ast.Assign(targets=[ast.Name(id='spam')], value=ast.Constant(value=-0.9877)),
+            "spam = -0.9877")
+
+        self.assertAstEqualsSource(
+            ast.Assign(targets=[ast.Name(id='spam')], value=ast.Constant(value=True)),
+            "spam = True")
+
+        self.assertAstEqualsSource(
+            ast.Assign(targets=[ast.Name(id='spam')], value=ast.Constant(value=False)),
+            "spam = False")
+
+        self.assertAstEqualsSource(
+            ast.Assign(targets=[ast.Name(id='spam')], value=ast.Constant(value=None)),
+            "spam = None")
+
+        self.assertAstEqualsSource(ast.Constant(value=Ellipsis), "...")
+
+        self.assertAstEqualsSource(
+            ast.Assign(targets=[ast.Name(id='spam')], value=ast.Constant(b"Bytes")),
+            "spam = b'Bytes'")
+
+        self.assertAstEqualsSource(
+            ast.Assign(targets=[ast.Name(id='spam')], value=ast.Constant(value="String")),
+            "spam = 'String'")
+
     def test_annassign(self):
         source = """
             a: int