Python: Parenthesize template results based on surrounding context (#6827)

* Python: Parenthesize template results based on surrounding context

Template.apply() now checks whether the returned expression needs
parentheses when placed in the surrounding AST context, mirroring
JavaTemplate.doApply()'s call to ParenthesizeVisitor.maybeParenthesize.

Restructures Template.apply() into three phases:
1. Placeholder substitution
2. Precedence-based parenthesization (new)
3. Coordinate application (prefix, statement wrapping, formatting)

* Python: Address review feedback on template parenthesization

Remove dead `coordinates` and `cursor` params from `apply_substitutions`,
extend `_get_precedence` to handle `not` unary (precedence 3), fix test
calling renamed `apply_coordinates`, and add clarifying docstring for the
outer/inner parenthesization boundary.

* Python: Fix template tests to match updated apply_substitutions signature

Remove stale `cursor=None` kwarg from test calls after the parameter
was removed from TemplateEngine.apply_substitutions in 7ca8044.

Author: knutwannheden

rewrite-python/rewrite/src/rewrite/python/template/engine.py

@@ -116,17 +116,13 @@ def apply_substitutions(
         cls,
         template_tree: J,
         values: Dict[str, J],
-        cursor: 'Cursor',
-        coordinates: Optional[PythonCoordinates] = None
     ) -> Optional[J]:
         """
         Substitute placeholder identifiers with actual values.
 
         Args:
             template_tree: The parsed template AST.
             values: Dict mapping capture names to their AST values.
-            cursor: Current cursor position.
-            coordinates: Where/how to apply the template.
 
         Returns:
             The template with placeholders replaced by actual values.
@@ -135,13 +131,7 @@ def apply_substitutions(
 
         # Replace placeholders with actual values
         visitor = PlaceholderReplacementVisitor(values)
-        result = visitor.visit(template_tree, None)
-
-        # Apply coordinates (wrap in statement if needed, etc.)
-        if coordinates and result is not None:
-            result = cls._apply_coordinates(result, cursor, coordinates)
-
-        return result
+        return visitor.visit(template_tree, None)
 
     @classmethod
     def _make_cache_key(
@@ -286,7 +276,7 @@ def _extract_from_method_body(cls, method: j.MethodDeclaration, is_expression: b
         return statements[0]
 
     @classmethod
-    def _apply_coordinates(
+    def apply_coordinates(
         cls,
         result: J,
         cursor: 'Cursor',

rewrite-python/rewrite/src/rewrite/python/template/replacement.py

@@ -27,7 +27,7 @@
 from .placeholder import from_placeholder
 
 if TYPE_CHECKING:
-    pass
+    from rewrite.visitor import Cursor
 
 
 # Operator precedence for Python binary operators (higher number = higher precedence).
@@ -67,11 +67,13 @@
 
 
 def _get_precedence(expr: J) -> Optional[int]:
-    """Return the precedence of an expression, or None if not a binary op."""
+    """Return the precedence of an expression, or None if not an operator node."""
     if isinstance(expr, j.Binary):
         return _BINARY_PRECEDENCE.get(expr.operator)
     if isinstance(expr, py.Binary):
         return _BINARY_PRECEDENCE.get(expr.operator)
+    if isinstance(expr, j.Unary) and expr.operator == j.Unary.Type.Not:
+        return 3  # `not` sits between `and` (2) and comparisons (4)
     return None
 
 
@@ -107,6 +109,41 @@ def _needs_parens_under_not(child: J) -> bool:
     return child_prec < 3
 
 
+def maybe_parenthesize(result: J, cursor: 'Cursor') -> J:
+    """Wrap *result* in parentheses when its precedence is lower than the
+    surrounding context (the cursor's parent tree node).
+
+    This handles the *outer* boundary: whether the template result as a whole
+    needs parentheses relative to where it is being inserted in the tree.
+    The *inner* operand parenthesization (within the template result itself)
+    is handled by ``PlaceholderReplacementVisitor.visit_binary`` / ``visit_unary``.
+
+    This mirrors ``ParenthesizeVisitor.maybeParenthesize`` in the Java
+    ``JavaTemplate`` implementation.
+    """
+    if not isinstance(result, (j.Binary, py.Binary, j.Unary)):
+        return result
+
+    parent_cursor = cursor.parent_tree_cursor()
+    parent = parent_cursor.value
+    if not isinstance(parent, J):
+        return result
+
+    # Parent is a binary operator — check precedence
+    if isinstance(parent, (j.Binary, py.Binary)):
+        parent_prec = _get_precedence(parent)
+        result_prec = _get_precedence(result)
+        if parent_prec is not None and result_prec is not None and result_prec < parent_prec:
+            return _wrap_in_parens(result)
+
+    # Parent is `not` — or/and need parens underneath
+    if isinstance(parent, j.Unary) and parent.operator == j.Unary.Type.Not:
+        if _needs_parens_under_not(result):
+            return _wrap_in_parens(result)
+
+    return result
+
+
 class PlaceholderReplacementVisitor(PythonVisitor[None]):
     """
     Visitor that replaces placeholder identifiers with actual values.

rewrite-python/rewrite/src/rewrite/python/template/template.py

@@ -138,27 +138,32 @@ def apply(
                 # Assume it's a MatchResult
                 values_dict = values.as_dict()
 
-        # Apply substitutions
+        # Phase 1: placeholder substitution (no coordinates yet)
         if values_dict:
             result = TemplateEngine.apply_substitutions(
                 template_tree,
                 values_dict,
-                cursor,
-                coordinates,
             )
         else:
             result = template_tree
 
-        # If no explicit coordinates, create default replacement coordinates
-        if coordinates is None and cursor is not None:
+        # Phase 2: parenthesize the result if it has lower precedence than
+        # the surrounding context, mirroring JavaTemplate.doApply().
+        # This must happen before coordinates are applied, because
+        # apply_coordinates may wrap the expression in ExpressionStatement.
+        if result is not None and cursor is not None:
+            from .replacement import maybe_parenthesize
+            result = maybe_parenthesize(result, cursor)
+
+        # Phase 3: apply coordinates (prefix preservation, statement wrapping, auto-format)
+        effective_coords = coordinates
+        if effective_coords is None and cursor is not None:
             tree = cursor.value
             if tree is not None:
-                result = TemplateEngine.apply_substitutions(
-                    result if values_dict else template_tree,
-                    values_dict,
-                    cursor,
-                    PythonCoordinates.replace(tree),
-                )
+                effective_coords = PythonCoordinates.replace(tree)
+
+        if effective_coords is not None and result is not None:
+            result = TemplateEngine.apply_coordinates(result, cursor, effective_coords)
 
         return result
 

rewrite-python/rewrite/tests/python/template/test_engine.py

@@ -329,7 +329,7 @@ def bar(self):
         )
 
     def test_apply_coordinates_no_cu_context_does_not_crash(self):
-        """_apply_coordinates with cursor lacking CU context doesn't crash."""
+        """apply_coordinates with cursor lacking CU context doesn't crash."""
         from rewrite.visitor import Cursor
         from rewrite.python.template.coordinates import PythonCoordinates
 
@@ -340,5 +340,5 @@ def test_apply_coordinates_no_cu_context_does_not_crash(self):
         coordinates = PythonCoordinates.replace(original)
         cursor = Cursor(parent=Cursor(None, Cursor.ROOT_VALUE), value=original)
 
-        out = TemplateEngine._apply_coordinates(result, cursor, coordinates)
+        out = TemplateEngine.apply_coordinates(result, cursor, coordinates)
         assert out is not None