Tolerate but ignore UseImportPolicy to cover additional recipes#53
Tolerate but ignore UseImportPolicy to cover additional recipes#53
UseImportPolicy to cover additional recipes#53Conversation
And remove `new UsesMethod<>("java.io.File exists(..)")`
|
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? |
|
Supporting |
|
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 testpackage 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); |
| @Target({ElementType.METHOD}) | ||
| @Retention(RetentionPolicy.SOURCE) | ||
| public @interface UseImportPolicy { | ||
| ImportPolicy value(); | ||
| } |
There was a problem hiding this comment.
This is just to show we are able to handle/ignore this annotation; I don't plan to support it's individual values.
| public J visitMethodInvocation(J.MethodInvocation elem, ExecutionContext ctx) { | ||
| JavaTemplate.Matcher matcher; | ||
| if ((matcher = before.matcher(getCursor())).find()) { | ||
| maybeAddImport("java.nio.file.Files", "exists"); |
There was a problem hiding this comment.
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.
|
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 |
|
With these changes we now go from supporting 94 refaster rule before methods in error-prone-support to covering 152 before methods. 🎉 |
timtebeek
changed the title
timtebeek
changed the title
|
Checked 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 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);
}
}
"""
)
);
}
} |
|
Closing this for now, as we don't yet have a method of conflict resolution if we were to aim for static imports. |
|
We do support these additional recipes with non-static imports, and uncovered and fixed a few more issues, so not much work lost. |
|
Ah no wait; we should still tolerate |
timtebeek
changed the title
UseImportPolicy to cover additional recipes
| beforeImports.forEach(anImport -> recipe.append(" maybeRemoveImport(\"").append(anImport).append("\");\n")); | ||
| } | ||
|
|
||
| private void maybeAddStaticImports(Map<JCTree.JCMethodDecl, Set<String>> importsByTemplate, StringBuilder recipe, JCTree.JCMethodDecl beforeTemplate, JCTree.JCMethodDecl afterTemplate) { |
There was a problem hiding this comment.
Can we really already remove this code?
2 participants
What's your motivation?
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?
maybeAddImporton any.staticImports("org.mockito.Mockito.never")it receives. That likely might be desirable anyway, but can be picked up separately.