Skip to content

Commit 9105dbd

Browse files
knutwannhedenknutwannheden
authored
Make ExtractExplicitConstructorInvocationArguments templates context-free (#1140)
* 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. * 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.
1 parent 1970361 commit 9105dbd

2 files changed

Lines changed: 173 additions & 9 deletions

File tree

‎src/main/java/org/openrewrite/java/migrate/lang/ExtractExplicitConstructorInvocationArguments.java‎

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -79,14 +79,19 @@ public J.MethodDeclaration visitMethodDeclaration(J.MethodDeclaration method, Ex
7979
return md;
8080
}
8181

82+
// Only act when at least one argument actually does work worth surfacing. This is the
83+
// cheapest decisive check, so make it first and bail before any name generation, scope
84+
// scanning, or templating that an unaffected constructor would not need.
85+
if (args.stream().noneMatch(arg -> arg instanceof J.MethodInvocation || arg instanceof J.NewClass)) {
86+
return md;
87+
}
88+
8289
// Plan the extraction: hoist everything but trivial, side-effect-free arguments, in order.
83-
// Only act when at least one extracted argument actually does work worth surfacing.
8490
Set<String> usedNames = new LinkedHashSet<>();
8591
String[] names = new String[args.size()];
8692
Set<String> imports = new LinkedHashSet<>();
8793
StringBuilder declarations = new StringBuilder();
8894
List<Expression> declarationArgs = new ArrayList<>();
89-
boolean anyComplex = false;
9095
for (int i = 0; i < args.size(); i++) {
9196
Expression arg = args.get(i);
9297
if (isInlineSafe(arg)) {
@@ -105,14 +110,10 @@ public J.MethodDeclaration visitMethodDeclaration(J.MethodDeclaration method, Ex
105110
if (paramType instanceof JavaType.FullyQualified) {
106111
imports.add(((JavaType.FullyQualified) paramType).getFullyQualifiedName());
107112
}
108-
anyComplex |= arg instanceof J.MethodInvocation || arg instanceof J.NewClass;
109-
}
110-
if (!anyComplex) {
111-
return md;
112113
}
113114

114115
// 1. Declare the extracted arguments right before the constructor invocation, preserving their order
115-
JavaTemplate.Builder declarationTemplate = JavaTemplate.builder(declarations.toString()).contextSensitive();
116+
JavaTemplate.Builder declarationTemplate = JavaTemplate.builder(declarations.toString());
116117
for (String fqn : imports) {
117118
declarationTemplate.imports(fqn);
118119
maybeAddImport(fqn);
@@ -146,8 +147,7 @@ public J.ClassDeclaration visitClassDeclaration(J.ClassDeclaration classDecl, Ex
146147
public J.MethodInvocation visitMethodInvocation(J.MethodInvocation mi, ExecutionContext ctx2) {
147148
mi = super.visitMethodInvocation(mi, ctx2);
148149
if (isExplicitConstructorInvocation(mi)) {
149-
return JavaTemplate.builder(argumentList.toString()).contextSensitive().build()
150-
.apply(getCursor(), mi.getCoordinates().replaceArguments(), inlineArgs.toArray());
150+
return JavaTemplate.apply(argumentList.toString(), getCursor(), mi.getCoordinates().replaceArguments(), inlineArgs.toArray());
151151
}
152152
return mi;
153153
}

‎src/test/java/org/openrewrite/java/migrate/lang/ExtractExplicitConstructorInvocationArgumentsTest.java‎

Lines changed: 164 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,9 +18,17 @@
1818
import org.junit.jupiter.api.Test;
1919
import org.junit.jupiter.api.condition.EnabledForJreRange;
2020
import org.openrewrite.DocumentExample;
21+
import org.openrewrite.java.JavaIsoVisitor;
22+
import org.openrewrite.java.tree.Expression;
23+
import org.openrewrite.java.tree.J;
24+
import org.openrewrite.java.tree.TypeUtils;
2125
import org.openrewrite.test.RecipeSpec;
2226
import org.openrewrite.test.RewriteTest;
2327

28+
import java.util.ArrayList;
29+
import java.util.List;
30+
31+
import static org.assertj.core.api.Assertions.assertThat;
2432
import static org.junit.jupiter.api.condition.JRE.JAVA_25;
2533
import static org.openrewrite.java.Assertions.java;
2634
import static org.openrewrite.java.Assertions.javaVersion;
@@ -499,6 +507,162 @@ class Child extends Parent {
499507
);
500508
}
501509

510+
@Test
511+
void extractWithSourcePathParameterType() {
512+
rewriteRun(
513+
//language=java
514+
java(
515+
"""
516+
package foo;
517+
public class Widget {
518+
}
519+
"""
520+
),
521+
//language=java
522+
java(
523+
"""
524+
package foo;
525+
526+
class Parent {
527+
Parent(Widget w) {
528+
}
529+
}
530+
531+
class Child extends Parent {
532+
Child(Widget w) {
533+
super(identity(w));
534+
}
535+
536+
static Widget identity(Widget w) {
537+
return w;
538+
}
539+
}
540+
""",
541+
"""
542+
package foo;
543+
544+
class Parent {
545+
Parent(Widget w) {
546+
}
547+
}
548+
549+
class Child extends Parent {
550+
Child(Widget w) {
551+
Widget w1 = identity(w);
552+
super(w1);
553+
}
554+
555+
static Widget identity(Widget w) {
556+
return w;
557+
}
558+
}
559+
""",
560+
// Even with a context-free template, the rewritten reference must carry both its type and
561+
// its binding to the freshly created local; otherwise downstream type-aware recipes break.
562+
spec -> spec.afterRecipe(cu -> {
563+
List<J.Identifier> refs = new ArrayList<>();
564+
new JavaIsoVisitor<Integer>() {
565+
@Override
566+
public J.MethodInvocation visitMethodInvocation(J.MethodInvocation mi, Integer p) {
567+
if ("super".equals(mi.getSimpleName())) {
568+
for (Expression a : mi.getArguments()) {
569+
if (a instanceof J.Identifier) {
570+
refs.add((J.Identifier) a);
571+
}
572+
}
573+
}
574+
return super.visitMethodInvocation(mi, p);
575+
}
576+
}.visit(cu, 0);
577+
assertThat(refs).hasSize(1);
578+
assertThat(refs.get(0).getType()).as("type").isNotNull();
579+
assertThat(refs.get(0).getFieldType()).as("fieldType (variable binding)").isNotNull();
580+
})
581+
)
582+
);
583+
}
584+
585+
@Test
586+
void extractWithSourcePathWideningSupertype() {
587+
rewriteRun(
588+
//language=java
589+
java(
590+
"""
591+
package foo;
592+
public class Base {
593+
}
594+
"""
595+
),
596+
//language=java
597+
java(
598+
"""
599+
package foo;
600+
public class Derived extends Base {
601+
}
602+
"""
603+
),
604+
//language=java
605+
java(
606+
"""
607+
package foo;
608+
609+
class Parent {
610+
Parent(Base b) {
611+
}
612+
}
613+
614+
class Child extends Parent {
615+
Child() {
616+
super(makeDerived());
617+
}
618+
619+
static Derived makeDerived() {
620+
return new Derived();
621+
}
622+
}
623+
""",
624+
"""
625+
package foo;
626+
627+
class Parent {
628+
Parent(Base b) {
629+
}
630+
}
631+
632+
class Child extends Parent {
633+
Child() {
634+
Base b = makeDerived();
635+
super(b);
636+
}
637+
638+
static Derived makeDerived() {
639+
return new Derived();
640+
}
641+
}
642+
""",
643+
// The extracted variable is declared with a widening supertype ('Base') that lives in a sibling
644+
// source and is referenced nowhere else in this unit. A context-free template still resolves it
645+
// via the shared type cache, so the declared type must be a real FQ type, not Unknown.
646+
spec -> spec.afterRecipe(cu -> {
647+
List<J.VariableDeclarations> decls = new ArrayList<>();
648+
new JavaIsoVisitor<Integer>() {
649+
@Override
650+
public J.VariableDeclarations visitVariableDeclarations(J.VariableDeclarations vd, Integer p) {
651+
if ("b".equals(vd.getVariables().get(0).getSimpleName())) {
652+
decls.add(vd);
653+
}
654+
return super.visitVariableDeclarations(vd, p);
655+
}
656+
}.visit(cu, 0);
657+
assertThat(decls).hasSize(1);
658+
assertThat(decls.get(0).getTypeExpression().getType()).as("declared type 'Base'").isNotNull();
659+
assertThat(TypeUtils.asFullyQualified(decls.get(0).getTypeExpression().getType()))
660+
.as("declared type is a resolved FQ type, not Unknown").isNotNull();
661+
})
662+
)
663+
);
664+
}
665+
502666
@Test
503667
void extractThisDelegationArgument() {
504668
rewriteRun(

0 commit comments

Comments
 (0)