Improve ast.literal_eval test suite coverage

[ncoghlan]

BPO	17490
Nosy	@gvanrossum, @rhettinger, @ncoghlan, @larryhastings, @benjaminp, @serhiy-storchaka, @JulienPalard, @RJ722, @isidentical, @iritkatriel
Files	issue17490_ast_literal_eval_converters.diff: Conversion function based extensible ast.literal_eval

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2020-10-02.06:57:20.158>
created_at = <Date 2013-03-20.02:34:35.293>
labels = ['easy', 'type-feature']
title = 'Improve ast.literal_eval test suite coverage'
updated_at = <Date 2020-10-02.06:57:20.158>
user = 'https://github.com/ncoghlan'

bugs.python.org fields:

activity = <Date 2020-10-02.06:57:20.158>
actor = 'serhiy.storchaka'
assignee = 'none'
closed = True
closed_date = <Date 2020-10-02.06:57:20.158>
closer = 'serhiy.storchaka'
components = []
creation = <Date 2013-03-20.02:34:35.293>
creator = 'ncoghlan'
dependencies = []
files = ['29520']
hgrepos = []
issue_num = 17490
keywords = ['patch', 'easy', 'newcomer friendly']
message_count = 21.0
messages = ['184722', '184733', '184736', '184826', '184827', '184839', '184842', '184873', '282113', '282132', '282160', '282164', '330200', '372500', '372507', '372512', '377560', '377562', '377571', '377785', '377788']
nosy_count = 10.0
nosy_names = ['gvanrossum', 'rhettinger', 'ncoghlan', 'larry', 'benjamin.peterson', 'serhiy.storchaka', 'mdk', 'RJ722', 'BTaskaya', 'iritkatriel']
pr_nums = []
priority = 'normal'
resolution = 'rejected'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue17490'
versions = []

[ncoghlan]

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).

[benjaminp]

Interesting patch. :)

[ncoghlan]

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

[ncoghlan]

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.

[ncoghlan]

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.

[rhettinger]

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.

[larryhastings]

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.

[ncoghlan]

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.

[JulienPalard]

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

Are they all tested in the current diff?

[ncoghlan]

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.

[serhiy-storchaka]

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

[ncoghlan]

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).

[serhiy-storchaka]

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

Some additional tests were added since this issue was created.