RFR: 8306914: Implementation of JEP Launch Multi-File Source-Code Programs
Jan Lahoda
jlahoda at openjdk.org
Mon Oct 16 13:35:33 UTC 2023
On Fri, 28 Apr 2023 08:00:36 GMT, Christian Stein <cstein at openjdk.org> wrote:
> Please review this change set implementing [JEP 458](https://openjdk.org/jeps/458) "Launch Multi-File Source-Code Programs" by introducing a new entry-point `SourceLauncher` in package `com.sun.tools.javac.launcher` of module `jdk.compiler`. The former `Main` entry-point is kept as-is in order to preserve (and possibly fall back to) existing behaviour of launching single-file source-code programs.
>
> The code of the new entry-point and other new classes introduced by this pull request is based on the original implementation. It extends it to dynamically resolve required types "on-the-fly" using an in-memory class loader capable to compile Java source files - applying a strict name-based pattern as described in the JEP.
>
> To support modular programs with user-provided additional modules (launched for example via `java -p . pkg/Prog1.java`) `ALL-MODULE-PATH` is now also part of the implicit list of modules name in source launch mode.
>
> ### Open Ends
>
> - [ ] Tests with [JEP ?: Implicitly Declared Classes and Instance Main Method (2nd Preview)](https://bugs.openjdk.org/browse/JDK-8315398)
>
> ### OpenJDK's Demo Programs
>
> OpenJDK's demo programs can be found in the [src/demo/share/jfc](https://github.com/openjdk/jdk/tree/master/src/demo/share/jfc) directory. These multi-file source-code programs can be launched via `java ${PROGRAM}` once this implementation is integrated; with `${PROGRAM}` being one of the following files:
>
> - [x] `src/demo/share/jfc/CodePointIM/CodePointIM.java`
> - [x] `src/demo/share/jfc/FileChooserDemo/FileChooserDemo.java`
> - [x] `src/demo/share/jfc/Font2DTest/Font2DTest.java`
> - [x] `src/demo/share/jfc/J2Ddemo/java2d/J2Ddemo.java`
> - [x] `src/demo/share/jfc/Metalworks/Metalworks.java`
> - [x] `src/demo/share/jfc/Notepad/Notepad.java`
> - [x] `src/demo/share/jfc/SampleTree/SampleTree.java`
> - [ ] `src/demo/share/jfc/Stylepad/Stylepad.java` — requires `src/demo/share/jfc/Notepad/Notepad.java`
> - [x] `src/demo/share/jfc/SwingSet2/SwingSet2.java`
> - [ ] `src/demo/share/jfc/TableExample/TableExample.java` — requires a database driver on the class path
> - [ ] `src/demo/share/jfc/TableExample/TableExample2.java` — requires a database driver on the class path
> - [x] `src/demo/share/jfc/TableExample/TableExample3.java`
> - [x] `src/demo/share/jfc/TableExample/TableExample4.java`
> - [x] `src/demo/share/jfc/TransparentRuler/transparentruler/Ruler.java`
I did an initial pass through the code. Added some small comments, mostly for future consideration, and one slightly bigger one around testing case where on-demand compilation fails.
src/jdk.compiler/share/classes/com/sun/tools/javac/launcher/MemoryContext.java line 146:
> 144:
> 145: byte[] compileJavaFileByName(String name) throws Fault {
> 146: var firstDollarSign = name.indexOf('$'); // [package . ] name [ $ enclosed ]
Nit - in principle, `$` is a valid identifier character:
jshell> Character.isJavaIdentifierStart('$')
$1 ==> true
So, in principle, the user's code may have classes with `$` in their names. OTOH, this seems like a very rare case given the context.
src/jdk.compiler/share/classes/com/sun/tools/javac/launcher/MemoryFileManager.java line 52:
> 50: * or deletion without notice.</b></p>
> 51: */
> 52: final class MemoryFileManager extends ForwardingJavaFileManager<JavaFileManager> {
Just FWIW, I note this class does not support reading from the classfiles, so everything will be load from the source files during compilation.
src/jdk.compiler/share/classes/com/sun/tools/javac/launcher/ProgramDescriptor.java line 104:
> 102: .filter(string -> !string.isEmpty())
> 103: .map(string -> string.replace('/', '.'))
> 104: .map(string -> string.replace('\\', '.'))
Nit: I'd be a bit careful with the `` -> `.` replacement. `` is a valid file name character on UNIX-like system. Better use `File.separatorChar` or `sourceRootPath.getFileSystem().getSeparator()`. Although the practical impact is probably very low.
src/jdk.compiler/share/classes/com/sun/tools/javac/launcher/SourceLauncher.java line 77:
> 75: try {
> 76: new SourceLauncher(System.err)
> 77: .checkSecurityManager()
Nit, fwiw: the SecurityManager check is not completely safe (as the user's program could install SM while providing -Djava.security.manager=allow), and then further compilation may be invoked, failing on SM. OTOH, given the state of SM, I don't think this needs changes.
test/langtools/tools/javac/launcher/Run.java line 49:
> 47: PrintStream prev = System.out;
> 48: ByteArrayOutputStream baos = new ByteArrayOutputStream();
> 49: try (PrintStream out = new PrintStream(baos, true)) {
So, I am looking at this, and I am not sure how it can work and test what is needed. If the compilation fails during on-demand class loading, we stop using `System.exit`, and it seems to me this code cannot handle that, and so that there's probably no test testing that scenario?
I mean:
---Test.java
public class Test {
public static void main(String... args) throws Throwable {
Class.forName("Extra");
}
}
----Extra.java
public class Broken
---
-------------
PR Review: https://git.openjdk.org/jdk/pull/13712#pullrequestreview-1679935689
PR Review Comment: https://git.openjdk.org/jdk/pull/13712#discussion_r1360634635
PR Review Comment: https://git.openjdk.org/jdk/pull/13712#discussion_r1360635959
PR Review Comment: https://git.openjdk.org/jdk/pull/13712#discussion_r1360643655
PR Review Comment: https://git.openjdk.org/jdk/pull/13712#discussion_r1360647244
PR Review Comment: https://git.openjdk.org/jdk/pull/13712#discussion_r1360671662
More information about the compiler-dev
mailing list