Skip to content

Fix ChangeMethodInvocationReturnType changing variable type for nested matches#7993

Merged
timtebeek merged 3 commits into
mainfrom
fix/change-method-invocation-return-type-nested-arg
Jun 12, 2026
Merged

Fix ChangeMethodInvocationReturnType changing variable type for nested matches#7993
timtebeek merged 3 commits into
mainfrom
fix/change-method-invocation-return-type-nested-arg

Conversation

@timtebeek

@timtebeek timtebeek commented Jun 12, 2026

Copy link
Copy Markdown
Member

What's changed?

ChangeMethodInvocationReturnType rewrote the declared type of a variable whenever a matching method invocation appeared anywhere in the variable declaration subtree — including when the match was nested as an argument to another call, rather than being the initializer itself.

visitVariableDeclarations set a methodUpdated flag from any descendant match and then unconditionally rewrote the declared type. This change additionally requires that one of the variables' initializer is the matched invocation before rewriting the declared type.

What's your motivation?

The Apache POI 3.17 migration runs:

- org.openrewrite.java.ChangeMethodInvocationReturnType:
    methodPattern: org.apache.poi.ss.usermodel.Cell getCellType()
    newReturnType: org.apache.poi.ss.usermodel.CellType

Against code like:

Cell xlsxCell = xlsxRow.createCell(xlsCell.getColumnIndex(), xlsCell.getCellType());

getCellType() is only an argument to createCell(...), but the recipe rewrote the declaration to CellType xlsxCell = ..., producing uncompilable code (Cell cannot be converted to CellType).

Anything in particular you'd like reviewers to focus on?

The nested method invocation's own return-type attribution is still updated (that is correct); only the enclosing variable's declared type is now left alone when the match isn't the initializer. Added a regression test covering both the legitimate (direct initializer) and the previously-broken (nested argument) cases.

Checklist

  • I've added unit tests to cover my changes

…d matches

When the matched method invocation appeared nested inside the initializer
(e.g. as an argument to another call) rather than as the initializer itself,
the recipe still rewrote the declared variable type. For example
`Cell c = row.createCell(i, other.getCellType())` would have `Cell` replaced
with `CellType`, producing uncompilable code.

Only rewrite the declared type when a variable's initializer is itself the
matched method invocation.

Fixes openrewrite/rewrite-apache#134
@timtebeek

Copy link
Copy Markdown
Member Author

Note for reviewers: the failing checks are all in :rewrite-csharp:integTest (java.lang.IllegalStateException: RPC process shut down early with exit code 0), affecting even basic tests like parseSimpleProject/xmlDocComment. This is the .NET RPC subprocess failing to start on the CI runner and is unrelated to this change, which only touches ChangeMethodInvocationReturnType in rewrite-java (whose tests pass). Re-running the failed jobs reproduced the identical C# RPC startup failure.

@timtebeek

Copy link
Copy Markdown
Member Author

Root cause of the red check (confirmed across 3 runs): :rewrite-csharp:integTest fails with IllegalStateException: RPC process shut down early with exit code 0 — the .NET RPC subprocess won't start on the CI runner. This affects even trivial tests (parseSimpleProject, xmlDocComment).

It's pre-existing and unrelated to this change: because this PR modifies rewrite-java (a dependency of rewrite-csharp), it invalidates the Gradle build-cache entry for :rewrite-csharp:integTest, so the task actually executes instead of being served from cache — exposing the broken C# RPC startup. PRs that don't touch rewrite-java get the cached result and stay green; PRs that do (e.g. #7988) hit the same failure. rewrite-java:test (including this change's new tests) passes.

This needs a maintainer to either merge past the known-broken C# integTest or fix the .NET RPC startup in CI.

Unwrap parentheses and recurse into ternary branches when checking
whether a variable is initialized by the matched invocation. Typecast
initializers stay excluded: the cast determines the declared type.

@MBoegers MBoegers left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a missing path where Cell xlsxCell = (xlsxRow.createCell()) and Cell xlsxCell = someFlag ? xlsxRow.createCell() : null would be missed.

@github-project-automation github-project-automation Bot moved this from In Progress to Ready to Review in OpenRewrite Jun 12, 2026
@timtebeek timtebeek merged commit a731e6b into main Jun 12, 2026
1 check failed
@timtebeek timtebeek deleted the fix/change-method-invocation-return-type-nested-arg branch June 12, 2026 13:13
@github-project-automation github-project-automation Bot moved this from Ready to Review to Done in OpenRewrite Jun 12, 2026
@Pumu3

Pumu3 commented Jun 12, 2026

Copy link
Copy Markdown

Thank you for all @timtebeek. Waiting the mext release for remove custom fix

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

Wrong Cell by CellType logic

3 participants