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 google/error-prone#4432 (comment) 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 😉

[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]

@timtebeek I've narrowed down yesterdays issue as far as i was able to and filed openrewrite/rewrite#4280 and (as a bonus discovery 😉) openrewrite/rewrite#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.