RFR: 8306914: Implementation of JEP Launch Multi-File Source-Code Programs

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