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 1 commit 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
1 commit
Select commit
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
40 changes: 35 additions & 5 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. Then the AST 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.

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

Expand All @@ -96,8 +97,13 @@ 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 node in ast.walk(root):
for field, child in ast.iter_fields(node):
Expand All @@ -116,6 +122,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 +125 to +146

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



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)


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