JDK-8305206: Add @spec tags in java.base/java.* (part 1) #13248

jonathan-gibbons · 2023-03-30T17:24:11Z

Please review a change to add @spec tags (and remove some equivalent @see tags) to the main "core-libs" packages in java.base module.

This is similar to, and a subset of, PR #11073. That PR was withdrawn, and based on the ensuing discussion and suggestion, is now being handled with a series of PRs for various separate parts of the system. Follow-up PRs will be provided for the rest of java.base, for java.desktop, and for XML APIs. The "LangTools" modules have already been updated. The "External Specifications" page has been temporarily disabled until this work is complete.

While the primary content of the change was automated, I've manually adjusted the formatting, to break long lines.

It is clear there is significant inconsistency in the ordering of block tags in doc comment. We might want to (separately) consider normalizing the order of the tags, perhaps according to the order defined for the tags in the generated output, as given here

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

Issue

JDK-8305206: Add @SPEC tags in java.base/java.* (part 1)

Reviewers

Alan Bateman (@AlanBateman - Reviewer) ⚠️ Review applies to 096a4188
Naoto Sato (@naotoj - Reviewer) ⚠️ Review applies to 096a4188
Joe Darcy (@jddarcy - Reviewer) ⚠️ Review applies to 096a4188
Lance Andersen (@LanceAndersen - Reviewer) ⚠️ Review applies to 096a4188
Daniel Fuchs (@dfuch - Reviewer) ⚠️ Review applies to 096a4188
Iris Clark (@irisclark - Reviewer)
Mandy Chung (@mlchung - Reviewer) ⚠️ Review applies to 096a4188

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 13248

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

Using diff file

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

Webrev

Link to Webrev Comment

bridgekeeper · 2023-03-30T17:25:24Z

👋 Welcome back jjg! 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-03-30T17:27:43Z

@jonathan-gibbons The following labels will be automatically applied to this pull request:

core-libs
i18n
net
nio
security

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 · 2023-03-30T17:31:11Z

Webrevs

AlanBateman

I skimmed through the changes and it looks quite good, much more workable than PR 11073.

Do you have a proposed ordering with other tags? I expected it would go with @see but I see several where @SPEC is before @author, and @see after @author. I know it doesn't really matter.

AlanBateman · 2023-03-30T19:42:33Z

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

     * @throws     SecurityException  if the current thread cannot modify this
     *             thread.
+     *
+     * @spec jni/index.html Java Native Interface Specification


The link to the JNI spec in this method is from implNote so I'm wondering if the spec link is needed here.

Right now, the tag is added for any declaration whose comment contains a reference to an external spec (i.e. with <a href-....>.

When we enable the "External Specifications" page, it will contain a link back to this page as part of the cross-reference info, which seems useful. That being said, if you feel strongly the tag should not be added here, I can remove it.

Leave it in for now and we can look at it again when the external spec page is in place. My comment is mostly that it will look a bit strange to link to this method because it's text in an implNote rather than spec.

AlanBateman · 2023-03-30T19:44:06Z

src/java.base/share/classes/java/nio/file/Files.java

     *          permission required by a file type detector implementation.
+     *
+     * @spec https://www.rfc-editor.org/info/rfc2045
+     *      RFC 2045: RFC 2045: Multipurpose Internet Mail Extensions (MIME) Part One: Format of Internet Message Bodies


Maybe this one can be put on two lines to avoid the wrapping when looking at is side-by-side.

Fixed. There was also a stutter-bug with a double "RFC 2045: RFC 2045:" which I have also fixed.

jonathan-gibbons · 2023-03-30T20:38:13Z

I skimmed through the changes and it looks quite good, much more workable than PR 11073.

Do you have a proposed ordering with other tags? I expected it would go with @see but I see several where @SPEC is before @author, and @see after @author. I know it doesn't really matter.

The initial assumption was "after the @param/@return/@throws group". Overall, as I said in the description for this PR, the block tags are not very consistent about ordering. I was thinking we might want to recommend an overall ordering, but I'm worried it would be too intrusive a change to apply generally. In the description, I suggested an ordering based on the
order in Docs.gmk which defines the order in which tags appear in the generated output.

naotoj

Changes in i18n-related classes look good to me.

openjdk · 2023-03-30T22:43:21Z

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

8305206: Add @spec tags in java.base/java.* (part 1)

Reviewed-by: alanb, naoto, darcy, lancea, dfuchs, iris, 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 55 new commits pushed to the master branch:

b062b1b: 8304743: Compile_lock and SystemDictionary updates
df819cf: 8304945: StringBuilder and StringBuffer should implement Appendable explicitly
312bbe7: 8305485: Problemlist runtime/Thread/TestAlwaysPreTouchStacks.java
50e31e0: 8305442: (bf) Direct and view implementations of CharBuffer.toString(int, int) do not need to catch SIOBE
85e3974: 8304014: Convert test/jdk/java/util/zip/ZipFile/CorruptedZipFiles.java to junit
40aea04: 8278268: (ch) InputStream returned by Channels.newInputStream should have fast path for FileChannel targets
9b9b5a7: 8302323: Add repeat methods to StringBuilder/StringBuffer
dd7ca75: 8305478: [REDO] disable gtest/NMTGtests.java sub-tests failing due to JDK-8305414
f9827ad: 8288109: HttpExchangeImpl.setAttribute does not allow null value after JDK-8266897
6010de0: 8305417: disable gtest/NMTGtests.java sub-tests failing due to JDK-8305414
... and 45 more: https://git.openjdk.org/jdk/compare/69152c3b18495754e52b90e320ca866f97d80752...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.

jddarcy

Good to see these changes.

AlanBateman · 2023-03-31T10:26:08Z

The initial assumption was "after the @param/@return/@throws group". Overall, as I said in the description for this PR, the block tags are not very consistent about ordering. I was thinking we might want to recommend an overall ordering, but I'm worried it would be too intrusive a change to apply generally. In the description, I suggested an ordering based on the order in Docs.gmk which defines the order in which tags appear in the generated output.

Okay, I think it's just a few of the usages that I sampled had author tags left over from early JDK releases and they end up between the spec and see tags. We can fix these things up at another time.

LanceAndersen

Hi Jon,

This looks fine. I was wondering if we should do the same for java.util.zip and the PKWare Zip Spec or where java.sql references the JDBC Spec?

dfuch

I had a look at the java.net part and it looks reasonable.

LanceAndersen · 2023-03-31T10:45:39Z

Hi Jon,

This looks fine. I was wondering if we should do the same for java.util.zip and the PKWare Zip Spec or where java.sql references the JDBC Spec?

Well, I must need coffee this morning as obviously JDBC is in the java.sql module, not java.base.... So scratch that comment ;-)

seanjmullan · 2023-03-31T16:28:14Z

I didn't see any changes to security APIs - are they coming in a follow-on issue?

irisclark · 2023-03-31T17:14:01Z

src/java.base/share/classes/java/io/ObjectOutputStream.java

 * @see java.io.Externalizable
- * @see <a href="{@docRoot}/../specs/serialization/output.html">
- *      <cite>Java Object Serialization Specification,</cite> Section 2, "Object Output Classes"</a>
 * @since       1.1


Just confirming... The changes to the java.io classes for the Serialization Spec now all point to the index rather than particular chapters/sections. I'm assuming that's intentional so that when the top-level Spec page appears, there is a single entry for that specification.

The @spec tag should point to the root, but we should not remove more specific references to within the spec. I will review places where @see has been removed/replaced.

mlchung

LGTM

jonathan-gibbons · 2023-03-31T18:26:44Z

Hi Jon,
This looks fine. I was wondering if we should do the same for java.util.zip and the PKWare Zip Spec or where java.sql references the JDBC Spec?

Well, I must need coffee this morning as obviously JDBC is in the java.sql module, not java.base.... So scratch that comment ;-)

The other modules will be done in due course.

…URL

jonathan-gibbons · 2023-03-31T22:15:00Z

I didn't see any changes to security APIs - are they coming in a follow-on issue?

Yes, this is Add @spec tags in java.base/java.* (part 1)
The rest of java.base will be in part 2. JDK-8305406

jonathan-gibbons · 2023-04-04T00:08:49Z

/integrate

openjdk · 2023-04-04T00:09:46Z

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

ccbb0e8: 8303798: REDO - Remove fdlibm C sources
9ce5fdc: 8305421: Work around JDK-8305420 in CDSJDITest.java
b062b1b: 8304743: Compile_lock and SystemDictionary updates
df819cf: 8304945: StringBuilder and StringBuffer should implement Appendable explicitly
312bbe7: 8305485: Problemlist runtime/Thread/TestAlwaysPreTouchStacks.java
50e31e0: 8305442: (bf) Direct and view implementations of CharBuffer.toString(int, int) do not need to catch SIOBE
85e3974: 8304014: Convert test/jdk/java/util/zip/ZipFile/CorruptedZipFiles.java to junit
40aea04: 8278268: (ch) InputStream returned by Channels.newInputStream should have fast path for FileChannel targets
9b9b5a7: 8302323: Add repeat methods to StringBuilder/StringBuffer
dd7ca75: 8305478: [REDO] disable gtest/NMTGtests.java sub-tests failing due to JDK-8305414
... and 47 more: https://git.openjdk.org/jdk/compare/69152c3b18495754e52b90e320ca866f97d80752...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-04-04T00:10:00Z

@jonathan-gibbons Pushed as commit c6bd489.

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

JDK-8305206: Add @SPEC tags in java.base/java.* (part 1)

3e1102a

openjdk bot added the rfr Pull request is ready for review label Mar 30, 2023

openjdk bot added security [email protected] nio [email protected] core-libs [email protected] net [email protected] i18n [email protected] labels Mar 30, 2023

AlanBateman reviewed Mar 30, 2023

View reviewed changes

address review feedback

096a418

naotoj approved these changes Mar 30, 2023

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Mar 30, 2023

jddarcy approved these changes Mar 31, 2023

View reviewed changes

AlanBateman approved these changes Mar 31, 2023

View reviewed changes

LanceAndersen approved these changes Mar 31, 2023

View reviewed changes

dfuch approved these changes Mar 31, 2023

View reviewed changes

irisclark approved these changes Mar 31, 2023

View reviewed changes

mlchung approved these changes Mar 31, 2023

View reviewed changes

revert removing @see tags where the URL was not the same as the @SPEC …

7f64cc3

…URL

irisclark approved these changes Apr 1, 2023

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Apr 4, 2023

openjdk bot closed this Apr 4, 2023

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

jonathan-gibbons deleted the 8305206.at-spec-java.base-java-1 branch April 4, 2023 00:10

jonathan-gibbons mentioned this pull request Apr 4, 2023

8305406: Add @spec tags in java.base/java.* (part 2) #13336

Closed

3 tasks

JDK-8305206: Add @spec tags in java.base/java.* (part 1) #13248

JDK-8305206: Add @spec tags in java.base/java.* (part 1) #13248

Uh oh!

Conversation

jonathan-gibbons commented Mar 30, 2023 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Webrev

Uh oh!

bridgekeeper bot commented Mar 30, 2023

Uh oh!

openjdk bot commented Mar 30, 2023

Uh oh!

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

Uh oh!

Webrevs

Uh oh!

AlanBateman left a comment

Choose a reason for hiding this comment

Uh oh!

AlanBateman Mar 30, 2023

Choose a reason for hiding this comment

Uh oh!

jonathan-gibbons Mar 30, 2023

Choose a reason for hiding this comment

Uh oh!

AlanBateman Mar 31, 2023

Choose a reason for hiding this comment

Uh oh!

AlanBateman Mar 30, 2023

Choose a reason for hiding this comment

Uh oh!

jonathan-gibbons Mar 30, 2023

Choose a reason for hiding this comment

Uh oh!

jonathan-gibbons commented Mar 30, 2023

Uh oh!

naotoj left a comment

Choose a reason for hiding this comment

Uh oh!

openjdk bot commented Mar 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jddarcy left a comment

Choose a reason for hiding this comment

Uh oh!

AlanBateman commented Mar 31, 2023

Uh oh!

LanceAndersen left a comment

Choose a reason for hiding this comment

Uh oh!

dfuch left a comment

Choose a reason for hiding this comment

Uh oh!

LanceAndersen commented Mar 31, 2023

Uh oh!

seanjmullan commented Mar 31, 2023

Uh oh!

irisclark Mar 31, 2023

Choose a reason for hiding this comment

Uh oh!

jonathan-gibbons Mar 31, 2023

Choose a reason for hiding this comment

Uh oh!

mlchung left a comment

Choose a reason for hiding this comment

Uh oh!

jonathan-gibbons commented Mar 31, 2023

Uh oh!

jonathan-gibbons commented Mar 31, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonathan-gibbons commented Apr 4, 2023

Uh oh!

openjdk bot commented Apr 4, 2023

Uh oh!

openjdk bot commented Apr 4, 2023

jonathan-gibbons commented Mar 30, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented Mar 30, 2023 •

edited

Loading

openjdk bot commented Mar 30, 2023 •

edited

Loading

jonathan-gibbons commented Mar 31, 2023 •

edited

Loading