Refaster includes fully qualified reference to the @AfterTemplate to local variable inside the template

[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);
                }
            });
        }
    }

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

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);
         }
     });

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();

During my first adventures with Refaster as part of an OpenRewrite project 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 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.

[Stephan202]

Hey @Philzen! It looks like you're using OpenRewrite's Refaster-to-recipe feature. OpenRewrite's implementation supports a subset of Refaster annotations, completely independently from the official Refaster implementation provided by Error Prone. I think you meant to file this issue against the openrewrite/rewrite-templating project.

(CC @timtebeek :).)

[Philzen]

@Stephan202 ah thanks for the clarification, i was not aware of that (yet) :)

Moving my issue report to openrewrite/rewrite-templating#90 then, assuming this issue never happened with the implementation in this repository (?)

[Stephan202]

assuming this issue never happened with the implementation in this repository (?)

I locally tested that to be sure (we have a project with a test framework for Refaster rules), and I can confirm that with stock Error Prone this rule behaves correctly.

With it, this code:

void testMigrateAssertEqualsFloatArrayDelta() {
  float[] actual = {0F};
  float[] expected = {1F};
  float delta = 0;
  Assert.assertEquals(actual, expected, delta);
}

Is rewritten to:

void testMigrateAssertEqualsFloatArrayDelta() {
  float[] actual = {0F};
  float[] expected = {1F};
  float delta = 0;
  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);
        }   
      }); 
}

(I see that Error Prone also unnecessarily escapes the single quote. In PicnicSupermarket/error-prone-support#1138 I worked around a similar issue. I just noticed that Error Prone also has a utility method for this, but apparently the Refaster code path doesn't use it. @cushon WDYT of upstreaming a more efficient implementation? Happy to open a PR.)

[Philzen]

@Stephan202 thanks a lot for the confirmation!