Conversation
|
@thisisalexandercook Could you please take over this PR and see whether the API changes make sense? |
There was a problem hiding this comment.
Pull request overview
Applies the Gradle modernizer plugin across modules and updates several Java sources to prefer java.nio.file/stream APIs over legacy java.io usage.
Changes:
- Adds and configures the
com.github.andygoossens.modernizerGradle plugin (including in dev test script). - Replaces many
new File(...), string path concatenations, andFileInputStreamusages withPaths/Path,Files, andInputStream. - Minor API modernization in tests (e.g.,
Scannercharset overload,Optional.orElseThrow()).
Reviewed changes
Copilot reviewed 16 out of 17 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| javacutil/src/main/java/org/checkerframework/javacutil/SystemUtil.java | Switches to Path construction for tools.jar. |
| framework/src/main/java/org/checkerframework/framework/util/CheckerMain.java | Uses Paths/Path resolution and Files.newInputStream instead of FileInputStream. |
| framework-test/src/main/java/org/checkerframework/framework/test/diagnostics/JavaDiagnosticReader.java | Uses Paths to derive filename from JavaFileObject. |
| framework-test/src/main/java/org/checkerframework/framework/test/TestUtilities.java | Migrates several path/file constructions to Path/resolve. |
| framework-test/src/main/java/org/checkerframework/framework/test/TestConfigurationBuilder.java | Creates output dir via Paths.get(...).toFile(). |
| framework-test/src/main/java/org/checkerframework/framework/test/RootedSuite.java | Uses Path.of(...).toFile() for test root resolution. |
| framework-test/src/main/java/org/checkerframework/framework/test/CheckerFrameworkWPIPerDirectoryTest.java | Uses Scanner(File, Charset) overload. |
| framework-test/src/main/java/org/checkerframework/framework/test/CheckerFrameworkRootedTest.java | Uses Paths.get(...).toFile() for test root resolution. |
| framework-test/src/main/java/org/checkerframework/framework/test/CheckerFrameworkPerFileTest.java | Constructs directory paths via Path.resolve(...).toString(). |
| framework-test/src/main/java/org/checkerframework/framework/test/CheckerFrameworkPerDirectoryTest.java | Constructs directory paths via Path.resolve(...).toString(). |
| framework-test/src/main/java/org/checkerframework/framework/test/AinferValidatePerDirectoryTest.java | Replaces File.exists() with Files.exists(Paths.get(...)). |
| dataflow/src/main/java/org/checkerframework/dataflow/cfg/visualize/DOTCFGVisualizer.java | Uses Paths.get(outDir, "methods.txt") instead of string concatenation. |
| dataflow/src/main/java/org/checkerframework/dataflow/cfg/visualize/CFGVisualizeOptions.java | Switches to Path.of(...).toFile() for input file handling. |
| checker/src/main/java/org/checkerframework/checker/units/UnitsAnnotatedTypeFactory.java | Uses Paths.get(...).toFile().exists() for units dir existence check. |
| checker/bin-devel/test-misc.sh | Adds ./gradlew modernizer check. |
| checker-util/src/test/java/org/checkerframework/checker/optional/util/OptionalUtilTest.java | Uses Optional.orElseThrow() instead of get(). |
| build.gradle | Adds and configures the modernizer plugin for all projects. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
| } | ||
| File toolsJarFile = new File(new File(javaHome, "lib"), "tools.jar"); | ||
| File toolsJarFile = Path.of(javaHome, "lib", "tools.jar").toFile(); |
There was a problem hiding this comment.
Path.of(...) is a Java 11+ API. This change will break compilation when targeting Java 8 (and conflicts with the Gradle modernizer configuration setting javaVersion = 8). Use Paths.get(javaHome, \"lib\", \"tools.jar\") (Java 7+) instead.
| return Path.of(annotation.value()).toFile(); | ||
| } | ||
| return new File("tests"); | ||
| return Path.of("tests").toFile(); |
There was a problem hiding this comment.
Path.of(...) requires Java 11+. If this project is intended to be compatible with Java 8 (as indicated by modernizer javaVersion = 8), replace these with Paths.get(...).
| } | ||
| String input = args[0]; | ||
| File file = new File(input); | ||
| File file = Path.of(input).toFile(); |
There was a problem hiding this comment.
Path.of(...) is not available on Java 8. To preserve Java 8 compatibility, use Paths.get(input).toFile() here.
|
|
||
| this.checkerJar = checkerJar; | ||
| File searchPath = checkerJar.getParentFile(); | ||
| Path searchPath = checkerJar.getParentFile().toPath(); |
There was a problem hiding this comment.
Behavior change: checkerJar.getParentFile() can be null, but new File(parent, child) previously tolerated a null parent (interpreting child as a relative path). The new code will throw NullPointerException when checkerJar has no parent. Consider deriving searchPath from checkerJar.toPath().getParent() and falling back to a sensible default (e.g., current directory) when the parent is null.
| Path searchPath = checkerJar.getParentFile().toPath(); | |
| Path parent = checkerJar.toPath().getParent(); | |
| Path searchPath = (parent != null) ? parent : Paths.get("."); |
| extractFileArg( | ||
| CHECKER_QUAL_PATH_OPT, new File(searchPath, "checker-qual.jar"), args); | ||
| CHECKER_QUAL_PATH_OPT, | ||
| searchPath.resolve("checker-qual.jar").toFile(), |
There was a problem hiding this comment.
Behavior change: checkerJar.getParentFile() can be null, but new File(parent, child) previously tolerated a null parent (interpreting child as a relative path). The new code will throw NullPointerException when checkerJar has no parent. Consider deriving searchPath from checkerJar.toPath().getParent() and falling back to a sensible default (e.g., current directory) when the parent is null.
| searchPath.resolve("checker-util.jar").toFile(), | ||
| args); | ||
|
|
||
| this.javacJar = extractFileArg(JAVAC_PATH_OPT, new File(searchPath, "javac.jar"), args); | ||
| this.javacJar = | ||
| extractFileArg(JAVAC_PATH_OPT, searchPath.resolve("javac.jar").toFile(), args); |
There was a problem hiding this comment.
Behavior change: checkerJar.getParentFile() can be null, but new File(parent, child) previously tolerated a null parent (interpreting child as a relative path). The new code will throw NullPointerException when checkerJar has no parent. Consider deriving searchPath from checkerJar.toPath().getParent() and falling back to a sensible default (e.g., current directory) when the parent is null.
| public static List<Object[]> findFilesInParent(File parent, String... fileNames) { | ||
| return CollectionsPlume.mapList( | ||
| (String fileName) -> new Object[] {new File(parent, fileName)}, fileNames); | ||
| (String fileName) -> new Object[] {parent.toPath().resolve(fileName)}, fileNames); |
There was a problem hiding this comment.
This changes the element type from File to Path inside the returned Object[]. Callers (especially JUnit parameterized tests) that expect a File will now fail with a ClassCastException or method signature mismatch. Convert the resolved path back to File (e.g., .toFile()) to preserve the original API behavior.
| (String fileName) -> new Object[] {parent.toPath().resolve(fileName)}, fileNames); | |
| (String fileName) -> | |
| new Object[] {parent.toPath().resolve(fileName).toFile()}, | |
| fileNames); |
| Assert.assertTrue(nonEmptyOpt.isPresent()); | ||
| @Present Optional<String> foo = OptionalUtil.castPresent(nonEmptyOpt); | ||
| Assert.assertEquals(foo.get(), "non-empty"); | ||
| Assert.assertEquals(foo.orElseThrow(), "non-empty"); |
There was a problem hiding this comment.
Assert.assertEquals conventionally uses (expected, actual). Swapping the arguments makes failure messages confusing. Prefer Assert.assertEquals(\"non-empty\", foo.orElseThrow());.
| Assert.assertEquals(foo.orElseThrow(), "non-empty"); | |
| Assert.assertEquals("non-empty", foo.orElseThrow()); |
|
@thisisalexandercook Should we abandon this PR? |
I have let this rot without giving the plugin a good try. I will experiment this week to see how useful it is. |
Apply the "modernizer" plugin to all sources to ensure modern APIs are used.