Skip to content

Reject non-literal binary operations at any AST depth #715

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Popy21 wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Reject non-literal binary operations at any AST depth #715

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e98ad59
Reject non-literal binary operations at any AST depth
Popy21 Oct 16, 2026
fa6ace5
Check binary operations during the bare-word replacement walk
Popy21 Nov 6, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 35 additions & 6 deletions fire/parser.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,9 +79,10 @@ def DefaultParseValue(value):
def _LiteralEval(value):
"""Parse value as a Python literal, or container of containers and literals.

First the AST of the value is updated so that bare-words are turned into
strings. Then the resulting AST is evaluated as a literal or container of
only containers and literals.
First the AST of the value is checked for binary operations, which are not
literals, and updated so that bare-words are turned into strings, in a single
walk. Then the resulting AST is evaluated as a literal or container of only
containers and literals.

This allows for the YAML-like syntax {a: b} to represent the dict {'a': 'b'}

Expand All @@ -96,10 +97,14 @@ def _LiteralEval(value):
SyntaxError: If the value string has a syntax error.
"""
root = ast.parse(value, mode='eval')
if isinstance(root.body, ast.BinOp):
raise ValueError(value)

for node in ast.walk(root):
# Binary operations are not literals, so they are rejected wherever they
# appear. Note that some versions of ast.literal_eval evaluate them
# instead, e.g. turning '[1+1]' into [2]. The only exception is the + or -
# used to write a complex number, which is a literal.
if isinstance(node, ast.BinOp) and not _IsComplexLiteral(node):
raise ValueError(value)

for field, child in ast.iter_fields(node):
if isinstance(child, list):
for index, subchild in enumerate(child):
Expand All @@ -116,6 +121,30 @@ def _LiteralEval(value):
return ast.literal_eval(root)


def _IsComplexLiteral(node):
"""Returns whether a binary operation expresses a complex number literal.

Python has no single-token syntax for a complex number with both a real and
an imaginary part, so such numbers are written as the sum or difference of a
real number and an imaginary one, e.g. 1+2j. These are the only binary
operations ast.literal_eval accepts, so they are the only ones Fire accepts.

Args:
node: A node of type BinOp.
Returns:
True if the node has the form <real number> + <imaginary number>, with
either + or -, False otherwise.
"""
if not isinstance(node.op, (ast.Add, ast.Sub)):
return False
try:
real = ast.literal_eval(node.left)
imaginary = ast.literal_eval(node.right)
except (TypeError, ValueError):
return False
return isinstance(real, (int, float)) and isinstance(imaginary, complex)
Comment on lines +124 to +145

@gemini-code-assist gemini-code-assist Bot Oct 16, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The current implementation of _IsComplexLiteral has two issues:

  1. Performance Bottleneck: It calls ast.literal_eval on node.left and node.right without checking their types first. If either operand is a large container (e.g., a list or dictionary with thousands of elements), ast.literal_eval will fully evaluate it, leading to unnecessary overhead and potential performance degradation.
  2. Incorrect Ordering Restriction: It assumes the real part must always come first (e.g., 1+2j). However, Python's ast.literal_eval natively supports complex numbers with the imaginary part first (e.g., 2j+1).

We can resolve both issues by:

  • Adding a fast helper _IsNumericNode to verify that both operands are simple numeric literals (or unary operations on them) before calling ast.literal_eval.
  • Checking both orderings (real + imaginary and imaginary + real) to fully support standard Python complex literals.
def _IsNumericNode(node):
  if isinstance(node, ast.Constant):
    return isinstance(node.value, (int, float, complex))
  if isinstance(node, ast.UnaryOp) and isinstance(node.op, (ast.UAdd, ast.USub)):
    return _IsNumericNode(node.operand)
  if hasattr(ast, 'Num') and isinstance(node, ast.Num):
    return isinstance(node.n, (int, float, complex))
  return False


def _IsComplexLiteral(node):
  """Returns whether a binary operation expresses a complex number literal.

  Python has no single-token syntax for a complex number with both a real and
  an imaginary part, so such numbers are written as the sum or difference of a
  real number and an imaginary one, e.g. 1+2j. These are the only binary
  operations ast.literal_eval accepts, so they are the only ones Fire accepts.

  Args:
    node: A node of type BinOp.
  Returns:
    True if the node has the form <real number> + <imaginary number>, with
    either + or -, False otherwise.
  """
  if not isinstance(node.op, (ast.Add, ast.Sub)):
    return False
  if not (_IsNumericNode(node.left) and _IsNumericNode(node.right)):
    return False
  try:
    left_val = ast.literal_eval(node.left)
    right_val = ast.literal_eval(node.right)
  except (TypeError, ValueError):
    return False
  return (
      (isinstance(left_val, (int, float)) and isinstance(right_val, complex)) or
      (isinstance(left_val, complex) and isinstance(right_val, (int, float)))
  )

@Popy21 Popy21 Nov 6, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checked both points against CPython. I'm keeping this code as is.

Imaginary part first: ast.literal_eval does not accept it. Its BinOp branch is left = _convert_signed_num(node.left), right = _convert_num(node.right), then isinstance(left, (int, float)) and isinstance(right, complex). That code is the same in Lib/ast.py from 3.7.17 through 3.14.0, and main and 2.7.18 have the same rule.

$ python3 --version
Python 3.14.6
$ python3 -c "import ast; ast.literal_eval('2j+1')"
ValueError: malformed node or string on line 1: BinOp(left=Constant(value=2j, kind=None), op=Add(), right=Constant(value=1, kind=None))

3.8.20, 3.9.6, 3.10.19, 3.11.15, 3.12.8 and 3.13.3 raise the same ValueError for 2j+1 and 2j-1. All of them accept 1+2j and -1-2j. Also, _LiteralEval ends with ast.literal_eval(root), so accepting 2j+1 here would change nothing: with the suggested _IsComplexLiteral applied, DefaultParseValue('2j+1') still returns '2j+1'.

Performance: a container operand does reach ast.literal_eval here, e.g. [0,1,...]+1j. But the walk raises at the first rejected BinOp, so at most one operand gets evaluated. A nested BinOp operand fails inside literal_eval before anything is evaluated. With 50 such BinOps in one argument, a single list is evaluated. For a 108,894-byte argument, which is close to Linux's 128 KiB per-argument limit, _IsComplexLiteral takes about 1 ms. The ast.parse call before it takes 13 to 17 ms (3.8, 3.12, 3.14).

A node-type pre-check would also need ast.Num on 3.7, because the 3.7 parser does not emit ast.Constant (the same reason _StrNode exists). And the suggested ast.Num lookup emits a DeprecationWarning on 3.12 and 3.13. For about 1 ms on input that gets rejected anyway, I don't think the extra code is worth it.



def _Replacement(node):
"""Returns a node to use in place of the supplied node in the AST.

Expand Down
21 changes: 21 additions & 0 deletions fire/parser_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,5 +136,26 @@ def testDefaultParseValueIgnoreBinOp(self):
self.assertEqual(parser.DefaultParseValue('2017-10-10'), '2017-10-10')
self.assertEqual(parser.DefaultParseValue('1+1'), '1+1')

def testDefaultParseValueIgnoreNestedBinOp(self):
# Binary operations are not literals, wherever they appear.
self.assertEqual(parser.DefaultParseValue('[1+1]'), '[1+1]')
self.assertEqual(parser.DefaultParseValue('{key: 1+1}'), '{key: 1+1}')
self.assertEqual(parser.DefaultParseValue('(1+1,)'), '(1+1,)')
self.assertEqual(parser.DefaultParseValue('[[2017-10-10]]'),
'[[2017-10-10]]')
self.assertEqual(parser.DefaultParseValue('{1: [a*b]}'), '{1: [a*b]}')
self.assertEqual(parser.DefaultParseValue('1+1+2j'), '1+1+2j')

def testDefaultParseValueComplexNumbers(self):
# The + and - of a complex number are literals, and are still parsed.
self.assertEqual(parser.DefaultParseValue('2j'), 2j)
self.assertEqual(parser.DefaultParseValue('1+2j'), 1 + 2j)
self.assertEqual(parser.DefaultParseValue('-1.5-2j'), -1.5 - 2j)
self.assertEqual(parser.DefaultParseValue('[1+2j]'), [1 + 2j])
self.assertEqual(parser.DefaultParseValue('{alpha: 1+2j}'),
{'alpha': 1 + 2j})
# The real part must come first, as it must for ast.literal_eval.
self.assertEqual(parser.DefaultParseValue('2j+1'), '2j+1')
Comment on lines +157 to +158

@gemini-code-assist gemini-code-assist Bot Oct 16, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Since we updated _IsComplexLiteral to support both orderings of complex literals (matching Python's standard ast.literal_eval behavior), we should update this test case to assert that 2j+1 is successfully parsed as a complex number 1 + 2j instead of being treated as a string.

Suggested change
# The real part must come first, as it must for ast.literal_eval.
self.assertEqual(parser.DefaultParseValue('2j+1'), '2j+1')
# Both real part first and imaginary part first are supported.
self.assertEqual(parser.DefaultParseValue('2j+1'), 1 + 2j)

@Popy21 Popy21 Nov 6, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm keeping this assertion. ast.literal_eval('2j+1') raises ValueError: malformed node or string on 3.8 through 3.14 (all tested), and 3.7 has the same code in Lib/ast.py. Details are in the parser.py thread. So returning the string matches literal_eval.

The suggested assertion would fail even with the suggested _IsComplexLiteral change: DefaultParseValue('2j+1') still returns '2j+1', because the final ast.literal_eval(root) rejects the BinOp.


if __name__ == '__main__':
testutils.main()
Loading