Currently, ast.literal_eval uses a long if/elif chain to implement the limited parsing of the AST. This issue proposes changing the implementation to make it easy to extend to cover some of the implementation details of PEP 436 (Argument Clinic).

Interesting patch. :)

Heh, that's what I get for wanting to go get dinner. I'll regenerate it in
the morning.

Updated patch attached. The complexity grew a bit, as it turned out to be useful for the converter functions to be able to tell the difference between top level conversions and recursive invocations. I also realised that:

- the complex number conversion currently allows arbitrarily deep nesting of addition and subtraction
- handling this properly made an exception based "Failed to convert" API far more sensible

The docstrings are updated, but ast.convert_node is tested only indirectly through the ast.literal_eval tests and the main docs haven't been updated at all.

At this point, I'm most interested in feedback from Larry as to whether or not this is helpful to him in implementing argument clinic.

Now that I've actually implemented it, I'm fairly sure it's not a good idea. It might be better if we remove the recursive behaviour.

As it is, Larry's fairly happy with what he is already doing (calling ast.parse and walking the tree) and I think that's actually simpler than using this API would be.

FWIW, I like the current version of the code and find it easily to understand what it does and how it does it.   The new code is longer and full of indirections.   I question whether it is a real improvement.

For what it's worth, here's what we're doing.  The new Argument Clinic "parameter" line looks a great deal like the middle bit of a Python function declaration.  So much so that we can do this:

    ast_string = "def x({}): pass".format(parameter_line)
    root = ast.parse(ast_string)

We then walk the node tree and pull out what we need.  We do a similar trick to parse the return converter, which comes from another line.  ("def x() {}: pass")

The "converter function" can look like a literal string, an id, or a function call.  When we see the function call form, we pull out each keyword and its value, then pass the value node to ast.literal_eval().  Easy peasy and Bob's your uncle.

I don't think we should change the implementation, but I also discovered several holes in the test suite coverage while refactoring it. We should pursue some of the test suite changes, including switching them over to be data driven and take advantage of the new subtest support.

> I also discovered several holes in the test suite coverage while refactoring it.

Are they all tested in the current diff?

I don't know if the suggested test suite diff gives full coverage, but unless other improvements have been made to those tests in the meantime, it should give more coverage than we have right now.

I agreed with Raymond, the current version of the code looks clearer.

Right, the only reason this is still open is because I thought the extra test cases might be interesting in their own right (just not interesting enough to sit down and apply).

Nick, do you mind to create a PR for new tests?

Some additional tests were added since this issue was created.

Belatedly removing the issue assignment here, as I'm not actively working on this.

I've also marked this as an easy newcomer friendly task, as all that's involved is taking the `test_ast.py` changes from https://bugs.python.org/file29520/issue17490_ast_literal_eval_converters.diff and turning them into a test suite PR on GitHub.

Nick, hello! I'd like to take it onwards from here.

Some of the test cases from Nick's patch are not passing on master:

    ast.literal_eval('')  # raises SyntaxError; expected: ValueError
    ast.literal_eval('6j--3')  # expected: 3+6j
    ast.literal_eval('(2j+4j)+(1+2)')  # expected: 3+6j
    ast.literal_eval('(2j+4j)-(1+2)')  # expected: -3+6j

I'm assuming that new changes in ast.py do not allow for these. Can anyone confirm that this is indeed the case, and not a bug with literal_eval?

I think this is a bug in literal_eval:

>>> 3+6j
(3+6j)
>>> 6j+3
(3+6j)
>>> ast.literal_eval('3+6j')
(3+6j)
>>> ast.literal_eval('6j+3')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "C:\Users\User\src\cpython\lib\ast.py", line 105, in literal_eval
    return _convert(node_or_string)
  File "C:\Users\User\src\cpython\lib\ast.py", line 104, in _convert
    return _convert_signed_num(node)
  File "C:\Users\User\src\cpython\lib\ast.py", line 78, in _convert_signed_num
    return _convert_num(node)
  File "C:\Users\User\src\cpython\lib\ast.py", line 69, in _convert_num
    _raise_malformed_node(node)
  File "C:\Users\User\src\cpython\lib\ast.py", line 66, in _raise_malformed_node
    raise ValueError(f'malformed node or string: {node!r}')
ValueError: malformed node or string: <ast.BinOp object at 0x023D0910>


literal_eval accepts complex rhs, but not lhs: 
https://github.com/python/cpython/blame/master/Lib/ast.py#L99
if isinstance(left, (int, float)) and isinstance(right, complex):

This was introduced here, I'm not sure why:
https://github.com/python/cpython/commit/d8ac4d1d5ac256ebf3d8d38c226049abec82a2a0


In Nick's aborted patch, he removed that check.

I just noticed that the patch by Serhiy has unit tests for this case, expecting ValueError. So this is apprently a feature and not a bug.

literal_eval() is not purposed to evaluate arbitrary arithmetic expressions. It is only purposed to handle strings produced by repr() of some simple builtin objects. repr(6j+3) is '(3+6j)', not '(6j+3)' and not '(6j--3)', so it accepts the former form and not the latters.

If Nick no longer works on this I propose to close this issue. literal_eval() supports now more types of expressions, but it is more strict in other cases. I have doubts that any test changes are applicable to the current code.

I agree that it's better to just close this issue, the patch is 7 years old.

Hm, I don't think PR 22469 is relevant to this issue?