Refaster adds fully qualified reference to local variables in `@AfterTemplate`-lambdas → migration can't compile

[Philzen]

The following template:

    public static class MigrateAssertEqualsFloatArrayDelta {

        @BeforeTemplate void before(float[] actual, float[] expected, float delta) {
            Assert.assertEquals(actual, expected, delta);
        }

        @AfterTemplate
        void after(float[] actual, float[] expected, float delta) {
            Assertions.assertAll(() -> {
                Assertions.assertEquals(expected.length, actual.length, "Arrays don't have the same size.");
                for (int i = 0; i < actual.length; i++) {
                    Assertions.assertEquals(expected[i], actual[i], delta);
                }
            });
        }
    }

Results in the following unexpected code:

     Assertions.assertAll(() -> {
-        Assertions.assertEquals(expected.length, actual.length, "Arrays don't have the same size.");
-        for (int i = 0; i < actual.length; i++) {
-            Assertions.assertEquals(expected[i], actual[i], 0.1f);
+        Assertions.assertEquals(expected.length, actual.length, "Arrays don\'t have the same size.");
+        for (int i = 0; MigrateAssertions.MigrateAssertEqualsFloatArrayDelta.after.i < actual.length; MigrateAssertions.MigrateAssertEqualsFloatArrayDelta.after.i++) {
+            Assertions.assertEquals(expected[MigrateAssertions.MigrateAssertEqualsFloatArrayDelta.after.i], actual[MigrateAssertions.MigrateAssertEqualsFloatArrayDelta.after.i], 0.1f);
         }
     });

(minor noteworthy detail: the single quote became needlessly escaped)

Looking into the recipe, it does hardcode that reference:

final JavaTemplate after = JavaTemplate
        .builder("org.junit.jupiter.api.Assertions.assertAll(()->{\n    org.junit.jupiter.api.Assertions.assertEquals(#{expected:anyArray(float)}.length, #{actual:anyArray(float)}.length, \"Arrays don\\'t have the same size.\");\n    for (final int i = 0; io.github.mboegers.openrewrite.testngtojupiter.MigrateAssertions.MigrateAssertEqualsFloatArrayDelta.after.i < #{actual}.length; io.github.mboegers.openrewrite.testngtojupiter.MigrateAssertions.MigrateAssertEqualsFloatArrayDelta.after.i++) {\n        org.junit.jupiter.api.Assertions.assertEquals(#{expected}[io.github.mboegers.openrewrite.testngtojupiter.MigrateAssertions.MigrateAssertEqualsFloatArrayDelta.after.i], #{actual}[io.github.mboegers.openrewrite.testngtojupiter.MigrateAssertions.MigrateAssertEqualsFloatArrayDelta.after.i], #{delta:any(float)});\n    }\n});")
        .javaParser(JavaParser.fromJavaVersion().classpath(JavaParser.runtimeClasspath()))
        .build();

I've already bumped into the unfortunate limitation that the @AfterTemplate may only contain a single statement. That's why i was hoping to be able to use a lambda here.

Looking at the refaster documentation, i could not find anything on limitations regarding variable access in the @AfterTemplate, only regarding placeholders and the @BeforeTemplate. Anyway this is not even accessing a variable that's out of scope, it is declared, initialized and thus scope-limited in that same block, which makes it even stranger that Refaster would try to put a full qualification on it.

This happens on rewrite-templating 1.9.2

[timtebeek]

Hi @Philzen ; thanks for reporting this issue! Seems indeed like a bug. We're only using the API that Refaster offers and convert that into OpenRewrite recipes; as such we unfortunately don't yet support the full breadth of what's possible with Refaster itself, in part from not having seen a large variety yet.

In this case I'd lean towards at first recognize this as something uncovered and skip such templates for now; that's how we're only covering ~half of the Picnic Refaster rules. Then we can followup by looking at what might be necessary to support such cases, and how many recipes might benefit from this effort. Help in either department is much appreciated!

[Philzen]

The contributor over at https://github.com/google/error-prone/issues/4432#issuecomment-2163912860 was so kind to do a test confirming that this issue does not exist in "stock" Error Prone. So either it's a bug that was fixed in the upstream and hasn't been pulled into rewrite-templating yet, or it's one of its own customizations that causes this.

On an unrelated side note i just realised that my intended refaster recipe would be a very bad OpenRewrite citizen anyway and should never be allowed. That is b/c the lambda captures variables from the local scope – if they are not (effectively) final, applying the recipe would break the code it is run on, it could not compile anymore. The only way i could think of that would allow such a replacement is to include a check if all of the captured variables are final or effectively final, and skip the replacement if they aren't.

[timtebeek]

thanks for reporting that insight back here; should we already close this issue then if the recipe is not a good fit for automated code changes? Or at least close it until we find a better fit?

[Philzen]

My take is that this is an actual bug, as the variable that's generated faulty here is declared inside the lambda and therefore should be perfectly legal.

Regarding the mentioned insight on final / effectively final variables in a lambda my personal approach would actually be to even file another issue, because that may sooner or later lead to someone somewhere publishing a recipe that breaks code. Given the exceptional experiences i had so far with OpenRewrite and the project's sophistication perceived from my side i'd want to avoid that and improve the refaster generator to analyze JavaTemplates containing lambdas for any local scope variable references and their finality.

But ultimately I'd leave the decision whether to close or not up to you as i can sympathize with the desire to keep the issue count at a minimum.

[Philzen]

It transpired this happens not only to variables declared within the lambda scope, it happens to ALL variables belonging to that scope. Only the references from enclosing scope are OK.

I was hoping that using expression lambdas instead of statement lambdas (which seem get some autoformatting that includes line-breaks) could be a temporary workaround for #92, and this issue struck again:

public static class MigrateAssertEqualsDeep {
//…
    @AfterTemplate void after(Map<?, ?> actual, Map<?, ?> expected) {
        Assertions.assertIterableEquals(
            expected.entrySet().stream().map(entry -> {
                if(!entry.getValue().getClass().isArray()) return entry;
                // convert array to List as the assertion needs an Iterable for proper comparison
                return new AbstractMap.SimpleEntry<>(entry.getKey(), Arrays.toString((Object[]) entry.getValue()));
            }).collect(Collectors.toList()),
            actual.entrySet().stream().map(entry -> {
                if(!entry.getValue().getClass().isArray()) return entry;
                // convert array to List as the assertion needs an Iterable for proper comparison
                return new AbstractMap.SimpleEntry<>(entry.getKey(), Arrays.toString((Object[]) entry.getValue()));
            }).collect(Collectors.toList())
        );
    }
}

produces:

Assertions.assertIterableEquals(Map.of("expected", 11).entrySet().stream().map((entry) -> {
    if (!MigrateAssertions.MigrateAssertEqualsDeep.after.entry.getValue().getClass().isArray()) return MigrateAssertions.MigrateAssertEqualsDeep.after.entry;
    return new AbstractMap.SimpleEntry<>(MigrateAssertions.MigrateAssertEqualsDeep.after.entry.getKey(), Arrays.toString((Object[]) MigrateAssertions.MigrateAssertEqualsDeep.after.entry.getValue()));
}).collect(Collectors.toList()), Map.of("actual", 12).entrySet().stream().map((entry) -> {
    if (!MigrateAssertions.MigrateAssertEqualsDeep.after.entry.getValue().getClass().isArray()) return MigrateAssertions.MigrateAssertEqualsDeep.after.entry;
    return new AbstractMap.SimpleEntry<>(MigrateAssertions.MigrateAssertEqualsDeep.after.entry.getKey(), Arrays.toString((Object[]) MigrateAssertions.MigrateAssertEqualsDeep.after.entry.getValue()));
}).collect(Collectors.toList()));

It does not matter whether i use an expression lambda or a statement lambda as in the above example, all usages of the entry variable will get changed to MigrateAssertions.MigrateAssertEqualsDeep.after.entry.

Unless there is any workaround i haven't thought of yet (any ideas greatly appreciated) i would have to implement the above migration imperatively, forcing me to violate the "If it can be declarative, it should be declarative"-rule. Having said that, it's not really a violation as it currently simply cannot be implemented in a declarative way due to this bug :wink:

[Philzen]

@timtebeek I now tried to implement aforementioned migration as an imperative recipe. Running this recipe blows up with:

java.lang.IllegalStateException: LST contains missing or invalid type information
MethodInvocation->MethodInvocation->MethodInvocation->MethodInvocation->MethodInvocation->Block->MethodDeclaration->Block->ClassDeclaration->CompilationUnit
/*~~(MethodInvocation type is missing or malformed)~~>*/Map.of("expected", 11).entrySet()

Identifier->MethodInvocation->Binary->Binary->Ternary->Lambda->MethodInvocation->MethodInvocation->MethodInvocation->Block->MethodDeclaration->Block->ClassDeclaration->CompilationUnit
/*~~(Identifier type is missing or malformed)~~>*/entry

Identifier->MethodInvocation->MethodInvocation->MethodInvocation->Unary->Binary->Ternary->Lambda->MethodInvocation->MethodInvocation->MethodInvocation->Block->MethodDeclaration->Block->ClassDeclaration->CompilationUnit
/*~~(Identifier type is missing or malformed)~~>*/entry

Identifier->Ternary->Lambda->MethodInvocation->MethodInvocation->MethodInvocation->Block->MethodDeclaration->Block->ClassDeclaration->CompilationUnit
/*~~(Identifier type is missing or malformed)~~>*/entry

Identifier->MethodInvocation->NewClass->Ternary->Lambda->MethodInvocation->MethodInvocation->MethodInvocation->Block->MethodDeclaration->Block->ClassDeclaration->CompilationUnit
/*~~(Identifier type is missing or malformed)~~>*/entry

Identifier->MethodInvocation->TypeCast->MethodInvocation->NewClass->Ternary->Lambda->MethodInvocation->MethodInvocation->MethodInvocation->Block->MethodDeclaration->Block->ClassDeclaration->CompilationUnit
/*~~(Identifier type is missing or malformed)~~>*/entry

MethodInvocation->MethodInvocation->MethodInvocation->MethodInvocation->MethodInvocation->Block->MethodDeclaration->Block->ClassDeclaration->CompilationUnit
/*~~(MethodInvocation type is missing or malformed)~~>*/Map.of("actual", 12).entrySet()

Identifier->MethodInvocation->Binary->Binary->Ternary->Lambda->MethodInvocation->MethodInvocation->MethodInvocation->Block->MethodDeclaration->Block->ClassDeclaration->CompilationUnit
/*~~(Identifier type is missing or malformed)~~>*/entry

Identifier->MethodInvocation->MethodInvocation->MethodInvocation->Unary->Binary->Ternary->Lambda->MethodInvocation->MethodInvocation->MethodInvocation->Block->MethodDeclaration->Block->ClassDeclaration->CompilationUnit
/*~~(Identifier type is missing or malformed)~~>*/entry

Identifier->Ternary->Lambda->MethodInvocation->MethodInvocation->MethodInvocation->Block->MethodDeclaration->Block->ClassDeclaration->CompilationUnit
/*~~(Identifier type is missing or malformed)~~>*/entry

Identifier->MethodInvocation->NewClass->Ternary->Lambda->MethodInvocation->MethodInvocation->MethodInvocation->Block->MethodDeclaration->Block->ClassDeclaration->CompilationUnit
/*~~(Identifier type is missing or malformed)~~>*/entry

Identifier->MethodInvocation->TypeCast->MethodInvocation->NewClass->Ternary->Lambda->MethodInvocation->MethodInvocation->MethodInvocation->Block->MethodDeclaration->Block->ClassDeclaration->CompilationUnit
/*~~(Identifier type is missing or malformed)~~>*/entry

    at org.openrewrite.java.Assertions.assertValidTypes(Assertions.java:87)
    at org.openrewrite.java.Assertions.validateTypes(Assertions.java:57)
    at org.openrewrite.test.RewriteTest.rewriteRun(RewriteTest.java:509)
    at org.openrewrite.test.RewriteTest.rewriteRun(RewriteTest.java:133)
    at org.openrewrite.test.RewriteTest.rewriteRun(RewriteTest.java:128)
    at org.philzen.oss.testng.MigrateAssertEqualsDeepTest$WithoutErrorMessage.map(MigrateAssertEqualsDeepTest.java:21)
    at java.base/java.lang.reflect.Method.invoke(Method.java:580)
    at java.base/java.util.ArrayList.forEach(ArrayList.java:1596)
    at java.base/java.util.ArrayList.forEach(ArrayList.java:1596)
    at java.base/java.util.ArrayList.forEach(ArrayList.java:1596)

I made a dedicated branch for this issue, here are permalinks

• imperative visitor: https://github.com/Philzen/rewrite-TestNG-to-JUnit5/blob/a48da64d3911b853ad5f096e31c6a7bc92aea5af/src/main/java/org/philzen/oss/testng/MigrateAssertEqualsDeep.java
• test (which produces above error message): https://github.com/Philzen/rewrite-TestNG-to-JUnit5/blob/a48da64d3911b853ad5f096e31c6a7bc92aea5af/src/test/java/org/philzen/oss/testng/MigrateAssertEqualsDeepTest.java

:grey_exclamation: Very notable: This seems to happen during some kind of doAfterVisit or second pass (but really no clue what is going on here) – if you simply change a single character in the test (just adding or removing one space of indentation will do) will not produce the LST exception above.

In conclusion this suggests to me this bug is not related to refaster per-se, but a. may be more serious than initially thought and b. probably should be moved to the rewrite-repository.

~For me this is now a showstopper to tick-off the last items on my list for https://github.com/Philzen/rewrite-TestNG-to-JUnit5/pull/3 (or is there anything i have overlooked?).~

:grey_exclamation: I've build the project and quickly ran the imperative recipe on another project → works :partying_face: (so showstopper is lifted at least for imperative style). So this bug must be located in some LST analysis step that is invoked after rewriteRun(java(…)) before and after have been successfully matched.

[Philzen]

To make the confusion perfect: I am able to implement the example from the issue description as an imperative recipe, and it works.

Visitor: https://github.com/Philzen/rewrite-TestNG-to-JUnit5/blob/bba68886dc3f47e49eb9aaca705d68412b0ccc24/src/main/java/org/philzen/oss/testng/MigrateMismatchedAssertions.java

Test: https://github.com/Philzen/rewrite-TestNG-to-JUnit5/blob/bba68886dc3f47e49eb9aaca705d68412b0ccc24/src/test/java/org/philzen/oss/testng/MigrateMismatchedAssertionsTest.java

:exploding_head:

[timtebeek]

You're not setting a classpath on your parser for the test. You'll want to update these lines to do something similar to what you see here. And if you're mainly using Maven, then you might be interested in these improvements to set the classpath recipe jars.

[Philzen]

You're not setting a classpath on your parser for the test.

I did see that classpath setting on other code examples but found all my test suites to work fine without it, that's why it wasn't there. I have now added that and updated the permalinks in my comment above accordingly. Same result.

Also note my latest finding at the bottom of the comment – when compiled and executed on another project the imperative recipe works fine.

And if you're mainly using Maven, then you might be interested in these improvements to set the classpath recipe jars.

Thanks for pointing out those recent updates, i shall definitely sync them into my pom.xml (and also report back if that has any effect on the issue).

[Philzen]

you might be interested in these improvements to set the classpath recipe jars.

I've now applied those updates, no change regarding the LST errors when running the tests. However while refining the implementation of the imperative recipe it transpired that the LST errors are apparently unrelated to this issue, but instead revolving around an edge-case involving Generics. I found that basically any test containing Assert.assertEqualsDeep(Map.of(…), Map.of(…)); will produce it, even if it does not involve any lambdas whatsoever.

I will investigate further to narrow it down and report back tomorrow, probably documenting my findings in a separate issue over at https://github.com/openrewrite/rewrite/.

[Philzen]

@timtebeek I've narrowed down yesterdays issue as far as i was able to and filed

• https://github.com/openrewrite/rewrite/issues/4280 and (as a bonus discovery :wink:) - https://github.com/openrewrite/rewrite/issues/4281 (which also concerns Refaster, but the root cause is out of Refaster's scope).

Have hidden yesterday's comments as "off-topic" in an effort to get the scope of this thread back on track. Kind thanks for your patience with those.