Repository navigation
Only plain literals should be parsed #97
Description
Activity
First, something that confuses me:
The literal_eval documentation says it is not capable of evaluating arbitrarily complex expressions, for example involving operators or indexing.This made me wonder, why does
ast.literal_eval("1+1")evaluate to 2? A: It doesn't (in Python 2).
So why doesDefaultParseValue('[1+1]')give [2]? A: It doesn't (in Python 2).
Turns out this is a Python 2 vs Python 3 distinction.ast.literal_eval("1+1")does give 2 in Python 3 :(.So if we can get a port of Python 2's literal_eval into Python 3, I think that resolves our issues and we can remove the BinOp special case altogether. I haven't investigated whether this exists yet.
Aside:
Something to keep in the back of your mind as you think about this: it would be amazing if there were a standard way of parsing literals from the command line that was consistent across languages. This way if someone were to implement, say, Java Fire, and if someone were to implement piping args from the command line into Fire CLIs, then Fire could become a mechanism for interoperating across languages.The Python 3 documentation for literal_eval also suggests it won't parse expressions with operators.
The discussion at https://bugs.python.org/issue22525 suggests the reason that + and - are supported is to support complex literals like 4+2i. I don't think that's sufficient justification for considering "1+1" a literal; from my understanding, "1+1" is not a literal and so I'm inclined to think this is a Python 3 bug.Opened issue in Python bug tracker at https://bugs.python.org/issue31778
Even if this issue with Python is fixed, we'll still want Fire to work with builds of Python that have the current behavior. So, we should walk the AST and check for BinOps ourself, and raise a ValueError if there is a BinOp in our AST.
The ast.literal_eval code (Python 2.7.3 and Python 3) may be useful for reference.
The next steps here are to check for BinOps in the ast of the input. This will happen here:
Lines 97 to 98 in 72604f4
If there are any BinOps in the ast (this requires traversing the ast to determine), then we will raise a ValueError as above, rather than parsing this non-literal. The only exception to this is that the BinOps + and - used in the context of expressing a complex number (e.g. 2 + 3j) are allowed.
The Python 2.7.3 implementation of literal_eval demonstrates how we can do this AST walk to determine if any non-allowed BinOps are present.
Looks like I've got some spare time in the next week, I'll give this one a whirl.
Hey Alex, keep us posted if you try anything.
Hi, can I have a look at this issue?
Yes! Sorry for the long radio silence, I should've commented earlier that I was no longer working on this one.
I plan to address the remaining top-level complex-number inconsistency: DefaultParseValue("1+2j") returns a string, while DefaultParseValue("[1+2j]") returns a complex value in a list. The supported Python versions already reject ordinary arithmetic in ast.literal_eval. I will add regression coverage for complex values, nested containers, and non-literal expressions before submitting. This investigation and comment are assisted by OpenAI Codex.
@dbieber correctly pointed out in #95 that there is a more general problem with the parsing code. This is a follow up issue to that issue which only dealt with the most common case of a single argument.
Here are some cases demonstrating
Some options are:
raise ValueErrorin ``fire.parser._LiteralEval`For 2. and 3. I think these would be the valid types from the
astmodule.StrName(converted toStr)NumListTupleDictSetUnaryOp