From e35aa89c1846db3afb7a2c6e370ee0429279fc37 Mon Sep 17 00:00:00 2001 From: Knut Wannheden Date: Tue, 23 Jun 2026 10:59:42 +0200 Subject: [PATCH 1/2] Make ExtractExplicitConstructorInvocationArguments templates context-free The two JavaTemplate uses were context-sensitive, which reconstructs and reparses the enclosing class and method on every application. Neither needs it: contextSensitive() only reconstructs surrounding local scope into the parse stub, not type resolution. Every type these templates declare is the type of an existing super(..)/this(..) argument, so it is already present in the unit and resolves via the shared type cache, and the spliced result is re-attributed against the real cursor afterward. Also bail on the cheapest decisive check (no method-invocation/object-creation argument) before any variable-name generation, scope scanning, or templating. Add regression tests covering a source-path parameter type and a widening source-path supertype, asserting the rewritten reference keeps its variable binding and the extracted declaration keeps a resolved type. --- ...xplicitConstructorInvocationArguments.java | 24 ++- ...citConstructorInvocationArgumentsTest.java | 164 ++++++++++++++++++ 2 files changed, 180 insertions(+), 8 deletions(-) diff --git a/src/main/java/org/openrewrite/java/migrate/lang/ExtractExplicitConstructorInvocationArguments.java b/src/main/java/org/openrewrite/java/migrate/lang/ExtractExplicitConstructorInvocationArguments.java index 21fc0554a2..f40f170868 100644 --- a/src/main/java/org/openrewrite/java/migrate/lang/ExtractExplicitConstructorInvocationArguments.java +++ b/src/main/java/org/openrewrite/java/migrate/lang/ExtractExplicitConstructorInvocationArguments.java @@ -79,14 +79,26 @@ 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. + boolean anyComplex = false; + for (Expression arg : args) { + if (arg instanceof J.MethodInvocation || arg instanceof J.NewClass) { + anyComplex = true; + break; + } + } + if (!anyComplex) { + 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 usedNames = new LinkedHashSet<>(); String[] names = new String[args.size()]; Set imports = new LinkedHashSet<>(); StringBuilder declarations = new StringBuilder(); List declarationArgs = new ArrayList<>(); - boolean anyComplex = false; for (int i = 0; i < args.size(); i++) { Expression arg = args.get(i); if (isInlineSafe(arg)) { @@ -105,14 +117,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); @@ -146,7 +154,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() + return JavaTemplate.builder(argumentList.toString()).build() .apply(getCursor(), mi.getCoordinates().replaceArguments(), inlineArgs.toArray()); } return mi; diff --git a/src/test/java/org/openrewrite/java/migrate/lang/ExtractExplicitConstructorInvocationArgumentsTest.java b/src/test/java/org/openrewrite/java/migrate/lang/ExtractExplicitConstructorInvocationArgumentsTest.java index b0d8175d5d..a12e5341fa 100644 --- a/src/test/java/org/openrewrite/java/migrate/lang/ExtractExplicitConstructorInvocationArgumentsTest.java +++ b/src/test/java/org/openrewrite/java/migrate/lang/ExtractExplicitConstructorInvocationArgumentsTest.java @@ -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; @@ -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 refs = new ArrayList<>(); + new JavaIsoVisitor() { + @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 decls = new ArrayList<>(); + new JavaIsoVisitor() { + @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( From 0169784fb1b14b1407df40717521832c83d2b495 Mon Sep 17 00:00:00 2001 From: Knut Wannheden Date: Tue, 23 Jun 2026 11:24:19 +0200 Subject: [PATCH 2/2] Apply review feedback: collapse complex-arg guard and template call - Replace the `anyComplex` flag + loop + `if` with a single `noneMatch` guard. - Use the static `JavaTemplate.apply(..)` for the argument-list rewrite. --- ...xtractExplicitConstructorInvocationArguments.java | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-) diff --git a/src/main/java/org/openrewrite/java/migrate/lang/ExtractExplicitConstructorInvocationArguments.java b/src/main/java/org/openrewrite/java/migrate/lang/ExtractExplicitConstructorInvocationArguments.java index f40f170868..6a4eb7bcff 100644 --- a/src/main/java/org/openrewrite/java/migrate/lang/ExtractExplicitConstructorInvocationArguments.java +++ b/src/main/java/org/openrewrite/java/migrate/lang/ExtractExplicitConstructorInvocationArguments.java @@ -82,14 +82,7 @@ public J.MethodDeclaration visitMethodDeclaration(J.MethodDeclaration method, Ex // 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. - boolean anyComplex = false; - for (Expression arg : args) { - if (arg instanceof J.MethodInvocation || arg instanceof J.NewClass) { - anyComplex = true; - break; - } - } - if (!anyComplex) { + if (args.stream().noneMatch(arg -> arg instanceof J.MethodInvocation || arg instanceof J.NewClass)) { return md; } @@ -154,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()).build() - .apply(getCursor(), mi.getCoordinates().replaceArguments(), inlineArgs.toArray()); + return JavaTemplate.apply(argumentList.toString(), getCursor(), mi.getCoordinates().replaceArguments(), inlineArgs.toArray()); } return mi; }