[issue31753] Unnecessary closure in ast.literal_eval

[Aaron Hall]

Aaron Hall <aaronchall at yahoo.com> added the comment:

Static analysis:

My mental model currently says the rebuilt function every outer call is an expense with no offsetting benefit. It seems that a function shouldn't build a closure on every call if the closure doesn't close over anything immediately used by the functionality. But I can't explain why the cost doesn't amortize toward zero in my testing.

Usage analysis:

On the other hand, this doesn't seem used very much at all in the std lib.
I'm not sure what the entire global benefit is to moving the closure to be a global instead - but there are about 88000 potential uses of the code on github: https://github.com/search?p=3&q=literal_eval&type=Code&utf8=%E2%9C%93 One use seems to be scanning Python code - so potentially it gets a lot of use?

Alternatively: - to echo Serhiy ("Maybe it is worth to spend some time for optimizing closure creation."), perhaps the matter could be made irrelevant by looking at how we handle closures. I'm not sure why the difference didn't amortize to nearly nothing in my testing - I used Anaconda's Python 3.6.1 distribution on Linux - if that matters.

Potential improvement:

So to be clear, the suggested change would probably be to move _convert to a global, maybe named _literal_eval_convert (this is less half-baked than my first code post, which I somewhat regret. Note that the recursive calls would need to be edited as well as the move and dedent.):


def literal_eval(node_or_string):
    """
    Safely evaluate an expression node or a string containing a Python
    expression.  The string or node provided may only consist of the following
    Python literal structures: strings, bytes, numbers, tuples, lists, dicts,
    sets, booleans, and None.
    """
    if isinstance(node_or_string, str):
        node_or_string = parse(node_or_string, mode='eval')
    if isinstance(node_or_string, Expression):
        node_or_string = node_or_string.body
    return _literal_eval_convert(node_or_string)


def _literal_eval_convert(node):
    if isinstance(node, Constant):
        return node.value
    elif isinstance(node, (Str, Bytes)):
        return node.s
    elif isinstance(node, Num):
        return node.n
    elif isinstance(node, Tuple):
        return tuple(map(_literal_eval_convert, node.elts))
    elif isinstance(node, List):
        return list(map(_literal_eval_convert, node.elts))
    elif isinstance(node, Set):
        return set(map(_literal_eval_convert, node.elts))
    elif isinstance(node, Dict):
        return dict((_literal_eval_convert(k), _literal_eval_convert(v)) for k, v
                    in zip(node.keys, node.values))
    elif isinstance(node, NameConstant):
        return node.value
    elif isinstance(node, UnaryOp) and isinstance(node.op, (UAdd, USub)):
        operand = _literal_eval_convert(node.operand)
        if isinstance(operand, _NUM_TYPES):
            if isinstance(node.op, UAdd):
                return + operand
            else:
                return - operand
    elif isinstance(node, BinOp) and isinstance(node.op, (Add, Sub)):
        left = _literal_eval_convert(node.left)
        right = _literal_eval_convert(node.right)
        if isinstance(left, _NUM_TYPES) and isinstance(right, _NUM_TYPES):
            if isinstance(node.op, Add):
                return left + right
            else:
                return left - right
    raise ValueError('malformed node or string: ' + repr(node))

Note that I am not strongly committed to this issue, and won't feel badly if it is closed. It just seemed to be some low-hanging fruit in the standard library that I happened across.

----------

_______________________________________
Python tracker <report at bugs.python.org>
<https://bugs.python.org/issue31753>
_______________________________________