Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8335989: Implement JEP 494: Module Import Declarations (Second Preview) #21431

Closed
wants to merge 21 commits into from

Conversation

lahodaj
Copy link
Contributor

@lahodaj lahodaj commented Oct 9, 2024

This is a current patch for module imports declarations, second preview. At least the JEP number and preview revision will need to be updated in jdk.internal.javac.PreviewFeature.Feature, but otherwise I believe this is ready to receive feedback.

The main changes are:

  • requires transitive java.base; is permitted, under the preview flag. Both javac and the runtime module system are updated to accept this directive when preview is enabled.
  • the java.se module is using requires transitive java.base;, and is deemed to be participating in preview, so its classfile version is not tainted. Runtime is updated to access requires transitive java.base; in any java.*, considering all of them to be participating in preview. Can be tighten up to only java.se is desired.
  • the types imported through module imports can be shadowed using on-demand imports. So, for example, having:
import module java.base;
import module java.desktop;
...
List l;//ambigous

but:

import module java.base;
import module java.desktop;
import java.util.*;
...
List l;//not ambigous, reference to java.util.List

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 a JEP request to be targeted
  • Change requires CSR request JDK-8341865 to be approved

Issues

  • JDK-8335989: Implement JEP 494: Module Import Declarations (Second Preview) (Sub-task - P4)
  • JDK-8335987: JEP 494: Module Import Declarations (Second Preview) (JEP)
  • JDK-8341865: Implement Module Import Declarations (Second Preview) (CSR)

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 21431

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

Using diff file

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

Using Webrev

Link to Webrev Comment

@bridgekeeper
Copy link

bridgekeeper bot commented Oct 9, 2024

👋 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
Copy link

openjdk bot commented Oct 9, 2024

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

8335989: Implement JEP 494: Module Import Declarations (Second Preview)

Reviewed-by: vromero, abimpoudis, mcimadamore, alanb

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 21 new commits pushed to the master branch:

  • 168b18e: 8343958: Remove security manager impl in java.lang.Process and java.lang.Runtime.exec
  • 5ac330b: 8344039: Remove security manager dependency in java.time
  • 1eb38c8: 8343219: Manual clientlibs test failures after SM removal
  • dde6230: 8343416: CDS dump fails when unregistered class can also be loaded from system modules
  • ffea980: 8344023: Unnecessary Hashtable usage in LdapClient.defaultBinaryAttrs
  • 5e01c40: 8343981: Remove usage of security manager from Thread and related classes
  • dbf2346: 8341260: Add Float16 to jdk.incubator.vector
  • a5f11b5: 8343483: Remove unnecessary @SuppressWarnings annotations (serviceability)
  • 7be7772: 8344112: Remove code to support security manager execution mode from DatagramChannel implementation
  • bd3fec3: 8344086: Remove security manager dependency in FFM
  • ... and 11 more: https://git.openjdk.org/jdk/compare/79345bbbae2564f9f523859d1227a1784293b20f...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 openjdk bot added the rfr Pull request is ready for review label Oct 9, 2024
@lahodaj
Copy link
Contributor Author

lahodaj commented Oct 9, 2024

/jep JDK-8335987

@openjdk
Copy link

openjdk bot commented Oct 9, 2024

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

  • build
  • compiler
  • core-libs

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.

@openjdk openjdk bot added build build-dev@openjdk.org core-libs core-libs-dev@openjdk.org compiler compiler-dev@openjdk.org labels Oct 9, 2024
@openjdk
Copy link

openjdk bot commented Oct 9, 2024

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

@openjdk openjdk bot added jep csr Pull request needs approved CSR before integration labels Oct 9, 2024
@mlbridge
Copy link

mlbridge bot commented Oct 9, 2024

Webrevs

assertEquals(javaBase.modifiers(), Set.of(Modifier.TRANSITIVE));
}

private byte[] setClassFileVersion(int major, int minor, byte[] bytecode) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be possible to add a method description and re-format the overly long lines as it's currently impossible to look at the change side-by-side.

@AlanBateman
Copy link
Contributor

Can you check if test/jdk/java/lang/module/ClassFileVersionsTest.java should be updated too?

@@ -189,6 +190,7 @@ private Attributes doRead(DataInput input) throws IOException {

int minor_version = in.readUnsignedShort();
int major_version = in.readUnsignedShort();
boolean previewClassfile = minor_version == ClassFile.PREVIEW_MINOR_VERSION;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you rename this to isPreview then it would make the uses further down easier to read.

!previewClassfile &&
//java.* modules are deemed participating in preview
//and are allowed to use requires transitive java.base:
!mn.startsWith("java."))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A block comment before the if statement would be a lot clearer here.


enum ScopeType {
ORDINARY,
START_IMPORT,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a "start" import or a "star"* import? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Star import. Will fix.

Copy link
Member

@biboudis biboudis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

return
s == sym &&
!it.hasNext();
for (Scope scope : new Scope[] {toplevel.namedImportScope,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we could deal with this with a compound scope? (and avoid the loop)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In principle, we could, but the CompoundScope is a bit heavier, so it might be better to keep the loop here.

@@ -450,16 +451,20 @@ private void doImport(JCImport tree) {
if (name == names.asterisk) {
// Import on demand.
chk.checkCanonical(imp.selected);
if (tree.staticImport)
if (tree.staticImport) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I note a discrepancy here - static imports are reflected in the AST, but module import aren't. This is why we need to pass an extra parameter to this method, I believe. Should we generalize the import tree node to have a bunch of flags?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nevermind, I see why you need the flag - the code is building synthetic import statements on the fly, based on the packages exported by a module - and then you need to import these synthetic imports, but remember that they originated from an import module...

Copy link
Contributor

@mcimadamore mcimadamore left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes in the import machinery look good (TypeEnter/Resolve).

Copy link
Member

@biboudis biboudis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look good!

@@ -189,6 +190,7 @@ private Attributes doRead(DataInput input) throws IOException {

int minor_version = in.readUnsignedShort();
int major_version = in.readUnsignedShort();
boolean isPreview = minor_version == ClassFile.PREVIEW_MINOR_VERSION;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this will need to throw invalidModuleDescriptor if isPreview && major version != ClassFile.latestMajorVersion, otherwise the check in readModuleAttribute will be defeated by hand craft class files (the VM doesn't read module-info class files so it won't reject it, and this code is called from exposed APIs such as ModuleDescriptor.read).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the check is already being done, right on the next line:

if (!VM.isSupportedModuleDescriptorVersion(major_version, minor_version)) {

which leads to:
public static boolean isSupportedModuleDescriptorVersion(int major, int minor) {

which then leads to:
public static boolean isSupportedClassFileVersion(int major, int minor) {

which has this check:
return minor == 0 || (minor == ClassFile.PREVIEW_MINOR_VERSION && major == ClassFile.latestMajorVersion());

I tried with a module-info.class with major version 65, and minor version 0xFFFF, and it correctly threw the exception for me.

Do I miss something?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do I miss something?

Okay, so this must have been modified at some point to add/use isSupportedModuleDescriptorVersion, in which case readModuleAttribute won't be called. In that case, what you have is okay.

Minor nit, can you fix up the code style in the modified expression as it has ended up with && at the start and end of lines which looks a bit messy.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, done:
9ae10e2

@openjdk openjdk bot added the merge-conflict Pull request has merge conflict with target branch label Nov 1, 2024
@lahodaj lahodaj changed the title 8335989: Implement Module Import Declarations (Second Preview) 8335989: JEP 494: Implement Module Import Declarations (Second Preview) Nov 6, 2024
@openjdk openjdk bot removed the csr Pull request needs approved CSR before integration label Nov 8, 2024
@openjdk openjdk bot removed the merge-conflict Pull request has merge conflict with target branch label Nov 8, 2024
@lahodaj lahodaj changed the title 8335989: JEP 494: Implement Module Import Declarations (Second Preview) 8335989: Implement JEP 494: Module Import Declarations (Second Preview) Nov 13, 2024
Copy link
Contributor

@vicente-romero-oracle vicente-romero-oracle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good

@openjdk openjdk bot added ready Pull request is ready to be integrated and removed jep labels Nov 13, 2024
@lahodaj
Copy link
Contributor Author

lahodaj commented Nov 14, 2024

/integrate

@openjdk
Copy link

openjdk bot commented Nov 14, 2024

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

  • e7d90b9: 8343460: ZGC: Crash in ZRemembered::scan_page_and_clear_remset
  • 95a00f8: 8343875: Minor improvements of jpackage test library
  • 90e9234: 8344074: RISC-V: C1: More accurate _exception_handler_size and _deopt_handler_size
  • 3b28354: 8339288: Improve diagnostic logging runtime/cds/DeterministicDump.java
  • 0dab920: 8343984: Fix Unsafe address overflow
  • 168b18e: 8343958: Remove security manager impl in java.lang.Process and java.lang.Runtime.exec
  • 5ac330b: 8344039: Remove security manager dependency in java.time
  • 1eb38c8: 8343219: Manual clientlibs test failures after SM removal
  • dde6230: 8343416: CDS dump fails when unregistered class can also be loaded from system modules
  • ffea980: 8344023: Unnecessary Hashtable usage in LdapClient.defaultBinaryAttrs
  • ... and 16 more: https://git.openjdk.org/jdk/compare/79345bbbae2564f9f523859d1227a1784293b20f...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Nov 14, 2024
@openjdk openjdk bot closed this Nov 14, 2024
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Nov 14, 2024
@openjdk
Copy link

openjdk bot commented Nov 14, 2024

@lahodaj Pushed as commit 1e97c1c.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler compiler-dev@openjdk.org core-libs core-libs-dev@openjdk.org integrated Pull request has been integrated
Development

Successfully merging this pull request may close these issues.

6 participants