Tolerate but ignore UseImportPolicy to cover additional recipes

[timtebeek]

What's your motivation?

• Support additional Refaster features seen in error-prone-support #47

Anything in particular you'd like reviewers to focus on?

We now skip templates that start with if ; no checks yet on only having a single statement, or not using while/for/...
We could track support for any of those separately in #47 if we think it's worthwhile to support.

Have you considered any alternatives or workarounds?

• We could have generated lambdas that start with the FQN, but that diverges from what folks specified and would require a subsequent clean up.
• We could have changed JavaTemplate to maybeAddImport on any .staticImports("org.mockito.Mockito.never") it receives. That likely might be desirable anyway, but can be picked up separately.

[knutwannheden]

I am unsure about this. Ideally I would like to not use any imports in the template code and then let the qualified name shortening recipe remove any unnecessary qualified names. Do we have any examples where this doesn't work?

[knutwannheden]

Supporting UseImportPolicy on top of that would be nice, but not very critical, IMHO. Possibly we would in the future want a code style to control specific methods to always import statically. That would then possibly be at odds with this annotation. But this is of course all very hypothetical.

[timtebeek]

I've tried this out in rewrite-migrate-java, but from this recipe

package org.openrewrite.java.migrate.io;

import com.google.errorprone.refaster.annotation.AfterTemplate;
import com.google.errorprone.refaster.annotation.BeforeTemplate;

import java.nio.file.Path;

import static java.nio.file.Files.exists;

public class FileExists {
    @BeforeTemplate
    boolean before(Path path) {
        return path.toFile().exists();
    }

    @AfterTemplate
    boolean after(Path path) {
        return exists(path);
    }
}

we end up with a failing test

package org.openrewrite.java.migrate.io;

import org.junit.jupiter.api.Test;
import org.openrewrite.test.RewriteTest;

import static org.openrewrite.java.Assertions.java;

class FileExistsTest implements RewriteTest {
    @Test
    void fileExists() {
        rewriteRun(
          recipeSpec -> recipeSpec.recipe(new FileExistsRecipe()),
          //language=java
          java(
            """
              import java.nio.file.Path;
              class Test {
                  boolean test(Path path) {
                      return path.toFile().exists();
                  }
              }
              """,
            """
              import java.nio.file.Path;
              
              import static java.nio.file.Files.exists;
              
              class Test {
                  boolean test(Path path) {
                      return exists(path);
                  }
              }
              """
          )
        );
    }
}

diff --git a/Test.java b/Test.java
index c502b8f..e381ae5 100644
--- a/Test.java
+++ b/Test.java
@@ -1,7 +1,4 @@ 
 import java.nio.file.Path;
-
-import static java.nio.file.Files.exists;
-
 class Test {
     boolean test(Path path) {
         return exists(path);

[timtebeek]

This is just to show we are able to handle/ignore this annotation; I don't plan to support it's individual values.

[timtebeek]

Note that this maybeAddImport is not necessary when we use Files.exists(path) in the @AfterTemplate instead of a static import there. But unfortunately in error-prone-support they use this style often, such as in MockitoRules.

[timtebeek]

Found the source of the failure in

class CheckIndexConditional {
    @BeforeTemplate
    void before(int index, int size) {
        if (index < 0 || index >= size) {
            throw new IndexOutOfBoundsException();
        }
    }

    @AfterTemplate
    void after(int index, int size) {
        checkIndex(index, size);
    }
}

The if conditional leads to a compilation error when we produce the lambda.

[timtebeek]

With these changes we now go from supporting 94 refaster rule before methods in error-prone-support to covering 152 before methods. 🎉

[timtebeek]

Checked main branch again after the latest changes; for this recipe

import com.google.errorprone.refaster.annotation.AfterTemplate;
import com.google.errorprone.refaster.annotation.BeforeTemplate;

import java.nio.file.Path;

public class FileExists {
    @BeforeTemplate
    boolean before(Path path) {
        return path.toFile().exists();
    }

    @AfterTemplate
    boolean after(Path path) {
        return java.nio.file.Files.exists(path);
    }
}

we generate non-static imports on main

import org.junit.jupiter.api.Test;
import org.openrewrite.test.RewriteTest;

import static org.openrewrite.java.Assertions.java;

class FileExistsTest implements RewriteTest {
    @Test
    void fileExists() {
        rewriteRun(
          recipeSpec -> recipeSpec.recipe(new FileExistsRecipe()),
          //language=java
          java(
            """
              import java.nio.file.Path;
              class Test {
                  boolean test(Path path) {
                      return path.toFile().exists();
                  }
              }
              """,
            """
              import java.nio.file.Files;
              import java.nio.file.Path;
              
              class Test {
                  boolean test(Path path) {
                      return Files.exists(path);
                  }
              }
              """
          )
        );
    }
}

[timtebeek]

Closing this for now, as we don't yet have a method of conflict resolution if we were to aim for static imports.

[timtebeek]

We do support these additional recipes with non-static imports, and uncovered and fixed a few more issues, so not much work lost.

[timtebeek]

Ah no wait; we should still tolerate UseImportPolicy, even if we ignore it's hints. 🙃

[knutwannheden]

Can we really already remove this code?