Skip to content

Make ExtractExplicitConstructorInvocationArguments templates context-free #1140

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
knutwannheden merged 2 commits into from
Jun 23, 2026
+173 −9
Merged

Make ExtractExplicitConstructorInvocationArguments templates context-free #1140

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e35aa89
Make ExtractExplicitConstructorInvocationArguments templates context-…
knutwannheden Jun 23, 2026
0169784
Apply review feedback: collapse complex-arg guard and template call
knutwannheden Jun 23, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 9 additions & 9 deletions ...java/org/openrewrite/java/migrate/lang/ExtractExplicitConstructorInvocationArguments.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,14 +79,19 @@ public J.MethodDeclaration visitMethodDeclaration(J.MethodDeclaration method, Ex
return md;
}

// Only act when at least one argument actually does work worth surfacing. This is the
// cheapest decisive check, so make it first and bail before any name generation, scope
// scanning, or templating that an unaffected constructor would not need.
if (args.stream().noneMatch(arg -> arg instanceof J.MethodInvocation || arg instanceof J.NewClass)) {
return md;
}

// Plan the extraction: hoist everything but trivial, side-effect-free arguments, in order.
// Only act when at least one extracted argument actually does work worth surfacing.
Set<String> usedNames = new LinkedHashSet<>();
String[] names = new String[args.size()];
Set<String> imports = new LinkedHashSet<>();
StringBuilder declarations = new StringBuilder();
List<Expression> declarationArgs = new ArrayList<>();
boolean anyComplex = false;
for (int i = 0; i < args.size(); i++) {
Expression arg = args.get(i);
if (isInlineSafe(arg)) {
Expand All @@ -105,14 +110,10 @@ public J.MethodDeclaration visitMethodDeclaration(J.MethodDeclaration method, Ex
if (paramType instanceof JavaType.FullyQualified) {
imports.add(((JavaType.FullyQualified) paramType).getFullyQualifiedName());
}
anyComplex |= arg instanceof J.MethodInvocation || arg instanceof J.NewClass;
}
if (!anyComplex) {
return md;
}

// 1. Declare the extracted arguments right before the constructor invocation, preserving their order
JavaTemplate.Builder declarationTemplate = JavaTemplate.builder(declarations.toString()).contextSensitive();
JavaTemplate.Builder declarationTemplate = JavaTemplate.builder(declarations.toString());
for (String fqn : imports) {
declarationTemplate.imports(fqn);
maybeAddImport(fqn);
Expand Down Expand Up @@ -146,8 +147,7 @@ public J.ClassDeclaration visitClassDeclaration(J.ClassDeclaration classDecl, Ex
public J.MethodInvocation visitMethodInvocation(J.MethodInvocation mi, ExecutionContext ctx2) {
mi = super.visitMethodInvocation(mi, ctx2);
if (isExplicitConstructorInvocation(mi)) {
return JavaTemplate.builder(argumentList.toString()).contextSensitive().build()
.apply(getCursor(), mi.getCoordinates().replaceArguments(), inlineArgs.toArray());
return JavaTemplate.apply(argumentList.toString(), getCursor(), mi.getCoordinates().replaceArguments(), inlineArgs.toArray());
}
return mi;
}
Expand Down
164 changes: 164 additions & 0 deletions .../org/openrewrite/java/migrate/lang/ExtractExplicitConstructorInvocationArgumentsTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,17 @@
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.condition.EnabledForJreRange;
import org.openrewrite.DocumentExample;
import org.openrewrite.java.JavaIsoVisitor;
import org.openrewrite.java.tree.Expression;
import org.openrewrite.java.tree.J;
import org.openrewrite.java.tree.TypeUtils;
import org.openrewrite.test.RecipeSpec;
import org.openrewrite.test.RewriteTest;

import java.util.ArrayList;
import java.util.List;

import static org.assertj.core.api.Assertions.assertThat;
import static org.junit.jupiter.api.condition.JRE.JAVA_25;
import static org.openrewrite.java.Assertions.java;
import static org.openrewrite.java.Assertions.javaVersion;
Expand Down Expand Up @@ -499,6 +507,162 @@ class Child extends Parent {
);
}

@Test
void extractWithSourcePathParameterType() {
rewriteRun(
//language=java
java(
"""
package foo;
public class Widget {
}
"""
),
//language=java
java(
"""
package foo;

class Parent {
Parent(Widget w) {
}
}

class Child extends Parent {
Child(Widget w) {
super(identity(w));
}

static Widget identity(Widget w) {
return w;
}
}
""",
"""
package foo;

class Parent {
Parent(Widget w) {
}
}

class Child extends Parent {
Child(Widget w) {
Widget w1 = identity(w);
super(w1);
}

static Widget identity(Widget w) {
return w;
}
}
""",
// Even with a context-free template, the rewritten reference must carry both its type and
// its binding to the freshly created local; otherwise downstream type-aware recipes break.
spec -> spec.afterRecipe(cu -> {
List<J.Identifier> refs = new ArrayList<>();
new JavaIsoVisitor<Integer>() {
@Override
public J.MethodInvocation visitMethodInvocation(J.MethodInvocation mi, Integer p) {
if ("super".equals(mi.getSimpleName())) {
for (Expression a : mi.getArguments()) {
if (a instanceof J.Identifier) {
refs.add((J.Identifier) a);
}
}
}
return super.visitMethodInvocation(mi, p);
}
}.visit(cu, 0);
assertThat(refs).hasSize(1);
assertThat(refs.get(0).getType()).as("type").isNotNull();
assertThat(refs.get(0).getFieldType()).as("fieldType (variable binding)").isNotNull();
})
)
);
}

@Test
void extractWithSourcePathWideningSupertype() {
rewriteRun(
//language=java
java(
"""
package foo;
public class Base {
}
"""
),
//language=java
java(
"""
package foo;
public class Derived extends Base {
}
"""
),
//language=java
java(
"""
package foo;

class Parent {
Parent(Base b) {
}
}

class Child extends Parent {
Child() {
super(makeDerived());
}

static Derived makeDerived() {
return new Derived();
}
}
""",
"""
package foo;

class Parent {
Parent(Base b) {
}
}

class Child extends Parent {
Child() {
Base b = makeDerived();
super(b);
}

static Derived makeDerived() {
return new Derived();
}
}
""",
// The extracted variable is declared with a widening supertype ('Base') that lives in a sibling
// source and is referenced nowhere else in this unit. A context-free template still resolves it
// via the shared type cache, so the declared type must be a real FQ type, not Unknown.
spec -> spec.afterRecipe(cu -> {
List<J.VariableDeclarations> decls = new ArrayList<>();
new JavaIsoVisitor<Integer>() {
@Override
public J.VariableDeclarations visitVariableDeclarations(J.VariableDeclarations vd, Integer p) {
if ("b".equals(vd.getVariables().get(0).getSimpleName())) {
decls.add(vd);
}
return super.visitVariableDeclarations(vd, p);
}
}.visit(cu, 0);
assertThat(decls).hasSize(1);
assertThat(decls.get(0).getTypeExpression().getType()).as("declared type 'Base'").isNotNull();
assertThat(TypeUtils.asFullyQualified(decls.get(0).getTypeExpression().getType()))
.as("declared type is a resolved FQ type, not Unknown").isNotNull();
})
)
);
}

@Test
void extractThisDelegationArgument() {
rewriteRun(
Expand Down
Loading