Python: keep expression/statement wrappers type-correct after visits - #8341
Closed
kdelay wants to merge 5 commits into
Closed
Python: keep expression/statement wrappers type-correct after visits#8341kdelay wants to merge 5 commits into
kdelay wants to merge 5 commits into
Conversation
Py.ExpressionStatement declares an Expression child and Py.StatementExpression a Statement child, but PythonVisitor rebuilt both wrappers with whatever the child visit returned. Python dataclasses do not enforce the annotation, so a visitor that replaces the child with a node of the other kind (e.g. migrating `yield from x` to `await x`, where Py.Await is an Expression but not a Statement) silently produced StatementExpression(Await). The invalid tree only failed later, on the Java side of the RPC round trip, where PythonReceiver.visitStatementExpression casts the child to Statement and threw ClassCastException. Detect the kind change in visit_expression_statement / visit_statement_expression and swap to the matching wrapper. Nodes implementing both Expression and Statement keep their original wrapper, and both wrappers delegate prefix and markers to the child, so no formatting is lost. Verified: new regression tests fail before the fix (invalid wrappers for both directions) and pass after; full rewrite-python suite green (1755 passed, 85 skipped). Fixes openrewrite#8322
isinstance(None, Statement)/isinstance(None, Expression) is already False, so the explicit None guard could never change the outcome. Reorder the two remaining checks to read as the positive statement first.
github-project-automation
Bot
moved this from In Progress
to Ready to Review
in OpenRewrite
timtebeek
marked this pull request as draft
July 29, 2026 11:25
Member
|
Found some more gaps that require a closer look before we merge, and I'd like to get some colleagues involved as well. What made you pick this up here? As I can see you're broadly doing contributions across a wide set of projects. |
timtebeek
force-pushed
the
fix/issue-8322-statement-expression-wrapper
branch
from
July 29, 2026 11:35
89bc46a to
525db98
Compare
Follow-up to the ExpressionStatement/StatementExpression normalization. The previous guard only fired for a replacement that was exclusively the opposite kind, which left three cases building invalid trees: - A replacement that is both Expression and Statement (J.Assignment, J.MethodInvocation) stayed in a stale StatementExpression. The printer only treats Block/CompilationUnit/ExpressionStatement parents as regular assignments, so `yield 1` -> `a = 1` printed the walrus form `a := 1`, which is not valid in statement position. - An already-wrapped replacement passed the guard (both wrappers are themselves Expression and Statement) and got wrapped a second time. - A deleted child (visit returning None) fell through to replace(), leaving a wrapper holding None that crashed on the delegating markers property or silently emitted a function with an empty body. Both methods now share one helper that applies the least wrapping making the child usable as both an expression and a statement, and propagate None so the enclosing block drops the statement.
ExpressionStatement stores neither prefix nor markers, exposing the wrapped expression's instead. Without a replace() override, replace_if_changed rebuilds only from init fields, so replace(prefix=...) and replace(markers=...) were silently dropped: a pointless clone came back with the original values. This hit every ExpressionStatement, including the ones the parser emits for `x: int`, so auto-format and blank-line normalization no-oped on them and marker attachment such as SearchResult vanished. StatementExpression already carried this override. Give the behavior a name and share it between the two wrappers rather than duplicating the logic.
timtebeek
force-pushed
the
fix/issue-8322-statement-expression-wrapper
branch
from
July 29, 2026 11:38
525db98 to
b020b5f
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Py.ExpressionStatementdeclares anExpressionchild andPy.StatementExpressionaStatementchild, butPythonVisitorrebuilt both wrappers with whatever the child visit returned. Python dataclasses do not enforce the annotation, so a visitor replacing the child with a node of the other kind (e.g. migratingyield from xtoawait x, wherePy.Awaitis anExpressionbut not aStatement) silently producedStatementExpression(Await). The invalid tree only failed later, on the Java side of the RPC round trip, wherePythonReceiver.visitStatementExpressioncasts the child toStatementand throwsClassCastException.Fix (
visitor.py)visit_expression_statement/visit_statement_expressionnow check the visited child's kind. When it no longer fits the wrapper's declared type but fits the opposite one, the wrapper is swapped (ExpressionStatement<->StatementExpression) instead of rebuilding an invalid node.ExpressionandStatementkeep their original wrapper. Both wrappers delegate prefix and markers to the child, so no formatting is lost.Verification
New regression tests (
tests/python/test_wrapper_normalization.py) cover both directions plus the no-op cases; before the fix 3 of 4 fail with the invalid wrappers, after the fix all 4 pass.Full
rewrite-pythonsuite green: 1755 passed, 85 skipped.Fixes ClassCastException: Py.Await wrapped in Py.StatementExpression cannot be cast to Statement on RPC receive #8322