[Popy21]

Fixes #97.

Reject non-literal binary operations at any AST depth

---

Prepared with AI assistance (Claude Code) as part of a personal daily
open-source contribution effort. Validated locally before submission: this
repository's test suite (python3 -m pytest -q) passes on this branch. Happy to adjust
based on review — and happy to stop opening PRs on this repo if the
maintainers would rather not receive automated contributions.

[gemini-code-assist]

Code Review

This pull request updates the AST parser in fire/parser.py to reject binary operations anywhere in the parsed expression, with an exception for complex number literals, and adds corresponding tests. The review feedback identifies a performance bottleneck in _IsComplexLiteral when evaluating large containers, as well as an artificial restriction that prevents parsing complex numbers with the imaginary part written first (e.g., 2j+1). Suggestions are provided to optimize the type checking before evaluation and to support both orderings of complex literals.

[gemini-code-assist]

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]

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.

[gemini-code-assist]

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]

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.