This issue tracker has been migrated to GitHub, and is currently read-only.
For more information, see the GitHub FAQs in the Python's Developer Guide.

Title: Improve ast.literal_eval test suite coverage
Type: enhancement Stage: resolved
Components: Versions:
Status: closed Resolution: rejected
Dependencies: Superseder:
Assigned To: Nosy List: BTaskaya, RJ722, benjamin.peterson, gvanrossum, iritkatriel, larry, mdk, ncoghlan, rhettinger, serhiy.storchaka
Priority: normal Keywords: easy, newcomer friendly, patch

Created on 2013-03-20 02:34 by ncoghlan, last changed 2022-04-11 14:57 by admin. This issue is now closed.

File name Uploaded Description Edit
issue17490_ast_literal_eval_converters.diff ncoghlan, 2013-03-21 01:44 Conversion function based extensible ast.literal_eval review
Messages (21)
msg184722 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2013-03-20 02:34
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).
msg184733 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2013-03-20 04:17
Interesting patch. :)
msg184736 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2013-03-20 04:24
Heh, that's what I get for wanting to go get dinner. I'll regenerate it in
the morning.
msg184826 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2013-03-21 01:44
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.
msg184827 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2013-03-21 01:56
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.
msg184839 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2013-03-21 03:19
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.
msg184842 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2013-03-21 04:45
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.
msg184873 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2013-03-21 15:02
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.
msg282113 - (view) Author: Julien Palard (mdk) * (Python committer) Date: 2016-11-30 23:18
> I also discovered several holes in the test suite coverage while refactoring it.

Are they all tested in the current diff?
msg282132 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2016-12-01 03:51
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.
msg282160 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-12-01 12:06
I agreed with Raymond, the current version of the code looks clearer.
msg282164 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2016-12-01 12:21
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).
msg330200 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-11-21 13:06
Nick, do you mind to create a PR for new tests?

Some additional tests were added since this issue was created.
msg372500 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2020-06-28 07:15
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 `` changes from and turning them into a test suite PR on GitHub.
msg372507 - (view) Author: Rahul Jha (RJ722) * Date: 2020-06-28 09:35
Nick, hello! I'd like to take it onwards from here.
msg372512 - (view) Author: Rahul Jha (RJ722) * Date: 2020-06-28 11:01
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 do not allow for these. Can anyone confirm that this is indeed the case, and not a bug with literal_eval?
msg377560 - (view) Author: Irit Katriel (iritkatriel) * (Python committer) Date: 2020-09-27 12:10
I think this is a bug in literal_eval:

>>> 3+6j
>>> 6j+3
>>> ast.literal_eval('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\", line 105, in literal_eval
    return _convert(node_or_string)
  File "C:\Users\User\src\cpython\lib\", line 104, in _convert
    return _convert_signed_num(node)
  File "C:\Users\User\src\cpython\lib\", line 78, in _convert_signed_num
    return _convert_num(node)
  File "C:\Users\User\src\cpython\lib\", line 69, in _convert_num
  File "C:\Users\User\src\cpython\lib\", 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:
if isinstance(left, (int, float)) and isinstance(right, complex):

This was introduced here, I'm not sure why:

In Nick's aborted patch, he removed that check.
msg377562 - (view) Author: Irit Katriel (iritkatriel) * (Python committer) Date: 2020-09-27 12:15
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.
msg377571 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2020-09-27 14:24
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.
msg377785 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2020-10-01 23:08
I agree that it's better to just close this issue, the patch is 7 years old.
msg377788 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2020-10-01 23:26
Hm, I don't think PR 22469 is relevant to this issue?
Date User Action Args
2022-04-11 14:57:43adminsetgithub: 61692
2020-10-02 06:57:20serhiy.storchakasetstatus: open -> closed
resolution: rejected
stage: patch review -> resolved
2020-10-02 06:56:53serhiy.storchakasetpull_requests: - pull_request21504
2020-10-01 23:26:19gvanrossumsetmessages: + msg377788
2020-10-01 23:10:22BTaskayasetkeywords: + patch
stage: needs patch -> patch review
pull_requests: + pull_request21504
2020-10-01 23:08:20gvanrossumsetnosy: + gvanrossum
messages: + msg377785
2020-09-27 14:24:43serhiy.storchakasetmessages: + msg377571
2020-09-27 12:15:23iritkatrielsetmessages: + msg377562
2020-09-27 12:10:40iritkatrielsetnosy: + iritkatriel
messages: + msg377560
2020-07-10 19:19:04BTaskayasetnosy: + BTaskaya
2020-06-28 11:01:34RJ722setmessages: + msg372512
2020-06-28 09:35:18RJ722setnosy: + RJ722
messages: + msg372507
2020-06-28 07:15:41ncoghlansetkeywords: + easy, newcomer friendly, - patch
assignee: ncoghlan ->
messages: + msg372500

stage: patch review -> needs patch
2018-11-22 13:00:10ncoghlansetassignee: ncoghlan
2018-11-21 13:06:36serhiy.storchakasetmessages: + msg330200
2016-12-01 12:21:42ncoghlansetmessages: + msg282164
2016-12-01 12:06:04serhiy.storchakasetnosy: + serhiy.storchaka
messages: + msg282160
2016-12-01 03:51:36ncoghlansetmessages: + msg282132
2016-11-30 23:18:41mdksetnosy: + mdk
messages: + msg282113
2013-03-21 15:02:10ncoghlansetmessages: + msg184873
title: Use converter functions to implement ast.literal_eval -> Improve ast.literal_eval test suite coverage
2013-03-21 04:45:40larrysetmessages: + msg184842
2013-03-21 03:19:20rhettingersetnosy: + rhettinger
messages: + msg184839
2013-03-21 01:56:31ncoghlansetmessages: + msg184827
2013-03-21 01:44:26ncoghlansetfiles: - ast_literal_eval_converter_functions.diff
2013-03-21 01:44:04ncoghlansetfiles: + issue17490_ast_literal_eval_converters.diff

messages: + msg184826
2013-03-20 04:24:51ncoghlansetmessages: + msg184736
2013-03-20 04:17:43benjamin.petersonsetnosy: + benjamin.peterson
messages: + msg184733
2013-03-20 02:35:01ncoghlansetnosy: + larry
2013-03-20 02:34:35ncoghlancreate