8306729: Add nominal descriptors of modules and packages to Constants API #13615

asotona · 2023-04-24T11:59:03Z

Constants API already provides models for all loadable constants to help programs manipulating class files and modelling bytecode instructions. However no models of module and package constants are provided by Constants API. Every program manipulating class files must implement own models and validation of modules and packages constants.

This pul request adds java.lang.constant.ModuleDesc and java.lang.constant.PackageDesc to the Constants API.

Classfile API will follow up and remove its internal implementations of PackageDesc and ModuleDesc.

Please review this pull request and attached CSR.

Thank you,
Adam

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

Issues

JDK-8306729: Add nominal descriptors of modules and packages to Constants API
JDK-8306730: Add nominal descriptors of modules and packages to Constants API (CSR)

Reviewers

Mandy Chung (@mlchung - Reviewer) ⚠️ Review applies to efab745b

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 13615

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

Using diff file

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

Webrev

Link to Webrev Comment

… API

bridgekeeper · 2023-04-24T11:59:47Z

👋 Welcome back asotona! 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 · 2023-04-24T12:02:40Z

@asotona The following label will be automatically applied to this pull request:

core-libs

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

mlbridge · 2023-04-24T12:07:18Z

Webrevs

src/java.base/share/classes/java/lang/constant/PackageDescImpl.java

src/java.base/share/classes/java/lang/constant/PackageDesc.java

RogerRiggs · 2023-04-24T14:24:30Z

src/java.base/share/classes/java/lang/constant/ConstantUtils.java

+    /**
+     * Validates the correctness of a binary package name. In particular checks for the presence of
+     * invalid characters in the name.
+     *
+     * @param name the package name
+     * @return the package name passed if valid
+     * @throws IllegalArgumentException if the package name is invalid
+     */
+    public static String validateBinaryPackageName(String name) {
+        for (int i=0; i<name.length(); i++) {
+            char ch = name.charAt(i);
+            if (ch == ';' || ch == '[' || ch == '/')
+                throw new IllegalArgumentException("Invalid package name: " + name);
+        }
+        return name;
+    }
+
+    /**
+     * Validates the correctness of an internal package name.
+     * In particular checks for the presence of invalid characters in the name.
+     *
+     * @param name the package name
+     * @return the package name passed if valid
+     * @throws IllegalArgumentException if the package name is invalid
+     */
+    public static String validateInternalPackageName(String name) {
+        for (int i=0; i<name.length(); i++) {
+            char ch = name.charAt(i);
+            if (ch == ';' || ch == '[' || ch == '.')
+                throw new IllegalArgumentException("Invalid package name: " + name);
+        }
+        return name;
+    }
+
+    /**
+     * Validates the correctness of a module name. In particular checks for the presence of
+     * invalid characters in the name.
+     *
+     * {@jvms 4.2.3} Module and Package Names
+     *
+     * @param name the module name
+     * @return the module name passed if valid
+     * @throws IllegalArgumentException if the module name is invalid
+     */
+    public static String validateModuleName(String name) {
+        for (int i=name.length() - 1; i >= 0; i--) {
+            char ch = name.charAt(i);
+            if ((ch >= '\u0000' && ch <= '\u001F')
+            || ((ch == '\\' || ch == ':' || ch =='@') && (i == 0 || name.charAt(--i) != '\\')))
+                throw new IllegalArgumentException("Invalid module name: " + name);
+        }
+        return name;
+    }


If these are performance sensitive or get used a lot, should there be an array of flags indexed by the byte/char to indicate the valid cases?

It is just a range check and three escaped characters check. The range check can be implemented bit-wise and escaped check may be by a switch case, however I don't think there would be any performance benefits.

src/java.base/share/classes/java/lang/constant/ModuleDescImpl.java

src/java.base/share/classes/java/lang/constant/ConstantUtils.java

src/java.base/share/classes/java/lang/constant/package-info.java

test/jdk/java/lang/constant/ModuleDescTest.java

test/jdk/java/lang/constant/PackageDescTest.java

ExE-Boss · 2023-04-24T16:27:28Z

Note that other *Impl classes in java.lang.constant perform validation in their constructors and provide custom toString() formatting and they also don’t use records.

liach · 2023-04-24T16:33:58Z

Note that other *Impl classes in java.lang.constant perform validation in their constructors and provide custom toString() formatting and they also don’t use records.

Records were only added in Java 16; the constant API was added in 12, so this is not a reason not to use records. But performing validation in constructors is better than in factory methods, to avoid code paths that potentially create invalid instances.

asotona · 2023-04-25T07:38:41Z

Note that other *Impl classes in java.lang.constant perform validation in their constructors and provide custom toString() formatting and they also don’t use records.

Performing validation in constructors I see as a blocker for potential performance optimisations from trusted code within the java.lang.constant package.

Custom toString() is a good point, returning for example ModuleDescImpl[moduleName=mymodule] is not ideal.
I'll fix it, thanks.

Records are very well designed exactly for this purpose and I'm not aware of any reason to don't use them.

asotona · 2023-04-25T07:52:34Z

Note that other *Impl classes in java.lang.constant perform validation in their constructors and provide custom toString() formatting and they also don’t use records.

BTW: for example ClassDesc:of(String name) performs repeated validations and I see it as a performance bug.
ClassDesc::of calls ConstantUtils::validateBinaryClassName, then it performs conversion binaryToInternal and calls ClassDesc::ofDescriptor, which checks for ConstantUtils::arrayDepth and calls ReferenceClassDescImpl::new, which again performns validation by calling ConstantUtils::skipOverFieldSignature and checking the result. There is plenty of space for performance improvements here, as all the validation and conversion can be done in one pass only.

mlchung

A side note: I notice that the type names are linked with @linkplain in this package mixed with some @link. I'm not sure if it's intentional as type names are generally used @link or @code.

src/java.base/share/classes/java/lang/constant/PackageDesc.java

src/java.base/share/classes/java/lang/constant/ModuleDesc.java

src/java.base/share/classes/java/lang/constant/PackageDesc.java

src/java.base/share/classes/java/lang/constant/package-info.java

Co-authored-by: Mandy Chung <mandy.chung@oracle.com>

mlchung

I wonder if packageName() and packageInternalName() methods can simply be name() and internalName() as the type name is PackageDesc and package prefix seems to be unnecessary. Similarly, moduleName() can be name(). Have this be discussed and rejected?

src/java.base/share/classes/java/lang/constant/PackageDesc.java

src/java.base/share/classes/java/lang/constant/ModuleDesc.java

src/java.base/share/classes/java/lang/constant/PackageDesc.java

minborg · 2023-04-26T06:18:10Z

Have we considered moving the Impl classes into a non-public area such as jdk.internal to improve encapsulation and reduce perceived footprint?

Co-authored-by: Mandy Chung <mandy.chung@oracle.com>

asotona · 2023-04-26T10:12:48Z

I wonder if packageName() and packageInternalName() methods can simply be name() and internalName() as the type name is PackageDesc and package prefix seems to be unnecessary. Similarly, moduleName() can be name(). Have this be discussed and rejected?

I've searched the history of Classfile API discussions and it actually wasn't discussed.
I agree the prefix is unnecessary.
Thanks for pointing it out.

mlchung · 2023-04-26T16:17:12Z

Have we considered moving the Impl classes into a non-public area such as jdk.internal to improve encapsulation and reduce perceived footprint?

I don't see much of a problem to keep the non-public classes in java.lang.constant since it's not open and no reflective access. Not sure what perceived footprint you referred to as the JDK footprint won't change?

mlchung

Thanks for the update. Two typos. please do make docs to verify the output.

src/java.base/share/classes/java/lang/constant/PackageDesc.java

AlanBateman · 2023-04-26T18:15:34Z

src/java.base/share/classes/java/lang/constant/ConstantUtils.java

+            char ch = name.charAt(i);
+            if ((ch >= '\u0000' && ch <= '\u001F')
+            || ((ch == '\\' || ch == ':' || ch =='@') && (i == 0 || name.charAt(--i) != '\\')))
+                throw new IllegalArgumentException("Invalid module name: " + name);


test/jdk/java/lang/module/ModuleNames.java has tables of legal and illegal module names, including tests that escape backslash, @, and :. It might be useful to run these tests on this method.

I've updated ModuleDescTest with all the positive and negative cases from test/jdk/java/lang/module/ModuleNames.java, except for the empty name.
Thanks for the review.

mlchung · 2023-04-26T19:09:33Z

I keep pondering if this API should allow creating ModuleDesc and PackageDesc with an empty name.

JLS 6.5.3. Meaning of Module Names and Package Names

The module name M, whether simple or qualified, denotes the module (if any) with that name.

The module system will reject a CONSTANT_Module_info of zero-length name while JVMS does not prohibit it.

The module system rejects the unnamed package in a named module. CONSTANT_Package_info represents a package exported or opened by a module.

Co-authored-by: Mandy Chung <mandy.chung@oracle.com>

mlchung · 2023-04-27T18:51:08Z

Discussed with @briangoetz offline. We can let PackageDesc and ModuleDesc model unnamed package/module but the ClassFile API should check to make sure their names are not empty before using them to write to a classfile.

For example, if we had a constructor in ClassDesc(PackageDesc package, String name), it could be tolerant of empty packages and treat it as just new ClassDesc(name).

So this PR is good to go.

openjdk · 2023-05-02T19:52:09Z

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

8306729: Add nominal descriptors of modules and packages to Constants API

Reviewed-by: mchung

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

8c106b0: 8303784: no-@target annotations should be applicable to type parameter declarations
b76f320: 8307123: Fix deprecation warnings in DPrinter
a8bf2ac: 8304888: Add dedicated VMProps for linker and fallback linker
75a4edc: 8301223: Replace NULL with nullptr in share/gc/g1/
544bd26: 8305505: NPE in javazic compiler
ea9201f: 8306510: Print number of threads and stack sizes in error reports
a8d16de: 8300197: Freeze/thaw an interpreter frame using a single copy_to_chunk() call
1532a1b: 8307164: TestSegmentCopy times out (mainline)
860bf9b: 8307104: [AIX] VM crashes with UseRTMLocking on Power10
86f97fe: 8305919: java/lang/Thread/virtual/HoldsLock.java#id0 failed, ThreadInfo.getLockInfo() return null
... and 137 more: https://git.openjdk.org/jdk/compare/136dad7197a1969b2b1fc325f4336c20386c5d3b...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.

asotona · 2023-05-03T06:33:15Z

/integrate

openjdk · 2023-05-03T06:35:07Z

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

0b5b642: 8307150: RISC-V: Remove remaining StoreLoad barrier with UseCondCardMark for Serial/Parallel GC
418a825: 8306466: Open source more AWT Drag & Drop related tests
74667e3: 8303919: Instant.ofEpochMilli says it can throw an exception that it can't
76991c8: 8282232: [Win] GetMousePositionWithPopup test fails due to wrong mouse position
05b9b58: 8302496: Runtime.exit incorrectly says it never throws an exception
8a70664: 8293117: Add atomic bitset functions
8c106b0: 8303784: no-@target annotations should be applicable to type parameter declarations
b76f320: 8307123: Fix deprecation warnings in DPrinter
a8bf2ac: 8304888: Add dedicated VMProps for linker and fallback linker
75a4edc: 8301223: Replace NULL with nullptr in share/gc/g1/
... and 143 more: https://git.openjdk.org/jdk/compare/136dad7197a1969b2b1fc325f4336c20386c5d3b...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-05-03T06:35:18Z

@asotona Pushed as commit c8f3756.

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

openjdk/jdk#13615 openjdk/jdk#13979

8306729: Add nominal descriptors of modules and packages to Constants…

aede94f

… API

openjdk bot added csr Pull request needs approved CSR before integration rfr Pull request is ready for review labels Apr 24, 2023

openjdk bot added the core-libs core-libs-dev@openjdk.org label Apr 24, 2023

liach reviewed Apr 24, 2023

View reviewed changes

src/java.base/share/classes/java/lang/constant/PackageDescImpl.java Outdated Show resolved Hide resolved

src/java.base/share/classes/java/lang/constant/PackageDesc.java Show resolved Hide resolved

added links to JVMS and utility methods moved to ConstantUtils

0ca0b56

RogerRiggs reviewed Apr 24, 2023

View reviewed changes

Doc fixes + added null and empty tests

47aa52d

added custom toString() methods

7c45b34

mlchung reviewed Apr 25, 2023

View reviewed changes

asotona and others added 8 commits April 25, 2023 20:51

Apply suggestions from code review

2c67f83

Co-authored-by: Mandy Chung <mandy.chung@oracle.com>

Update PackageDesc.java

b094cb0

Update ModuleDesc.java

5f9664f

Update PackageDesc.java

ecae26c

Update PackageDesc.java

bf3001d

Update ModuleDesc.java

2d28353

Update PackageDesc.java

696788f

Update ModuleDesc.java

b37a239

mlchung reviewed Apr 25, 2023

View reviewed changes

asotona and others added 3 commits April 26, 2023 11:50

Apply suggestions from code review

79459b4

Co-authored-by: Mandy Chung <mandy.chung@oracle.com>

Update ModuleDesc.java

11c054b

Update PackageDesc.java

ca91db4

removed prefixes from name methods

efab745

mlchung approved these changes Apr 26, 2023

View reviewed changes

src/java.base/share/classes/java/lang/constant/PackageDesc.java Outdated Show resolved Hide resolved

src/java.base/share/classes/java/lang/constant/PackageDesc.java Outdated Show resolved Hide resolved

AlanBateman reviewed Apr 26, 2023

View reviewed changes

asotona and others added 2 commits April 27, 2023 08:41

Apply suggestions from code review

39980e0

Co-authored-by: Mandy Chung <mandy.chung@oracle.com>

tests update

0456f77

openjdk bot added ready Pull request is ready to be integrated and removed csr Pull request needs approved CSR before integration labels May 2, 2023

openjdk bot added the integrated Pull request has been integrated label May 3, 2023

openjdk bot closed this May 3, 2023

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

Glavo mentioned this pull request May 18, 2023

Sync JDK-8306729 and JDK-8307326 Glavo/classfile#12

Merged

Glavo added a commit to Glavo/classfile that referenced this pull request May 18, 2023

Sync JDK-8306729 and JDK-8307326 (#12)

e950e9d

openjdk/jdk#13615 openjdk/jdk#13979

8306729: Add nominal descriptors of modules and packages to Constants API #13615

8306729: Add nominal descriptors of modules and packages to Constants API #13615

Uh oh!

Conversation

asotona commented Apr 24, 2023 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issues

Reviewers

Reviewing

Webrev

Uh oh!

bridgekeeper bot commented Apr 24, 2023

Uh oh!

openjdk bot commented Apr 24, 2023

Uh oh!

mlbridge bot commented Apr 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

Uh oh!

Uh oh!

RogerRiggs Apr 24, 2023

Choose a reason for hiding this comment

Uh oh!

asotona Apr 24, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ExE-Boss commented Apr 24, 2023

Uh oh!

liach commented Apr 24, 2023

Uh oh!

asotona commented Apr 25, 2023

Uh oh!

asotona commented Apr 25, 2023

Uh oh!

mlchung left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mlchung left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

minborg commented Apr 26, 2023

Uh oh!

asotona commented Apr 26, 2023

Uh oh!

mlchung commented Apr 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mlchung left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

AlanBateman Apr 26, 2023

Choose a reason for hiding this comment

Uh oh!

asotona Apr 27, 2023

asotona commented Apr 24, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented Apr 24, 2023 •

edited

Loading

mlchung commented Apr 26, 2023 •

edited

Loading

mlchung commented Apr 27, 2023 •

edited

Loading