Introduce additional File and Path Refaster rules
#1282
Conversation
|
Looks good. All 4 mutations in this change were killed.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
| /** Prefer the more idiomatic {@link Path#of(URI)} over {@link Paths#get(URI)}. */ | ||
| static final class PathOfUri { | ||
| @BeforeTemplate | ||
| Path before(URI uri) { | ||
| return Paths.get(uri); | ||
| } | ||
|
|
||
| @AfterTemplate | ||
| Path after(URI uri) { | ||
| return Path.of(uri); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Prefer the more idiomatic {@link Path#of(String, String...)} over {@link Paths#get(String, | ||
| * String...)}. | ||
| */ | ||
| static final class PathOfString { | ||
| @BeforeTemplate | ||
| Path before(String first, @Repeated String more) { | ||
| return Paths.get(first, more); | ||
| } | ||
|
|
||
| @AfterTemplate | ||
| Path after(String first, @Repeated String more) { | ||
| return Path.of(first, more); | ||
| } | ||
| } |
There was a problem hiding this comment.
I recently added these two to my TODO list because I saw openrewrite/rewrite-migrate-java#519, but just now I noticed there was already an older identical TODO entry 😄
There was a problem hiding this comment.
One of the rare cases where our solutions are equal in length. ;)
| /** Avoid redundant conversions from {@link Path} to {@link File}. */ | ||
| // XXX: Review whether a rule such as this one is better handled by the `IdentityConversion` rule. | ||
| static final class PathInstance { | ||
| @BeforeTemplate | ||
| Path before(Path path) { | ||
| return path.toFile().toPath(); | ||
| } | ||
|
|
||
| @AfterTemplate | ||
| Path after(Path path) { | ||
| return path; | ||
| } | ||
| } |
There was a problem hiding this comment.
I added this because I realized that the two FilesCreateTempFile*ToFile rules below make it much more likely that a .toFile().toPath() chain is introduced.
| /** | ||
| * Prefer {@link Files#createTempFile(Path,String, String, FileAttribute[])} over alternatives | ||
| * that create files with more liberal permissions. | ||
| */ | ||
| static final class FilesCreateTempFileInCustomDirectoryToFile { | ||
| @BeforeTemplate | ||
| File before(File directory, String prefix, String suffix) throws IOException { | ||
| return File.createTempFile(prefix, suffix, directory); | ||
| } | ||
|
|
||
| @AfterTemplate | ||
| File after(File directory, String prefix, String suffix) throws IOException { | ||
| return Files.createTempFile(directory.toPath(), prefix, suffix).toFile(); | ||
| } | ||
| } |
There was a problem hiding this comment.
As suggested by @timtebeek here.
Didn't happen 👀. Likely due to the custom directory being specified. |
| static final class FilesCreateTempFileInCustomDirectoryToFile { | ||
| @BeforeTemplate | ||
| File before(File directory, String prefix, String suffix) throws IOException { | ||
| return File.createTempFile(prefix, suffix, directory); |
There was a problem hiding this comment.
Ah, @timtebeek one thing to consider: File.createTempFile(prefix, suffix, null) is now matched by both rules. Not sure how OpenRewrite handles that? (Our custom Refaster integration resolves this kind of ambiguity by selecting the rule that produces the smallest replacement string, which in this case means that it (correctly) selects the rule above.)
There was a problem hiding this comment.
Interesting! I suspect the generated recipe will contain a MethodPattern matching exactly one of these, not either one.
There was a problem hiding this comment.
I suppose that if OpenRewrite applies them in the listed order, the first one will win. Anyway, generated code attached 😄
Generated recipes
/**
* OpenRewrite recipe created for Refaster template {@code FileRules.FilesCreateTempFileToFile}.
*/
@SuppressWarnings("all")
@NonNullApi
@Generated("org.openrewrite.java.template.processor.RefasterTemplateProcessor")
public static class FilesCreateTempFileToFileRecipe extends Recipe {
/**
* Instantiates a new instance.
*/
public FilesCreateTempFileToFileRecipe() {}
@Override
public String getDisplayName() {
return "Prefer `Files#createTempFile(String, String, FileAttribute[])` over alternatives that create files with more liberal permissions";
}
@Override
public String getDescription() {
return "Recipe created for the following Refaster template:\n```java\nstatic final class FilesCreateTempFileToFile {\n \n @BeforeTemplate\n @SuppressWarnings(value = {\"FilesCreateTempFileInCustomDirectoryToFile\", \"java:S5443\", \"key-to-resolve-AnnotationUseStyle-and-TrailingComment-check-conflict\"})\n File before(String prefix, String suffix) throws IOException {\n return Refaster.anyOf(File.createTempFile(prefix, suffix), File.createTempFile(prefix, suffix, null));\n }\n \n @AfterTemplate\n @SuppressWarnings(value = \"java:S5443\")\n File after(String prefix, String suffix) throws IOException {\n return Files.createTempFile(prefix, suffix).toFile();\n }\n}\n```\n.";
}
@Override
public TreeVisitor<?, ExecutionContext> getVisitor() {
JavaVisitor<ExecutionContext> javaVisitor = new AbstractRefasterJavaVisitor() {
final JavaTemplate before$0 = JavaTemplate
.builder("java.io.File.createTempFile(#{prefix:any(java.lang.String)}, #{suffix:any(java.lang.String)})")
.build();
final JavaTemplate before$1 = JavaTemplate
.builder("java.io.File.createTempFile(#{prefix:any(java.lang.String)}, #{suffix:any(java.lang.String)}, null)")
.build();
final JavaTemplate after = JavaTemplate
.builder("java.nio.file.Files.createTempFile(#{prefix:any(java.lang.String)}, #{suffix:any(java.lang.String)}).toFile()")
.build();
@Override
public J visitMethodInvocation(J.MethodInvocation elem, ExecutionContext ctx) {
JavaTemplate.Matcher matcher;
if ((matcher = before$0.matcher(getCursor())).find()) {
return embed(
after.apply(getCursor(), elem.getCoordinates().replace(), matcher.parameter(0), matcher.parameter(1)),
getCursor(),
ctx,
SHORTEN_NAMES
);
}
if ((matcher = before$1.matcher(getCursor())).find()) {
return embed(
after.apply(getCursor(), elem.getCoordinates().replace(), matcher.parameter(0), matcher.parameter(1)),
getCursor(),
ctx,
SHORTEN_NAMES
);
}
return super.visitMethodInvocation(elem, ctx);
}
};
return Preconditions.check(
Preconditions.and(
new UsesType<>("java.io.File", true),
new UsesMethod<>("java.io.File createTempFile(..)")
),
javaVisitor
);
}
}
/**
* OpenRewrite recipe created for Refaster template {@code FileRules.FilesCreateTempFileInCustomDirectoryToFile}.
*/
@SuppressWarnings("all")
@NonNullApi
@Generated("org.openrewrite.java.template.processor.RefasterTemplateProcessor")
public static class FilesCreateTempFileInCustomDirectoryToFileRecipe extends Recipe {
/**
* Instantiates a new instance.
*/
public FilesCreateTempFileInCustomDirectoryToFileRecipe() {}
@Override
public String getDisplayName() {
return "Prefer `Files#createTempFile(Path,String, String, FileAttribute[])` over alternatives that create files with more liberal permissions";
}
@Override
public String getDescription() {
return "Recipe created for the following Refaster template:\n```java\nstatic final class FilesCreateTempFileInCustomDirectoryToFile {\n \n @BeforeTemplate\n File before(File directory, String prefix, String suffix) throws IOException {\n return File.createTempFile(prefix, suffix, directory);\n }\n \n @AfterTemplate\n File after(File directory, String prefix, String suffix) throws IOException {\n return Files.createTempFile(directory.toPath(), prefix, suffix).toFile();\n }\n}\n```\n.";
}
@Override
public TreeVisitor<?, ExecutionContext> getVisitor() {
JavaVisitor<ExecutionContext> javaVisitor = new AbstractRefasterJavaVisitor() {
final JavaTemplate before = JavaTemplate
.builder("java.io.File.createTempFile(#{prefix:any(java.lang.String)}, #{suffix:any(java.lang.String)}, #{directory:any(java.io.File)})")
.build();
final JavaTemplate after = JavaTemplate
.builder("java.nio.file.Files.createTempFile(#{directory:any(java.io.File)}.toPath(), #{prefix:any(java.lang.String)}, #{suffix:any(java.lang.String)}).toFile()")
.build();
@Override
public J visitMethodInvocation(J.MethodInvocation elem, ExecutionContext ctx) {
JavaTemplate.Matcher matcher;
if ((matcher = before.matcher(getCursor())).find()) {
return embed(
after.apply(getCursor(), elem.getCoordinates().replace(), matcher.parameter(2), matcher.parameter(0), matcher.parameter(1)),
getCursor(),
ctx,
SHORTEN_NAMES
);
}
return super.visitMethodInvocation(elem, ctx);
}
};
return Preconditions.check(
Preconditions.and(
new UsesType<>("java.io.File", true),
new UsesMethod<>("java.io.File createTempFile(..)")
),
javaVisitor
);
}
}
There was a problem hiding this comment.
Figured a test would be quickest way to check, but my IDE is throwing up some error
java: exporting a package from system module jdk.compiler is not allowed with --release
tech.picnic.errorprone.refasterrules.FileRulesRecipesTest
package tech.picnic.errorprone.refasterrules;
import org.junit.jupiter.api.Test;
import org.openrewrite.test.RecipeSpec;
import org.openrewrite.test.RewriteTest;
import static org.openrewrite.java.Assertions.java;
final class FileRulesRecipesTest implements RewriteTest {
@Override
public void defaults(RecipeSpec spec) {
spec.recipe(new FileRulesRecipes());
}
@Test
void stringValueOf() {
rewriteRun(
//language=java
java(
"""
import java.io.File;
import java.io.IOException;
class Test {
File twoArgs() throws IOException {
return File.createTempFile("foo", "bar");
}
File threeArgs() throws IOException {
return File.createTempFile("foo", "bar", new File("baz"));
}
}
""",
"""
import java.io.File;
import java.io.IOException;
import java.nio.file.Files;
class Test {
File twoArgs() throws IOException {
return Files.createTempFile("foo", "bar").toFile();
}
File threeArgs() throws IOException {
return Files.createTempFile(new File("baz").toPath(), "foo", "bar").toFile();
}
}
"""
)
);
}
}Now that you're using Java 17 you could also look at this TODO again.
There was a problem hiding this comment.
@timtebeek to fix that error; do the following:
If this happens, go to Settings -> Build, Execution, Deployment -> Compiler -> Java Compiler and deselect the option Use '--release' option for cross-compilation (Java 9 and later). See IDEA-288052 for details.
It's listed in the README :).
There was a problem hiding this comment.
Thanks! With that the above test now passes. 🎉
There was a problem hiding this comment.
I was worried about the File.createTempFile("foo", "bar", null), but when I modify the test like that it still passes 👍
| static final class PathInstance { | ||
| @BeforeTemplate | ||
| Path before(Path path) { | ||
| return path.toFile().toPath(); |
There was a problem hiding this comment.
Should we add a similar rule for: File > Path > File?
There was a problem hiding this comment.
I considered that, but IIUC none of the current rules would introduce such a chain. Also didn't find any occurrence internally. But if you prefer we can definitely add such a rule. You up for it? :)
| @@ -63,4 +113,20 @@ File after(String prefix, String suffix) throws IOException { | |||
| return Files.createTempFile(prefix, suffix).toFile(); | |||
| } | |||
| } | |||
|
|
|||
| /** | |||
| * Prefer {@link Files#createTempFile(Path,String, String, FileAttribute[])} over alternatives | |||
There was a problem hiding this comment.
| * Prefer {@link Files#createTempFile(Path,String, String, FileAttribute[])} over alternatives | |
| * Prefer {@link Files#createTempFile(Path, String, String, FileAttribute[])} over alternatives |
rickie
force-pushed
the
sschroevers/more-path-rules
branch
from
681ca67 to
571113f
Compare
|
Looks good. All 4 mutations in this change were killed.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
1 similar comment
|
Looks good. All 4 mutations in this change were killed.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
|
|
Anything else from you side @timtebeek 😄 ? Otherwise I'll merge it :). |
Suggested commit message: