Conversion failing with Python 3.9

[cfelton]

Python 3.9 was added to the travis regression, majority of the regression tests failed:
https://travis-ci.org/github/myhdl/myhdl/jobs/747059472

System information

MyHDL Version: 0.11
Python Version: 3.9

[hellow554]

I'm ran into this today: maybe emitting a warning, when using python 3.9 that it is not usable yet (or fix the regression fast ;) )

[hellow554]

Python slice ast has been changed:

https://docs.python.org/3.9/library/ast.html#ast.AST

Changed in version 3.9: Simple indices are represented by their value, extended slices are represented as tuples.

Deprecated since version 3.9: Old classes ast.Index and ast.ExtSlice are still available, but they will be removed in future Python releases. In the meantime, instantiating them will return an instance of a different class.

This means, that the AST has been changed:

slice=Index(
    value=Name(lineno=9, col_offset=13, end_lineno=9, end_col_offset=16, id='key', ctx=Load()),
),

slice=Name(lineno=9, col_offset=13, end_lineno=9, end_col_offset=16, id='key', ctx=Load()),

should be fairly easy to remove the Index thing, right?

[hellow554]

I'm not entirely sure if this is correct for all cases, but it works for simple cases, e.g. the stopwatch in the cookbook:

diff --git a/myhdl/conversion/_analyze.py b/myhdl/conversion/_analyze.py
index 9ad1111..9adcb08 100644
--- a/myhdl/conversion/_analyze.py
+++ b/myhdl/conversion/_analyze.py
@@ -976,7 +976,10 @@ class _AnalyzeVisitor(ast.NodeVisitor, _ConversionMixin):
     def accessIndex(self, node):
         self.visit(node.value)
         self.access = _access.INPUT
-        self.visit(node.slice.value)
+        if hasattr(node.slice, 'value'):
+            self.visit(node.slice.value)
+        else:
+            self.visit(node.slice)
         if isinstance(node.value.obj, _Ram):
             if isinstance(node.ctx, ast.Store):
                 self.raiseError(node, _error.ListElementAssign)
diff --git a/myhdl/conversion/_toVHDL.py b/myhdl/conversion/_toVHDL.py
index 7942963..f2011b2 100644
--- a/myhdl/conversion/_toVHDL.py
+++ b/myhdl/conversion/_toVHDL.py
@@ -986,7 +986,8 @@ class _ConvertVisitor(ast.NodeVisitor, _ConversionMixin):
         rhs = node.value
         # shortcut for expansion of ROM in case statement
         if isinstance(node.value, ast.Subscript) and \
-                isinstance(node.value.slice, ast.Index) and \
+                (isinstance(node.value.slice, ast.Index) or \
+                 isinstance(node.value.slice, ast.Call)) and \
                 isinstance(node.value.value.obj, _Rom):
             rom = node.value.value.obj.rom
             self.write("case ")
@@ -2391,7 +2392,10 @@ class _AnnotateTypesVisitor(ast.NodeVisitor, _ConversionMixin):
     def accessIndex(self, node):
         self.generic_visit(node)
         node.vhd = vhd_std_logic()  # XXX default
-        node.slice.value.vhd = vhd_int()
+        if hasattr(node.slice, 'value'):
+            node.slice.value.vhd = vhd_int()
+        else:
+            node.slice.vhd = vhd_int()
         obj = node.value.obj
         if isinstance(obj, list):
             assert len(obj)
diff --git a/myhdl/conversion/_toVerilog.py b/myhdl/conversion/_toVerilog.py
index 2f07fc8..060fb15 100644
--- a/myhdl/conversion/_toVerilog.py
+++ b/myhdl/conversion/_toVerilog.py
@@ -749,8 +749,10 @@ class _ConvertVisitor(ast.NodeVisitor, _ConversionMixin):

     def visit_Assign(self, node):
         # shortcut for expansion of ROM in case statement
+        print(f"{type(node.value.slice)}")
         if isinstance(node.value, ast.Subscript) and \
-                isinstance(node.value.slice, ast.Index) and\
+                (isinstance(node.value.slice, ast.Index) or\
+                 isinstance(node.value.slice, ast.Call)) and\
                 isinstance(node.value.value.obj, _Rom):
             rom = node.value.value.obj.rom
 #            self.write("// synthesis parallel_case full_case")

[cfelton]

@hellow554 thanks for the information on the 3.9 changes. I know it has been awhile, we will try and review this as soon as possible.

@hgomersall , @josyb

[josyb]

IMO Python version dependencies should be handled by checking the Python version rather than checking for the effect that an attribute is or isn't available.

[hgomersall]

I agree. I think this should be done with a version check. Probably just define 2 versions of the same check function depending on the python version...

[josyb]

We used to have a global variable PY2 in _compat.py, but as we now only support Python3 that has gone; I suggest we create a new one in __init__.py: PYVERSION = sys_version_info()[0] + sys_version_info()[1] * 0.1 and then we can check:

from myhdl import PYVERSION

if PYVERSION >= 3.9:
    action39()

(Uppercase because it is treated as a constant)
Oops it will break with Python 3.10 ... just a little bit more work :)

[josyb]

This is built-in and easy:
if sys.version_info >= (3, 9, 0): # Python 3.9+: no ast.Index wrapper

Actually the ast has constantly moving goalposts ...
E.g. 3.8 deprecates ast.Num, ast.Str, ast.NameConstant (and ast.Bytes and ast.Ellipsis which we don't support) and promotes ast.Constant to handle all these
And 3.9 added ast.Index and ast.ExtSlice (which we do not support) to the list.

C:\Programs\WPy64-3720\python-3.7.2.amd64\python.exe 3.7.2 (tags/v3.7.2:9a3ffc0492, Dec 23 2018, 23:09:28) [MSC v.1916 64 bit (AMD64)]
import ast
print(ast.dump(ast.parse('l[i]', mode='eval')))
Expression(body=Subscript(value=Name(id='l', ctx=Load()), slice=Index(value=Name(id='i', ctx=Load())), ctx=Load()))
print(ast.dump(ast.parse('l[i:j]', mode='eval')))
Expression(body=Subscript(value=Name(id='l', ctx=Load()), slice=Slice(lower=Name(id='i', ctx=Load()), upper=Name(id='j', ctx=Load()), step=None), ctx=Load()))
print(ast.dump(ast.parse('l[1]', mode='eval')))
Expression(body=Subscript(value=Name(id='l', ctx=Load()), slice=Index(value=Num(n=1)), ctx=Load()))

C:\Programs\WPy64-3910\python-3.9.1.amd64\python.exe 3.9.1 (tags/v3.9.1:1e5d33e, Dec  7 2020, 17:08:21) [MSC v.1927 64 bit (AMD64)]
import ast
print(ast.dump(ast.parse('l[i]', mode='eval')))
Expression(body=Subscript(value=Name(id='l', ctx=Load()), slice=Name(id='i', ctx=Load()), ctx=Load()))
print(ast.dump(ast.parse('l[1]', mode='eval')))
Expression(body=Subscript(value=Name(id='l', ctx=Load()), slice=Constant(value=1), ctx=Load()))
print(ast.dump(ast.parse('l[i:j]', mode='eval')))
Expression(body=Subscript(value=Name(id='l', ctx=Load()), slice=Slice(lower=Name(id='i', ctx=Load()), upper=Name(id='j', ctx=Load())), ctx=Load()))

[jck]

Maybe we could add a step which runs on older python versions which modifies the AST to what we expect it to look on 39:

for example:

class RewriteNum(ast.NodeTransformer):
    def visit_Num(self, node):
        # Deprecated since version 3.8: Replaced by Constant
        # https://greentreesnakes.readthedocs.io/en/latest/nodes.html#Num
        return ast.copy_location(ast.Subscript(
            value=ast.Constant(value=node.value, ctx=ast.Load()),
            ctx=node.ctx
        ), node)

if PY38:
    tree = RewriteNum().visit(tree)

This would allow us to keep the compatibility related stuff outside the main ast processing code and hopefully make it easier for us to maintain?

[josyb]

I guess it is 'six of the one and half a dozen of the other'

[josyb]

IMO The test if PY38: is not even necessary; we can always all on RewriteConstant() (with additonal entries for Str and NameConstant) as they will never be called upon in Python3.9+ (which means I could do this in #358 as well, but if we rewrite the ast we can effectively simplify that code as you indicate)

You specify ast.Subscript; but shouldn't we rewrite all occurrences?

[jck]

IMO The test if PY38: is not even necessary; we can always all on RewriteConstant() (with additonal entries for Str and NameConstant) as they will never be called upon in Python3.9+ (which means I could do this in #358 as well, but if we rewrite the ast we can effectively simplify that code as you indicate)

correct. my rewriting example was adapted from https://greentreesnakes.readthedocs.io/en/latest/manipulating.html#modifying-the-tree and i neglected to remove the ast.Subscript part.

[jck]

IMO The test if PY38: is not even necessary;

It is not necessary for the Num/Constant case, but I think it would be needed for the slices case?

[josyb]

It is not necessary for the Num/Constant case, but I think it would be needed for the slices case?

You are quite right, the conversion of slices need the test.

[josyb]

@jck Can you commit PR #358?
You can always introduce the ast.rewrite later, or better add it to #358

[jck]

@josyb sounds good. gimme a day to review it.

[hgomersall]

This looks fixed to me now. Can we close?