From 150211d1cd5699dcb35969c81e48c41a2b789a97 Mon Sep 17 00:00:00 2001 From: Abraham Cheung <58929011+cathaypacific8747@users.noreply.github.com> Date: Tue, 24 Jun 2025 15:07:23 +0800 Subject: [PATCH 1/8] fix: keep parentheses properly previously, stringifying expressions would concatenate the string representation of their parts without considering the order of operations, leading to incorrect removal of semantically necessary parentheses. - the binding strength of operators (1 for walrus, 18 for atoms) were added - parentheses are added when: - inner operation has a lower precedence than an outer one (e.g. `(a + b) * c`) - subexpression on the right of left associative operators have the same precedence (e.g. `(a - b) - c`) - subexpression on the left of right associative operators have the same precedence (e.g. `(a ** b) ** c`) - special cases like power operator binding less when an arithmetic or bitwise operator on its right were implemented --- src/_griffe/expressions.py | 150 +++++++++++++++++++++++++++++-------- tests/test_expressions.py | 28 +++++++ 2 files changed, 146 insertions(+), 32 deletions(-) diff --git a/src/_griffe/expressions.py b/src/_griffe/expressions.py index ac4a57b49..9222d1795 100644 --- a/src/_griffe/expressions.py +++ b/src/_griffe/expressions.py @@ -11,7 +11,6 @@ from dataclasses import dataclass from dataclasses import fields as getfields from functools import partial -from itertools import zip_longest from typing import TYPE_CHECKING, Any, Callable from _griffe.agents.nodes.parameters import get_parameters @@ -25,15 +24,84 @@ from _griffe.models import Class, Module +# https://docs.python.org/3/reference/expressions.html#operator-precedence +_PRECEDENCE = { + "ExprName": 18, + "ExprConstant": 18, + "ExprList": 18, + "ExprTuple": 18, + "ExprSet": 18, + "ExprDict": 18, + "ExprAttribute": 17, + "ExprSubscript": 17, + "ExprCall": 17, + "ExprUnaryOp": {"~": 12, "+": 12, "-": 12, "not": 6}, + "ExprBinOp": { + "**": 13, + "*": 11, + "@": 11, + "/": 11, + "//": 11, + "%": 11, + "+": 10, + "-": 10, + "<<": 9, + ">>": 9, + "&": 8, + "^": 7, + "|": 6, + }, + "ExprCompare": 7, + "ExprBoolOp": {"and": 5, "or": 4}, + "ExprIfExp": 3, + "ExprLambda": 2, + "ExprGeneratorExp": 2, + "ExprNamedExpr": 1, +} -def _yield(element: str | Expr | tuple[str | Expr, ...], *, flat: bool = True) -> Iterator[str | Expr]: - if isinstance(element, str): - yield element +def _get_precedence(expr: Expr) -> int: + if isinstance(expr, ExprUnaryOp): + return _PRECEDENCE["ExprUnaryOp"][expr.operator] + if isinstance(expr, ExprBinOp): + return _PRECEDENCE["ExprBinOp"][expr.operator] + if isinstance(expr, ExprBoolOp): + return _PRECEDENCE["ExprBoolOp"][expr.operator] + return _PRECEDENCE.get(expr.classname, 18) + + +def _yield(element: str | Expr | tuple[str | Expr, ...], *, flat: bool = True, is_left: bool = False, outer_precedence: int = 18) -> Iterator[str | Expr]: + if isinstance(element, Expr): + element_precedence = _get_precedence(element) + needs_parens = False + # lower inner precedence, e.g. (a + b) * c, +(10) < *(11) + if element_precedence < outer_precedence: + needs_parens = True + elif element_precedence == outer_precedence: + # right-assoc, e.g. parenthesise lhs in (a ** b) ** c + is_right_assoc = isinstance(element, ExprIfExp) or ( + isinstance(element, ExprBinOp) and element.operator == "**" + ) + if is_right_assoc: + if is_left: + needs_parens = True + # left-assoc, e.g. parenthesise rhs in a - (b - c) + elif isinstance(element, (ExprBinOp, ExprBoolOp)) and not is_left: + needs_parens = True + + if needs_parens: + yield "(" + if flat: + yield from element.iterate(flat=True) + else: + yield element + yield ")" + elif flat: + yield from element.iterate(flat=True) + else: + yield element elif isinstance(element, tuple): for elem in element: - yield from _yield(elem, flat=flat) - elif flat: - yield from element.iterate(flat=True) + yield from _yield(elem, flat=flat, outer_precedence=outer_precedence, is_left=is_left) else: yield element @@ -46,12 +114,13 @@ def _join( ) -> Iterator[str | Expr]: it = iter(elements) try: - yield from _yield(next(it), flat=flat) + # dont parenthesise items within a sequence + yield from _yield(next(it), flat=flat, outer_precedence=0) except StopIteration: return for element in it: - yield from _yield(joint, flat=flat) - yield from _yield(element, flat=flat) + yield from _yield(joint, flat=flat, outer_precedence=0) + yield from _yield(element, flat=flat, outer_precedence=0) def _field_as_dict( @@ -184,7 +253,11 @@ class ExprAttribute(Expr): """The different parts of the dotted chain.""" def iterate(self, *, flat: bool = True) -> Iterator[str | Expr]: - yield from _join(self.values, ".", flat=flat) + precedence = _get_precedence(self) + yield from _yield(self.values[0], flat=flat, outer_precedence=precedence, is_left=True) + for value in self.values[1:]: + yield "." + yield from _yield(value, flat=flat, outer_precedence=precedence) def append(self, value: ExprName) -> None: """Append a name to this attribute. @@ -232,9 +305,14 @@ class ExprBinOp(Expr): """Right part.""" def iterate(self, *, flat: bool = True) -> Iterator[str | Expr]: - yield from _yield(self.left, flat=flat) + precedence = _get_precedence(self) + right_precedence = precedence + if self.operator == "**" and isinstance(self.right, ExprUnaryOp): + # footnote 5: unary operators on the right have higher precedence, e.g. a ** -b + right_precedence -= 1 + yield from _yield(self.left, flat=flat, outer_precedence=precedence, is_left=True) yield f" {self.operator} " - yield from _yield(self.right, flat=flat) + yield from _yield(self.right, flat=flat, outer_precedence=right_precedence, is_left=False) # YORE: EOL 3.9: Replace `**_dataclass_opts` with `slots=True` within line. @@ -248,7 +326,12 @@ class ExprBoolOp(Expr): """Operands.""" def iterate(self, *, flat: bool = True) -> Iterator[str | Expr]: - yield from _join(self.values, f" {self.operator} ", flat=flat) + precedence = _get_precedence(self) + it = iter(self.values) + yield from _yield(next(it), flat=flat, outer_precedence=precedence, is_left=True) + for value in it: + yield f" {self.operator} " + yield from _yield(value, flat=flat, outer_precedence=precedence, is_left=False) # YORE: EOL 3.9: Replace `**_dataclass_opts` with `slots=True` within line. @@ -267,7 +350,7 @@ def canonical_path(self) -> str: return self.function.canonical_path def iterate(self, *, flat: bool = True) -> Iterator[str | Expr]: - yield from _yield(self.function, flat=flat) + yield from _yield(self.function, flat=flat, outer_precedence=_get_precedence(self)) yield "(" yield from _join(self.arguments, ", ", flat=flat) yield ")" @@ -286,9 +369,11 @@ class ExprCompare(Expr): """Things compared.""" def iterate(self, *, flat: bool = True) -> Iterator[str | Expr]: - yield from _yield(self.left, flat=flat) - yield " " - yield from _join(zip_longest(self.operators, [], self.comparators, fillvalue=" "), " ", flat=flat) + precedence = _get_precedence(self) + yield from _yield(self.left, flat=flat, outer_precedence=precedence, is_left=True) + for op, comp in zip(self.operators, self.comparators): + yield f" {op} " + yield from _yield(comp, flat=flat, outer_precedence=precedence) # YORE: EOL 3.9: Replace `**_dataclass_opts` with `slots=True` within line. @@ -396,7 +481,7 @@ class ExprFormatted(Expr): def iterate(self, *, flat: bool = True) -> Iterator[str | Expr]: yield "{" - yield from _yield(self.value, flat=flat) + yield from _yield(self.value, flat=flat, outer_precedence=0) yield "}" @@ -429,11 +514,12 @@ class ExprIfExp(Expr): """Other expression.""" def iterate(self, *, flat: bool = True) -> Iterator[str | Expr]: - yield from _yield(self.body, flat=flat) + precedence = _get_precedence(self) + yield from _yield(self.body, flat=flat, outer_precedence=precedence, is_left=True) yield " if " - yield from _yield(self.test, flat=flat) + yield from _yield(self.test, flat=flat, outer_precedence=precedence + 1) yield " else " - yield from _yield(self.orelse, flat=flat) + yield from _yield(self.orelse, flat=flat, outer_precedence=precedence, is_left=False) # YORE: EOL 3.9: Replace `**_dataclass_opts` with `slots=True` within line. @@ -557,7 +643,7 @@ def iterate(self, *, flat: bool = True) -> Iterator[str | Expr]: if index < length: yield ", " yield ": " - yield from _yield(self.body, flat=flat) + yield from _yield(self.body, flat=flat, outer_precedence=0) # YORE: EOL 3.9: Replace `**_dataclass_opts` with `slots=True` within line. @@ -685,11 +771,9 @@ class ExprNamedExpr(Expr): """Value.""" def iterate(self, *, flat: bool = True) -> Iterator[str | Expr]: - yield "(" yield from _yield(self.target, flat=flat) yield " := " yield from _yield(self.value, flat=flat) - yield ")" # YORE: EOL 3.9: Replace `**_dataclass_opts` with `slots=True` within line. @@ -773,9 +857,9 @@ class ExprSubscript(Expr): """Slice part.""" def iterate(self, *, flat: bool = True) -> Iterator[str | Expr]: - yield from _yield(self.left, flat=flat) + yield from _yield(self.left, flat=flat, outer_precedence=_get_precedence(self)) yield "[" - yield from _yield(self.slice, flat=flat) + yield from _yield(self.slice, flat=flat, outer_precedence=0) yield "]" @property @@ -825,7 +909,9 @@ class ExprUnaryOp(Expr): def iterate(self, *, flat: bool = True) -> Iterator[str | Expr]: yield self.operator - yield from _yield(self.value, flat=flat) + if self.operator == "not": + yield " " + yield from _yield(self.value, flat=flat, outer_precedence=_get_precedence(self)) # YORE: EOL 3.9: Replace `**_dataclass_opts` with `slots=True` within line. @@ -858,7 +944,7 @@ def iterate(self, *, flat: bool = True) -> Iterator[str | Expr]: _unary_op_map = { ast.Invert: "~", - ast.Not: "not ", + ast.Not: "not", ast.UAdd: "+", ast.USub: "-", } @@ -1113,7 +1199,7 @@ def _build_subscript( "typing_extensions.Literal", }: literal_strings = True - slice = _build( + slice_val = _build( node.slice, parent, parse_strings=True, @@ -1122,8 +1208,8 @@ def _build_subscript( **kwargs, ) else: - slice = _build(node.slice, parent, in_subscript=True, **kwargs) - return ExprSubscript(left, slice) + slice_val = _build(node.slice, parent, in_subscript=True, **kwargs) + return ExprSubscript(left, slice_val) def _build_tuple( diff --git a/tests/test_expressions.py b/tests/test_expressions.py index 0f43fca72..40014f16b 100644 --- a/tests/test_expressions.py +++ b/tests/test_expressions.py @@ -110,3 +110,31 @@ def __init__(self, x: int): """, ) as module: assert module["Class.x"].value.canonical_path == "module.Class(x)" + + +@pytest.mark.parametrize( + "code", + [ + # core + "a * (b + c)", # lower precedence as a subexpression of one that has higher precedence + "(a and b) == c" + "((a | b) + c).d" + "a - (b - c)", # left-assoc + "(a ** b) ** c", # right-assoc + # unary operator and edge cases (python docs 6.17 footnote 5) + "a ** -b", + "-a ** b", + "(-a) ** b", + # misc: conditional, lambda, comprehensions and generator + "(lambda: 0).a", + "(lambda x: a + x if b else c)(d).e", + "a if (b if c else d) else e", # right-assoc + "(a if b else c) if d else e", # forced left-assoc + "(a for a in b).gi_code", + ], +) +def test_parentheses_preserved(code: str) -> None: + """Parentheses used to enforce an order of operations should not be removed.""" + with temporary_visited_module(f"val = {code}") as module: + value_expr = module["val"].value + assert str(value_expr) == code From d587bf48f62de569c537db67a65f0cf9b876b1cd Mon Sep 17 00:00:00 2001 From: Abraham Cheung <58929011+cathaypacific8747@users.noreply.github.com> Date: Wed, 25 Jun 2025 03:48:10 +0800 Subject: [PATCH 2/8] chore: update comments MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Timothée Mazzucotelli --- src/_griffe/expressions.py | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/_griffe/expressions.py b/src/_griffe/expressions.py index 5b3363847..061fe049c 100644 --- a/src/_griffe/expressions.py +++ b/src/_griffe/expressions.py @@ -73,18 +73,18 @@ def _yield(element: str | Expr | tuple[str | Expr, ...], *, flat: bool = True, i if isinstance(element, Expr): element_precedence = _get_precedence(element) needs_parens = False - # lower inner precedence, e.g. (a + b) * c, +(10) < *(11) + # Lower inner precedence, e.g. `(a + b) * c`, `+(10) < *(11)`. if element_precedence < outer_precedence: needs_parens = True elif element_precedence == outer_precedence: - # right-assoc, e.g. parenthesise lhs in (a ** b) ** c + # Right-association, e.g. parenthesize left-hand side in `(a ** b) ** c`. is_right_assoc = isinstance(element, ExprIfExp) or ( isinstance(element, ExprBinOp) and element.operator == "**" ) if is_right_assoc: if is_left: needs_parens = True - # left-assoc, e.g. parenthesise rhs in a - (b - c) + # Left-association, e.g. parenthesize right-hand side in `a - (b - c)`. elif isinstance(element, (ExprBinOp, ExprBoolOp)) and not is_left: needs_parens = True @@ -114,7 +114,7 @@ def _join( ) -> Iterator[str | Expr]: it = iter(elements) try: - # dont parenthesise items within a sequence + # Don't parenthesize items within a sequence. yield from _yield(next(it), flat=flat, outer_precedence=0) except StopIteration: return @@ -308,7 +308,7 @@ def iterate(self, *, flat: bool = True) -> Iterator[str | Expr]: precedence = _get_precedence(self) right_precedence = precedence if self.operator == "**" and isinstance(self.right, ExprUnaryOp): - # footnote 5: unary operators on the right have higher precedence, e.g. a ** -b + # Unary operators on the right have higher precedence, e.g. `a ** -b`. right_precedence -= 1 yield from _yield(self.left, flat=flat, outer_precedence=precedence, is_left=True) yield f" {self.operator} " @@ -1199,7 +1199,7 @@ def _build_subscript( "typing_extensions.Literal", }: literal_strings = True - slice_val = _build( + slice_expr = _build( node.slice, parent, parse_strings=True, @@ -1208,8 +1208,8 @@ def _build_subscript( **kwargs, ) else: - slice_val = _build(node.slice, parent, in_subscript=True, **kwargs) - return ExprSubscript(left, slice_val) + slice_expr = _build(node.slice, parent, in_subscript=True, **kwargs) + return ExprSubscript(left, slice_expr) def _build_tuple( From 866e35be90f667a6f80df7e0d77f4ace726ba3ee Mon Sep 17 00:00:00 2001 From: Abraham Cheung <58929011+cathaypacific8747@users.noreply.github.com> Date: Wed, 25 Jun 2025 03:54:00 +0800 Subject: [PATCH 3/8] chore: update comments for tests/test_expressions.py MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Timothée Mazzucotelli --- tests/test_expressions.py | 24 +++++++++++++----------- 1 file changed, 13 insertions(+), 11 deletions(-) diff --git a/tests/test_expressions.py b/tests/test_expressions.py index 40014f16b..f2d326a7e 100644 --- a/tests/test_expressions.py +++ b/tests/test_expressions.py @@ -115,22 +115,24 @@ def __init__(self, x: int): @pytest.mark.parametrize( "code", [ - # core - "a * (b + c)", # lower precedence as a subexpression of one that has higher precedence - "(a and b) == c" - "((a | b) + c).d" - "a - (b - c)", # left-assoc - "(a ** b) ** c", # right-assoc - # unary operator and edge cases (python docs 6.17 footnote 5) + # Core. + "a * (b + c)", # Lower precedence as a sub-expression of one that has higher precedence. + "(a and b) == c", + "((a | b) + c).d", + "a - (b - c)", # Left-association. + "(a ** b) ** c", # Right-association. + # Unary operator and edge cases: + # > The power operator `**` binds less tightly than an arithmetic + # > or bitwise unary operator on its right, that is, `2**-1` is `0.5`. "a ** -b", "-a ** b", "(-a) ** b", - # misc: conditional, lambda, comprehensions and generator + # Misc: conditionals, lambdas, comprehensions and generators. "(lambda: 0).a", "(lambda x: a + x if b else c)(d).e", - "a if (b if c else d) else e", # right-assoc - "(a if b else c) if d else e", # forced left-assoc - "(a for a in b).gi_code", + "a if (b if c else d) else e", # Right-association. + "(a if b else c) if d else e", # Forced left-association. + "(a for a in b).c", ], ) def test_parentheses_preserved(code: str) -> None: From 70d36fd42a3783e85ef3f91dbecdd5049ed6978c Mon Sep 17 00:00:00 2001 From: Abraham Cheung <58929011+cathaypacific8747@users.noreply.github.com> Date: Wed, 25 Jun 2025 04:16:06 +0800 Subject: [PATCH 4/8] refactor: use `IntEnum` for operator precedence - improve comments on `NONE` sentinel value --- src/_griffe/expressions.py | 184 ++++++++++++++++++++++++++----------- 1 file changed, 131 insertions(+), 53 deletions(-) diff --git a/src/_griffe/expressions.py b/src/_griffe/expressions.py index 061fe049c..10cfa5eb5 100644 --- a/src/_griffe/expressions.py +++ b/src/_griffe/expressions.py @@ -10,6 +10,7 @@ import sys from dataclasses import dataclass from dataclasses import fields as getfields +from enum import IntEnum, auto from functools import partial from typing import TYPE_CHECKING, Any, Callable @@ -24,52 +25,113 @@ from _griffe.models import Class, Module -# https://docs.python.org/3/reference/expressions.html#operator-precedence -_PRECEDENCE = { - "ExprName": 18, - "ExprConstant": 18, - "ExprList": 18, - "ExprTuple": 18, - "ExprSet": 18, - "ExprDict": 18, - "ExprAttribute": 17, - "ExprSubscript": 17, - "ExprCall": 17, - "ExprUnaryOp": {"~": 12, "+": 12, "-": 12, "not": 6}, - "ExprBinOp": { - "**": 13, - "*": 11, - "@": 11, - "/": 11, - "//": 11, - "%": 11, - "+": 10, - "-": 10, - "<<": 9, - ">>": 9, - "&": 8, - "^": 7, - "|": 6, - }, - "ExprCompare": 7, - "ExprBoolOp": {"and": 5, "or": 4}, - "ExprIfExp": 3, - "ExprLambda": 2, - "ExprGeneratorExp": 2, - "ExprNamedExpr": 1, -} -def _get_precedence(expr: Expr) -> int: +class _OperatorPrecedence(IntEnum): + # Adapted from: + # - https://docs.python.org/3/reference/expressions.html#operator-precedence + # - https://github.com/python/cpython/blob/main/Lib/_ast_unparse.py + # - https://github.com/astral-sh/ruff/blob/6abafcb56575454f2caeaa174efcb9fd0a8362b1/crates/ruff_python_ast/src/operator_precedence.rs + NONE = auto() + """A virtual precedence level for contexts that provide their own grouping, like list brackets + or function call parentheses. This ensures parentheses will never be added for the direct + children of these nodes.""" # HACK: `ruff_python_formatter::expression::parentheses`'s state machine would be more robust but would introduce significant complexity + YIELD = auto() # `yield`, `yield from` + ASSIGN = auto() # `target := expr` + STARRED = auto() # `*expr`, NOTE: Omitted by Python docs, see ruff impl + LAMBDA = auto() + IF_ELSE = auto() # `expr if cond else expr` + OR = auto() + AND = auto() + NOT = auto() + COMPARISON_MEMBERSHIP_IDENTITY = auto() # `<`, `<=`, `>`, `>=`, `!=`, `==`, `in`, `not in`, `is`, `is not` + BIT_OR = auto() # `|` + BIT_XOR = auto() # `^` + BIT_AND = auto() # `&` + LEFT_RIGHT_SHIFT = auto() # `<<`, `>>` + ADD_SUB = auto() # `+`, `-` + MUL_DIV_REMAIN = auto() # `*`, `@`, `/`, `//`, `%` + POS_NEG_BIT_NOT = auto() # +x, -x, ~x + EXPONENT = auto() # ** + AWAIT = auto() + CALL_ATTRIBUTE = auto() # x[index], x[index:index], x(arguments...), x.attribute + ATOMIC = auto() # (expressions...), [expressions...], {key: value...}, {expressions...} + + +def _get_precedence(expr: Expr) -> _OperatorPrecedence: + """Get the precedence of an expression.""" + if isinstance( + expr, + ( + ExprName, + ExprConstant, + ExprList, + ExprTuple, + ExprSet, + ExprDict, + ExprListComp, + ExprSetComp, + ExprDictComp, + ), + ): + return _OperatorPrecedence.ATOMIC + if isinstance(expr, (ExprAttribute, ExprSubscript, ExprCall)): + return _OperatorPrecedence.CALL_ATTRIBUTE + # TODO: implement ast.Await if isinstance(expr, ExprUnaryOp): - return _PRECEDENCE["ExprUnaryOp"][expr.operator] + if expr.operator == "not": + return _OperatorPrecedence.NOT + return _OperatorPrecedence.POS_NEG_BIT_NOT if isinstance(expr, ExprBinOp): - return _PRECEDENCE["ExprBinOp"][expr.operator] + op = expr.operator + if op == "**": + return _OperatorPrecedence.EXPONENT + if op in {"*", "@", "/", "//", "%"}: + return _OperatorPrecedence.MUL_DIV_REMAIN + if op in {"+", "-"}: + return _OperatorPrecedence.ADD_SUB + if op in {"<<", ">>"}: + return _OperatorPrecedence.LEFT_RIGHT_SHIFT + if op == "&": + return _OperatorPrecedence.BIT_AND + if op == "^": + return _OperatorPrecedence.BIT_XOR + if op == "|": + return _OperatorPrecedence.BIT_OR + if isinstance(expr, ExprCompare): + return _OperatorPrecedence.COMPARISON_MEMBERSHIP_IDENTITY if isinstance(expr, ExprBoolOp): - return _PRECEDENCE["ExprBoolOp"][expr.operator] - return _PRECEDENCE.get(expr.classname, 18) - - -def _yield(element: str | Expr | tuple[str | Expr, ...], *, flat: bool = True, is_left: bool = False, outer_precedence: int = 18) -> Iterator[str | Expr]: + if expr.operator == "and": + return _OperatorPrecedence.AND + if expr.operator == "or": + return _OperatorPrecedence.OR + if isinstance(expr, ExprIfExp): + return _OperatorPrecedence.IF_ELSE + if isinstance( + expr, + ( + ExprLambda, + ExprGeneratorExp, # NOTE: Ruff categorizes as atomic, but (a for a in b).gi_code implies its less than CALL_ATTRIBUTE + ), + ): + return _OperatorPrecedence.LAMBDA + if isinstance(expr, ExprVarPositional): + return _OperatorPrecedence.STARRED + if isinstance(expr, ExprNamedExpr): + return _OperatorPrecedence.ASSIGN + if isinstance(expr, (ExprYield, ExprYieldFrom)): + return _OperatorPrecedence.YIELD + + logger.warning(f"Could not determine precedence for expression type {type(expr).__name__}. ") + return _OperatorPrecedence.NONE + + +def _yield( + element: str | Expr | tuple[str | Expr, ...], + *, + flat: bool = True, + is_left: bool = False, + outer_precedence: _OperatorPrecedence = _OperatorPrecedence.ATOMIC, +) -> Iterator[str | Expr]: if isinstance(element, Expr): element_precedence = _get_precedence(element) needs_parens = False @@ -77,7 +139,7 @@ def _yield(element: str | Expr | tuple[str | Expr, ...], *, flat: bool = True, i if element_precedence < outer_precedence: needs_parens = True elif element_precedence == outer_precedence: - # Right-association, e.g. parenthesize left-hand side in `(a ** b) ** c`. + # Right-association, e.g. parenthesize left-hand side in `(a ** b) ** c`, (a if b else c) if d else e is_right_assoc = isinstance(element, ExprIfExp) or ( isinstance(element, ExprBinOp) and element.operator == "**" ) @@ -112,15 +174,19 @@ def _join( *, flat: bool = True, ) -> Iterator[str | Expr]: + """Apply a separator between elements. The caller is assumed to provide their own grouping ( + e.g. lists, tuples, slice) and will prevent parentheses from being added. + """ it = iter(elements) try: - # Don't parenthesize items within a sequence. - yield from _yield(next(it), flat=flat, outer_precedence=0) + # Since we are in a sequence, don't parenthesize items. + # Avoids [a + b, c + d] being serialized as [(a + b), (c + d)] + yield from _yield(next(it), flat=flat, outer_precedence=_OperatorPrecedence.NONE) except StopIteration: return for element in it: - yield from _yield(joint, flat=flat, outer_precedence=0) - yield from _yield(element, flat=flat, outer_precedence=0) + yield from _yield(joint, flat=flat, outer_precedence=_OperatorPrecedence.NONE) + yield from _yield(element, flat=flat, outer_precedence=_OperatorPrecedence.NONE) def _field_as_dict( @@ -309,7 +375,7 @@ def iterate(self, *, flat: bool = True) -> Iterator[str | Expr]: right_precedence = precedence if self.operator == "**" and isinstance(self.right, ExprUnaryOp): # Unary operators on the right have higher precedence, e.g. `a ** -b`. - right_precedence -= 1 + right_precedence = _OperatorPrecedence(precedence - 1) yield from _yield(self.left, flat=flat, outer_precedence=precedence, is_left=True) yield f" {self.operator} " yield from _yield(self.right, flat=flat, outer_precedence=right_precedence, is_left=False) @@ -481,7 +547,8 @@ class ExprFormatted(Expr): def iterate(self, *, flat: bool = True) -> Iterator[str | Expr]: yield "{" - yield from _yield(self.value, flat=flat, outer_precedence=0) + # Prevent parentheses from being added, avoiding `{(1 + 1)}` + yield from _yield(self.value, flat=flat, outer_precedence=_OperatorPrecedence.NONE) yield "}" @@ -517,9 +584,18 @@ def iterate(self, *, flat: bool = True) -> Iterator[str | Expr]: precedence = _get_precedence(self) yield from _yield(self.body, flat=flat, outer_precedence=precedence, is_left=True) yield " if " - yield from _yield(self.test, flat=flat, outer_precedence=precedence + 1) + # If the test itself is another if/else, its precedence is the same, which will not give + # a parenthesis: force it. + test_outer_precedence = _OperatorPrecedence(precedence + 1) + yield from _yield(self.test, flat=flat, outer_precedence=test_outer_precedence) yield " else " - yield from _yield(self.orelse, flat=flat, outer_precedence=precedence, is_left=False) + # If/else is right associative. For example, a nested if/else + # `a if b else c if d else e` is effectively `a if b else (c if d else e)`, so produce a + # flattened version without parentheses. + if isinstance(self.orelse, ExprIfExp): + yield from self.orelse.iterate(flat=flat) + else: + yield from _yield(self.orelse, flat=flat, outer_precedence=precedence, is_left=False) # YORE: EOL 3.9: Replace `**_dataclass_opts` with `slots=True` within line. @@ -643,7 +719,8 @@ def iterate(self, *, flat: bool = True) -> Iterator[str | Expr]: if index < length: yield ", " yield ": " - yield from _yield(self.body, flat=flat, outer_precedence=0) + # Body of lambda should not have parentheses, avoiding `lambda: a.b` + yield from _yield(self.body, flat=flat, outer_precedence=_OperatorPrecedence.NONE) # YORE: EOL 3.9: Replace `**_dataclass_opts` with `slots=True` within line. @@ -859,7 +936,8 @@ class ExprSubscript(Expr): def iterate(self, *, flat: bool = True) -> Iterator[str | Expr]: yield from _yield(self.left, flat=flat, outer_precedence=_get_precedence(self)) yield "[" - yield from _yield(self.slice, flat=flat, outer_precedence=0) + # Prevent parentheses from being added, avoiding `a[(b)]` + yield from _yield(self.slice, flat=flat, outer_precedence=_OperatorPrecedence.NONE) yield "]" @property From 1a22560dc9d1422b0e2c6dea8009c23b37406999 Mon Sep 17 00:00:00 2001 From: Abraham Cheung <58929011+cathaypacific8747@users.noreply.github.com> Date: Wed, 25 Jun 2025 04:49:24 +0800 Subject: [PATCH 5/8] fix: ensure precedence is exhaustive --- src/_griffe/expressions.py | 28 +++++++++++++++++++++++----- 1 file changed, 23 insertions(+), 5 deletions(-) diff --git a/src/_griffe/expressions.py b/src/_griffe/expressions.py index 10cfa5eb5..c68427b31 100644 --- a/src/_griffe/expressions.py +++ b/src/_griffe/expressions.py @@ -34,7 +34,7 @@ class _OperatorPrecedence(IntEnum): NONE = auto() """A virtual precedence level for contexts that provide their own grouping, like list brackets or function call parentheses. This ensures parentheses will never be added for the direct - children of these nodes.""" # HACK: `ruff_python_formatter::expression::parentheses`'s state machine would be more robust but would introduce significant complexity + children of these nodes.""" # NOTE: `ruff_python_formatter::expression::parentheses`'s state machine would be more robust but would introduce significant complexity YIELD = auto() # `yield`, `yield from` ASSIGN = auto() # `target := expr` STARRED = auto() # `*expr`, NOTE: Omitted by Python docs, see ruff impl @@ -62,12 +62,17 @@ def _get_precedence(expr: Expr) -> _OperatorPrecedence: if isinstance( expr, ( + # Literals and names ExprName, ExprConstant, + ExprJoinedStr, + ExprFormatted, + # Container displays ExprList, ExprTuple, ExprSet, ExprDict, + # Comprehensions are self-contained units that produce a container ExprListComp, ExprSetComp, ExprDictComp, @@ -114,14 +119,25 @@ def _get_precedence(expr: Expr) -> _OperatorPrecedence: ), ): return _OperatorPrecedence.LAMBDA - if isinstance(expr, ExprVarPositional): + if isinstance(expr, (ExprVarPositional, ExprVarKeyword)): return _OperatorPrecedence.STARRED if isinstance(expr, ExprNamedExpr): return _OperatorPrecedence.ASSIGN if isinstance(expr, (ExprYield, ExprYieldFrom)): return _OperatorPrecedence.YIELD + if isinstance( + expr, + ( + ExprComprehension, # NOTE: `for ... in ... if` part, not the whole `[...]`. + ExprExtSlice, + ExprKeyword, + ExprParameter, + ExprSlice, + ), + ): # These are not standalone, they appear in specific contexts where precendence is not a concern + return _OperatorPrecedence.NONE - logger.warning(f"Could not determine precedence for expression type {type(expr).__name__}. ") + logger.warning("Could not determine precedence", extra={"expr": expr}) return _OperatorPrecedence.NONE @@ -174,8 +190,10 @@ def _join( *, flat: bool = True, ) -> Iterator[str | Expr]: - """Apply a separator between elements. The caller is assumed to provide their own grouping ( - e.g. lists, tuples, slice) and will prevent parentheses from being added. + """Apply a separator between elements. + + The caller is assumed to provide their own grouping + (e.g. lists, tuples, slice) and will prevent parentheses from being added. """ it = iter(elements) try: From 301d19a8f9618ca6d4287cdabfbdb80826bde6ef Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Timoth=C3=A9e=20Mazzucotelli?= Date: Thu, 26 Jun 2025 22:23:54 +0200 Subject: [PATCH 6/8] fixup! fix: keep parentheses properly --- src/_griffe/expressions.py | 176 +++++++++++++++++-------------------- 1 file changed, 83 insertions(+), 93 deletions(-) diff --git a/src/_griffe/expressions.py b/src/_griffe/expressions.py index c68427b31..09cd0bceb 100644 --- a/src/_griffe/expressions.py +++ b/src/_griffe/expressions.py @@ -28,117 +28,39 @@ class _OperatorPrecedence(IntEnum): # Adapted from: + # # - https://docs.python.org/3/reference/expressions.html#operator-precedence # - https://github.com/python/cpython/blob/main/Lib/_ast_unparse.py # - https://github.com/astral-sh/ruff/blob/6abafcb56575454f2caeaa174efcb9fd0a8362b1/crates/ruff_python_ast/src/operator_precedence.rs + + # The enum members are declared in ascending order of precedence. + + # A virtual precedence level for contexts that provide their own grouping, like list brackets or + # function call parentheses. This ensures parentheses will never be added for the direct children of these nodes. + # NOTE: `ruff_python_formatter::expression::parentheses`'s state machine would be more robust + # but would introduce significant complexity. NONE = auto() - """A virtual precedence level for contexts that provide their own grouping, like list brackets - or function call parentheses. This ensures parentheses will never be added for the direct - children of these nodes.""" # NOTE: `ruff_python_formatter::expression::parentheses`'s state machine would be more robust but would introduce significant complexity + YIELD = auto() # `yield`, `yield from` ASSIGN = auto() # `target := expr` - STARRED = auto() # `*expr`, NOTE: Omitted by Python docs, see ruff impl + STARRED = auto() # `*expr` (omitted by Python docs, see ruff impl) LAMBDA = auto() IF_ELSE = auto() # `expr if cond else expr` OR = auto() AND = auto() NOT = auto() - COMPARISON_MEMBERSHIP_IDENTITY = auto() # `<`, `<=`, `>`, `>=`, `!=`, `==`, `in`, `not in`, `is`, `is not` + COMP_MEMB_ID = auto() # `<`, `<=`, `>`, `>=`, `!=`, `==`, `in`, `not in`, `is`, `is not` BIT_OR = auto() # `|` BIT_XOR = auto() # `^` BIT_AND = auto() # `&` LEFT_RIGHT_SHIFT = auto() # `<<`, `>>` ADD_SUB = auto() # `+`, `-` MUL_DIV_REMAIN = auto() # `*`, `@`, `/`, `//`, `%` - POS_NEG_BIT_NOT = auto() # +x, -x, ~x - EXPONENT = auto() # ** + POS_NEG_BIT_NOT = auto() # `+x`, `-x`, `~x` + EXPONENT = auto() # `**` AWAIT = auto() - CALL_ATTRIBUTE = auto() # x[index], x[index:index], x(arguments...), x.attribute - ATOMIC = auto() # (expressions...), [expressions...], {key: value...}, {expressions...} - - -def _get_precedence(expr: Expr) -> _OperatorPrecedence: - """Get the precedence of an expression.""" - if isinstance( - expr, - ( - # Literals and names - ExprName, - ExprConstant, - ExprJoinedStr, - ExprFormatted, - # Container displays - ExprList, - ExprTuple, - ExprSet, - ExprDict, - # Comprehensions are self-contained units that produce a container - ExprListComp, - ExprSetComp, - ExprDictComp, - ), - ): - return _OperatorPrecedence.ATOMIC - if isinstance(expr, (ExprAttribute, ExprSubscript, ExprCall)): - return _OperatorPrecedence.CALL_ATTRIBUTE - # TODO: implement ast.Await - if isinstance(expr, ExprUnaryOp): - if expr.operator == "not": - return _OperatorPrecedence.NOT - return _OperatorPrecedence.POS_NEG_BIT_NOT - if isinstance(expr, ExprBinOp): - op = expr.operator - if op == "**": - return _OperatorPrecedence.EXPONENT - if op in {"*", "@", "/", "//", "%"}: - return _OperatorPrecedence.MUL_DIV_REMAIN - if op in {"+", "-"}: - return _OperatorPrecedence.ADD_SUB - if op in {"<<", ">>"}: - return _OperatorPrecedence.LEFT_RIGHT_SHIFT - if op == "&": - return _OperatorPrecedence.BIT_AND - if op == "^": - return _OperatorPrecedence.BIT_XOR - if op == "|": - return _OperatorPrecedence.BIT_OR - if isinstance(expr, ExprCompare): - return _OperatorPrecedence.COMPARISON_MEMBERSHIP_IDENTITY - if isinstance(expr, ExprBoolOp): - if expr.operator == "and": - return _OperatorPrecedence.AND - if expr.operator == "or": - return _OperatorPrecedence.OR - if isinstance(expr, ExprIfExp): - return _OperatorPrecedence.IF_ELSE - if isinstance( - expr, - ( - ExprLambda, - ExprGeneratorExp, # NOTE: Ruff categorizes as atomic, but (a for a in b).gi_code implies its less than CALL_ATTRIBUTE - ), - ): - return _OperatorPrecedence.LAMBDA - if isinstance(expr, (ExprVarPositional, ExprVarKeyword)): - return _OperatorPrecedence.STARRED - if isinstance(expr, ExprNamedExpr): - return _OperatorPrecedence.ASSIGN - if isinstance(expr, (ExprYield, ExprYieldFrom)): - return _OperatorPrecedence.YIELD - if isinstance( - expr, - ( - ExprComprehension, # NOTE: `for ... in ... if` part, not the whole `[...]`. - ExprExtSlice, - ExprKeyword, - ExprParameter, - ExprSlice, - ), - ): # These are not standalone, they appear in specific contexts where precendence is not a concern - return _OperatorPrecedence.NONE - - logger.warning("Could not determine precedence", extra={"expr": expr}) - return _OperatorPrecedence.NONE + CALL_ATTRIBUTE = auto() # `x[index]`, `x[index:index]`, `x(arguments...)`, `x.attribute` + ATOMIC = auto() # `(expressions...)`, `[expressions...]`, `{key: value...}`, `{expressions...}` def _yield( @@ -1079,6 +1001,74 @@ def iterate(self, *, flat: bool = True) -> Iterator[str | Expr]: ast.NotIn: "not in", } +# TODO: Support `ast.Await`. +_precedence_map = { + # Literals and names. + ExprName: lambda _: _OperatorPrecedence.ATOMIC, + ExprConstant: lambda _: _OperatorPrecedence.ATOMIC, + ExprJoinedStr: lambda _: _OperatorPrecedence.ATOMIC, + ExprFormatted: lambda _: _OperatorPrecedence.ATOMIC, + + # Container displays. + ExprList: lambda _: _OperatorPrecedence.ATOMIC, + ExprTuple: lambda _: _OperatorPrecedence.ATOMIC, + ExprSet: lambda _: _OperatorPrecedence.ATOMIC, + ExprDict: lambda _: _OperatorPrecedence.ATOMIC, + + # Comprehensions are self-contained units that produce a container. + ExprListComp: lambda _: _OperatorPrecedence.ATOMIC, + ExprSetComp: lambda _: _OperatorPrecedence.ATOMIC, + ExprDictComp: lambda _: _OperatorPrecedence.ATOMIC, + + ExprAttribute: lambda _: _OperatorPrecedence.CALL_ATTRIBUTE, + ExprSubscript: lambda _: _OperatorPrecedence.CALL_ATTRIBUTE, + ExprCall: lambda _: _OperatorPrecedence.CALL_ATTRIBUTE, + + ExprUnaryOp: lambda e: {"not": _OperatorPrecedence.NOT}.get(e.operator, _OperatorPrecedence.POS_NEG_BIT_NOT), + ExprBinOp: lambda e: { + "**": _OperatorPrecedence.EXPONENT, + "*": _OperatorPrecedence.MUL_DIV_REMAIN, + "@": _OperatorPrecedence.MUL_DIV_REMAIN, + "/": _OperatorPrecedence.MUL_DIV_REMAIN, + "//": _OperatorPrecedence.MUL_DIV_REMAIN, + "%": _OperatorPrecedence.MUL_DIV_REMAIN, + "+": _OperatorPrecedence.ADD_SUB, + "-": _OperatorPrecedence.ADD_SUB, + "<<": _OperatorPrecedence.LEFT_RIGHT_SHIFT, + ">>": _OperatorPrecedence.LEFT_RIGHT_SHIFT, + "&": _OperatorPrecedence.BIT_AND, + "^": _OperatorPrecedence.BIT_XOR, + "|": _OperatorPrecedence.BIT_OR, + }.get(e.operator), + ExprBoolOp: lambda e: {"and": _OperatorPrecedence.AND, "or": _OperatorPrecedence.OR}.get(e.operator), + + ExprCompare: lambda _: _OperatorPrecedence.COMP_MEMB_ID, + ExprIfExp: lambda _: _OperatorPrecedence.IF_ELSE, + ExprNamedExpr: lambda _: _OperatorPrecedence.ASSIGN, + + ExprLambda: lambda _: _OperatorPrecedence.LAMBDA, + # NOTE: Ruff categorizes as atomic, but `(a for a in b).c` implies its less than `CALL_ATTRIBUTE`. + ExprGeneratorExp: lambda _: _OperatorPrecedence.LAMBDA, + + ExprVarPositional: lambda _: _OperatorPrecedence.STARRED, + ExprVarKeyword: lambda _: _OperatorPrecedence.STARRED, + + ExprYield: lambda _: _OperatorPrecedence.YIELD, + ExprYieldFrom: lambda _: _OperatorPrecedence.YIELD, + + # These are not standalone, they appear in specific contexts where precendence is not a concern. + # NOTE: `for ... in ... if` part, not the whole `[...]`. + ExprComprehension: lambda _: _OperatorPrecedence.NONE, + ExprExtSlice: lambda _: _OperatorPrecedence.NONE, + ExprKeyword: lambda _: _OperatorPrecedence.NONE, + ExprParameter: lambda _: _OperatorPrecedence.NONE, + ExprSlice: lambda _: _OperatorPrecedence.NONE, +} + + +def _get_precedence(expr: Expr) -> _OperatorPrecedence: + return _precedence_map.get(type(expr), lambda _: _OperatorPrecedence.NONE)(expr) + def _build_attribute(node: ast.Attribute, parent: Module | Class, **kwargs: Any) -> Expr: left = _build(node.value, parent, **kwargs) From 08adb48086874e0bdfd03025cbc859831054b735 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Timoth=C3=A9e=20Mazzucotelli?= Date: Thu, 26 Jun 2025 22:24:31 +0200 Subject: [PATCH 7/8] fixup! fix: keep parentheses properly --- src/_griffe/expressions.py | 11 +---------- 1 file changed, 1 insertion(+), 10 deletions(-) diff --git a/src/_griffe/expressions.py b/src/_griffe/expressions.py index 09cd0bceb..406fbd758 100644 --- a/src/_griffe/expressions.py +++ b/src/_griffe/expressions.py @@ -34,7 +34,7 @@ class _OperatorPrecedence(IntEnum): # - https://github.com/astral-sh/ruff/blob/6abafcb56575454f2caeaa174efcb9fd0a8362b1/crates/ruff_python_ast/src/operator_precedence.rs # The enum members are declared in ascending order of precedence. - + # A virtual precedence level for contexts that provide their own grouping, like list brackets or # function call parentheses. This ensures parentheses will never be added for the direct children of these nodes. # NOTE: `ruff_python_formatter::expression::parentheses`'s state machine would be more robust @@ -1008,22 +1008,18 @@ def iterate(self, *, flat: bool = True) -> Iterator[str | Expr]: ExprConstant: lambda _: _OperatorPrecedence.ATOMIC, ExprJoinedStr: lambda _: _OperatorPrecedence.ATOMIC, ExprFormatted: lambda _: _OperatorPrecedence.ATOMIC, - # Container displays. ExprList: lambda _: _OperatorPrecedence.ATOMIC, ExprTuple: lambda _: _OperatorPrecedence.ATOMIC, ExprSet: lambda _: _OperatorPrecedence.ATOMIC, ExprDict: lambda _: _OperatorPrecedence.ATOMIC, - # Comprehensions are self-contained units that produce a container. ExprListComp: lambda _: _OperatorPrecedence.ATOMIC, ExprSetComp: lambda _: _OperatorPrecedence.ATOMIC, ExprDictComp: lambda _: _OperatorPrecedence.ATOMIC, - ExprAttribute: lambda _: _OperatorPrecedence.CALL_ATTRIBUTE, ExprSubscript: lambda _: _OperatorPrecedence.CALL_ATTRIBUTE, ExprCall: lambda _: _OperatorPrecedence.CALL_ATTRIBUTE, - ExprUnaryOp: lambda e: {"not": _OperatorPrecedence.NOT}.get(e.operator, _OperatorPrecedence.POS_NEG_BIT_NOT), ExprBinOp: lambda e: { "**": _OperatorPrecedence.EXPONENT, @@ -1041,21 +1037,16 @@ def iterate(self, *, flat: bool = True) -> Iterator[str | Expr]: "|": _OperatorPrecedence.BIT_OR, }.get(e.operator), ExprBoolOp: lambda e: {"and": _OperatorPrecedence.AND, "or": _OperatorPrecedence.OR}.get(e.operator), - ExprCompare: lambda _: _OperatorPrecedence.COMP_MEMB_ID, ExprIfExp: lambda _: _OperatorPrecedence.IF_ELSE, ExprNamedExpr: lambda _: _OperatorPrecedence.ASSIGN, - ExprLambda: lambda _: _OperatorPrecedence.LAMBDA, # NOTE: Ruff categorizes as atomic, but `(a for a in b).c` implies its less than `CALL_ATTRIBUTE`. ExprGeneratorExp: lambda _: _OperatorPrecedence.LAMBDA, - ExprVarPositional: lambda _: _OperatorPrecedence.STARRED, ExprVarKeyword: lambda _: _OperatorPrecedence.STARRED, - ExprYield: lambda _: _OperatorPrecedence.YIELD, ExprYieldFrom: lambda _: _OperatorPrecedence.YIELD, - # These are not standalone, they appear in specific contexts where precendence is not a concern. # NOTE: `for ... in ... if` part, not the whole `[...]`. ExprComprehension: lambda _: _OperatorPrecedence.NONE, From 86d7ef5194fbb809ab0c36fd4cd3b2691300e54c Mon Sep 17 00:00:00 2001 From: Abraham Cheung <58929011+cathaypacific8747@users.noreply.github.com> Date: Fri, 18 Jul 2025 20:04:59 +0800 Subject: [PATCH 8/8] ci: fix mypy --- src/_griffe/expressions.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/_griffe/expressions.py b/src/_griffe/expressions.py index e8aab4845..190f46fb6 100644 --- a/src/_griffe/expressions.py +++ b/src/_griffe/expressions.py @@ -1035,8 +1035,8 @@ def iterate(self, *, flat: bool = True) -> Iterator[str | Expr]: "&": _OperatorPrecedence.BIT_AND, "^": _OperatorPrecedence.BIT_XOR, "|": _OperatorPrecedence.BIT_OR, - }.get(e.operator), - ExprBoolOp: lambda e: {"and": _OperatorPrecedence.AND, "or": _OperatorPrecedence.OR}.get(e.operator), + }[e.operator], + ExprBoolOp: lambda e: {"and": _OperatorPrecedence.AND, "or": _OperatorPrecedence.OR}[e.operator], ExprCompare: lambda _: _OperatorPrecedence.COMP_MEMB_ID, ExprIfExp: lambda _: _OperatorPrecedence.IF_ELSE, ExprNamedExpr: lambda _: _OperatorPrecedence.ASSIGN,