modernizer-maven-plugin icon indicating copy to clipboard operation
modernizer-maven-plugin copied to clipboard

Should recommend Path.of() instead of Paths.get()

Open Bananeweizen opened this issue 11 months ago • 13 comments

I appreciate the recent phase out of old java.io.File usage via #301. However, in my eyes the recommended replacement is not good. Paths.get() works fine, but it has a note in its Javadoc which recommends the usage of Path.of() instead, and that's what modernizer should suggest, too: https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/java/nio/file/Paths.java#L36-L38

However, that method is only available from Java 11, therefore there is the tiny window of people updating from 7 to 8 only (instead of from 7 to 11+), which would need Paths.get(). Not sure if that tiny fraction can be ignored, or whether the error message should point to both replacements (one for Java 8, one for Java 11+).

Bananeweizen avatar Dec 31 '24 11:12 Bananeweizen

@mkarg , please share your thoughts on this

romani avatar Dec 31 '24 15:12 romani

Maybe just add another violation for Paths.get recommending Path.of? This will annoy users who fix one violation and are greeted by a second but at least gives them a path to the recommended state. I could see more involved solutions like adding an untilVersion element to modernizer.xml which could trigger on a version range instead of all newer ones.

gaul avatar Dec 31 '24 18:12 gaul

Actually I was under the impression to have implemented the violations in a way that with target 8, 9 and 10 Paths.get() is recommended, and with target 11+ Path.of() is recommended. If that is not the case, then I made a mistake (please check an tell me). Thanks.

mkarg avatar Dec 31 '24 18:12 mkarg

what is problem with usage of new File(path) https://github.com/checkstyle/checkstyle/pull/16095#issuecomment-2565949133 ? what should be used instead ?

romani avatar Jan 03 '25 22:01 romani

what is problem with usage of new File(path) checkstyle/checkstyle#16095 (comment) ? what should be used instead ?

Since Java 7 there is no need to use java.io.File. In particular, it behaves wrong by design in some cases, and it is less efficient than java.nio.file.Files / java.nio.file.Path.

Replace it by using NIO2 API, which is more complex thant simply replacing a single class.

For target Java 7-10 go with Paths.get, for Java 11+ go with Path.of.

mkarg avatar Jan 04 '25 10:01 mkarg

-            allFiles.add(new File(fileName));
+            allFiles.add(java.nio.file.Path.of(fileName).toFile());

romani avatar Jan 04 '25 15:01 romani

-            allFiles.add(new File(fileName));
+            allFiles.add(java.nio.file.Path.of(fileName).toFile());

I am sorry but I need to say I do not understand what you like to tell us. Can you please elaborate what your question, problem, and / or proposal is? Thanks. 😳

mkarg avatar Jan 04 '25 18:01 mkarg

I don't think the intent should be to create a Path to call toFile. Rather, codebases should adopt Path throughout, e.g., https://github.com/gaul/s3proxy/commit/26dd4a46e9d017c037bc53692d2c9a64b7d387fa. Perhaps this seems pedantic to some applications but using Path has advantages, e.g., using jimfs for in-memory testing. I was also able to fix some Linux/Windows issues with its emulation capabilities.

gaul avatar Jan 04 '25 18:01 gaul

I don't think the intent should be to create a Path to call toFile. Rather, codebases should adopt Path throughout, e.g., gaul/s3proxy@26dd4a4. Perhaps this seems pedantic to some applications but using Path has advantages, e.g., using jimfs for in-memory testing. I was also able to fix some Linux/Windows issues with its emulation capabilities.

In modern Java (7+) there is no justified use for File other than staying compatible with non-modernized binaries. If the binary can be modernized, modernize it. If the binary cannot be modernized, don't create a Path just to create a File in turn, but stick with new File in that code location and annotate it to be ignored by the modernizer plugin.

Having said that, I would say, this thread is OT.

mkarg avatar Jan 04 '25 18:01 mkarg

I generally like the idea. We encountered some cases with Desktop.getDesktop().open(new File(filePath)); from java.awt It accepts only File. There are a couple of other awt methods And also Process builder directory still requires File()

Siedlerchr avatar Jan 27 '25 18:01 Siedlerchr

Could you report an issue against the JDK? This sounds like an oversight on their part. I previously encountered similar situations where some APIs only accepted StringBuffer instead of modern StringBuilder.

gaul avatar Jan 27 '25 19:01 gaul

I generally like the idea. We encountered some cases with Desktop.getDesktop().open(new File(filePath)); from java.awt It accepts only File. There are a couple of other awt methods And also Process builder directory still requires File()

You are right, the JRE contains (few) APIs that still make use of File, and it is a good idea to use Path.of().toFile() in that situations, as it will provide an optimized, fast, and more correct implementation that way.

mkarg avatar Mar 15 '25 18:03 mkarg

Could you report an issue against the JDK? This sounds like an oversight on their part. I previously encountered similar situations where some APIs only accepted StringBuffer instead of modern StringBuilder.

Just proposed the needed changed. Waiting for approval by OpenJDK team.

mkarg avatar Mar 15 '25 18:03 mkarg