8344706: Compiler Implementation of Compact Source Files and Instance Main Methods #24438

lahodaj · 2025-04-04T07:20:23Z

This is a PR that implements JEP: Compact Source Files and Instance Main Methods. Changes include:

java.io.IO moved to java.lang.IO, and no longer uses System.console() to implement the methods (thanks to @stuart-marks)
java. ... .IO is no longer automatically imported in any compilation unit
the feature is finalized (i.e. no longer requires --enable-preview)

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue
Change requires CSR request JDK-8353437 to be approved
Change requires a JEP request to be targeted

Issues

JDK-8344706: Compiler Implementation of Compact Source Files and Instance Main Methods (Sub-task - P4)
JDK-8344699: Compact Source Files and Instance Main Methods (JEP)
JDK-8353437: Compiler Implementation of Compact Source Files and Instance Main Methods (CSR)

Reviewers

Christian Stein (@sormuras - Committer) 🔄 Re-review required (review applies to a5d223e1)
Vicente Romero (@vicente-romero-oracle - Reviewer) 🔄 Re-review required (review applies to be8846e0)

Contributors

Stuart Marks <smarks@openjdk.org>

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/24438/head:pull/24438
$ git checkout pull/24438

Update a local copy of the PR:
$ git checkout pull/24438
$ git pull https://git.openjdk.org/jdk.git pull/24438/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 24438

View PR using the GUI difftool:
$ git pr show -t 24438

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/24438.diff

Using Webrev

Link to Webrev Comment

…spec. Augment class spec to discuss buffering of bytes from System.in. Change "terminates the line" with "writes a line separator".

…concise-source-files

bridgekeeper · 2025-04-04T07:21:15Z

👋 Welcome back jlahoda! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2025-04-04T07:21:42Z

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

openjdk · 2025-04-04T07:22:06Z

⚠️ @lahodaj This pull request contains merges that bring in commits not present in the target repository. Since this is not a "merge style" pull request, these changes will be squashed when this pull request in integrated. If this is your intention, then please ignore this message. If you want to preserve the commit structure, you must change the title of this pull request to Merge <project>:<branch> where <project> is the name of another project in the OpenJDK organization (for example Merge jdk:master).

lahodaj · 2025-04-04T07:22:12Z

/contributor add @stuart-marks

lahodaj · 2025-04-04T07:22:43Z

/jep JDK-8344699

openjdk · 2025-04-04T08:56:00Z

@lahodaj
Contributor Stuart Marks <smarks@openjdk.org> successfully added.

openjdk · 2025-04-04T08:56:28Z

@lahodaj
This pull request will not be integrated until the JEP JDK-8344699 has been targeted.

openjdk · 2025-04-04T08:56:59Z

@lahodaj The following labels will be automatically applied to this pull request:

compiler
core-libs
kulla

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

liach · 2025-04-05T00:20:29Z

src/java.base/share/classes/java/lang/IO.java

+     *
+     * @return the internal BufferedReader instance
+     */
+    static synchronized BufferedReader reader() {


Is the lock only inteneded for initialization, i.e. should it apply for simply getting the reader?

I believe there needs to be some barriers (at least volatile) even for getting the already initialized reader. And then, given the code here, it is probably fine and less error prone to simply use synchronized that something more complex.

But, given we'll hopefully have @StableValue soon, maybe we could use that? What do you think @stuart-marks?

Thanks!

It's even more complex than volatile. If a thread comes along and reads a null here, it still needs to take a lock (or something) to prevent more than one thread from creating the BufferedReader and storing it back into the field. This leads down the path of double-checked locking, initialize-on-demand holder, etc.

It's not clear to me this is in such a critical path that it warrants adding any of this stuff. StableValue probably has the right semantics. However, it's not even in the JDK yet (though it is Targeted to 25 as of this moment). But I don't think we want to add a dependency on a Preview API.

test/jdk/java/io/IO/IO.java

lukellmann · 2025-04-05T00:55:55Z

src/java.base/share/classes/java/lang/IO.java

+        System.out.print(obj);
+        System.out.flush();


Is it worth using a local variable to avoid calling print and flush on different streams in case System.out is reassigned in between?

var out = System.out; out.print(obj); out.flush();

This code is not that perf sensitive, guess it is fine as-is

I think there's a correctness angle. If you print something to a stream you want to flush the same stream. If System.out is changed in between (unlikely, but possible) then output destined for the old stream could be stranded in its buffer.

Updated as suggested; thanks @lukellmann .

lukellmann · 2025-04-05T00:58:44Z

src/java.base/share/classes/java/lang/IO.java

+     * @throws IOError if an I/O error occurs
+     */
+    public static String readln() {
+        try {
+            return reader().readLine();
+        } catch (IOException ioe) {
+            throw new IOError(ioe);
+        }


Was UncheckedIOException considered? It might be better for beginners to learn throwing exceptions instead of errors.

This implementation is not to be emulated by beginners. If an Exception.is thrown here, a user might be tempted to add handlers in their code, while this is an issue with the setup.

Someone is bound to ask why the readln method throw but the println methods don't.

The IOError was carried over from Console.readLine(), which throws IOError on error. Of course we're decoupled from Console now, but that was the original reason.

My guess is that Console methods throw IOError because (a) they didn't want to make everybody catch IOException, (b) they didn't want to maintain an internal error state like PrintStream, and (c) they wanted to throw a throwable whose type conveyed the meaning that it wasn't expected to be handled. IOError and Console were both added in JDK 1.6. Maybe that reasoning still applies here.

UncheckedIOException wasn't added until JDK 1.8. Note also that UncheckedIOException has a cause of IOException, so it can't arise from some other throwable. This was used in the previous implementation, where IOError was thrown if Console was null. That doesn't apply anymore, but IOError is still somewhat more flexible than UncheckedIOException in this regard.

I think we need to say something, implicitly or explicitly, about error handling. Note that PrintStream has this internal error state so output is covered already. For input it seems like IOError is reasonable, but maybe there's a better choice.

The readln methods handle malformed-input and unmappable-character errors by dropping, and using a replacement value. So erroneous input doesn't throw IOError with a CharacterCodingException as the cause.

System.in.close() would a be wild thing to do, but a case where readln would throw IOError.

Ah. Does the specification here need to state the policy around malformed input and unmappable characters? (Or, maybe in the class specification.)

People have ended up in cases where System.in is closed. It can occur when combining use of Scanner to read from System.in and applying the anything-that-you-open-must-be-closed rule, e.g.,

try (Scanner sc = new Scanner(System.in)) { String s = sc.nextLine(); }

which is a mistake, but it takes a long time to explain....

I added a clause to the class specification that covers malformed/unmappable byte sequences.

mlbridge · 2025-04-07T07:01:32Z

Webrevs

sormuras

Changes in source launcher related tests look good.

AlanBateman · 2025-04-07T09:12:27Z

src/java.base/share/classes/java/lang/IO.java

+ * If this property is not present, or if the charset it names cannot be loaded, then
+ * UTF-8 is used instead. These internal objects are created upon the first call to
+ * either of the {@code readln} methods and are stored for subsequent reuse by these
+ * methods.


@stuart-marks Can we rephrase this paragraph so that it doesn't use phrase "internal objects"? The class does speak of buffering and how it might impact code that mixes use of System.in and IO.readln so I agree with that part it's just the "internal objects" phrase that is confusing to read in this class.

I used "internal objects" because I wanted to avoid naming concrete classes, which might or might not be used. Previous drafts mentioned BufferedReader, InputStreamReader, and CharsetDecoder.

I could replace "internal objects" with something more descriptive like "objects to handle buffering and charset decoding" but I'd still need a noun phrase to refer to them later. Maybe "buffering and decoding objects" ?

Can we rephrase the 1st sentence to read:
"The readln() and readln(String) methods in this class decode bytes read from System.in into characters."

And remove the last "these internal objects" sentence.

And for the next paragraph, change the first line to:
"The readln methods may buffer additional bytes..."

So we remove references to "internal objects."

I'm in the midst of rewriting all of this stuff. The main concern here is that applications can use System.in more or less as they please (presumably without buffering) until the first call to a readln method, at which time something happens and effectively the application cannot use System.in anymore. I'll post the revision when I've finished it.

naotoj · 2025-04-07T17:58:45Z

test/jdk/java/lang/IO/IO/IO.java

Is there reason to create a test subdirectory IO under java.lang.IO package?

naotoj · 2025-04-07T18:05:41Z

test/jdk/java/lang/IO/IO/IO.java

@@ -51,7 +51,7 @@
 /*
 * @test
 * @bug 8305457 8342936 8351435


Issue id needs to be added (and other test files too)

vicente-romero-oracle

javac compiler related code and tests lgtm

…jects."

AlanBateman · 2025-04-09T10:22:05Z

src/java.base/share/classes/java/lang/IO.java

+ * Charset decoding is set up upon the first call to one of the {@code readln} methods.
+ * Decoding may buffer additional bytes beyond those that have been decoded to characters
+ * returned to the application. After the first call to one of the {@code readln} methods,
+ * any subsequent use of {@code System.in} results in unspecified behavior.


I wonder if we should add a note about mixing API usage into System.in too. There are tutorials and books that show examples build on System.in that will add buffering on that input source.

Might be a good idea. I'll handle this separately. I don't want it to refer to a Preview API. I'll probably just use Scanner and InputStreamReader as examples. (Oh wait, this supposedly isn't a Preview API anymore...)

AlanBateman · 2025-04-09T10:46:40Z

src/java.base/share/classes/java/lang/IO.java

+ * The {@link #readln()} and {@link #readln(String)} methods decode bytes read from
+ * {@code System.in} into characters. The charset used for decoding is specified by the
+ * {@link System#getProperties stdin.encoding} property. If this property is not present,
+ * or if the charset it names cannot be loaded, then UTF-8 is used instead.


stdout.encoding and stderr.encoding are specified (in System.getProperties) to lead to unspecified behavior if started with either property set to a value other than UTF-8. We should work through the issues of introducing stdin.encoding as soon as we can, esp. the redirect cases and whether there is use cases for setting any of these properties on the command line.

Yes, believe it or not, I am still studying this...

stuart-marks and others added 21 commits January 13, 2025 19:41

Initial cut at java.lang.IO (from 2024-12-19).

6e9e093

Merge branch 'master' into JavaLangIO

e801c97

Merge remote-tracking branch 'upstream/master' into JavaLangIO

7c98234

Revise and strip down java.lang.IO

4ebd601

Various tweaks to charset decoding and line reading.

fb80e69

Updatiing javac and related tests to changes in java.lang.IO.

2c5ff02

Merge branch 'master' into JavaLangIO

276fdfa

Remove autoflush, and add explicit flushing only within print().

2f0dfc9

Code to finalize compact source files.

5c7ee00

More cleanup.

0177b4c

Fixing tests.

bc2d958

Merge branch 'JavaLangIO' into finalize-concise-source-files

7d18bdc

Adjusting tests.

7997850

More test fixes.

39a28ea

Removing preview flag from new main method handling.

643083d

Merge branch 'master' into finalize-concise-source-files

5cb7ffa

Merge branch 'master' into JavaLangIO

0dba1c9

Clean up some charset stuff, use stdin.encoding property.

c2b3637

Responses to Alan's comments: clean up readln() and align with class …

7bacc29

…spec. Augment class spec to discuss buffering of bytes from System.in. Change "terminates the line" with "writes a line separator".

Merge remote-tracking branch 'stuart-marks/JavaLangIO' into finalize-…

9f45f29

…concise-source-files

Merge branch 'master' into finalize-concise-source-files

6b88025

openjdk bot added the csr label Apr 4, 2025

Removing unnecessary enable preview from tests.

3d181d9

openjdk bot added the jep label Apr 4, 2025

openjdk bot added core-libs compiler kulla labels Apr 4, 2025

lahodaj added 3 commits April 4, 2025 11:26

Fixing test.

596e8cf

Fixing test.

9d4d7bb

Fixing test.

d3b984b

stuart-marks mentioned this pull request Apr 4, 2025

First cut at java.lang.IO #23220

Closed

3 tasks

liach reviewed Apr 5, 2025

View reviewed changes

test/jdk/java/io/IO/IO.java Outdated Show resolved Hide resolved

lukellmann reviewed Apr 5, 2025

View reviewed changes

Moving tests as suggested, removing unused import.

a5d223e

lahodaj marked this pull request as ready for review April 7, 2025 06:57

openjdk bot added the rfr label Apr 7, 2025

sormuras approved these changes Apr 7, 2025

View reviewed changes

AlanBateman reviewed Apr 7, 2025

View reviewed changes

Use local variable for System.out in print().

be8846e

naotoj reviewed Apr 7, 2025

View reviewed changes

vicente-romero-oracle approved these changes Apr 7, 2025

View reviewed changes

Rewrite bits about charset decoding, removing mention of "internal ob…

33e280d

…jects."

AlanBateman reviewed Apr 9, 2025

View reviewed changes

Add clause about handling of malformed/unmappable bytes.

05cc336

openjdk bot mentioned this pull request Apr 11, 2025

8320255: Add method to java.lang.Class to find main method #24594

Draft

4 tasks

8344706: Compiler Implementation of Compact Source Files and Instance Main Methods #24438

Are you sure you want to change the base?

8344706: Compiler Implementation of Compact Source Files and Instance Main Methods #24438

Conversation

lahodaj commented Apr 4, 2025 • edited by openjdk bot Loading

Progress

Issues

Reviewers

Contributors

Reviewing

bridgekeeper bot commented Apr 4, 2025

openjdk bot commented Apr 4, 2025

openjdk bot commented Apr 4, 2025

lahodaj commented Apr 4, 2025

lahodaj commented Apr 4, 2025

openjdk bot commented Apr 4, 2025

openjdk bot commented Apr 4, 2025

openjdk bot commented Apr 4, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stuart-marks Apr 10, 2025 • edited Loading

Choose a reason for hiding this comment

mlbridge bot commented Apr 7, 2025 • edited Loading

Webrevs

sormuras left a comment

Choose a reason for hiding this comment

AlanBateman Apr 7, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vicente-romero-oracle left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stuart-marks Apr 9, 2025 • edited Loading

Choose a reason for hiding this comment

AlanBateman Apr 9, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lahodaj commented Apr 4, 2025 •

edited by openjdk bot

Loading

stuart-marks Apr 10, 2025 •

edited

Loading

mlbridge bot commented Apr 7, 2025 •

edited

Loading

AlanBateman Apr 7, 2025 •

edited

Loading

stuart-marks Apr 9, 2025 •

edited

Loading

AlanBateman Apr 9, 2025 •

edited

Loading