8328481: Implement JEP 476: Module Import Declarations (Preview) #18614

lahodaj · 2024-04-04T07:30:34Z

This is an implementation of JEP JDK-8315129: Module Import Declarations (Preview). Please see the JEP for details:
https://bugs.openjdk.org/browse/JDK-8315129

It is mostly straightforward - the module imports are parsed, and then expanded to import-on-demand in TypeEnter.
There is a few notable aspects, however:

the AST node for import (JCImport) is holding the imported element as a field access, because so far, the imported element always had to have a '.' (even for import-on-demand). But for module imports, it is permissible to import from a module whose name does not have a dot (import module m;). The use of field access for ordinary import seems very useful, so I preferred to keep that, and created a new internal-only AST node for module imports. There is still only one public API AST node/interface, so this is purely an implementation choice.
JShell now supports module imports as well; and the default, implicit, script is changed to use it to import all of java.base if preview is enabled. It is expected that the default would be changed if/when the module imports feature is finalized.

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-8328935 to be approved

Issues

JDK-8328481: Implement JEP 476: Module Import Declarations (Preview) (Sub-task - P4)
JDK-8328935: Implement Module Imports (CSR)

Reviewers

Maurizio Cimadamore (@mcimadamore - Reviewer) ⚠️ Review applies to 432393ab
Vicente Romero (@vicente-romero-oracle - Reviewer) ⚠️ Review applies to cbc363ab

Contributors

Jim Laskey <jlaskey@openjdk.org>

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 18614

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

Using diff file

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

Webrev

Link to Webrev Comment

…mports

bridgekeeper · 2024-04-04T07:31:03Z

👋 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 · 2024-04-04T07:31:53Z

@lahodaj This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8328481: Implement JEP 476: Module Import Declarations (Preview)

Co-authored-by: Jim Laskey <jlaskey@openjdk.org>
Reviewed-by: mcimadamore, vromero

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 63 new commits pushed to the master branch:

553d45b: 8323724: Remove potential re-inflation from FastHashCode under LM_LIGHTWEIGHT
dd0b641: 8330813: Don't call methods from Compressed(Oops|Klass) if the associated mode is inactive
389f6fe: 8331344: No compiler replay file with CompilerCommand MemLimit
a024eed: 8331478: JFR: Rename printHelp methods for jdk.jfr.internal.dcmd classes
9912abf: 8331011: [XWayland] TokenStorage fails under Security Manager
257a07d: 8331511: Tests should not use the "Classpath" exception form of the legal header
c21672d: 8331077: nroff man page update for jar tool
cccc953: 8329258: TailCall should not use frame pointer register for jump target
4a78906: 8331537: Test code should not use the "Classpath" exception clause
2056968: 8331513: Tests should not use the "Classpath" exception form of the legal header
... and 53 more: https://git.openjdk.org/jdk/compare/4e5c25ee43d4ec31ed5160fd93a2fd15e35182f8...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

openjdk · 2024-04-04T07:32:12Z

⚠️ @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).

openjdk · 2024-04-04T07:32:19Z

@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.

mlbridge · 2024-04-04T07:35:32Z

Webrevs

lahodaj · 2024-04-04T09:01:54Z

/contributor add jlaskey

openjdk · 2024-04-04T09:02:55Z

@lahodaj
Contributor Jim Laskey <jlaskey@openjdk.org> successfully added.

Thihup · 2024-04-04T11:43:07Z

Should the pull request incorporate a modification to the JAVASE script file for JShell, specifically substituting it with import module java.se? (right now the imports are computed, not read from a file)

sormuras · 2024-04-10T08:41:19Z

src/jdk.compiler/share/classes/com/sun/tools/javac/resources/compiler.properties

+
+# 0: symbol
+compiler.err.import.module.does.not.read=\
+    current modules does not read: {0}


Suggested change

current modules does not read: {0}

current module does not read: {0}

It would even be better to print the name of the "current" module, so that the user-facing message reads:
current module foo does not read module bar - possible fix: add "requires bar;" to foo's module descriptor

Thanks. I've tweaked the error to include the module name here:
7cfaff8

Regarding possible fix(es) - it may be better to introduce some framework for that, and convert the most typical errors, separately. (As I don't think we do that for other errors.)

mcimadamore · 2024-04-10T11:52:31Z

src/jdk.compiler/share/classes/com/sun/tools/javac/tree/JCTree.java

+     */
+    public static class JCModuleImport extends JCImportBase {
+        /** The module name. */
+        public JCExpression module;


Does it need to be an expression? Or is a name enough?

Expression is more consistent with e.g. the module directives (JCRequires has JCExpression moduleName). Also allows us to keep positions for the name.

AlanBateman · 2024-04-18T05:43:03Z

src/java.base/share/classes/jdk/internal/javac/PreviewFeature.java

@@ -81,6 +81,8 @@ public enum Feature {
        CLASSFILE_API,
        @JEP(number=461, title="Stream Gatherers", status="Preview")
        STREAM_GATHERERS,
+        @JEP(number=0, title="Module Imports", status="Preview")


I see this has been assigned JEP 476 so I assume you can set the number for the first integration.

Done, thanks:
432393a

mcimadamore · 2024-04-18T10:47:11Z

src/jdk.compiler/share/classes/com/sun/tools/javac/resources/compiler.properties

+
+# 0: symbol, 1: symbol
+compiler.err.import.module.does.not.read=\
+    {1} module does not read: {0}


shouldn't it be module {1} does not read: {0} ? Also, maybe worth reordering the params?

Thanks! Fixed:
846b038

… read (target).

vicente-romero-oracle · 2024-04-18T16:54:30Z

src/jdk.compiler/share/classes/com/sun/source/tree/ImportTree.java

+     * @since 23
+     */
+    @PreviewFeature(feature=PreviewFeature.Feature.MODULE_IMPORTS, reflective=true)
+    boolean isModule();


we are not including any test that stresses this new method

vicente-romero-oracle

lgtm, thanks!

lahodaj · 2024-05-06T05:48:20Z

/integrate

openjdk · 2024-05-06T05:49:30Z

Going to push as commit f2c4a41.
Since your change was applied there have been 98 commits pushed to the master branch:

9347bb7: 8330247: C2: CTW fail with assert(adr_t->is_known_instance_field()) failed: instance required
b20fa7b: 8329982: compiler/jvmci/jdk.vm.ci.code.test/src/jdk/vm/ci/code/test/SimpleDebugInfoTest.java failed assert(oopDesc::is_oop_or_null(val)) failed: bad oop found
c1a1645: 8331655: ClassFile API ClassCastException with verbose output of certain class files
36c9607: 8331591: sun.font.CharSequenceCodePointIterator is buggy and unused
b33096f: 8295153: java/util/Base64/TestEncodingDecodingLength.java ran out of memory
cf2c80e: 8331582: Incorrect default Console provider comment
77b7122: 8312104: Update java man pages to include new security category in -XshowSettings
87bb66c: 8331569: G1: Rename G1HRPrinter to G1HeapRegionPrinter
37c2469: 8331633: Use MIN2 in bound_minus_alignment
1d083eb: 8331562: G1: Remove API to force allocation of new regions
... and 88 more: https://git.openjdk.org/jdk/compare/4e5c25ee43d4ec31ed5160fd93a2fd15e35182f8...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-05-06T05:49:36Z

@lahodaj Pushed as commit f2c4a41.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

lahodaj added 15 commits March 14, 2024 18:45

Separating module imports, originally by jlaskey.

c991ee5

Tweaks to errors.

42af989

Test conflicts among imports.

67c9301

Fixes.

9d29967

Adding PreviewFeature.

5117584

Merge branch 'module-imports' of github.com:lahodaj/jdk into module-i…

6f62c46

…mports

More correct handling of requires transitive.

1ce1f9b

Adding support for import modules to JShell.

de9e6bf

Fixing CheckExamples.

e9cfbcf

Cleanup.

c14328b

Merge branch 'master' into module-imports

b57b21d

Fixing file name computation.

b7406ac

Fixing disambiguation of module imports.

636a9de

Fixing test.

3e13b6c

Merge branch 'master' into module-imports

9cab0ae

openjdk bot added the rfr Pull request is ready for review label Apr 4, 2024

openjdk bot added core-libs core-libs-dev@openjdk.org compiler compiler-dev@openjdk.org kulla kulla-dev@openjdk.org labels Apr 4, 2024

lahodaj changed the title ~~8328935: Implement Module Imports.~~ 8328481: Implement Module Imports. Apr 4, 2024

openjdk bot added the csr Pull request needs approved CSR before integration label Apr 4, 2024

lahodaj changed the title ~~8328481: Implement Module Imports.~~ 8328481: Implement Module Imports Apr 4, 2024

Adding more tests for ambiguities.

0ca05b7

sormuras reviewed Apr 10, 2024

View reviewed changes

mcimadamore reviewed Apr 10, 2024

View reviewed changes

lahodaj added 4 commits April 10, 2024 17:00

Including current module name as suggested.

7cfaff8

Merge branch 'master' into module-imports

c75b8b7

Cleanup.

27f9cfb

Fixing ListModuleDeps test.

7fa0ad5

AlanBateman reviewed Apr 18, 2024

View reviewed changes

Updating JEP number and caption.

432393a

mcimadamore approved these changes Apr 18, 2024

View reviewed changes

mcimadamore reviewed Apr 18, 2024

View reviewed changes

Reflecting review feedback - improving the 'module (current) does not…

846b038

… read (target).

vicente-romero-oracle reviewed Apr 18, 2024

View reviewed changes

Adding test for ImportTree.isModule, as suggested.

cbc363a

vicente-romero-oracle approved these changes Apr 19, 2024

View reviewed changes

Fixing Imports test on Windows.

1a62144

openjdk bot added ready Pull request is ready to be integrated and removed csr Pull request needs approved CSR before integration labels Apr 26, 2024

lahodaj added 2 commits April 28, 2024 13:39

Fixing test on Windows (2).

2c6c2a3

Merge branch 'master' into module-imports

43a1e0f

openjdk bot removed the ready Pull request is ready to be integrated label May 2, 2024

lahodaj changed the title ~~8328481: Implement Module Imports~~ 8328481: Implement JEP 476: Module Import Declarations (Preview) May 2, 2024

openjdk bot added the ready Pull request is ready to be integrated label May 2, 2024

openjdk bot added the integrated Pull request has been integrated label May 6, 2024

openjdk bot closed this May 6, 2024

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels May 6, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8328481: Implement JEP 476: Module Import Declarations (Preview) #18614

8328481: Implement JEP 476: Module Import Declarations (Preview) #18614

lahodaj commented Apr 4, 2024 •

edited by openjdk bot

Loading

bridgekeeper bot commented Apr 4, 2024

openjdk bot commented Apr 4, 2024 •

edited

Loading

openjdk bot commented Apr 4, 2024

openjdk bot commented Apr 4, 2024

mlbridge bot commented Apr 4, 2024 •

edited

Loading

lahodaj commented Apr 4, 2024

openjdk bot commented Apr 4, 2024

Thihup commented Apr 4, 2024

sormuras Apr 10, 2024 •

edited

Loading

lahodaj Apr 10, 2024

mcimadamore Apr 10, 2024

lahodaj Apr 10, 2024

AlanBateman Apr 18, 2024

lahodaj Apr 18, 2024

mcimadamore Apr 18, 2024

lahodaj Apr 18, 2024

vicente-romero-oracle Apr 18, 2024

vicente-romero-oracle left a comment

lahodaj commented May 6, 2024

openjdk bot commented May 6, 2024

openjdk bot commented May 6, 2024

	current modules does not read: {0}
	current module does not read: {0}

8328481: Implement JEP 476: Module Import Declarations (Preview) #18614

8328481: Implement JEP 476: Module Import Declarations (Preview) #18614

Conversation

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

Progress

Issues

Reviewers

Contributors

Reviewing

Webrev

bridgekeeper bot commented Apr 4, 2024

openjdk bot commented Apr 4, 2024 • edited Loading

openjdk bot commented Apr 4, 2024

openjdk bot commented Apr 4, 2024

mlbridge bot commented Apr 4, 2024 • edited Loading

Webrevs

lahodaj commented Apr 4, 2024

openjdk bot commented Apr 4, 2024

Thihup commented Apr 4, 2024

sormuras Apr 10, 2024 • 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

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

lahodaj commented May 6, 2024

openjdk bot commented May 6, 2024

openjdk bot commented May 6, 2024

lahodaj commented Apr 4, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Apr 4, 2024 •

edited

Loading

mlbridge bot commented Apr 4, 2024 •

edited

Loading

sormuras Apr 10, 2024 •

edited

Loading